[webkit-reviews] review canceled: [Bug 53205] Sketch out new XSS filter design (disabled by default) : [Attachment 80272] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Jan 27 10:20:47 PST 2011


Adam Barth <abarth at webkit.org> has canceled Adam Barth <abarth at webkit.org>'s
request for review:
Bug 53205: Sketch out new XSS filter design (disabled by default)
https://bugs.webkit.org/show_bug.cgi?id=53205

Attachment 80272: Patch
https://bugs.webkit.org/attachment.cgi?id=80272&action=review

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

>> Source/WebCore/WebCore.xcodeproj/project.pbxproj:3102
>> +		977E2E0D12F0FC9C00C13379 /* create-html-entity-table in
Resources */ = {isa = PBXBuildFile; fileRef = 977E2E0A12F0FC9C00C13379 /*
create-html-entity-table */; };
> 
> Did you intend for this change to be part of this patch?

Probably not.  :)

>> Source/WebCore/html/parser/XSSFilter.cpp:40
>> +namespace {
> 
> Out of curiosity, how did you come to the decision to use an anonymous
namespace as opposed to declaring all of the functions in this namespace as
static?

I always forget which one we're supposed to use.  All I remember is that
whichever one I pick, someone tells me I picked the wrong one.	):

>> Source/WebCore/html/parser/XSSFilter.cpp:45
>> +	int length = attribute.m_valueRange.m_end -
attribute.m_valueRange.m_start;
>> +	if (length > 7)
> 
> Where does the 7 come from? Notice, "http://" is seven characters in length.
Won't this lead to false positives?
> 
> Consider a web page search.php with <script
src="http://www.example.com/script.js"></script>. Suppose we request
search.php?q=http://www.apple.com. Then we will clear the value of the src
attribute of the <script> in decodeURL() since "http://" is a substring of
"http://www.apple.com"; => we won't load/execute the external script? This is a
false positive.
> 
> Moreover, this changes seems like it would also cause false positives in
other HTML tags, including <img> and <base>.

I just made up the number 7.  I can remove it from this patch, if you like. 
It's certainly putting the cart in front of the horse at this point.

w.r.t. your comments about false positives, my plan is to use more context than
we've used previously.	In your script example, the snippet we'll be looking
for it ' src="http://' (or however much of the attribute value we grab).  My
hope is that will help a lot to reduce false positives.  We can do this easily
in this design because we're integrated with the parser and we can get the
characters in the input for each token (and each part of the token).

>> Source/WebCore/html/parser/XSSFilter.cpp:52
>> +	if (name.size() < 5)
> 
> Where does the 5 come from? I take it that this was chosen with respect to
the shortest onX attribute, oncut. Hence, I would suggest we have a comment
that describes how we chose 5.

Yeah, 5 matches what IE does here too.	Definitely worth explaining!

>> Source/WebCore/html/parser/XSSFilter.cpp:91
>> +	HTMLToken::AttributeList::const_iterator iter =
token.attributes().begin()
> 
> You're missing a semicolon at the end of this line.

Wow, how does that even compile?

>> Source/WebCore/html/parser/XSSFilter.cpp:104
>> +	// FIXME: We should grab one character before the name also.
> 
> OK; If we consider at least one or more characters that precede the attribute
that will reduce the number of false positives with respect to matching the
substring "http://" (without quotes).

We can't take too many characters before the name because then we won't catch
attribute splitting:

<button id=$foo>

where $foo=" onclick=alert(1)"


More information about the webkit-reviews mailing list