Skip to content

Commit

Permalink
Merge pull request #1559 from fl4via/2.2.x_backport_bug_fixes
Browse files Browse the repository at this point in the history 
[UNDERTOW-2280][UNDERTOW-2336][UNDERTOW-2339] CVE-2023-5379 CVE-2024-1459 CVE-2024-1635 Backport bug fixes
  • Loading branch information
@fl4via
fl4via authored Mar 2, 2024
2 parents 883bd50 + e824766 commit 3cdb104
Show file tree
Hide file tree
Showing 5 changed files with 126 additions and 23 deletions.
82 changes: 61 additions & 21 deletions core/src/main/java/io/undertow/conduits/WriteTimeoutStreamSinkConduit.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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;
}
}
}
}
}

Expand All @@ -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;
}
}
}
}

Expand All @@ -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);
}
}
}
}
}
4 changes: 2 additions & 2 deletions core/src/main/java/io/undertow/server/protocol/ajp/AjpReadListener.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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());

Expand Down
23 changes: 23 additions & 0 deletions core/src/main/java/io/undertow/server/protocol/http/HttpRequestParser.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
*
Expand All @@ -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]) {
Expand All @@ -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();
Expand Down Expand Up @@ -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);
}

Expand Down
1 change: 1 addition & 0 deletions core/src/main/java/io/undertow/server/protocol/http/ParseState.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
39 changes: 39 additions & 0 deletions core/src/test/java/io/undertow/server/protocol/http/SimpleParserTestCase.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down

0 comments on commit 3cdb104

Please sign in to comment.