Bug 53205 – Sketch out new XSS filter design (disabled by default)

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adam Barth]

Sketch out new XSS filter design (disabled by default)

[Adam Barth]

Created attachment 80272 [details]
Patch

[Daniel Bates]

Comment on attachment 80272 [details]
Patch

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?

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:9493
> +		977E2E0A12F0FC9C00C13379 /* create-html-entity-table */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text; name = "create-html-entity-table"; path = "parser/create-html-entity-table"; sourceTree = "<group>"; };

Ditto.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:15930
> +				977E2E0A12F0FC9C00C13379 /* create-html-entity-table */,

Ditto.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:22579
> +				977E2E0D12F0FC9C00C13379 /* create-html-entity-table in Resources */,

Ditto.

> Source/WebCore/html/parser/XSSFilter.cpp:2
> + * Copyright (C) 2010 Adam Barth. All Rights Reserved.

Nit: 2010 => 2011.

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

> Source/WebCore/html/parser/XSSFilter.cpp:45
> +    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>.

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

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

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

> Source/WebCore/html/parser/XSSFilter.h:2
> + * Copyright (C) 2010 Adam Barth. All Rights Reserved.

Nit: 2010 => 2011.

[Adam Barth]

Comment on attachment 80272 [details]
Patch

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)"

[Adam Barth]

Created attachment 80364 [details]
Patch

[Daniel Bates]

Comment on attachment 80364 [details]
Patch

Thanks for making the changes. This looks good to me.
r=me

[Adam Barth]

Comment on attachment 80364 [details]
Patch

Thanks Dan!

[WebKit Commit Bot]

Comment on attachment 80364 [details]
Patch

Rejecting attachment 80364 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-8', 'apply-..." exit_code: 2

Last 500 characters of output:
eeded at 15969 (offset 2 lines).
Hunk #4 succeeded at 22454 with fuzz 1 (offset 4 lines).
Hunk #5 succeeded at 25151 (offset 4 lines).
patching file Source/WebCore/html/parser/HTMLDocumentParser.cpp
patching file Source/WebCore/html/parser/HTMLDocumentParser.h
patching file Source/WebCore/html/parser/XSSFilter.cpp
patching file Source/WebCore/html/parser/XSSFilter.h

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Daniel Bates', u'--for..." exit_code: 1

Full output: http://queues.webkit.org/results/7606385

[Adam Barth]

Created attachment 80483 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 80483 [details]
Patch for landing

Rejecting attachment 80483 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-4', 'apply-..." exit_code: 2

Last 500 characters of output:
504.
1 out of 1 hunk FAILED -- saving rejects to file Source/WebCore/WebCore.vcproj/WebCore.vcproj.rej
patching file Source/WebCore/WebCore.xcodeproj/project.pbxproj
patching file Source/WebCore/html/parser/HTMLDocumentParser.cpp
patching file Source/WebCore/html/parser/HTMLDocumentParser.h
patching file Source/WebCore/html/parser/XSSFilter.cpp
patching file Source/WebCore/html/parser/XSSFilter.h

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/7651074

[Adam Barth]

Committed r76980: <http://trac.webkit.org/changeset/76980>

[WebKit Review Bot]

http://trac.webkit.org/changeset/76980 might have broken Leopard Intel Release (Tests)