From 72a81593447ee774d4e265b0260d09956c0a8325 Mon Sep 17 00:00:00 2001 From: Trustin Lee Date: Fri, 30 Dec 2011 18:00:42 +0900 Subject: [PATCH] Issue #141: hashdos security vulnerability in QueryStringDecoder and possibl * Limited maximum number of parameters to 1024 by default and made the limitation configurable * QueryStringDecoder is now able to handle an HTTP POST content * Backported the improvements from master --- .../codec/http/QueryStringDecoder.java | 257 +++++++++++++++--- .../codec/http/QueryStringDecoderTest.java | 67 +++++ 2 files changed, 293 insertions(+), 31 deletions(-) diff --git a/src/main/java/org/jboss/netty/handler/codec/http/QueryStringDecoder.java b/src/main/java/org/jboss/netty/handler/codec/http/QueryStringDecoder.java index 8d21543e32..80805b05f0 100644 --- a/src/main/java/org/jboss/netty/handler/codec/http/QueryStringDecoder.java +++ b/src/main/java/org/jboss/netty/handler/codec/http/QueryStringDecoder.java @@ -15,27 +15,42 @@ */ package org.jboss.netty.handler.codec.http; -import java.io.UnsupportedEncodingException; import java.net.URI; import java.net.URLDecoder; import java.nio.charset.Charset; -import java.nio.charset.UnsupportedCharsetException; import java.util.ArrayList; import java.util.Collections; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; +import org.jboss.netty.util.CharsetUtil; + /** * Splits an HTTP query string into a path string and key-value parameter pairs. * This decoder is for one time use only. Create a new instance for each URI: * 
  * {@link QueryStringDecoder} decoder = new {@link QueryStringDecoder}("/hello?recipient=world&x=1;y=2");
  * assert decoder.getPath().equals("/hello");
- * assert decoder.getParameters().get("recipient").equals("world");
- * assert decoder.getParameters().get("x").equals("1");
- * assert decoder.getParameters().get("y").equals("2");
+ * assert decoder.getParameters().get("recipient").get(0).equals("world");
+ * assert decoder.getParameters().get("x").get(0).equals("1");
+ * assert decoder.getParameters().get("y").get(0).equals("2");
  *
+ * + * This decoder can also decode the content of an HTTP POST request whose + * content type is application/x-www-form-urlencoded: + * 
+ * {@link QueryStringDecoder} decoder = new {@link QueryStringDecoder}("recipient=world&x=1;y=2", false);
+ * ...
+ *
+ * + *

HashDOS vulnerability fix

+ * + * As a workaround to the HashDOS + * vulnerability, the decoder limits the maximum number of decoded key-value + * parameter pairs, up to {@literal 1024} by default, and you can configure it + * when you construct the decoder by passing an additional integer parameter. + * * @see QueryStringEncoder * * @apiviz.stereotype utility @@ -43,10 +58,15 @@ import java.util.Map; */ public class QueryStringDecoder { + private static final int DEFAULT_MAX_PARAMS = 1024; + private final Charset charset; private final String uri; + private final boolean hasPath; + private final int maxParams; private String path; private Map> params; + private int nParams; /** * Creates a new decoder that decodes the specified URI. The decoder will @@ -56,21 +76,51 @@ public class QueryStringDecoder { this(uri, HttpCodecUtil.DEFAULT_CHARSET); } + /** + * Creates a new decoder that decodes the specified URI encoded in the + * specified charset. + */ + public QueryStringDecoder(String uri, boolean hasPath) { + this(uri, HttpCodecUtil.DEFAULT_CHARSET, hasPath); + } + /** * Creates a new decoder that decodes the specified URI encoded in the * specified charset. */ public QueryStringDecoder(String uri, Charset charset) { + this(uri, charset, true); + } + + /** + * Creates a new decoder that decodes the specified URI encoded in the + * specified charset. + */ + public QueryStringDecoder(String uri, Charset charset, boolean hasPath) { + this(uri, charset, hasPath, DEFAULT_MAX_PARAMS); + } + + /** + * Creates a new decoder that decodes the specified URI encoded in the + * specified charset. + */ + public QueryStringDecoder(String uri, Charset charset, boolean hasPath, int maxParams) { if (uri == null) { throw new NullPointerException("uri"); } if (charset == null) { throw new NullPointerException("charset"); } + if (maxParams <= 0) { + throw new IllegalArgumentException( + "maxParams: " + maxParams + " (expected: a positive integer)"); + } // http://en.wikipedia.org/wiki/Query_string this.uri = uri.replace(';', '&'); this.charset = charset; + this.maxParams = maxParams; + this.hasPath = hasPath; } /** @@ -94,16 +144,30 @@ public class QueryStringDecoder { * specified charset. */ public QueryStringDecoder(URI uri, Charset charset){ + this(uri, charset, DEFAULT_MAX_PARAMS); + } + + /** + * Creates a new decoder that decodes the specified URI encoded in the + * specified charset. + */ + public QueryStringDecoder(URI uri, Charset charset, int maxParams) { if (uri == null) { throw new NullPointerException("uri"); } if (charset == null) { throw new NullPointerException("charset"); } + if (maxParams <= 0) { + throw new IllegalArgumentException( + "maxParams: " + maxParams + " (expected: a positive integer)"); + } // http://en.wikipedia.org/wiki/Query_string this.uri = uri.toASCIIString().replace(';', '&'); this.charset = charset; + this.maxParams = maxParams; + hasPath = false; } /** @@ -119,6 +183,10 @@ public class QueryStringDecoder { */ public String getPath() { if (path == null) { + if (!hasPath) { + return path = ""; + } + int pathEndPos = uri.indexOf('?'); if (pathEndPos < 0) { path = uri; @@ -135,17 +203,25 @@ public class QueryStringDecoder { */ public Map> getParameters() { if (params == null) { - int pathLength = getPath().length(); - if (uri.length() == pathLength) { - return Collections.emptyMap(); + if (hasPath) { + int pathLength = getPath().length(); + if (uri.length() == pathLength) { + return Collections.emptyMap(); + } + decodeParams(uri.substring(pathLength + 1)); + } else { + if (uri.isEmpty()) { + return Collections.emptyMap(); + } + decodeParams(uri); } - params = decodeParams(uri.substring(pathLength + 1)); } return params; } - private Map> decodeParams(String s) { - Map> params = new LinkedHashMap>(); + private void decodeParams(String s) { + Map> params = this.params = new LinkedHashMap>(); + nParams = 0; String name = null; int pos = 0; // Beginning of the unprocessed region int i; // End of the unprocessed region @@ -162,9 +238,13 @@ public class QueryStringDecoder { // We haven't seen an `=' so far but moved forward. // Must be a param of the form '&a&' so add it with // an empty value. - addParam(params, decodeComponent(s.substring(pos, i), charset), ""); + if (!addParam(params, decodeComponent(s.substring(pos, i), charset), "")) { + return; + } } else if (name != null) { - addParam(params, name, decodeComponent(s.substring(pos, i), charset)); + if (!addParam(params, name, decodeComponent(s.substring(pos, i), charset))) { + return; + } name = null; } pos = i + 1; @@ -173,35 +253,150 @@ public class QueryStringDecoder { if (pos != i) { // Are there characters we haven't dealt with? if (name == null) { // Yes and we haven't seen any `='. - addParam(params, decodeComponent(s.substring(pos, i), charset), ""); + if (!addParam(params, decodeComponent(s.substring(pos, i), charset), "")) { + return; + } } else { // Yes and this must be the last value. - addParam(params, name, decodeComponent(s.substring(pos, i), charset)); + if (!addParam(params, name, decodeComponent(s.substring(pos, i), charset))) { + return; + } } } else if (name != null) { // Have we seen a name without value? - addParam(params, name, ""); - } - - return params; - } - - private static String decodeComponent(String s, Charset charset) { - if (s == null) { - return ""; - } - - try { - return URLDecoder.decode(s, charset.name()); - } catch (UnsupportedEncodingException e) { - throw new UnsupportedCharsetException(charset.name()); + if (!addParam(params, name, "")) { + return; + } } } - private static void addParam(Map> params, String name, String value) { + private boolean addParam(Map> params, String name, String value) { + if (nParams >= maxParams) { + return false; + } + List values = params.get(name); if (values == null) { values = new ArrayList(1); // Often there's only 1 value. params.put(name, values); } values.add(value); + nParams ++; + return true; + } + + /** + * Decodes a bit of an URL encoded by a browser. + *

+ * This is equivalent to calling {@link #decodeComponent(String, Charset)} + * with the UTF-8 charset (recommended to comply with RFC 3986, Section 2). + * @param s The string to decode (can be empty). + * @return The decoded string, or {@code s} if there's nothing to decode. + * If the string to decode is {@code null}, returns an empty string. + * @throws IllegalArgumentException if the string contains a malformed + * escape sequence. + */ + public static String decodeComponent(final String s) { + return decodeComponent(s, HttpCodecUtil.DEFAULT_CHARSET); + } + + /** + * Decodes a bit of an URL encoded by a browser. + *

+ * The string is expected to be encoded as per RFC 3986, Section 2. + * This is the encoding used by JavaScript functions {@code encodeURI} + * and {@code encodeURIComponent}, but not {@code escape}. For example + * in this encoding, é (in Unicode {@code U+00E9} or in UTF-8 + * {@code 0xC3 0xA9}) is encoded as {@code %C3%A9} or {@code %c3%a9}. + *

+ * This is essentially equivalent to calling + * {@link URLDecoder#decode(String, String) URLDecoder.decode}(s, charset.name()) + * except that it's over 2x faster and generates less garbage for the GC. + * Actually this function doesn't allocate any memory if there's nothing + * to decode, the argument itself is returned. + * @param s The string to decode (can be empty). + * @param charset The charset to use to decode the string (should really + * be {@link CharsetUtil#UTF_8}. + * @return The decoded string, or {@code s} if there's nothing to decode. + * If the string to decode is {@code null}, returns an empty string. + * @throws IllegalArgumentException if the string contains a malformed + * escape sequence. + */ + @SuppressWarnings("fallthrough") + public static String decodeComponent(final String s, + final Charset charset) { + if (s == null) { + return ""; + } + final int size = s.length(); + boolean modified = false; + for (int i = 0; i < size; i++) { + final char c = s.charAt(i); + switch (c) { + case '%': + i++; // We can skip at least one char, e.g. `%%'. + // Fall through. + case '+': + modified = true; + break; + } + } + if (!modified) { + return s; + } + final byte[] buf = new byte[size]; + int pos = 0; // position in `buf'. + for (int i = 0; i < size; i++) { + char c = s.charAt(i); + switch (c) { + case '+': + buf[pos++] = ' '; // "+" -> " " + break; + case '%': + if (i == size - 1) { + throw new IllegalArgumentException("unterminated escape" + + " sequence at end of string: " + s); + } + c = s.charAt(++i); + if (c == '%') { + buf[pos++] = '%'; // "%%" -> "%" + break; + } else if (i == size - 1) { + throw new IllegalArgumentException("partial escape" + + " sequence at end of string: " + s); + } + c = decodeHexNibble(c); + final char c2 = decodeHexNibble(s.charAt(++i)); + if (c == Character.MAX_VALUE || c2 == Character.MAX_VALUE) { + throw new IllegalArgumentException( + "invalid escape sequence `%" + s.charAt(i - 1) + + s.charAt(i) + "' at index " + (i - 2) + + " of: " + s); + } + c = (char) (c * 16 + c2); + // Fall through. + default: + buf[pos++] = (byte) c; + break; + } + } + return new String(buf, 0, pos, charset); + } + + /** + * Helper to decode half of a hexadecimal number from a string. + * @param c The ASCII character of the hexadecimal number to decode. + * Must be in the range {@code [0-9a-fA-F]}. + * @return The hexadecimal value represented in the ASCII character + * given, or {@link Character#MAX_VALUE} if the character is invalid. + */ + private static char decodeHexNibble(final char c) { + if ('0' <= c && c <= '9') { + return (char) (c - '0'); + } else if ('a' <= c && c <= 'f') { + return (char) (c - 'a' + 10); + } else if ('A' <= c && c <= 'F') { + return (char) (c - 'A' + 10); + } else { + return Character.MAX_VALUE; + } } } diff --git a/src/test/java/org/jboss/netty/handler/codec/http/QueryStringDecoderTest.java b/src/test/java/org/jboss/netty/handler/codec/http/QueryStringDecoderTest.java index 44bae4932a..77536a4d2f 100644 --- a/src/test/java/org/jboss/netty/handler/codec/http/QueryStringDecoderTest.java +++ b/src/test/java/org/jboss/netty/handler/codec/http/QueryStringDecoderTest.java @@ -15,6 +15,9 @@ */ package org.jboss.netty.handler.codec.http; +import java.util.List; +import java.util.Map; + import org.jboss.netty.util.CharsetUtil; import org.junit.Assert; import org.junit.Test; @@ -93,6 +96,70 @@ public class QueryStringDecoderTest { assertQueryString("/foo?a=1&a=&a=", "/foo?a=1&a&a="); } + @Test + public void testHashDos() throws Exception { + StringBuilder buf = new StringBuilder(); + buf.append('?'); + for (int i = 0; i < 65536; i ++) { + buf.append('k'); + buf.append(i); + buf.append("=v"); + buf.append(i); + buf.append('&'); + } + Assert.assertEquals(1024, new QueryStringDecoder(buf.toString()).getParameters().size()); + } + + @Test + public void testHasPath() throws Exception { + QueryStringDecoder decoder = new QueryStringDecoder("1=2", false); + Assert.assertEquals("", decoder.getPath()); + Map> params = decoder.getParameters(); + Assert.assertEquals(1, params.size()); + Assert.assertTrue(params.containsKey("1")); + List param = params.get("1"); + Assert.assertNotNull(param); + Assert.assertEquals(1, param.size()); + Assert.assertEquals("2", param.get(0)); + } + + @Test + public void testUrlDecoding() throws Exception { + final String caffe = new String( + // "Caffé" but instead of putting the literal E-acute in the + // source file, we directly use the UTF-8 encoding so as to + // not rely on the platform's default encoding (not portable). + new byte[] {'C', 'a', 'f', 'f', (byte) 0xC3, (byte) 0xA9}, + "UTF-8"); + final String[] tests = { + // Encoded -> Decoded or error message substring + "", "", + "foo", "foo", + "f%%b", "f%b", + "f+o", "f o", + "f++", "f ", + "fo%", "unterminated escape sequence", + "%42", "B", + "%5f", "_", + "f%4", "partial escape sequence", + "%x2", "invalid escape sequence `%x2' at index 0 of: %x2", + "%4x", "invalid escape sequence `%4x' at index 0 of: %4x", + "Caff%C3%A9", caffe, + }; + for (int i = 0; i < tests.length; i += 2) { + final String encoded = tests[i]; + final String expected = tests[i + 1]; + try { + final String decoded = QueryStringDecoder.decodeComponent(encoded); + Assert.assertEquals(expected, decoded); + } catch (IllegalArgumentException e) { + Assert.assertTrue("String \"" + e.getMessage() + "\" does" + + " not contain \"" + expected + '"', + e.getMessage().contains(expected)); + } + } + } + private static void assertQueryString(String expected, String actual) { QueryStringDecoder ed = new QueryStringDecoder(expected, CharsetUtil.UTF_8); QueryStringDecoder ad = new QueryStringDecoder(actual, CharsetUtil.UTF_8);