Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(133)

Issue 141103006: Protect document.execCommand() from recursive call and DOM mutation events (Closed)

Created:
6 years, 10 months ago by yosin_UTC9
Modified:
6 years, 10 months ago
Reviewers:
tkent, esprehn
CC:
blink-reviews, dglazkov+blink, sof, eae+blinkwatch, adamk+blink_chromium.org, Inactive
Visibility:
Public.

Description

Extend applying document.execCommand() protection to all commands Before this patch, preventing recursive call of |document.execCommand()| and delaying DOM mutation event after editing command completion to ensure assumption of DOM tree in editing command, are applying to part of editing commands invoked by |CompositeEditCommand::apply()|, which is introduced by blink@162080. However, editing commands are also invoked by |document.execCommand()| not via |CompositeEditCommand::apply()|. This patch changes to move recursive call checking to |document.execCommand()|, because it is entry point for scripts, to postpone DOM mutation event until |document.execCommand()| finished. We still need to postpone DOM mutation event during |CompositeEditCommand::apply()|, which |DragController| calls. This patch also updates test expectations for test scripts which calls |document.execCommand()| recursively; all of them are for crash testing caused by recursive document.execCommand calls. Note: Preventing recursive call of |document.execCommand()| is right way to do so. Although, recursive call of |document.execCommand()| isn't common case, rather it is done by attacker. This patch reduces chances of attacking. BUG=338558 TEST=LayoutTests/editing/inserting/insert-with-mutation-event.html Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=166294

Patch Set 1 #

Total comments: 2

Patch Set 2 : 2014-01-29T16:48:25 #

Total comments: 2

Patch Set 3 : 2014-01-29T17:06:03 #

Total comments: 2

Patch Set 4 : 2014-02-03T14:21:08 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+179 lines, -33 lines) Patch
M LayoutTests/editing/execCommand/apply-style-iframe-crash-expected.txt View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
M LayoutTests/editing/execCommand/replaceSelectorCommand-crash-expected.txt View 1 2 3 1 chunk +95 lines, -3 lines 0 comments Download
M LayoutTests/editing/inserting/insert-paragraph-separator-crash-expected.txt View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
M LayoutTests/editing/inserting/insert-with-javascript-protocol-crash-expected.txt View 1 2 3 1 chunk +6 lines, -0 lines 0 comments Download
A LayoutTests/editing/inserting/insert-with-mutation-event.html View 1 chunk +22 lines, -0 lines 0 comments Download
A + LayoutTests/editing/inserting/insert-with-mutation-event-expected.txt View 1 2 3 0 chunks +-1 lines, --1 lines 0 comments Download
M LayoutTests/editing/style/iframe-onload-crash-mac-expected.txt View 1 2 3 1 chunk +4 lines, -0 lines 0 comments Download
M LayoutTests/editing/style/iframe-onload-crash-unix-expected.txt View 1 2 3 1 chunk +4 lines, -0 lines 0 comments Download
M LayoutTests/editing/style/iframe-onload-crash-win-expected.txt View 1 2 3 1 chunk +4 lines, -0 lines 0 comments Download
M LayoutTests/editing/undo/undo-after-event-edited-expected.txt View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
M LayoutTests/fast/parser/nested-fragment-parser-crash-expected.txt View 1 2 3 1 chunk +23 lines, -4 lines 0 comments Download
M Source/core/dom/Document.cpp View 1 2 3 1 chunk +16 lines, -0 lines 0 comments Download
M Source/core/editing/CompositeEditCommand.cpp View 1 2 3 3 chunks +0 lines, -27 lines 0 comments Download

Messages

Total messages: 29 (0 generated)
yosin_UTC9
Could you review this patch? Thanks in advance. Note: win/mac bots failures aren't related to ...
6 years, 10 months ago (2014-01-29 04:47:59 UTC) #1
tkent
https://codereview.chromium.org/141103006/diff/1/Source/core/dom/Document.cpp File Source/core/dom/Document.cpp (right): https://codereview.chromium.org/141103006/diff/1/Source/core/dom/Document.cpp#newcode4165 Source/core/dom/Document.cpp:4165: return false; Can we show a console message to ...
6 years, 10 months ago (2014-01-29 05:06:00 UTC) #2
esprehn_google.com
Why is this a static bool and not a member of document? You want to ...
6 years, 10 months ago (2014-01-29 07:05:15 UTC) #3
yosin_UTC9
On 2014/01/29 07:05:15, esprehn_google.com wrote: > Why is this a static bool and not a ...
6 years, 10 months ago (2014-01-29 07:28:44 UTC) #4
yosin_UTC9
PTAL https://codereview.chromium.org/141103006/diff/1/Source/core/dom/Document.cpp File Source/core/dom/Document.cpp (right): https://codereview.chromium.org/141103006/diff/1/Source/core/dom/Document.cpp#newcode4165 Source/core/dom/Document.cpp:4165: return false; On 2014/01/29 05:06:00, tkent wrote: > ...
6 years, 10 months ago (2014-01-29 07:50:41 UTC) #5
tkent
https://codereview.chromium.org/141103006/diff/80001/Source/core/dom/Document.cpp File Source/core/dom/Document.cpp (right): https://codereview.chromium.org/141103006/diff/80001/Source/core/dom/Document.cpp#newcode4161 Source/core/dom/Document.cpp:4161: String message = "document.execCommand() is called recursively. Please revise ...
6 years, 10 months ago (2014-01-29 07:56:06 UTC) #6
yosin_UTC9
PTAL https://codereview.chromium.org/141103006/diff/80001/Source/core/dom/Document.cpp File Source/core/dom/Document.cpp (right): https://codereview.chromium.org/141103006/diff/80001/Source/core/dom/Document.cpp#newcode4161 Source/core/dom/Document.cpp:4161: String message = "document.execCommand() is called recursively. Please ...
6 years, 10 months ago (2014-01-29 08:06:42 UTC) #7
tkent
https://codereview.chromium.org/141103006/diff/100001/LayoutTests/editing/inserting/insert-with-mutation-event.html File LayoutTests/editing/inserting/insert-with-mutation-event.html (right): https://codereview.chromium.org/141103006/diff/100001/LayoutTests/editing/inserting/insert-with-mutation-event.html#newcode22 LayoutTests/editing/inserting/insert-with-mutation-event.html:22: </script> I don't understand how recursive execCommand happens in ...
6 years, 10 months ago (2014-01-29 08:22:00 UTC) #8
esprehn_google.com
On Tuesday, January 28, 2014, <yosin@chromium.org> wrote: > On 2014/01/29 07:05:15, esprehn_google.com wrote: > >> ...
6 years, 10 months ago (2014-01-29 08:41:52 UTC) #9
yosin_UTC9
On 2014/01/29 08:41:52, esprehn_google.com wrote: > On Tuesday, January 28, 2014, <mailto:yosin@chromium.org> wrote: > > ...
6 years, 10 months ago (2014-01-29 09:08:28 UTC) #10
yosin_UTC9
https://codereview.chromium.org/141103006/diff/100001/LayoutTests/editing/inserting/insert-with-mutation-event.html File LayoutTests/editing/inserting/insert-with-mutation-event.html (right): https://codereview.chromium.org/141103006/diff/100001/LayoutTests/editing/inserting/insert-with-mutation-event.html#newcode22 LayoutTests/editing/inserting/insert-with-mutation-event.html:22: </script> On 2014/01/29 08:22:01, tkent wrote: > I don't ...
6 years, 10 months ago (2014-01-29 09:13:28 UTC) #11
yosin_UTC9
On 2014/01/29 09:13:28, Yoshi wrote: > https://codereview.chromium.org/141103006/diff/100001/LayoutTests/editing/inserting/insert-with-mutation-event.html > File LayoutTests/editing/inserting/insert-with-mutation-event.html (right): > > https://codereview.chromium.org/141103006/diff/100001/LayoutTests/editing/inserting/insert-with-mutation-event.html#newcode22 > ...
6 years, 10 months ago (2014-01-29 09:45:48 UTC) #12
yosin_UTC9
PTAL I fixed test failures, which caused by recursive call warning logs.
6 years, 10 months ago (2014-01-30 06:19:38 UTC) #13
tkent
lgtm. Preventing recursive calls is not a right fix. But it will reduce attacker's choices.
6 years, 10 months ago (2014-01-30 06:40:40 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yosin@chromium.org/141103006/140001
6 years, 10 months ago (2014-01-30 07:46:45 UTC) #15
commit-bot: I haz the power
Retried try job too often on linux_blink for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blink&number=9867
6 years, 10 months ago (2014-01-30 11:13:40 UTC) #16
commit-bot: I haz the power
CQ bit was unchecked on CL. Ignoring.
6 years, 10 months ago (2014-01-30 11:13:59 UTC) #17
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yosin@chromium.org/141103006/140001
6 years, 10 months ago (2014-01-31 01:10:32 UTC) #18
commit-bot: I haz the power
Retried try job too often on linux_blink for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blink&number=10113
6 years, 10 months ago (2014-01-31 04:12:15 UTC) #19
commit-bot: I haz the power
CQ bit was unchecked on CL. Ignoring.
6 years, 10 months ago (2014-01-31 04:12:20 UTC) #20
commit-bot: I haz the power
CQ bit was unchecked on CL. Ignoring.
6 years, 10 months ago (2014-01-31 04:12:29 UTC) #21
yosin_UTC9
PTAL The last patch moves both recursive call check and EventQueueScrope from CompositeEditCommand::apply() to Document::execCommand(). ...
6 years, 10 months ago (2014-02-03 06:50:58 UTC) #22
tkent
lgtm
6 years, 10 months ago (2014-02-03 07:15:09 UTC) #23
yosin_UTC9
The CQ bit was checked by yosin@chromium.org
6 years, 10 months ago (2014-02-03 07:20:27 UTC) #24
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yosin@chromium.org/141103006/170015
6 years, 10 months ago (2014-02-03 07:20:32 UTC) #25
commit-bot: I haz the power
Change committed as 166294
6 years, 10 months ago (2014-02-03 08:06:18 UTC) #26
commit-bot: I haz the power
CQ bit was unchecked on CL. Ignoring.
6 years, 10 months ago (2014-02-03 08:06:25 UTC) #27
commit-bot: I haz the power
CQ bit was unchecked on CL. Ignoring.
6 years, 10 months ago (2014-02-03 08:06:28 UTC) #28
commit-bot: I haz the power
6 years, 10 months ago (2014-02-03 08:06:28 UTC) #29
Message was sent while issue was closed.
CQ bit was unchecked on CL. Ignoring.

Powered by Google App Engine
This is Rietveld 408576698