Bug 27895

Created attachment 33902 [details] Proposed path and test This is a proposed implementation. If we go with it, we may want to split this into two separate bugs, one for the addition of the MappedAttributeWithRawCharacters class and one for the XSSAuditor fix.

Hum... The static_casts (which are really manual dynamic_casts) make me sad. I think it comes down to what MappedAttribute represents. Can you try working up a patch that doesn't used MappedAttribute so we can compare? IE8 just blocks attributes that start with "on" and are at least 5 characters. Also, you should explain where we came up with the magic constant substring(0, 7).

Created attachment 34165 [details] Proposed Patch 1 with HTML Base href fix This is an updated version of the original proposed patch with base href fix. I'll file this under a separate bug, but for comparison with the alternative patch that will follow, I have left this fix in for now. Also, the change logs in this patch do not reflect the addition of the base href fix.

Created attachment 34166 [details] Alternative patch - as per Adam's suggestion An alternative patch for this issue as suggested by Adam. Includes Base href fix.

Let "Proposed Patch 1" be called Patch 1, and "Alternative patch" be called Patch 2. I am tending to favor the Patch 1, because it has the advantage of fixing the issue without clobbering the value of the inline event handler or base href. Thus, a JavaScript DOM traversal matches the structure seen when you View Source, even though the inline event handler and base href are ignored. Additionally, it does not produce any false positives compared to Patch 2 (though, from a conversation with Adam, we can probably get away with it since such false positives are detected as part of an injection attempt). Examples: 1) Inline Event Handler http://good.webblaze.org/dbates/xsstest-printDOM.php?q=%3Cimg%20src=%22about:blank%22%20onerror=%22alert(/XSS/)%22/%3E 2) Base Href http://good.webblaze.org/dbates/xsstest-printDOM-basehref.php?q=<base href="http://evil.webblaze.org/dbates/base-href/"> Notice, either patch prevents the execution of the above, but Patch 1 does not nullify neither the onerror nor href attributes so the the DOM traversal matches the View Source structure. False positive for Patch 2: http://good.webblaze.org/dbates/xsstest.php?q=<img src="about:blank" ondummy="dummy"/> Because Patch 2 only looks for attributes that start with "on" and whose length is >= 5 characters (*), this is detected as an attack, even though ondummy is not a valid attribute Difference between "Proposed Patch 1" (Patch 1) and "Alternative patch" (Patch 2): Patch 1 defines a subclass of MappedAttribute, called MappedAttributeWithRawCharacters (I'm open to name suggestions) that stores the value of |m_rawAttributeBeforeValue| extracted in the HTMLTokenizer. The HTMLTokenizer creates attributes of class MappedAttributeWithRawCharacter. So, such attributes are passed along through the WebKit framework. If the attribute A corresponds to an inline event handler and A->isMappedAttributeWithRawCharacters() is true then in ScriptEventListener we cast it to MappedAttributeWithRawCharacter and pass the stored | m_rawAttributeBeforeValue| to the XSSAuditor. Similarly, we do the same in HTMLBaseElement. Patch 2 is implemented in the HTMLTokenizer. It detects attributes described by (*) or detects the href attribute of a <base> tag. It then passes this information to the XSSAuditor. If the XSSAuditor methods return false then the corresponding attribute is set to the empty string (hence the clobbered result).

Oh, forgot to mention. An alternative implementation of Patch 1, to get rid of the static_cast Adam brought up, is to add the method rawAttributeBeforeValue() directly to the MappedAttribute class.

Here's another test case for this issue: <script>alert(1);/*<!--

Created attachment 40145 [details] Patch with test cases I went with a modified Proposed Patch 1. Instead of defining a new class MappedAttributeWithRawCharacters and using static_casts, I moved the functionality I needed into Attribute.h (i.e. Attribute:rawAttributeBeforeValue() and Attribute::setRawAttributeBeforeValue(const String& string)). Hence, there are no static_casts in this patch.

Comment on attachment 40145 [details] Patch with test cases Want to add a few more test cases

We might want to make the " ' < > change before landing this one.

Created attachment 40154 [details] Patch with test cases Added an additional test case img-onerror-HTML-comment.html. We may want to file a separate bug to fix comments within script tags such as: <script>alert(1);/*<!-- Example: http://good.webblaze.org/dbates/xsstest-script-comment.php?q=%3Cscript%3Ealert%28/XSS/%29%3B/*%3C%21-- <script>alert(1);/* Example: http://good.webblaze.org/dbates/xsstest-script-comment.php?q=%3Cscript%3Ealert%28/XSS/%29%3B/* <script>alert(1);// Example: http://good.webblaze.org/dbates/xsstest-script-comment.php?q=%3Cscript%3Ealert%28/XSS/%29%3B// <script>alert(1);<!-- Example: http://good.webblaze.org/dbates/xsstest-script-comment.php?q=%3Cscript%3Ealert%28/XSS/%29%3B%3C%21--

Did you mean to mark that last patch for review?

Comment on attachment 40154 [details] Patch with test cases Marked for review. Will need to file a separate bug to fix comments in <script> tags.

Created attachment 40683 [details] Patch v1

Dan, here's the code we wrote during the meeting. It seems to cause some of the other XSS auditor tests to fail. Would you be willing to look into it and see what we did wrong?

Sure. I'll take a look. (In reply to comment #15) > Dan, here's the code we wrote during the meeting. It seems to cause some of > the other XSS auditor tests to fail. Would you be willing to look into it and > see what we did wrong?

(In reply to comment #16) > Sure. I'll take a look. Thanks. :)

We should fix bugs #30241 and #30242 because this patch does not address the following single-line JavaScript comment attack: http://good.webblaze.org/dbates/xsstest.php?q=%3Ciframe%20src=%22javascript:%20//%250Aalert(/XSS/)%22%3E%3C/iframe%3E

Created attachment 40929 [details] Patch with test cases Updated patch and modified test cases to pass all XSSAuditor tests.

Created attachment 41022 [details] Patch with test cases

Comment on attachment 41022 [details] Patch with test cases I think there is a subtle bug with this patch. Imagine that an HTML entity is straddling the 7th character: /*xx&copy;*/alert(/xss); When we substring, we'll get /*xx&co and the entity won't decode and the string won't match. We need to canonicalize() first before calling substring...

Created attachment 41206 [details] Patch with test cases

Comment on attachment 41206 [details] Patch with test cases Ok. Let's see if this works!

Committed r49644: <http://trac.webkit.org/changeset/49644>

Committed r49668: <http://trac.webkit.org/changeset/49668>

Re-opening this bug, due to bug #30418. We need to think about this patch some more.

Comment on attachment 41206 [details] Patch with test cases This patch turned out to be a bad idea.

Created attachment 41835 [details] Patch v1

Here's a more conservative version of this patch. Instead of doing all the APIs, the patch changes only inline script and inline event handlers. I think we should try this first. If it's successful, we can move on to the URL-based APIs.

Created attachment 41836 [details] Patch v1

Created attachment 42732 [details] Patch with test cases Updated patch to reflect changes made in bug #31098.

Adam, can you review Dan's patch? Or suggest someone who can? (We're up to 2 weeks on this one...)

Comment on attachment 42732 [details] Patch with test cases Clearing review flag for now. Will extract structural changes into separate bug. Then make a smaller patch to correct this bug.

*** Bug 43502 has been marked as a duplicate of this bug. ***

<rdar://problem/8272897>

It sounds like the plan is to first fix the remaining inline issues and then move on to script blocks. Makes sense, since I think that we will have to treat script blocks and attributes differently. For the inline case: 1. Trailing comment: q="onerror="alert(0)// 2. Trailing quote: q="onerror=alert(0)-" 3. Case 1, using entities: q="onerror=alert(0)%26%23x2f/ 4. Leading comment with encoded newline: q="onerror="//%0a alert(0)" An approach to this that Adam Barth and I have been talking about is to truncate the page snippet after the first //, entity, or quote/tick character (but the later only when the value is not introduced using quotes/ticks). Legitimate uses of these forms should be rare.

Created attachment 107107 [details] Changes to truncate match as indicated.

Attachment 107107 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http..." exit_code: 1 Source/WebCore/html/parser/XSSAuditor.cpp:473: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] Source/WebCore/html/parser/XSSAuditor.cpp:474: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] Source/WebCore/html/parser/XSSAuditor.cpp:482: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] Source/WebCore/html/parser/XSSAuditor.cpp:489: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] Total errors found: 4 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 107111 [details] Patch with style fix.

Comment on attachment 107107 [details] Changes to truncate match as indicated. View in context: https://bugs.webkit.org/attachment.cgi?id=107107&action=review > Source/WebCore/html/parser/XSSAuditor.cpp:479 > + String snippet(snippetForRange(token, start, end)); We usually use the assignment form of the constructor. > Source/WebCore/html/parser/XSSAuditor.cpp:493 > + size_t endPosition = snippet.length() <kMaximumSnippetLength ? snippet.length() : kMaximumSnippetLength; why not just call std::min ? (Also < should have a space after it.) > Source/WebCore/html/parser/XSSAuditor.cpp:505 > + if (snippet[startPosition] != '"' && snippet[startPosition] != '\'') { > + if ((foundPosition = snippet.find("\"", startPosition, endPosition - startPosition)) != notFound) > + endPosition = foundPosition + 1; > + if ((foundPosition = snippet.find("'", startPosition, endPosition - startPosition)) != notFound) > + endPosition = foundPosition + 1; > + } It's not clear why we need to do this work. Doesn't the HTML parser already do this work for us? The source snippet for an attribute won't even extend past the end of the attribute. > Source/WebCore/html/parser/XSSAuditor.cpp:515 > + if (isASCIIAlpha(snippet[foundPosition]) || snippet[foundPosition] == '#') { > + endPosition = foundPosition; > + break; > + } This seems fragile. What if we introduce a new kind of HTML entity? Probably best to bail out at the first & regardless of what follows. We can always make this more elaborate if it triggers too many false positives.

I should say that this approach looks very promising. I think you've gone a little bit overboard on complexity. I'm inclined to try something simple first and add complexity as needed.

Comment on attachment 107107 [details] Changes to truncate match as indicated. View in context: https://bugs.webkit.org/attachment.cgi?id=107107&action=review >> Source/WebCore/html/parser/XSSAuditor.cpp:479 >> + String snippet(snippetForRange(token, start, end)); > > We usually use the assignment form of the constructor. Done. >> Source/WebCore/html/parser/XSSAuditor.cpp:493 >> + size_t endPosition = snippet.length() <kMaximumSnippetLength ? snippet.length() : kMaximumSnippetLength; > > why not just call std::min ? (Also < should have a space after it.) Done. >> Source/WebCore/html/parser/XSSAuditor.cpp:505 >> + } > > It's not clear why we need to do this work. Doesn't the HTML parser already do this work for us? The source snippet for an attribute won't even extend past the end of the attribute. Not quite. This is for the special case where we have |value=attribute-"foo |, in other words a quote in the middle of an attribute which doesn't introduce a quoted region in the ordinary way. >> Source/WebCore/html/parser/XSSAuditor.cpp:515 >> + } > > This seems fragile. What if we introduce a new kind of HTML entity? Probably best to bail out at the first & regardless of what follows. We can always make this more elaborate if it triggers too many false positives. Sure. Can make it simpler at the risk of more false positives.

(In reply to comment #42) > (From update of attachment 107107 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=107107&action=review > > >> Source/WebCore/html/parser/XSSAuditor.cpp:505 > >> + } > > > > It's not clear why we need to do this work. Doesn't the HTML parser already do this work for us? The source snippet for an attribute won't even extend past the end of the attribute. > > Not quite. This is for the special case where we have |value=attribute-"foo |, in other words a quote in the middle of an attribute which doesn't introduce a quoted region in the ordinary way. I'm not sure I quite follow. What wrong with the "stop after the first // or & after the first =" rule? Adam

(In reply to comment #43) > (In reply to comment #42) > > (From update of attachment 107107 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=107107&action=review > > > > >> Source/WebCore/html/parser/XSSAuditor.cpp:505 > > >> + } > > > > > > It's not clear why we need to do this work. Doesn't the HTML parser already do this work for us? The source snippet for an attribute won't even extend past the end of the attribute. > > > > Not quite. This is for the special case where we have |value=attribute-"foo |, in other words a quote in the middle of an attribute which doesn't introduce a quoted region in the ordinary way. > > I'm not sure I quite follow. What wrong with the "" rule? Ok. That was hard to read. What wrong with this rule: Stop after the first // or & after the first =

Comment on attachment 107107 [details] Changes to truncate match as indicated. View in context: https://bugs.webkit.org/attachment.cgi?id=107107&action=review >>>>> Source/WebCore/html/parser/XSSAuditor.cpp:505 >>>>> + } >>>> >>>> It's not clear why we need to do this work. Doesn't the HTML parser already do this work for us? The source snippet for an attribute won't even extend past the end of the attribute. >>> >>> Not quite. This is for the special case where we have |value=attribute-"foo |, in other words a quote in the middle of an attribute which doesn't introduce a quoted region in the ordinary way. >> >> I'm not sure I quite follow. What wrong with the "stop after the first // or & after the first =" rule? >> >> Adam > > Ok. That was hard to read. What wrong with this rule: > > Stop after the first // or & after the first = As in a snippet |onload="alert(0)"| gets turned into |onload=| ? Not sure that was what you meant, but if so, then there's a big risk of false positives.

> > Ok. That was hard to read. What wrong with this rule: > > > > Stop after the first // or & after the first = > > As in a snippet |onload="alert(0)"| gets turned into |onload=| ? Not sure that was what you meant, but if so, then there's a big risk of false positives. If there are no // & or = characters, we can just stop at the end of the snippet.

Created attachment 107229 [details] Patch simplified. Still testting locally.

Comment on attachment 107229 [details] Patch simplified. View in context: https://bugs.webkit.org/attachment.cgi?id=107229&action=review This is turning out very nicely. Let me know when you're ready for a full review. > Source/WebCore/html/parser/XSSAuditor.cpp:81 > +static bool isHTMLQuote(UChar c) > +{ > + return (c == '"' || c == '\''); > +} Should this go in the same place as isHTMLSpace ? > Source/WebCore/html/parser/XSSAuditor.cpp:511 > + && (position = snippet.find(isNotHTMLSpace, position + 1)) != notFound Does find range check? position + 1 could be out of bounds.

Comment on attachment 107229 [details] Patch simplified. Ok. Please review.

Comment on attachment 107229 [details] Patch simplified. View in context: https://bugs.webkit.org/attachment.cgi?id=107229&action=review This looks like a great start. We might need to iterate on the ordering between the truncation and the canonicalization, but we can do that in a separate patch. I'd like to hear your answers to the questions in my last review before landing. > Source/WebCore/html/parser/XSSAuditor.cpp:492 > + // Limit the length of the fragment to avoid comparing very long strings. > + snippet.truncate(kMaximumSnippetLength); Will this cause a problem if we truncate in the middle of a %-escape sequence of a UTF-16 surrogate? I'm worried that the canonicalization step will end up with a different result. Maybe we should canonicalize before trimming the snippet?

Comment on attachment 107229 [details] Patch simplified. View in context: https://bugs.webkit.org/attachment.cgi?id=107229&action=review >> Source/WebCore/html/parser/XSSAuditor.cpp:81 >> +} > > Should this go in the same place as isHTMLSpace ? We could, that would be the parseridioms file, though the function won't be called elsewhere in the parser itself. Your call. >> Source/WebCore/html/parser/XSSAuditor.cpp:492 >> + snippet.truncate(kMaximumSnippetLength); > > Will this cause a problem if we truncate in the middle of a %-escape sequence of a UTF-16 surrogate? I'm worried that the canonicalization step will end up with a different result. Maybe we should canonicalize before trimming the snippet? I had convinced myself that this had to occur before canonicalization. I guess I was thinking a literal "%26" in the page itself isn't an entity and shouldn't be treated as such. We're going to toss out any surrogates during the high check during canonicalization, so I don't think this matters if we spilt it. We'll just toss out the first half in one case and both in the other. >> Source/WebCore/html/parser/XSSAuditor.cpp:511 >> + && (position = snippet.find(isNotHTMLSpace, position + 1)) != notFound > > Does find range check? position + 1 could be out of bounds. Yep, and I'd assumed that find() would do this for me. WTF::find() in WTFStrings.h does a "while (index < length)" before making any access, where length got populated by the actual string length from the string impl.

Comment on attachment 107229 [details] Patch simplified. Sounds good to me.

Created attachment 107246 [details] Patch removing max snippet len truncation Yes, the %xx case is a problem if it straddles the kMaximumSnippetLength boundary. Need not be a surrogate pair, even a simple %2f (for example) if it looses the final f won't parse, and the snippet will contain a % that the url won't after it decodes. So I pulled the initial truncation.

Comment on attachment 107246 [details] Patch removing max snippet len truncation Clearing flags on attachment: 107246 Committed r95065: <http://trac.webkit.org/changeset/95065>

All reviewed patches have been landed. Closing bug.

Re-opening to add patch for revised tests. If the xssauditor is stopping on the first [/'"&], then tests of the form alert(/XSS/)<some hairy hack goes here to trick auditor> are always going to pass because of the / in /XSS/ truncating the snippet even when <some hairy hack> is viable.

Created attachment 107387 [details] Revised tests

Comment on attachment 107387 [details] Revised tests Ok. In the future, please open new bugs for these sorts of things instead of re-opening the original bug. We can always cross-link them.

More I think about this, right fix seems to be to detect // properly and leave tests alone. Thoughts, Adam?

> More I think about this, right fix seems to be to detect // properly and leave tests alone. Thoughts, Adam? I liked detecting / alone because that covers the URL case as well as the /* case. It's fine to change these tests. We're just getting better test coverage.

If we run into too many false positives, we can re-evaluate.

Comment on attachment 107387 [details] Revised tests Clearing flags on attachment: 107387 Committed r95161: <http://trac.webkit.org/changeset/95161>

All reviewed patches have been landed. Closing bug.