[Webkit-unassigned] [Bug 68094] xssauditor - script block ending in comment can bypass auditor.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=68094





--- Comment #12 from Adam Barth <abarth at webkit.org>  2011-09-21 14:42:10 PST ---
(From update of attachment 108232)
View in context: https://bugs.webkit.org/attachment.cgi?id=108232&action=review

> Source/WebCore/html/parser/XSSAuditor.cpp:311
> +        int start = 0;
> +        int end = token.endIndex() - token.startIndex();

snippetForRange doesn't use the same indicies as token.startIndex?  I guess this is the same way the code used to work.  (Also, these indicies should probably all be size_t, but that's a yak for another day.)

> Source/WebCore/html/parser/XSSAuditor.cpp:313
> +        if (isContainedInRequest(fullyDecodeString(snippet, decoder))) {

I see, we're not requiring the two pieces to be contiguous in the request.  Is that intentional?   I guess the code fragment isn't necessarily at the beginning of the script.

> Source/WebCore/html/parser/XSSAuditor.cpp:542
> +String XSSAuditor::extractCodeFragment(const String& string)

If this is specific to JavaScript, maybe we should call it snippetForJavaScript ?

> Source/WebCore/html/parser/XSSAuditor.cpp:548
> +    size_t foundPosition;

I would initialize this scalar with notFound, but that's just because I'm paranoid.

> Source/WebCore/html/parser/XSSAuditor.cpp:561
> +    if (string.substring(startPosition, 4) == "<!--") {
> +        while (startPosition < endPosition && string[startPosition] != '\r' && string[startPosition] != '\n')
> +            startPosition++;
> +        while (startPosition < endPosition && isHTMLSpace(string[startPosition]))
> +            startPosition++;
> +    }

I'm not sure why this is a special case.  It seems like we could just fold this into the following loop.  <!-- is really the same thing as //.

> Source/WebCore/html/parser/XSSAuditor.cpp:566
> +        if (string.substring(startPosition, 2) == "//") {

Here too we're doing a malloc for each character we scan.  This might be worth adding to WTFString.

> Source/WebCore/html/parser/XSSAuditor.cpp:568
> +            while (startPosition < endPosition && string[startPosition] != '\r' && string[startPosition] != '\n')
> +                startPosition++;

Maybe have an inline function to test for isLinebreak ?

> Source/WebCore/html/parser/XSSAuditor.cpp:584
> +    foundPosition = startPosition + kMaximumFragmentLength;

So, kMaximumFragmentLength isn't really the maximum.  It's some sort of target for the maximum.

> Source/WebCore/html/parser/XSSAuditor.cpp:595
> +    // Stop at next comment.
> +    if ((foundPosition = string.find("//", startPosition, endPosition - startPosition)) != notFound)
> +        endPosition = foundPosition;
> +    if ((foundPosition = string.find("/*", startPosition, endPosition - startPosition)) != notFound)
> +        endPosition = foundPosition;

We should probably stop at the first <!-- too.  In JavaScript <!-- is just like //.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.