Repository: netty/netty
Issue: HTTP multipart request with filename containing ";" causes exception
Labels: defect

Netty version: 3.10.0.Final

When I send HTTP request something like:

```
POST / HTTP/1.1
Content-Type: multipart/form-data; boundary=dLV9Wyq26L_-JQxk6ferf-RT153LhOO
--dLV9Wyq26L_-JQxk6ferf-RT153LhOO
Content-Disposition: form-data; name="file"; filename="tmp;0.txt"
Content-Type: image/gif

asdf
--dLV9Wyq26L_-JQxk6ferf-RT153LhOO--
```

It causes the following stack trace.

```
java.lang.ArrayIndexOutOfBoundsException: 1
    at org.jboss.netty.handler.codec.http.multipart.HttpPostMultipartRequestDecoder.findMultipartDisposition(HttpPostMultipartRequestDecoder.java:540)
    at org.jboss.netty.handler.codec.http.multipart.HttpPostMultipartRequestDecoder.decodeMultipart(HttpPostMultipartRequestDecoder.java:345)
    at org.jboss.netty.handler.codec.http.multipart.HttpPostMultipartRequestDecoder.findMultipartDelimiter(HttpPostMultipartRequestDecoder.java:482)
    at org.jboss.netty.handler.codec.http.multipart.HttpPostMultipartRequestDecoder.decodeMultipart(HttpPostMultipartRequestDecoder.java:332)
    at org.jboss.netty.handler.codec.http.multipart.HttpPostMultipartRequestDecoder.parseBodyMultipart(HttpPostMultipartRequestDecoder.java:294)
    at org.jboss.netty.handler.codec.http.multipart.HttpPostMultipartRequestDecoder.parseBody(HttpPostMultipartRequestDecoder.java:265)
    at org.jboss.netty.handler.codec.http.multipart.HttpPostMultipartRequestDecoder.<init>(HttpPostMultipartRequestDecoder.java:167)
    at org.jboss.netty.handler.codec.http.multipart.HttpPostRequestDecoder.<init>(HttpPostRequestDecoder.java:80)
    at org.jboss.netty.handler.codec.http.multipart.HttpPostRequestDecoder.<init>(HttpPostRequestDecoder.java:56)
    ...
```

Test case (add to `HttpPostRequestDecoderTest`):

``` java
final String boundary = "dLV9Wyq26L_-JQxk6ferf-RT153LhOO";

final DefaultHttpRequest req = new DefaultHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.POST,
        "http://localhost");

req.headers().add(HttpHeaders.Names.CONTENT_TYPE, "multipart/form-data; boundary=" + boundary);

// Force to use memory-based data.
final DefaultHttpDataFactory inMemoryFactory = new DefaultHttpDataFactory(false);

final String data = "asdf";
final String filename = "tmp;0.txt";

final String body =
        "--" + boundary + "\r\n" +
                "Content-Disposition: form-data; name=\"file\"; filename=\"" + filename + "\"\r\n" +
                "Content-Type: image/gif\r\n" +
                "\r\n" +
                data + "\r\n" +
                "--" + boundary + "--\r\n";

req.setContent(ChannelBuffers.wrappedBuffer(body.getBytes(CharsetUtil.UTF_8.name())));
// Create decoder instance to test.
final HttpPostRequestDecoder decoder = new HttpPostRequestDecoder(inMemoryFactory, req);
assertFalse(decoder.getBodyHttpDatas().isEmpty());
assertEquals(filename, ((MemoryFileUpload) decoder.getBodyHttpDatas().get(0)).getFilename());
```

Although I only tested 3.x release, but it seems that also 4.x/5.0 have the same bug.


Issue comments:
@dittos I confirm.
The issue is in splitMultipartHeader where, once Content-Disposition is found, we split the data according to ';'. In this case, this is wrong.
Nothing prevents a filename to include a ";" (even if ';' is really not compatible with most of OS like Linux).
I do not have time right now to make a fix, but I feel like the splitMultipartHeader should be changed such that we split the additionnal arguments using ';' but including a check on ' " ' pairs in order to not check within this pair.
Something like:
- find first "
- find ; (possible multiple) between current position and first " and split (keeping only right side), updating current position
- loop on previous step as long as we found ;
- find next ", set new position to after it
- loop on first item until position is not the end

@dittos could you propose a fix and a test case associated as you did already ?


Pull request review comments:

Patch that resolved the issue:
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/multipart/HttpPostMultipartRequestDecoder.java b/codec-http/src/main/java/io/netty/handler/codec/http/multipart/HttpPostMultipartRequestDecoder.java
index 012cdb339cf..e242937ad0c 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/multipart/HttpPostMultipartRequestDecoder.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/multipart/HttpPostMultipartRequestDecoder.java
@@ -1784,7 +1784,7 @@ private static String[] splitMultipartHeader(String sb) {
         String svalue = sb.substring(valueStart, valueEnd);
         String[] values;
         if (svalue.indexOf(';') >= 0) {
-            values = StringUtil.split(svalue, ';');
+            values = splitMultipartHeaderValues(svalue);
         } else {
             values = StringUtil.split(svalue, ',');
         }
@@ -1797,4 +1797,38 @@ private static String[] splitMultipartHeader(String sb) {
         }
         return array;
     }
+
+    /**
+     * Split one header value in Multipart
+     * @return an array of String where values that were separated by ';' or ','
+     */
+    private static String[] splitMultipartHeaderValues(String svalue) {
+        ArrayList<String> values = new ArrayList<String>(1);
+        boolean inQuote = false;
+        boolean escapeNext = false;
+        int start = 0;
+        for (int i = 0; i < svalue.length(); i++) {
+            char c = svalue.charAt(i);
+            if (inQuote) {
+                if (escapeNext) {
+                    escapeNext = false;
+                } else {
+                    if (c == '\\') {
+                        escapeNext = true;
+                    } else if (c == '"') {
+                        inQuote = false;
+                    }
+                }
+            } else {
+                if (c == '"') {
+                    inQuote = true;
+                } else if (c == ';') {
+                    values.add(svalue.substring(start, i));
+                    start = i + 1;
+                }
+            }
+        }
+        values.add(svalue.substring(start));
+        return values.toArray(new String[values.size()]);
+    }
 }


Test patch:
diff --git a/codec-http/src/test/java/io/netty/handler/codec/http/multipart/HttpPostRequestDecoderTest.java b/codec-http/src/test/java/io/netty/handler/codec/http/multipart/HttpPostRequestDecoderTest.java
index a6514df2932..9aa46337b11 100644
--- a/codec-http/src/test/java/io/netty/handler/codec/http/multipart/HttpPostRequestDecoderTest.java
+++ b/codec-http/src/test/java/io/netty/handler/codec/http/multipart/HttpPostRequestDecoderTest.java
@@ -321,4 +321,30 @@ public void testChunkCorrect() throws Exception {
         decoder.offer(part3);
         decoder.offer(part4);
     }
+
+    // See https://github.com/netty/netty/issues/3326
+    @Test
+    public void testFilenameContainingSemicolon() throws Exception {
+        final String boundary = "dLV9Wyq26L_-JQxk6ferf-RT153LhOO";
+        final DefaultFullHttpRequest req = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.POST,
+                "http://localhost");
+        req.headers().add(HttpHeaders.Names.CONTENT_TYPE, "multipart/form-data; boundary=" + boundary);
+        // Force to use memory-based data.
+        final DefaultHttpDataFactory inMemoryFactory = new DefaultHttpDataFactory(false);
+        final String data = "asdf";
+        final String filename = "tmp;0.txt";
+        final String body =
+                "--" + boundary + "\r\n" +
+                        "Content-Disposition: form-data; name=\"file\"; filename=\"" + filename + "\"\r\n" +
+                        "Content-Type: image/gif\r\n" +
+                        "\r\n" +
+                        data + "\r\n" +
+                        "--" + boundary + "--\r\n";
+
+        req.content().writeBytes(body.getBytes(CharsetUtil.UTF_8.name()));
+        // Create decoder instance to test.
+        final HttpPostRequestDecoder decoder = new HttpPostRequestDecoder(inMemoryFactory, req);
+        assertFalse(decoder.getBodyHttpDatas().isEmpty());
+        decoder.destroy();
+    }
 }