Prevent recursive call of Document::execCommand() to protect from attack code

Prevent recursive call of Document::execCommand() to protect from attack code

This patch prevents recursive call of Document::execCommand() to protect from attack code, e.g. executing editing operation in <iframe onload="... editing command">, <iframe src="javascript:...">.

Before this patch, editing operations moving iframe executes JavaScript script, specified in load event handler or src with javascript protocol. Although, implementation of editing operations don't handle DOM tree modification during executing editing operation.

Note: This change reduces security risk but introduces browser incompatibility to Blink. However, compatibility risk is low because use case of recursive execution of Document::execCommand() is very tricky and the result is different in each browser. We've seen usage of this in attack code only so far.

BUG=314609
TEST=LayoutTests/editing/execCommand/apply-style-iframe-crash.html

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

[yosin_UTC9, 2013-11-13 09:47:20]

Could you review this patch?
Thanks in advance.

[yosin_UTC9, 2013-11-13 09:52:55]

+morrita This is his idea.

[leviw_travelin_and_unemployed, 2013-11-13 21:16:18]

https://codereview.chromium.org/71163005/diff/40001/Source/core/editing/Compo...
File Source/core/editing/CompositeEditCommand.cpp (right):

https://codereview.chromium.org/71163005/diff/40001/Source/core/editing/Compo...
Source/core/editing/CompositeEditCommand.cpp:191: ++applyNestingCounter;
I like the idea... Can we make this RAII instead?

[Hajime Morrita, 2013-11-14 04:18:38]

Yeah, let's define a scope class.

I'm not sure if it is enough to prevent only nested editing
as attackers can do same with usual DOM manipulation.
Thats's said, this is good start.
The impact to compatibility and code complexity is small.

[yosin_UTC9, 2013-11-14 06:37:30]

PTAL

Win bot failure doesn't relate to this patch.

[Hajime Morrita, 2013-11-14 07:00:24]

lgtm.

https://codereview.chromium.org/71163005/diff/120001/Source/core/editing/Comp...
File Source/core/editing/CompositeEditCommand.cpp (right):

https://codereview.chromium.org/71163005/diff/120001/Source/core/editing/Comp...
Source/core/editing/CompositeEditCommand.cpp:77: class PreventRecursiveApply {
Nit: Let's name more noun-like

[yosin_UTC9, 2013-11-14 07:15:45]

On 2013/11/14 07:00:24, morrita1 wrote:
> Source/core/editing/CompositeEditCommand.cpp:77: class PreventRecursiveApply {
> Nit: Let's name more noun-like
levi, please suggest...

[leviw_travelin_and_unemployed, 2013-11-14 18:15:09]

On 2013/11/14 07:15:45, Yoshi wrote:
> On 2013/11/14 07:00:24, morrita1 wrote:
> > Source/core/editing/CompositeEditCommand.cpp:77: class PreventRecursiveApply
{
> > Nit: Let's name more noun-like
> levi, please suggest...

How about ReentrancyGuard?

[yosin_UTC9, 2013-11-15 05:31:51]

On 2013/11/14 18:15:09, Levi wrote:
> On 2013/11/14 07:15:45, Yoshi wrote:
> > On 2013/11/14 07:00:24, morrita1 wrote:
> > > Source/core/editing/CompositeEditCommand.cpp:77: class
PreventRecursiveApply
> {
> > > Nit: Let's name more noun-like
> > levi, please suggest...
> 
> How about ReentrancyGuard?
Thanks! Good name.
Committing...

[commit-bot: I haz the power, 2013-11-15 05:33:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yosin@chromium.org/71163005/200001

[commit-bot: I haz the power, 2013-11-15 07:05:22]

Change committed as 162080

[eseidel, 2013-11-15 07:29:51]

https://codereview.chromium.org/71163005/diff/200001/LayoutTests/editing/exec...
File LayoutTests/editing/execCommand/apply-style-iframe-crash.html (right):

https://codereview.chromium.org/71163005/diff/200001/LayoutTests/editing/exec...
LayoutTests/editing/execCommand/apply-style-iframe-crash.html:3:
</h1><input><iframe xonload="
why is this xonload?

https://codereview.chromium.org/71163005/diff/200001/Source/core/editing/Comp...
File Source/core/editing/CompositeEditCommand.cpp (right):

https://codereview.chromium.org/71163005/diff/200001/Source/core/editing/Comp...
Source/core/editing/CompositeEditCommand.cpp:77: class ReentrancyGuard {
We don't have one of these already somewhere?

[yosin_UTC9, 2013-11-15 09:35:16]

I submitted other patches:

Fix typo https://codereview.chromium.org/73873002
Guard http://crrev.com/72463005

https://codereview.chromium.org/71163005/diff/200001/LayoutTests/editing/exec...
File LayoutTests/editing/execCommand/apply-style-iframe-crash.html (right):

https://codereview.chromium.org/71163005/diff/200001/LayoutTests/editing/exec...
LayoutTests/editing/execCommand/apply-style-iframe-crash.html:3:
</h1><input><iframe xonload="
On 2013/11/15 07:29:52, eseidel wrote:
> why is this xonload?
Oops, I forgot to restore the test.
Fix in https://codereview.chromium.org/73873002/

https://codereview.chromium.org/71163005/diff/200001/Source/core/editing/Comp...
File Source/core/editing/CompositeEditCommand.cpp (right):

https://codereview.chromium.org/71163005/diff/200001/Source/core/editing/Comp...
Source/core/editing/CompositeEditCommand.cpp:77: class ReentrancyGuard {
On 2013/11/15 07:29:52, eseidel wrote:
> We don't have one of these already somewhere?
No. How about using Locker<T>? See http://crrev.com/72463005