[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 <frainone@redhat.com>

Author: TomasHofman

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

@@ -388,6 +388,10 @@ private void sanitazeListTypeHeaders(final HttpServerExchange builder) {
     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
      *
@@ -403,6 +407,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]) {
@@ -426,6 +432,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();
@@ -459,6 +470,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");