Bug 27895 – [XSSAuditor] Inline Event Handler with single-line JavaScript comment can bypass XSSAuditor

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Daniel Bates]

An inline event handler that ends with a single-line JavaScript quote (i.e '//') can bypass the XSSAuditor.

[Daniel Bates]

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.

[Adam Barth]

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

[Daniel Bates]

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.

[Daniel Bates]

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.

[Daniel Bates]

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

[Daniel Bates]

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.

[Adam Barth]

Here's another test case for this issue:

<script>alert(1);/*<!--

[Daniel Bates]

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.

[Daniel Bates]

Comment on attachment 40145 [details]
Patch with test cases

Want to add a few more test cases

[Adam Barth]

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

[Daniel Bates]

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

[Adam Barth]

Did you mean to mark that last patch for review?

[Daniel Bates]

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.

[Adam Barth]

Created attachment 40683 [details]
Patch v1

[Adam Barth]

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?

[Daniel Bates]

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?

[Adam Barth]

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

Thanks.  :)

[Daniel Bates]

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

[Daniel Bates]

Created attachment 40929 [details]
Patch with test cases

Updated patch and modified test cases to pass all XSSAuditor tests.

[Daniel Bates]

Created attachment 41022 [details]
Patch with test cases

[Adam Barth]

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©*/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...

[Daniel Bates]

Created attachment 41206 [details]
Patch with test cases

[Adam Barth]

Comment on attachment 41206 [details]
Patch with test cases

Ok.  Let's see if this works!

[Daniel Bates]

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

[Daniel Bates]

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

[Daniel Bates]

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

[Adam Barth]

Comment on attachment 41206 [details]
Patch with test cases

This patch turned out to be a bad idea.

[Adam Barth]

Created attachment 41835 [details]
Patch v1

[Adam Barth]

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.

[Adam Barth]

Created attachment 41836 [details]
Patch v1

[Daniel Bates]

Created attachment 42732 [details]
Patch with test cases

Updated patch to reflect changes made in bug #31098.

[Eric Seidel (no email)]

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

[Daniel Bates]

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.

[Adam Barth]

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

[David Kilzer (:ddkilzer)]

<rdar://problem/8272897>

[Thomas Sepez]

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.

[Thomas Sepez]

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

[WebKit Review Bot]

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.

[Thomas Sepez]

Created attachment 107111 [details]
Patch with style fix.

[Adam Barth]

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.

[Adam Barth]

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.

[Thomas Sepez]

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.

[Adam Barth]

(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

[Adam Barth]

(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 =

[Thomas Sepez]

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.

[Adam Barth]

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

[Thomas Sepez]

Created attachment 107229 [details]
Patch simplified.

Still testting locally.

[Adam Barth]

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.

[Thomas Sepez]

Comment on attachment 107229 [details]
Patch simplified.

Ok.  Please review.

[Adam Barth]

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?

[Thomas Sepez]

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.

[Adam Barth]

Comment on attachment 107229 [details]
Patch simplified.

Sounds good to me.

[Thomas Sepez]

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.

[WebKit Review Bot]

Comment on attachment 107246 [details]
Patch removing max snippet len truncation

Clearing flags on attachment: 107246

Committed r95065: <http://trac.webkit.org/changeset/95065>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Thomas Sepez]

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.

[Thomas Sepez]

Created attachment 107387 [details]
Revised tests

[Adam Barth]

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.

[Thomas Sepez]

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

[Adam Barth]

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

[Adam Barth]

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

[WebKit Review Bot]

Comment on attachment 107387 [details]
Revised tests

Clearing flags on attachment: 107387

Committed r95161: <http://trac.webkit.org/changeset/95161>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.