From 88df48a8c35e2f791a9ab30bccaa5ce8647b29d7 Mon Sep 17 00:00:00 2001 From: Flavia Rainone Date: Thu, 8 Jun 2023 01:22:47 -0300 Subject: [PATCH 1/3] [UNDERTOW-2280] CVE-2023-5379 At AjpReadListener, do not close the connection if read is larger than maxRequestSize Signed-off-by: Flavia Rainone --- .../java/io/undertow/server/protocol/ajp/AjpReadListener.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/io/undertow/server/protocol/ajp/AjpReadListener.java b/core/src/main/java/io/undertow/server/protocol/ajp/AjpReadListener.java index 8f9c94abb0..a9631b3717 100644 --- a/core/src/main/java/io/undertow/server/protocol/ajp/AjpReadListener.java +++ b/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()); From 40bb3314f013247af8e222870bd5045ca8650c5c Mon Sep 17 00:00:00 2001 From: Tomas Hofman Date: Wed, 31 Jan 2024 10:56:37 +0100 Subject: [PATCH 2/3] [UNDERTOW-2339] CVE-2024-1459 Path segment "/..;" should not be treated as "/.." Proxies such as httpd proxy do not resolve the path segment "/..;/" to be a double dot segment, so they would pass such request path unchanged to target server. Undertow on the other hand resolves "/..;/" as double dot, which can cause essentially a path traversal problem, where client can request resources that should not be available to him per proxy configuration. Signed-off-by: Flavia Rainone --- .../protocol/http/HttpRequestParser.java | 23 +++++++++++ .../server/protocol/http/ParseState.java | 1 + .../protocol/http/SimpleParserTestCase.java | 39 +++++++++++++++++++ 3 files changed, 63 insertions(+) diff --git a/core/src/main/java/io/undertow/server/protocol/http/HttpRequestParser.java b/core/src/main/java/io/undertow/server/protocol/http/HttpRequestParser.java index 57113cd176..52df349b19 100644 --- a/core/src/main/java/io/undertow/server/protocol/http/HttpRequestParser.java +++ b/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); } diff --git a/core/src/main/java/io/undertow/server/protocol/http/ParseState.java b/core/src/main/java/io/undertow/server/protocol/http/ParseState.java index 454b374c55..60d937bcb2 100644 --- a/core/src/main/java/io/undertow/server/protocol/http/ParseState.java +++ b/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; diff --git a/core/src/test/java/io/undertow/server/protocol/http/SimpleParserTestCase.java b/core/src/test/java/io/undertow/server/protocol/http/SimpleParserTestCase.java index bca70bd68b..39bd7649ce 100644 --- a/core/src/test/java/io/undertow/server/protocol/http/SimpleParserTestCase.java +++ b/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"); From e824766df3b9d9614eff7700159e3aac5a35a1b8 Mon Sep 17 00:00:00 2001 From: Flavia Rainone Date: Mon, 8 Jan 2024 00:52:51 -0300 Subject: [PATCH 3/3] [UNDERTOW-2336] CVE-2024-1635 At WriteTimeoutStreamSinkConduit, add a close listener to guarantee that handle is removed if necessary. Also, synchronize when creating/removing the handle. Signed-off-by: Flavia Rainone --- .../WriteTimeoutStreamSinkConduit.java | 82 ++++++++++++++----- 1 file changed, 61 insertions(+), 21 deletions(-) diff --git a/core/src/main/java/io/undertow/conduits/WriteTimeoutStreamSinkConduit.java b/core/src/main/java/io/undertow/conduits/WriteTimeoutStreamSinkConduit.java index ce145c64bb..8398c8b43c 100644 --- a/core/src/main/java/io/undertow/conduits/WriteTimeoutStreamSinkConduit.java +++ b/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 { - 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) 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); + } + } } } }