XSSAuditor bypass with script tag and expression following injection point

This patch fixes a corner case in the XSSAuditor where the attacker can use an organically-occurring script tag to bypass the auditor. The trick is that injection<script>expr may be parsed by JS as (injection < script) > expr.

BUG=354109
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=169697

[Tom Sepez, 2014-03-19 22:22:44]

Adam, please take a look.

[abarth-chromium, 2014-03-19 22:29:10]

lgtm

[Tom Sepez, 2014-03-19 23:39:06]

The CQ bit was checked by tsepez@chromium.org

[commit-bot: I haz the power, 2014-03-19 23:39:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tsepez@chromium.org/205243002/40001

[commit-bot: I haz the power, 2014-03-20 00:49:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-20 00:49:25]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[Tom Sepez, 2014-03-20 16:49:18]

The CQ bit was checked by tsepez@chromium.org

[commit-bot: I haz the power, 2014-03-20 16:49:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tsepez@chromium.org/205243002/40001

[dbates, 2014-03-20 17:39:34]

I'm not a reviewer and I know this patch was already reviewed. I noticed some
minor stylistic nits and had a suggestion for an additional test case.

https://codereview.chromium.org/205243002/diff/40001/Source/core/html/parser/...
File Source/core/html/parser/XSSAuditor.cpp (right):

https://codereview.chromium.org/205243002/diff/40001/Source/core/html/parser/...
Source/core/html/parser/XSSAuditor.cpp:118: &&
WTF::toASCIILowerUnchecked(string[start+1]) == 's' &&
WTF::toASCIILowerUnchecked(string[start+2]) == 'c'
Nit: There should be space characters on either side of the addition operator
per (2) of section Spacing in the Blink Code Style Guidelines,
<http://www.chromium.org/blink/coding-style#TOC-Spacing>.

Notice that this spacing issue appears twice in this line and four more times
below.

https://codereview.chromium.org/205243002/diff/40001/Source/core/html/parser/...
Source/core/html/parser/XSSAuditor.cpp:691: // when we hit an opening <script>
tag, or when we  exceed the maximum length target. The comma rule
Nit: There are two space characters that precede the word "exceed".

https://codereview.chromium.org/205243002/diff/40001/Source/core/html/parser/...
Source/core/html/parser/XSSAuditor.cpp:692: // covers a common parameter
concatenation case performed by some webservers.
Nit: Although this isn't part of the patch, "webservers" should be "web
servers".

https://codereview.chromium.org/205243002/diff/40001/Source/core/html/parser/...
Source/core/html/parser/XSSAuditor.cpp:709: foundPosition =
lastNonSpacePosition;
The coverage included in this patch is sufficient. For your consideration, I
suggest we added a test where q :='<script>"<script>"%3Balert(/XSS/)' (without
single quotes) to test how we handle matching the start of an opening script tag
near the beginning of the injection string.

https://codereview.chromium.org/205243002/diff/40001/Source/core/html/parser/...
Source/core/html/parser/XSSAuditor.cpp:716: // not stopping inside a (possibly
multiply encoded) %-esacpe sequence by breaking on
Nit: esacpe => escape

[Tom Sepez, 2014-03-20 17:48:41]

The CQ bit was unchecked by tsepez@chromium.org

[Tom Sepez, 2014-03-20 17:49:09]

Thanks Dan, I'll clean up the nits and try your test.

[Tom Sepez, 2014-03-20 18:22:26]

Note that I had to change "<script>";alert() in your test to "<script>"-alert()
to get around issues with passing semicolons to the perl script.  It's still
syntactically valid in that form.

[Tom Sepez, 2014-03-20 18:22:40]

The CQ bit was checked by tsepez@chromium.org

[commit-bot: I haz the power, 2014-03-20 18:22:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tsepez@chromium.org/205243002/60001

[commit-bot: I haz the power, 2014-03-20 19:58:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-20 19:58:49]

Try jobs failed on following builders:
  tryserver.blink on win_blink_rel

[Tom Sepez, 2014-03-20 20:49:57]

The CQ bit was checked by tsepez@chromium.org

[commit-bot: I haz the power, 2014-03-20 20:50:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tsepez@chromium.org/205243002/60001

[commit-bot: I haz the power, 2014-03-20 20:52:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-20 20:52:03]

Try jobs failed on following builders:
  tryserver.blink on win_blink_rel

[Tom Sepez, 2014-03-20 20:58:36]

The CQ bit was checked by tsepez@chromium.org

[commit-bot: I haz the power, 2014-03-20 20:58:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tsepez@chromium.org/205243002/60001

[commit-bot: I haz the power, 2014-03-20 21:50:39]

Change committed as 169697