[webkit-reviews] review denied: [Bug 66588] XSS filter bypass via non-standard URL encoding : [Attachment 106296] Patch and layout tests

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Daniel Bates <dbates at webkit.org>'s
request for review:
Bug 66588: XSS filter bypass via non-standard URL encoding
https://bugs.webkit.org/show_bug.cgi?id=66588

Attachment 106296: Patch and layout tests
https://bugs.webkit.org/attachment.cgi?id=106296&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=106296&action=review


This looks good.  Below are a bunch of minor comments.	I'd like to look at it
one more time to make sure we get the multi-byte unicode stuff right.

> Source/WebCore/html/parser/XSSAuditor.cpp:126
> +static inline String commonDecodeURLEscapeSequences(const String& string,
const TextEncoding& encoding)

decodeCommonURLEscapeSequences ?  As written, the name doesn't seem to read
quite right.  Maybe decodeStandardURLEscapSequences ?

> Source/WebCore/html/parser/XSSAuditor.cpp:142
> +    workingString = decode16BitUnicodeEscapeSequences(workingString);

Don't we need to do this inside the loop too?

> Source/WebCore/platform/text/DecodeEscapeSequences.h:46
> +	       && WTF::isASCIIHexDigit(string[start + 2]) &&
WTF::isASCIIHexDigit(string[start + 3])
> +	       && WTF::isASCIIHexDigit(string[start + 4]) &&
WTF::isASCIIHexDigit(string[start + 5]);

You probably don't need the WTF:: parts.

> Source/WebCore/platform/text/DecodeEscapeSequences.h:52
> +	   size_t numSequences = runLength / size;

numSequences => numberOfSequences

> Source/WebCore/platform/text/DecodeEscapeSequences.h:53
> +	   Vector<UChar, 512> buffer;

Given that we need to return a String anyway, we should just use StringBuilder
and reserveCapacity.  We can't avoid the malloc in any case.

> Source/WebCore/platform/text/DecodeEscapeSequences.h:57
> +	       *p++ = (toASCIIHexValue(run[2]) << 12) |
(toASCIIHexValue(run[3]) << 8) | (toASCIIHexValue(run[4]) << 4) |
toASCIIHexValue(run[5]);

Is this correct even with surrogates?  I would have expected you to do
something like what we do here:

http://trac.webkit.org/browser/trunk/Source/WebCore/html/parser/HTMLEntityParse
r.cpp#L73

> Source/WebCore/platform/text/DecodeEscapeSequences.h:76
> +	   DEFINE_STATIC_LOCAL(CharBuffer, buffer, ());

Why are we declaring it static?  That can cause problems if this code is run on
multiple threads or re-entered.  Just allocating it on the stack should be find
(and essentially free).

> Source/WebCore/platform/text/DecodeEscapeSequences.h:78
> +	   size_t numSequences = runLength / size;

numSequences => numberOfSequences

> Source/WebCore/platform/text/DecodeEscapeSequences.h:84
> +	   }

I'd add an ASSERT about buffer.size.

> Source/WebCore/platform/text/DecodeEscapeSequences.h:92
> +    Vector<UChar> result;

StringBuilder is probably better here.

> Source/WebCore/platform/text/DecodeEscapeSequences.h:93
> +    unsigned length = string.length();

size_t <-- amounts of memory are measured in size_t so they work properly on
64-bit machines.

> Source/WebCore/platform/text/DecodeEscapeSequences.h:95
> +    unsigned decodedPosition = 0;
> +    unsigned searchPosition = 0;

Same here.