Teach the background parser to ignore certain elements inside '<select>'.

Teach the background parser to ignore certain elements inside '<select>'.

'HTMLTreeBuilderSimulator' doesn't currently understand that we shouldn't
hop into PLAINTEXTState or RAWTEXTState inside '<select>' elements. This
has the unfortunate side-effect of enabling dangling markup injection
attacks that exfiltrate data via '<select><option><plaintext>' and etc.

This patch ensures that `<select>` behaves as specified, matching Safari,
Firefox, and Edge's behavior.

Thanks to @zcorpan for pointing out Blink's error in the thread ad
https://github.com/whatwg/html/issues/2252.

BUG=680072
Review-Url: https://codereview.chromium.org/2625103002
Cr-Commit-Position: refs/heads/master@{#443573}
Committed: https://chromium.googlesource.com/chromium/src/+/8150200aff6ad60b092fd2ddb7eddcb6d0cc13df

[Mike West, 2017-01-11 12:28:44]

The CQ bit was checked by mkwst@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-11 12:28:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Mike West, 2017-01-11 12:29:58]

mkwst@chromium.org changed reviewers:
+ yoav@yoav.ws

[Mike West, 2017-01-11 12:29:59]

Yoav, WDYT of this change to the background parser? The new test passes in
Firefox... I'd like to match their behavior.

Thanks!

[Mike West, 2017-01-11 12:43:54]

mkwst@chromium.org changed reviewers:
+ csharrison@chromium.org

[Mike West, 2017-01-11 12:43:55]

+csharrison@ as well, as I worry that correctness might have performance
implications.

[Charlie Harrison, 2017-01-11 13:09:22]

I don't see anything wrong with this perf wise. One more threadSafeMatch seems
ok.

+kouhei to cc who may be interested in changes to HTML tree builder simulator.

[Mike West, 2017-01-11 14:12:25]

On 2017/01/11 at 13:09:22, csharrison wrote:
> I don't see anything wrong with this perf wise. One more threadSafeMatch seems
ok.

Great!

> +kouhei to cc who may be interested in changes to HTML tree builder simulator.

I'd note as an aside that there are other places where "any other element" is
ignored (at least "in colgroup", "in frameset", "after frameset", and "after
after frameset" all seem to ignore tags like these). `<select>` is the only one
I know of that has security implications, though, and the only one this patch
tries to address.

[commit-bot: I haz the power, 2017-01-11 14:46:07]

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

[commit-bot: I haz the power, 2017-01-11 14:46:07]

Dry run: This issue passed the CQ dry run.

[Mike West, 2017-01-11 20:48:31]

Description was changed from

==========
Teach the background parser to ignore certain elements inside '<select>'.

'HTMLTreeBuilderSimulator' doesn't currently understand that we shouldn't
hop into PLAINTEXTState or RAWTEXTState inside '<select>' elements. This
has the unfortunate side-effect of enabling dangling markup injection
attacks that exfiltrate data via '<select><option><plaintext>' and etc.

This patch ensures that we only change the tokenizer state when we're
supposed to.

BUG=680072
==========

to

==========
Teach the background parser to ignore certain elements inside '<select>'.

'HTMLTreeBuilderSimulator' doesn't currently understand that we shouldn't
hop into PLAINTEXTState or RAWTEXTState inside '<select>' elements. This
has the unfortunate side-effect of enabling dangling markup injection
attacks that exfiltrate data via '<select><option><plaintext>' and etc.

This patch ensures that `<select>` behaves as specified, matching Safari,
Firefox, and Edge's behavior.

Thanks to @zcorpan for pointing out Blink's error in the thread ad
https://github.com/whatwg/html/issues/2252.

BUG=680072
==========

[Mike West, 2017-01-12 12:00:26]

mkwst@chromium.org changed reviewers:
+ kouhei@chromium.org

[Mike West, 2017-01-12 12:00:27]

yoav@: Friendly ping. If you're busy, I'd be equally happy for csharrison@ or
kouhei@ to weigh in on the details. :)

[Yoav Weiss, 2017-01-12 13:45:44]

https://codereview.chromium.org/2625103002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/parser/HTMLTreeBuilderSimulator.cpp
(right):

https://codereview.chromium.org/2625103002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/parser/HTMLTreeBuilderSimulator.cpp:159:
!m_inSelectInsertionMode) {
I'm most probably missing something, but I don't see such a condition in the
HTML spec. What I do see is that moving the tokenizer to the PLAINTEXTState is
defined to only happen when parsing *in body*
https://html.spec.whatwg.org/#parsing-main-inbody

So IIUC (and it's fairly possible that this is not the case) this should be
blocked on in-body insertion, not just banned inside select.

https://codereview.chromium.org/2625103002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/parser/HTMLTreeBuilderSimulator.cpp:168:
m_options.scriptEnabled)) {
Here we're basically blocking anything not mentioned in the "in select insertion
mode" rather than allowing what's there? Is there a particular reason for that?
Can we inverse it to make it easier to correlate the spec to the code and vice
versa?

[Mike West, 2017-01-12 14:44:51]

https://codereview.chromium.org/2625103002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/parser/HTMLTreeBuilderSimulator.cpp
(right):

https://codereview.chromium.org/2625103002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/parser/HTMLTreeBuilderSimulator.cpp:159:
!m_inSelectInsertionMode) {
On 2017/01/12 at 13:45:44, Yoav Weiss wrote:
> I'm most probably missing something, but I don't see such a condition in the
HTML spec. What I do see is that moving the tokenizer to the PLAINTEXTState is
defined to only happen when parsing *in body*
https://html.spec.whatwg.org/#parsing-main-inbody
> 
> So IIUC (and it's fairly possible that this is not the case) this should be
blocked on in-body insertion, not just banned inside select.

Sure. However, we have no idea what mode we're in in this simulated tree
builder. I agree with you that this patch leaves correctness problems with
regard to switching into plaintext or rawtext tokenizing. From my perspective, I
don't particularly care about those correctness problems, because they don't
also turn into security problems.

Following those rules in a reasonable way almost certainly boils down to
implementing all the logic in HTMLTreeBuilder about insertion mode state
transitions that we're leaving out. I'd be thrilled if y'all would take that on,
because the disconnect between the tokenizer state and the insertion mode seems
very strange to me, and there are probably other problems that it causes. I'm
not signing up to do that work, though. :)

https://codereview.chromium.org/2625103002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/parser/HTMLTreeBuilderSimulator.cpp:168:
m_options.scriptEnabled)) {
On 2017/01/12 at 13:45:44, Yoav Weiss wrote:
> Here we're basically blocking anything not mentioned in the "in select
insertion mode" rather than allowing what's there? Is there a particular reason
for that? Can we inverse it to make it easier to correlate the spec to the code
and vice versa?

No? This basically just moves the existing code (from line 143 and line 148)
into an `if (!m_inSelectInsertionMode)` block, ensuring that we don't transition
the tokenizer state into PLAINTEXTState or RAWTEXTState if we're in "in select"
insertion mode.

Again, the actual work on the tokens (e.g. whether or not the `<plaintext>`
start tag is ignored or not) is handled in HTMLTreeBuilder. That's where the
insertion mode matters and is stored. I'm only porting over the smallest piece
necessary to teach this simulated treebuilder enough to stop it from changing
the tokenizer state in this particular case with security implications. :)

[Mike West, 2017-01-13 12:35:31]

yoav: Ping? :)

[Mike West, 2017-01-13 13:35:27]

The CQ bit was checked by mkwst@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-13 13:35:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Yoav Weiss, 2017-01-13 14:13:20]

LGTM

We probably want to align the code with the spec, but doesn't have to happen all
at once...

https://codereview.chromium.org/2625103002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/parser/HTMLTreeBuilderSimulator.cpp
(right):

https://codereview.chromium.org/2625103002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/parser/HTMLTreeBuilderSimulator.cpp:159:
!m_inSelectInsertionMode) {
On 2017/01/12 14:44:51, Mike West (sloooooow) wrote:
> On 2017/01/12 at 13:45:44, Yoav Weiss wrote:
> > I'm most probably missing something, but I don't see such a condition in the
> HTML spec. What I do see is that moving the tokenizer to the PLAINTEXTState is
> defined to only happen when parsing *in body*
> https://html.spec.whatwg.org/#parsing-main-inbody
> > 
> > So IIUC (and it's fairly possible that this is not the case) this should be
> blocked on in-body insertion, not just banned inside select.
> 
> Sure. However, we have no idea what mode we're in in this simulated tree
> builder. I agree with you that this patch leaves correctness problems with
> regard to switching into plaintext or rawtext tokenizing. From my perspective,
I
> don't particularly care about those correctness problems, because they don't
> also turn into security problems.
> 
> Following those rules in a reasonable way almost certainly boils down to
> implementing all the logic in HTMLTreeBuilder about insertion mode state
> transitions that we're leaving out. I'd be thrilled if y'all would take that
on,
> because the disconnect between the tokenizer state and the insertion mode
seems
> very strange to me, and there are probably other problems that it causes. I'm
> not signing up to do that work, though. :)

makes sense :)

https://codereview.chromium.org/2625103002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/parser/HTMLTreeBuilderSimulator.cpp:168:
m_options.scriptEnabled)) {
On 2017/01/12 14:44:51, Mike West (sloooooow) wrote:
> On 2017/01/12 at 13:45:44, Yoav Weiss wrote:
> > Here we're basically blocking anything not mentioned in the "in select
> insertion mode" rather than allowing what's there? Is there a particular
reason
> for that? Can we inverse it to make it easier to correlate the spec to the
code
> and vice versa?
> 
> No? This basically just moves the existing code (from line 143 and line 148)
> into an `if (!m_inSelectInsertionMode)` block, ensuring that we don't
transition
> the tokenizer state into PLAINTEXTState or RAWTEXTState if we're in "in
select"
> insertion mode.
> 
> Again, the actual work on the tokens (e.g. whether or not the `<plaintext>`
> start tag is ignored or not) is handled in HTMLTreeBuilder. That's where the
> insertion mode matters and is stored. I'm only porting over the smallest piece
> necessary to teach this simulated treebuilder enough to stop it from changing
> the tokenizer state in this particular case with security implications. :)

fair enough :)

[Mike West, 2017-01-13 14:30:53]

The CQ bit was unchecked by mkwst@chromium.org

[Mike West, 2017-01-13 14:30:55]

The CQ bit was checked by mkwst@chromium.org

[Mike West, 2017-01-13 14:31:05]

Merci!

[commit-bot: I haz the power, 2017-01-13 14:31:08]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-13 16:33:36]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1484317855948420,
"parent_rev": "8cea3f856adc43fdd72a9cda021c0ceaba3e3803", "commit_rev":
"8150200aff6ad60b092fd2ddb7eddcb6d0cc13df"}

[commit-bot: I haz the power, 2017-01-13 16:34:05]

Description was changed from

==========
Teach the background parser to ignore certain elements inside '<select>'.

'HTMLTreeBuilderSimulator' doesn't currently understand that we shouldn't
hop into PLAINTEXTState or RAWTEXTState inside '<select>' elements. This
has the unfortunate side-effect of enabling dangling markup injection
attacks that exfiltrate data via '<select><option><plaintext>' and etc.

This patch ensures that `<select>` behaves as specified, matching Safari,
Firefox, and Edge's behavior.

Thanks to @zcorpan for pointing out Blink's error in the thread ad
https://github.com/whatwg/html/issues/2252.

BUG=680072
==========

to

==========
Teach the background parser to ignore certain elements inside '<select>'.

'HTMLTreeBuilderSimulator' doesn't currently understand that we shouldn't
hop into PLAINTEXTState or RAWTEXTState inside '<select>' elements. This
has the unfortunate side-effect of enabling dangling markup injection
attacks that exfiltrate data via '<select><option><plaintext>' and etc.

This patch ensures that `<select>` behaves as specified, matching Safari,
Firefox, and Edge's behavior.

Thanks to @zcorpan for pointing out Blink's error in the thread ad
https://github.com/whatwg/html/issues/2252.

BUG=680072

Review-Url: https://codereview.chromium.org/2625103002
Cr-Commit-Position: refs/heads/master@{#443573}
Committed:
https://chromium.googlesource.com/chromium/src/+/8150200aff6ad60b092fd2ddb7ed...
==========

[commit-bot: I haz the power, 2017-01-13 16:34:06]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/8150200aff6ad60b092fd2ddb7ed...