[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 #14 from Thomas Sepez <tsepez at chromium.org>  2011-09-21 14:57:13 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 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.)

Will leave as is.

>> 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.

Yes, we need to deal with an injection like <script>// comment to trick you %0a alert(0) </script> now.  We won't include the comment in the snippet we extract.

>> 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 ?

Sure.  Will rename.

>> 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.

Sure.  would like an uninit reference to lead to string[notFound] taking an exception.

>> Source/WebCore/html/parser/XSSAuditor.cpp:561
>> +    }
> 
> 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 //.

Ok.  My recollection of the spec was that the first instance of <!-- was a comment, but that subsequent ones were <!-- , as in if (a<!--b) {}  but I may be wrong here.

>> 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.

Or just  sp + 1 < ep && string[sp] == '/' && string[sp+1] == '/'

>> Source/WebCore/html/parser/XSSAuditor.cpp:568
>> +                startPosition++;
> 
> Maybe have an inline function to test for isLinebreak ?

Sure.

>> 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.

Ok, I'll call it kMaximumFragmentLengthTarget.

>> Source/WebCore/html/parser/XSSAuditor.cpp:595
>> +        endPosition = foundPosition;
> 
> We should probably stop at the first <!-- too.  In JavaScript <!-- is just like //.

Yes, see above.

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