Merge pull request #1552 from fl4via/2.2.x_backport_bug_fixes

[UNDERTOW-2309][UNDERTOW-2330][UNDERTOW-2331][UNDERTOW-2337][UNDERTOW-2342][UNDERTOW-2304][UNDERTOW-2338] CVE-2023-4639 Backport bug fixes

core/src/main/java/io/undertow/protocols/ssl/SslConduit.java

@@ -206,7 +206,9 @@ public void run() {
         this.sink = delegate.getSinkChannel().getConduit();
         this.source = delegate.getSourceChannel().getConduit();
         this.engine = engine;
-        this.delegatedTaskExecutor = delegatedTaskExecutor;
+        // if delegatedTaskExecutor is the same as delegate.getWorker, don't set it (see handshake task execution below,
+        // we need to pick a thread that is different from the current thread running or run the handshake task immediately at the same thread)
+        this.delegatedTaskExecutor = this.delegate.getWorker() != delegatedTaskExecutor? delegatedTaskExecutor : null;
         this.bufferPool = bufferPool;
         delegate.getSourceChannel().getConduit().setReadReadyHandler(readReadyHandler = new SslReadReadyHandler(null));
         delegate.getSinkChannel().getConduit().setWriteReadyHandler(writeReadyHandler = new SslWriteReadyHandler(null));
@@ -1168,7 +1170,16 @@ public void run() {
                     }
                 };
                 try {
-                    getDelegatedTaskExecutor().execute(wrappedTask);
+                    // we want to pick a thread different from the one we are running, to prevent starvation scenarios
+                    // for example where we repeatedly attempt to write or read but are stuck waiting on a handshake task
+                    // and the handshake task rarely has the chance to run because it needs the read/write thread to complete execution
+                    // so, if io thread (read and write thread) is the same as current thread, and getDelegatedTaskExecutor will delegate
+                    // execution to the same set of threads we are currently at, just run the handshake task right away to prevent blocking
+                    if (delegate.getIoThread().equals(Thread.currentThread()) && delegatedTaskExecutor == null) {
+                        wrappedTask.run();
+                    } else {
+                        getDelegatedTaskExecutor().execute(wrappedTask);
+                    }
                 } catch (RejectedExecutionException e) {
                     UndertowLogger.REQUEST_IO_LOGGER.sslEngineDelegatedTaskRejected(e);
                     IoUtils.safeClose(connection);

core/src/main/java/io/undertow/server/DefaultByteBufferPool.java

@@ -26,7 +26,9 @@
 import java.nio.ByteBuffer;
 import java.util.ArrayDeque;
 import java.util.ArrayList;
+import java.util.HashMap;
 import java.util.List;
+import java.util.Map;
 import java.util.concurrent.ConcurrentLinkedQueue;
 import java.util.concurrent.atomic.AtomicIntegerFieldUpdater;
 
@@ -39,7 +41,7 @@
  */
 public class DefaultByteBufferPool implements ByteBufferPool {
 
-    private final ThreadLocal<ThreadLocalData> threadLocalCache = new ThreadLocal<>();
+    private final ThreadLocalCache threadLocalCache = new ThreadLocalCache();
     // Access requires synchronization on the threadLocalDataList instance
     private final List<WeakReference<ThreadLocalData>> threadLocalDataList = new ArrayList<>();
     private final ConcurrentLinkedQueue<ByteBuffer> queue = new ConcurrentLinkedQueue<>();
@@ -229,6 +231,7 @@ public void close() {
                     local.buffers.clear();
                 }
                 ref.clear();
+                threadLocalCache.remove(local);
             }
             threadLocalDataList.clear();
         }
@@ -327,4 +330,29 @@ protected void finalize() throws Throwable {
         }
     }
 
+    // This is used instead of Java ThreadLocal class. Unlike in the ThreadLocal class, the remove() method in this
+    // class can be called by a different thread than the one that initialized the data.
+    private static class ThreadLocalCache {
+
+        Map<Thread, ThreadLocalData> localsByThread = new HashMap<>();
+
+        ThreadLocalData get() {
+            return localsByThread.get(Thread.currentThread());
+        }
+
+        void set(ThreadLocalData threadLocalData) {
+            localsByThread.put(Thread.currentThread(), threadLocalData);
+        }
+
+        void remove(ThreadLocalData threadLocalData) {
+            // Find the entry containing given data instance and remove it from the map.
+            for (Map.Entry<Thread, ThreadLocalData> entry: localsByThread.entrySet()) {
+                if (threadLocalData.equals(entry.getValue())) {
+                    localsByThread.remove(entry.getKey(), entry.getValue());
+                    break;
+                }
+            }
+        }
+    }
+
 }

core/src/main/java/io/undertow/server/handlers/RequestDumpingHandler.java

@@ -164,7 +164,7 @@ private void dumpRequestBody(HttpServerExchange exchange, StringBuilder sb) {
                     sb.append(formField)
                             .append("=");
                     for (FormData.FormValue formValue : formValues) {
-                        sb.append(formValue.isFileItem() ? "[file-content]" : formValue.getValue());
+                        sb.append(formValue.isFileItem() && !formValue.isBigField() ? "[file-content]" : formValue.getValue());
                         sb.append("\n");
 
                         if (formValue.getHeaders() != null) {

core/src/main/java/io/undertow/server/handlers/form/FormData.java

@@ -34,6 +34,7 @@
 import java.util.Map;
 
 import io.undertow.UndertowMessages;
+import io.undertow.util.FileUtils;
 import io.undertow.util.HeaderMap;
 
 /**
@@ -102,11 +103,15 @@ public void add(String name, String value, String charset, final HeaderMap heade
     }
 
     public void add(String name, Path value, String fileName, final HeaderMap headers) {
+        add(name, value, fileName, headers, false, null);
+    }
+
+    public void add(String name, Path value, String fileName, final HeaderMap headers, boolean bigField, String charset) {
         Deque<FormValue> values = this.values.get(name);
         if (values == null) {
             this.values.put(name, values = new ArrayDeque<>(1));
         }
-        values.add(new FormValueImpl(value, fileName, headers));
+        values.add(new FormValueImpl(value, fileName, headers, bigField, charset));
         if (values.size() > maxValues) {
             throw new RuntimeException(UndertowMessages.MESSAGES.tooManyParameters(maxValues));
         }
@@ -211,6 +216,16 @@ public interface FormValue {
          * @return The headers that were present in the multipart request, or null if this was not a multipart request
          */
         HeaderMap getHeaders();
+
+        /**
+         * @return true if size of the FormValue comes from a multipart request exceeds the fieldSizeThreshold of
+         * {@link MultiPartParserDefinition} without filename specified.
+         *
+         * A big field is stored in disk, it is a file based FileItem.
+         *
+         * getValue() returns getCharset() decoded string from the file if it is true.
+         */
+        boolean isBigField();
     }
 
     public static class FileItem {
@@ -284,13 +299,15 @@ static class FormValueImpl implements FormValue {
         private final HeaderMap headers;
         private final FileItem fileItem;
         private final String charset;
+        private final boolean bigField;
 
         FormValueImpl(String value, HeaderMap headers) {
             this.value = value;
             this.headers = headers;
             this.fileName = null;
             this.fileItem = null;
             this.charset = null;
+            this.bigField = false;
         }
 
         FormValueImpl(String value, String charset, HeaderMap headers) {
@@ -299,14 +316,16 @@ static class FormValueImpl implements FormValue {
             this.headers = headers;
             this.fileName = null;
             this.fileItem = null;
+            this.bigField = false;
         }
 
-        FormValueImpl(Path file, final String fileName, HeaderMap headers) {
+        FormValueImpl(Path file, final String fileName, HeaderMap headers, boolean bigField, String charset) {
             this.fileItem = new FileItem(file);
             this.headers = headers;
             this.fileName = fileName;
             this.value = null;
-            this.charset = null;
+            this.charset = charset;
+            this.bigField = bigField;
         }
 
         FormValueImpl(byte[] data, String fileName, HeaderMap headers) {
@@ -315,12 +334,20 @@ static class FormValueImpl implements FormValue {
             this.headers = headers;
             this.value = null;
             this.charset = null;
+            this.bigField = false;
         }
 
 
         @Override
         public String getValue() {
             if (value == null) {
+                if (bigField) {
+                    try (InputStream inputStream = getFileItem().getInputStream()) {
+                        return FileUtils.readFile(inputStream, this.charset);
+                    } catch (IOException e) {
+                        // ignore
+                    }
+                }
                 throw UndertowMessages.MESSAGES.formValueIsAFile();
             }
             return value;
@@ -360,6 +387,10 @@ public FileItem getFileItem() {
             return fileItem;
         }
 
+        public boolean isBigField() {
+            return bigField;
+        }
+
         @Override
         public boolean isFileItem() {
             return fileItem != null;

core/src/main/java/io/undertow/server/handlers/form/MultiPartParserDefinition.java

@@ -350,8 +350,7 @@ public void endPart() {
                  if (fileName != null) {
                      data.add(currentName, file, fileName, headers);
                  } else {
-                     final Path fileNamePath = file.getFileName();
-                     data.add(currentName, file, fileNamePath != null ? fileNamePath.toString() : "", headers);
+                     data.add(currentName, file, null, headers, true, getCharset());
                  }
                 file = null;
                 contentBytes.reset();
@@ -365,18 +364,8 @@ public void endPart() {
                 data.add(currentName, Arrays.copyOf(contentBytes.toByteArray(), contentBytes.size()), fileName, headers);
                 contentBytes.reset();
             } else {
-
-
                 try {
-                    String charset = defaultEncoding;
-                    String contentType = headers.getFirst(Headers.CONTENT_TYPE);
-                    if (contentType != null) {
-                        String cs = Headers.extractQuotedValueFromHeader(contentType, "charset");
-                        if (cs != null) {
-                            charset = cs;
-                        }
-                    }
-
+                    String charset = getCharset();
                     data.add(currentName, new String(contentBytes.toByteArray(), charset), charset, headers);
                 } catch (UnsupportedEncodingException e) {
                     throw new RuntimeException(e);
@@ -385,6 +374,18 @@ public void endPart() {
             }
         }
 
+        private String getCharset() {
+            String charset = defaultEncoding;
+            String contentType = headers.getFirst(Headers.CONTENT_TYPE);
+            if (contentType != null) {
+                String cs = Headers.extractQuotedValueFromHeader(contentType, "charset");
+                if (cs != null) {
+                    charset = cs;
+                }
+            }
+            return charset;
+        }
+
         public List<Path> getCreatedFiles() {
             return createdFiles;
         }

core/src/main/java/io/undertow/server/protocol/http/HttpReadListener.java

@@ -139,12 +139,21 @@ public void handleEvent(final ConduitStreamSourceChannel channel) {
     public void handleEventWithNoRunningRequest(final ConduitStreamSourceChannel channel) {
         PooledByteBuffer existing = connection.getExtraBytes();
         if ((existing == null && connection.getOriginalSourceConduit().isReadShutdown()) || connection.getOriginalSinkConduit().isWriteShutdown()) {
+            UndertowLogger.REQUEST_IO_LOGGER.debug("Connection is closing, cancelling handling of request");
+            IoUtils.safeClose(connection);
+            channel.suspendReads();
+            return;
+        }
+        final PooledByteBuffer pooled;
+        try {
+            pooled = existing == null ? connection.getByteBufferPool().allocate() : existing;
+        } catch (IllegalStateException e) {
+            UndertowLogger.REQUEST_IO_LOGGER.debug("Connection is closing, cancelling handling of request", e);
+            // shutdown started after previous if statement, so treat it like previous statement
             IoUtils.safeClose(connection);
             channel.suspendReads();
             return;
         }
-
-        final PooledByteBuffer pooled = existing == null ? connection.getByteBufferPool().allocate() : existing;
         final ByteBuffer buffer = pooled.getBuffer();
         boolean free = true;
 

core/src/main/java/io/undertow/util/Cookies.java

@@ -316,6 +316,9 @@ private static void parseCookie(final String cookie, final Set<Cookie> parsedCoo
                         cookieCount = createCookie(name, containsEscapedQuotes ? unescapeDoubleQuotes(cookie.substring(start, i)) : cookie.substring(start, i), maxCookies, cookieCount, cookies, additional);
                         state = 0;
                         start = i + 1;
+                    } else if (c == ';' || (commaIsSeperator && c == ',')) {
+                        state = 0;
+                        start = i + 1;
                     }
                     // Skip the next double quote char '"' when it is escaped by backslash '\' (i.e. \") inside the quoted value
                     if (c == '\\' && (i + 1 < cookie.length()) && cookie.charAt(i + 1) == '"') {

core/src/main/java/io/undertow/util/FileUtils.java

@@ -22,7 +22,9 @@
 import java.io.IOException;
 import java.io.InputStream;
 import java.net.URL;
+import java.nio.charset.Charset;
 import java.nio.charset.StandardCharsets;
+import java.nio.charset.UnsupportedCharsetException;
 import java.nio.file.FileVisitResult;
 import java.nio.file.Files;
 import java.nio.file.Path;
@@ -51,16 +53,32 @@ public static String readFile(URL url) {
         }
     }
 
+    public static String readFile(InputStream file, String charset) {
+        try {
+            Charset charSet = charset != null ? Charset.forName(charset) : StandardCharsets.UTF_8;
+            return readFile(file, charSet);
+        } catch (UnsupportedCharsetException e) {
+            throw new RuntimeException(e);
+        }
+    }
+
     /**
      * Reads the {@link InputStream file} and converting it to {@link String} using UTF-8 encoding.
      */
     public static String readFile(InputStream file) {
+        return readFile(file, StandardCharsets.UTF_8);
+    }
+
+    /**
+     * Reads the {@link InputStream file} and converting it to {@link String} using <code>charSet</code> encoding.
+     */
+    public static String readFile(InputStream file, Charset charSet) {
         try (BufferedInputStream stream = new BufferedInputStream(file)) {
             byte[] buff = new byte[1024];
             StringBuilder builder = new StringBuilder();
             int read;
             while ((read = stream.read(buff)) != -1) {
-                builder.append(new String(buff, 0, read, StandardCharsets.UTF_8));
+                builder.append(new String(buff, 0, read, charSet));
             }
             return builder.toString();
         } catch (IOException e) {

core/src/test/java/io/undertow/protocols/http2/RapidResetDDoSUnitTestCase.java

@@ -243,7 +243,7 @@ public void assertDoSRstFramesHandled(int totalNumberOfRequests, int rstStreamLi
 
             // server sent go away before processing and responding client frames, sometimes this happens, depends on the order of threads
             // being executed
-            if (responses.isEmpty()) {
+            if (responses.size() < totalNumberOfRequests) {
                 Assert.assertTrue(errorExpected);
                 Assert.assertNotNull(exception);
                 Assert.assertTrue(exception instanceof ClosedChannelException);

core/src/test/java/io/undertow/server/handlers/form/MultipartFormDataParserTestCase.java

@@ -370,8 +370,7 @@ public void testLargeContentWithoutFileNameWithSmallFileSizeThreshold() throws E
 
             Assert.assertEquals("false", parsedResponse.get("in_memory"));
             Assert.assertEquals(DigestUtils.md5Hex(Files.newInputStream(file.toPath())), parsedResponse.get("hash"));
-            Assert.assertTrue(parsedResponse.get("file_name").startsWith("undertow"));
-            Assert.assertTrue(parsedResponse.get("file_name").endsWith("upload"));
+            Assert.assertEquals(parsedResponse.get("file_name"), "null");
         } finally {
             if (!file.delete()) {
                 file.deleteOnExit();

core/src/test/java/io/undertow/util/CookiesTestCase.java

@@ -449,6 +449,21 @@ public void testSameSiteCookie() {
         Assert.assertNull(cookie.getSameSiteMode());
     }
 
+    @Test
+    public void testNoDoubleQuoteTermination() {
+        Map<String, Cookie> cookies = Cookies.parseRequestCookies(4, false, Arrays.asList("CUSTOMER=\"WILE_E_COYOTE\"; BAD=\"X; SHIPPING=FEDEX"), true);
+        Assert.assertEquals(2, cookies.size());
+        Cookie cookie = cookies.get("CUSTOMER");
+        Assert.assertEquals("CUSTOMER", cookie.getName());
+        Assert.assertEquals("WILE_E_COYOTE", cookie.getValue());
+        cookie = cookies.get("BAD");
+        Assert.assertNull(cookie);
+        cookie = cookies.get("SHIPPING");
+        Assert.assertEquals("SHIPPING", cookie.getName());
+        Assert.assertEquals("FEDEX", cookie.getValue());
+        Assert.assertNotNull(cookie);
+    }
+
     // RFC6265 allows US-ASCII characters excluding CTLs, whitespace,
     // double quote, comma, semicolon and backslash as cookie value.
     // This does not change even if value is quoted.