Merge pull request #1559 from fl4via/2.2.x_backport_bug_fixes

[UNDERTOW-2280][UNDERTOW-2336][UNDERTOW-2339] CVE-2023-5379 CVE-2024-1459 CVE-2024-1635 Backport bug fixes

Author: fl4via

core/src/main/java/io/undertow/conduits/WriteTimeoutStreamSinkConduit.java

@@ -22,8 +22,8 @@
 import io.undertow.UndertowOptions;
 import io.undertow.server.OpenListener;
 import io.undertow.util.WorkerUtils;
-
 import org.xnio.Buffers;
+import org.xnio.ChannelListener;
 import org.xnio.ChannelListeners;
 import org.xnio.IoUtils;
 import org.xnio.Options;
@@ -47,7 +47,7 @@
  */
 public final class WriteTimeoutStreamSinkConduit extends AbstractStreamSinkConduit<StreamSinkConduit> {
 
-    private XnioExecutor.Key handle;
+    private volatile XnioExecutor.Key handle;
     private final StreamConnection connection;
     private volatile long expireTime = -1;
     private final OpenListener openListener;
@@ -82,6 +82,16 @@ public WriteTimeoutStreamSinkConduit(final StreamSinkConduit delegate, StreamCon
         super(delegate);
         this.connection = connection;
         this.openListener = openListener;
+        this.connection.getCloseSetter().set((ChannelListener<StreamConnection>) channel -> {
+            if (handle != null) {
+                synchronized (WriteTimeoutStreamSinkConduit.this) {
+                    if (handle != null) {
+                        handle.remove();
+                        handle = null;
+                    }
+                }
+            }
+        });
     }
 
     private void handleWriteTimeout(final long ret) throws IOException {
@@ -124,10 +134,14 @@ public long write(final ByteBuffer[] srcs, final int offset, final int length) t
     public int writeFinal(ByteBuffer src) throws IOException {
         int ret = super.writeFinal(src);
         handleWriteTimeout(ret);
-        if(!src.hasRemaining()) {
-            if(handle != null) {
-                handle.remove();
-                handle = null;
+        if (!src.hasRemaining()) {
+            if (handle != null) {
+                synchronized (this) {
+                    if (handle != null) {
+                        handle.remove();
+                        handle = null;
+                    }
+                }
             }
         }
         return ret;
@@ -137,10 +151,14 @@ public int writeFinal(ByteBuffer src) throws IOException {
     public long writeFinal(ByteBuffer[] srcs, int offset, int length) throws IOException {
         long ret = super.writeFinal(srcs, offset, length);
         handleWriteTimeout(ret);
-        if(!Buffers.hasRemaining(srcs)) {
-            if(handle != null) {
-                handle.remove();
-                handle = null;
+        if (!Buffers.hasRemaining(srcs)) {
+            if (handle != null) {
+                synchronized (this) {
+                    if (handle != null) {
+                        handle.remove();
+                        handle = null;
+                    }
+                }
             }
         }
         return ret;
@@ -200,19 +218,33 @@ private Integer getTimeout() {
 
     @Override
     public void terminateWrites() throws IOException {
-        super.terminateWrites();
-        if(handle != null) {
-            handle.remove();
-            handle = null;
+        try {
+            super.terminateWrites();
+        } finally {
+            if(handle != null) {
+                synchronized (this) {
+                    if (this.handle != null) {
+                        handle.remove();
+                        handle = null;
+                    }
+                }
+            }
         }
     }
 
     @Override
     public void truncateWrites() throws IOException {
-        super.truncateWrites();
-        if(handle != null) {
-            handle.remove();
-            handle = null;
+        try {
+            super.truncateWrites();
+        } finally {
+            if (handle != null) {
+                synchronized (this) {
+                    if (this.handle != null) {
+                        handle.remove();
+                        handle = null;
+                    }
+                }
+            }
         }
     }
 
@@ -233,8 +265,12 @@ public void suspendWrites() {
 
         XnioExecutor.Key handle = this.handle;
         if(handle != null) {
-            handle.remove();
-            this.handle = null;
+            synchronized (this) {
+                if (this.handle != null) {
+                    handle.remove();
+                    this.handle = null;
+                }
+            }
         }
     }
 
@@ -253,7 +289,11 @@ private void handleResumeTimeout() {
         expireTime = currentTime + timeout;
         XnioExecutor.Key key = handle;
         if (key == null) {
-            handle = connection.getIoThread().executeAfter(timeoutCommand, timeout, TimeUnit.MILLISECONDS);
+            synchronized (this) {
+                if (handle == null) {
+                    handle = connection.getIoThread().executeAfter(timeoutCommand, timeout, TimeUnit.MILLISECONDS);
+                }
+            }
         }
     }
 }

core/src/main/java/io/undertow/server/protocol/ajp/AjpReadListener.java

@@ -19,6 +19,7 @@
 package io.undertow.server.protocol.ajp;
 
 import io.undertow.UndertowLogger;
+import io.undertow.UndertowMessages;
 import io.undertow.UndertowOptions;
 import io.undertow.conduits.ConduitListener;
 import io.undertow.conduits.EmptyStreamSourceConduit;
@@ -165,8 +166,7 @@ public void handleEvent(final StreamSourceChannel channel) {
                 }
                 if (read > maxRequestSize) {
                     UndertowLogger.REQUEST_LOGGER.requestHeaderWasTooLarge(connection.getPeerAddress(), maxRequestSize);
-                    safeClose(connection);
-                    return;
+                    throw UndertowMessages.MESSAGES.badRequest();
                 }
             } while (!state.isComplete());
 

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

@@ -372,6 +372,10 @@ private void handleStateful(ByteBuffer buffer, ParseState currentState, HttpServ
     private static final int IN_PATH = 4;
     private static final int HOST_DONE = 5;
 
+    private static final int PATH_SEGMENT_START = 0;
+    private static final int PATH_DOT_SEGMENT = 1;
+    private static final int PATH_NON_DOT_SEGMENT = 2;
+
     /**
      * Parses a path value
      *
@@ -387,6 +391,8 @@ final void handlePath(ByteBuffer buffer, ParseState state, HttpServerExchange ex
         int canonicalPathStart = state.pos;
         boolean urlDecodeRequired = state.urlDecodeRequired;
 
+        int pathSubState = 0;
+
         while (buffer.hasRemaining()) {
             char next = (char) (buffer.get() & 0xFF);
             if(!allowUnescapedCharactersInUrl && !ALLOWED_TARGET_CHARACTER[next]) {
@@ -410,6 +416,11 @@ final void handlePath(ByteBuffer buffer, ParseState state, HttpServerExchange ex
                 state.urlDecodeRequired = urlDecodeRequired;
                 // store at canonical path the partial path parsed up until here
                 state.canonicalPath.append(stringBuilder.substring(canonicalPathStart));
+                if (parseState == IN_PATH && pathSubState == PATH_DOT_SEGMENT) {
+                    // Inside a dot-segment (".", ".."), we don't want to allow removal of the ';' character from
+                    // the path. This is to avoid path traversal issues - "/..;" should not be treated as "/..".
+                    state.canonicalPath.append(";");
+                }
                 state.stringBuilder.append(";");
                 // set position to end of path (possibly start of parameter name)
                 state.pos = state.stringBuilder.length();
@@ -443,6 +454,18 @@ final void handlePath(ByteBuffer buffer, ParseState state, HttpServerExchange ex
                 } else if (next == '/' && parseState != HOST_DONE) {
                     parseState = IN_PATH;
                 }
+
+                // This is helper state that tracks if the parser is currently in a path dot-segment (".", "..") or not.
+                if (parseState == IN_PATH) {
+                    if (next == '/') {
+                        pathSubState = PATH_SEGMENT_START;
+                    } else if (next == '.' && (pathSubState == PATH_SEGMENT_START || pathSubState == PATH_DOT_SEGMENT)) {
+                        pathSubState = PATH_DOT_SEGMENT;
+                    } else {
+                        pathSubState = PATH_NON_DOT_SEGMENT;
+                    }
+                }
+
                 stringBuilder.append(next);
             }
 

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

@@ -142,6 +142,7 @@ public void reset() {
         this.leftOver = 0;
         this.urlDecodeRequired = false;
         this.stringBuilder.setLength(0);
+        this.canonicalPath.setLength(0);
         this.nextHeader = null;
         this.nextQueryParam = null;
         this.mapCount = 0;

core/src/test/java/io/undertow/server/protocol/http/SimpleParserTestCase.java

@@ -676,6 +676,45 @@ public void testNonEncodedAsciiCharactersExplicitlyAllowed() throws UnsupportedE
         Assert.assertEquals("/bår", result.getRequestURI()); //not decoded
     }
 
+    @Test
+    public void testDirectoryTraversal() throws Exception {
+        byte[] in = "GET /path/..;/ HTTP/1.1\r\n\r\n".getBytes();
+        ParseState context = new ParseState(10);
+        HttpServerExchange result = new HttpServerExchange(null);
+        HttpRequestParser.instance(OptionMap.EMPTY).handle(ByteBuffer.wrap(in), context, result);
+        Assert.assertEquals("/path/..;/", result.getRequestURI());
+        Assert.assertEquals("/path/..;/", result.getRequestPath());
+        Assert.assertEquals("/path/..;/", result.getRelativePath());
+        Assert.assertEquals("", result.getQueryString());
+
+        in = "GET /path/../ HTTP/1.1\r\n\r\n".getBytes();
+        context = new ParseState(10);
+        result = new HttpServerExchange(null);
+        HttpRequestParser.instance(OptionMap.EMPTY).handle(ByteBuffer.wrap(in), context, result);
+        Assert.assertEquals("/path/../", result.getRequestURI());
+        Assert.assertEquals("/path/../", result.getRequestPath());
+        Assert.assertEquals("/path/../", result.getRelativePath());
+        Assert.assertEquals("", result.getQueryString());
+
+        in = "GET /path/..?/ HTTP/1.1\r\n\r\n".getBytes();
+        context = new ParseState(10);
+        result = new HttpServerExchange(null);
+        HttpRequestParser.instance(OptionMap.EMPTY).handle(ByteBuffer.wrap(in), context, result);
+        Assert.assertEquals("/path/..", result.getRequestURI());
+        Assert.assertEquals("/path/..", result.getRequestPath());
+        Assert.assertEquals("/path/..", result.getRelativePath());
+        Assert.assertEquals("/", result.getQueryString());
+
+        in = "GET /path/..~/ HTTP/1.1\r\n\r\n".getBytes();
+        context = new ParseState(10);
+        result = new HttpServerExchange(null);
+        HttpRequestParser.instance(OptionMap.EMPTY).handle(ByteBuffer.wrap(in), context, result);
+        Assert.assertEquals("/path/..~/", result.getRequestURI());
+        Assert.assertEquals("/path/..~/", result.getRequestPath());
+        Assert.assertEquals("/path/..~/", result.getRelativePath());
+        Assert.assertEquals("", result.getQueryString());
+    }
+
 
     private void runTest(final byte[] in) throws BadRequestException {
         runTest(in, "some value");