Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2683583003:
Bindings: perform security check before downcasting to LocalDOMWindow. (Closed)
DescriptionBindings: perform security check before downcasting to LocalDOMWindow.
Certain callbacks can be reached after the browsing context has been
navigated. In those instances, it's important to do the security check
before downcasting, since the normal security check on the holder
object won't be used.
BUG=689243
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2683583003
Cr-Commit-Position: refs/heads/master@{#448915}
Committed: https://chromium.googlesource.com/chromium/src/+/ae9143a9d87c734f5b31f72f296917cc8bc24588
Patch Set 1 #
Total comments: 5
Patch Set 2 : . #
Messages
Total messages: 24 (14 generated)
dcheng@chromium.org changed reviewers: + haraken@chromium.org, yukishiino@chromium.org
Not the nicest patch, but I couldn't think of a better alternative.
The CQ bit was checked by dcheng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM https://codereview.chromium.org/2683583003/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl (right): https://codereview.chromium.org/2683583003/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:48: {% if not attribute.is_check_security_for_receiver %} Can we avoid 'if not' - 'else'? https://codereview.chromium.org/2683583003/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl (right): https://codereview.chromium.org/2683583003/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl:34: {% if not method.is_check_security_for_receiver %} Ditto. https://codereview.chromium.org/2683583003/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl:40: {{cpp_class}}* uncheckedImpl = {{v8_class}}::toImpl(info.Holder()); Instead, can we check isLocalDOMWindow()? If it returns false, we can return immediately. If that's possible, we don't need to introduce |uncheckedImpl| and can share more code between local_dom_window_only and !local_dom_window_only.
https://codereview.chromium.org/2683583003/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl (right): https://codereview.chromium.org/2683583003/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:48: {% if not attribute.is_check_security_for_receiver %} On 2017/02/07 15:43:13, haraken wrote: > > Can we avoid 'if not' - 'else'? Done. https://codereview.chromium.org/2683583003/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl (right): https://codereview.chromium.org/2683583003/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl:40: {{cpp_class}}* uncheckedImpl = {{v8_class}}::toImpl(info.Holder()); On 2017/02/07 15:43:13, haraken wrote: > > Instead, can we check isLocalDOMWindow()? If it returns false, we can return > immediately. > > If that's possible, we don't need to introduce |uncheckedImpl| and can share > more code between local_dom_window_only and !local_dom_window_only. Unfortunately, it's not very easy to do that. If we check and return on line 66 to share code, then we won't throw a SecurityError. If we return earlier than that, we have to adapt any special early return handling as well as throwing the exception ourselves: some of the attribute helpers call v8SetReturnValue when doing an early return, for instance. So that's also duplicated code. One last alternative is to have BindingSecurity check itself... but that's also unusual. Normally, having a nullptr means we did not set up bookkeeping on the v8 Object correctly... so it would be nice not to weaken the checks in BindingSecurity. Given this, I opted to just keep this code here =/
The CQ bit was checked by dcheng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Bindings: perform security check before downcasting to LocalDOMWindow. Certain callbacks can be reached after the browsing context has been navigated. In those instances, it's important to do the security check before downcasting, since the normal security check on the holder object won't be used. BUG=689243 ========== to ========== Bindings: perform security check before downcasting to LocalDOMWindow. Certain callbacks can be reached after the browsing context has been navigated. In those instances, it's important to do the security check before downcasting, since the normal security check on the holder object won't be used. BUG=689243 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
On 2017/02/08 01:30:25, dcheng wrote: > https://codereview.chromium.org/2683583003/diff/1/third_party/WebKit/Source/b... > File third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl (right): > > https://codereview.chromium.org/2683583003/diff/1/third_party/WebKit/Source/b... > third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:48: {% if not > attribute.is_check_security_for_receiver %} > On 2017/02/07 15:43:13, haraken wrote: > > > > Can we avoid 'if not' - 'else'? > > Done. > > https://codereview.chromium.org/2683583003/diff/1/third_party/WebKit/Source/b... > File third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl (right): > > https://codereview.chromium.org/2683583003/diff/1/third_party/WebKit/Source/b... > third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl:40: {{cpp_class}}* > uncheckedImpl = {{v8_class}}::toImpl(info.Holder()); > On 2017/02/07 15:43:13, haraken wrote: > > > > Instead, can we check isLocalDOMWindow()? If it returns false, we can return > > immediately. > > > > If that's possible, we don't need to introduce |uncheckedImpl| and can share > > more code between local_dom_window_only and !local_dom_window_only. > > Unfortunately, it's not very easy to do that. If we check and return on line 66 > to share code, then we won't throw a SecurityError. > > If we return earlier than that, we have to adapt any special early return > handling as well as throwing the exception ourselves: some of the attribute > helpers call v8SetReturnValue when doing an early return, for instance. So > that's also duplicated code. > > One last alternative is to have BindingSecurity check itself... but that's also > unusual. Normally, having a nullptr means we did not set up bookkeeping on the > v8 Object correctly... so it would be nice not to weaken the checks in > BindingSecurity. > > Given this, I opted to just keep this code here =/ Makes sense. LGTM.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)
Going to try CQing even though linux_site_isolation is red... the test passes locally on my machine, so I'm not sure what's going on.
The CQ bit was checked by dcheng@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1486529478879820,
"parent_rev": "19fb57b4a9b7c6764e1898b7b4bee6e785a38b00", "commit_rev":
"ae9143a9d87c734f5b31f72f296917cc8bc24588"}
Message was sent while issue was closed.
Description was changed from ========== Bindings: perform security check before downcasting to LocalDOMWindow. Certain callbacks can be reached after the browsing context has been navigated. In those instances, it's important to do the security check before downcasting, since the normal security check on the holder object won't be used. BUG=689243 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Bindings: perform security check before downcasting to LocalDOMWindow. Certain callbacks can be reached after the browsing context has been navigated. In those instances, it's important to do the security check before downcasting, since the normal security check on the holder object won't be used. BUG=689243 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2683583003 Cr-Commit-Position: refs/heads/master@{#448915} Committed: https://chromium.googlesource.com/chromium/src/+/ae9143a9d87c734f5b31f72f2969... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/ae9143a9d87c734f5b31f72f2969...
Message was sent while issue was closed.
esprehn@chromium.org changed reviewers: + esprehn@chromium.org
Message was sent while issue was closed.
Is this putting a call to BindingSecurity::shouldAllowAccessTo(currentDOMWindow inside every window function? shouldAllowAccessTo is actually pretty expensive I think, how did this work before?
Message was sent while issue was closed.
On 2017/02/08 06:13:41, esprehn wrote: > Is this putting a call to BindingSecurity::shouldAllowAccessTo(currentDOMWindow > inside every window function? > > shouldAllowAccessTo is actually pretty expensive I think, how did this work > before? This call has always been there: we needed this check even before the LocalDOMWindow hacks.
Message was sent while issue was closed.
LGTM. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
