Bug 27895 - [XSSAuditor] Inline Event Handler with single-line JavaScript comment can bypass XSSAuditor
Summary: [XSSAuditor] Inline Event Handler with single-line JavaScript comment can byp...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Daniel Bates
URL: http://good.webblaze.org/dbates/xsste...
Keywords: InRadar, XSSAuditor
: 43502 (view as bug list)
Depends on:
Blocks: 66579
  Show dependency treegraph
 
Reported: 2009-07-31 13:56 PDT by Daniel Bates
Modified: 2011-09-14 21:24 PDT (History)
6 users (show)

See Also:


Attachments
Proposed path and test (26.98 KB, patch)
2009-07-31 14:03 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Proposed Patch 1 with HTML Base href fix (32.46 KB, patch)
2009-08-05 13:18 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Alternative patch - as per Adam's suggestion (14.61 KB, patch)
2009-08-05 13:19 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch with test cases (19.42 KB, patch)
2009-09-25 14:32 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch with test cases (20.84 KB, patch)
2009-09-25 17:04 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch v1 (16.19 KB, patch)
2009-10-05 21:19 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch with test cases (17.71 KB, patch)
2009-10-08 22:32 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch with test cases (17.67 KB, patch)
2009-10-12 01:17 PDT, Daniel Bates
abarth: review-
Details | Formatted Diff | Diff
Patch with test cases (26.17 KB, patch)
2009-10-14 22:16 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch v1 (21.85 KB, patch)
2009-10-25 17:49 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch v1 (20.44 KB, patch)
2009-10-25 17:52 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch with test cases (20.79 KB, patch)
2009-11-08 21:40 PST, Daniel Bates
no flags Details | Formatted Diff | Diff
Changes to truncate match as indicated. (12.45 KB, patch)
2011-09-12 16:34 PDT, Thomas Sepez
no flags Details | Formatted Diff | Diff
Patch with style fix. (12.44 KB, patch)
2011-09-12 16:42 PDT, Thomas Sepez
no flags Details | Formatted Diff | Diff
Patch simplified. (12.03 KB, patch)
2011-09-13 14:32 PDT, Thomas Sepez
no flags Details | Formatted Diff | Diff
Patch removing max snippet len truncation (11.83 KB, patch)
2011-09-13 15:49 PDT, Thomas Sepez
no flags Details | Formatted Diff | Diff
Revised tests (52.71 KB, patch)
2011-09-14 13:40 PDT, Thomas Sepez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 2009-07-31 13:56:23 PDT
An inline event handler that ends with a single-line JavaScript quote (i.e '//') can bypass the XSSAuditor.
Comment 1 Daniel Bates 2009-07-31 14:03:11 PDT
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.
Comment 2 Adam Barth 2009-07-31 14:11:38 PDT
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).
Comment 3 Daniel Bates 2009-08-05 13:18:37 PDT
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.
Comment 4 Daniel Bates 2009-08-05 13:19:52 PDT
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.
Comment 5 Daniel Bates 2009-08-05 17:36:58 PDT
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).
Comment 6 Daniel Bates 2009-08-05 17:40:15 PDT
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.
Comment 7 Adam Barth 2009-09-17 13:37:26 PDT
Here's another test case for this issue:

<script>alert(1);/*<!--
Comment 8 Daniel Bates 2009-09-25 14:32:33 PDT
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 9 Daniel Bates 2009-09-25 14:38:08 PDT
Comment on attachment 40145 [details]
Patch with test cases

Want to add a few more test cases
Comment 10 Adam Barth 2009-09-25 14:51:07 PDT
We might want to make the " ' < > change before landing this one.
Comment 11 Daniel Bates 2009-09-25 17:04:37 PDT
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--
Comment 12 Adam Barth 2009-09-25 17:49:38 PDT
Did you mean to mark that last patch for review?
Comment 13 Daniel Bates 2009-09-25 19:12:18 PDT
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.
Comment 14 Adam Barth 2009-10-05 21:19:40 PDT
Created attachment 40683 [details]
Patch v1
Comment 15 Adam Barth 2009-10-05 21:20:44 PDT
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?
Comment 16 Daniel Bates 2009-10-05 21:24:10 PDT
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?
Comment 17 Adam Barth 2009-10-05 21:27:30 PDT
(In reply to comment #16)
> Sure. I'll take a look.

Thanks.  :)
Comment 18 Daniel Bates 2009-10-08 17:17:05 PDT
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
Comment 19 Daniel Bates 2009-10-08 22:32:57 PDT
Created attachment 40929 [details]
Patch with test cases

Updated patch and modified test cases to pass all XSSAuditor tests.
Comment 20 Daniel Bates 2009-10-12 01:17:49 PDT
Created attachment 41022 [details]
Patch with test cases
Comment 21 Adam Barth 2009-10-12 09:24:49 PDT
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...
Comment 22 Daniel Bates 2009-10-14 22:16:56 PDT
Created attachment 41206 [details]
Patch with test cases
Comment 23 Adam Barth 2009-10-14 22:41:44 PDT
Comment on attachment 41206 [details]
Patch with test cases

Ok.  Let's see if this works!
Comment 24 Daniel Bates 2009-10-15 12:05:20 PDT
Committed r49644: <http://trac.webkit.org/changeset/49644>
Comment 25 Daniel Bates 2009-10-15 18:27:58 PDT
Committed r49668: <http://trac.webkit.org/changeset/49668>
Comment 26 Daniel Bates 2009-10-15 18:29:36 PDT
Re-opening this bug, due to bug #30418. We need to think about this patch some more.
Comment 27 Adam Barth 2009-10-18 23:21:54 PDT
Comment on attachment 41206 [details]
Patch with test cases

This patch turned out to be a bad idea.
Comment 28 Adam Barth 2009-10-25 17:49:50 PDT
Created attachment 41835 [details]
Patch v1
Comment 29 Adam Barth 2009-10-25 17:51:08 PDT
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.
Comment 30 Adam Barth 2009-10-25 17:52:56 PDT
Created attachment 41836 [details]
Patch v1
Comment 31 Daniel Bates 2009-11-08 21:40:35 PST
Created attachment 42732 [details]
Patch with test cases

Updated patch to reflect changes made in bug #31098.
Comment 32 Eric Seidel (no email) 2009-11-26 21:08:47 PST
Adam, can you review Dan's patch?  Or suggest someone who can?  (We're up to 2 weeks on this one...)
Comment 33 Daniel Bates 2009-12-23 15:08:48 PST
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.
Comment 34 Adam Barth 2011-08-19 13:40:55 PDT
*** Bug 43502 has been marked as a duplicate of this bug. ***
Comment 35 David Kilzer (:ddkilzer) 2011-08-19 23:42:30 PDT
<rdar://problem/8272897>
Comment 36 Thomas Sepez 2011-09-09 12:32:52 PDT
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.
Comment 37 Thomas Sepez 2011-09-12 16:34:02 PDT
Created attachment 107107 [details]
Changes to truncate match as indicated.
Comment 38 WebKit Review Bot 2011-09-12 16:37:08 PDT
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.
Comment 39 Thomas Sepez 2011-09-12 16:42:30 PDT
Created attachment 107111 [details]
Patch with style fix.
Comment 40 Adam Barth 2011-09-12 16:47:32 PDT
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.
Comment 41 Adam Barth 2011-09-12 16:48:33 PDT
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 42 Thomas Sepez 2011-09-12 16:56:23 PDT
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.
Comment 43 Adam Barth 2011-09-12 16:58:26 PDT
(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
Comment 44 Adam Barth 2011-09-12 16:59:10 PDT
(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 45 Thomas Sepez 2011-09-12 17:24:03 PDT
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.
Comment 46 Adam Barth 2011-09-12 17:49:42 PDT
> > 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.
Comment 47 Thomas Sepez 2011-09-13 14:32:20 PDT
Created attachment 107229 [details]
Patch simplified.

Still testting locally.
Comment 48 Adam Barth 2011-09-13 14:52:47 PDT
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 49 Thomas Sepez 2011-09-13 15:09:18 PDT
Comment on attachment 107229 [details]
Patch simplified.

Ok.  Please review.
Comment 50 Adam Barth 2011-09-13 15:19:08 PDT
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 51 Thomas Sepez 2011-09-13 15:35:44 PDT
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 52 Adam Barth 2011-09-13 15:37:58 PDT
Comment on attachment 107229 [details]
Patch simplified.

Sounds good to me.
Comment 53 Thomas Sepez 2011-09-13 15:49:16 PDT
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 54 WebKit Review Bot 2011-09-13 18:18:09 PDT
Comment on attachment 107246 [details]
Patch removing max snippet len truncation

Clearing flags on attachment: 107246

Committed r95065: <http://trac.webkit.org/changeset/95065>
Comment 55 WebKit Review Bot 2011-09-13 18:18:17 PDT
All reviewed patches have been landed.  Closing bug.
Comment 56 Thomas Sepez 2011-09-14 13:39:11 PDT
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.
Comment 57 Thomas Sepez 2011-09-14 13:40:16 PDT
Created attachment 107387 [details]
Revised tests
Comment 58 Adam Barth 2011-09-14 13:48:48 PDT
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.
Comment 59 Thomas Sepez 2011-09-14 14:46:14 PDT
More I think about this, right fix seems to be to detect // properly and leave tests alone.  Thoughts, Adam?
Comment 60 Adam Barth 2011-09-14 15:02:02 PDT
> 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.
Comment 61 Adam Barth 2011-09-14 15:06:58 PDT
If we run into too many false positives, we can re-evaluate.
Comment 62 WebKit Review Bot 2011-09-14 21:23:57 PDT
Comment on attachment 107387 [details]
Revised tests

Clearing flags on attachment: 107387

Committed r95161: <http://trac.webkit.org/changeset/95161>
Comment 63 WebKit Review Bot 2011-09-14 21:24:06 PDT
All reviewed patches have been landed.  Closing bug.