Project: chromium Issues People Development process History Sign in
New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 1 user
Status: Fixed
Owner:
Closed: Apr 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug-Security



Sign in to add a comment
Security: Universal XSS using iterables
Reported by marius.mlynski@gmail.com, Apr 22 2016 Back to list
VULNERABILITY DETAILS
From /third_party/WebKit/Source/bindings/core/v8/Iterable.h:
----------------
    void forEachForBinding(...)
    {
        (...)
        v8::Local<v8::Object> creationContext(scriptState->context()->Global());
        v8::Local<v8::Function> v8Callback(callback.v8Value().As<v8::Function>());
        v8::Local<v8::Value> v8ThisArg(thisArg.v8Value());
        v8::Local<v8::Value> args[3];

        args[2] = thisValue.v8Value();

        while (true) {
            KeyType key;
            ValueType value;

            if (!source->next(scriptState, key, value, exceptionState))
                return;
            (...)
            args[0] = toV8(value, creationContext, isolate);
            args[1] = toV8(key, creationContext, isolate);
            (...)
            v8::Local<v8::Value> result;
            if (!V8ScriptRunner::callFunction(v8Callback, scriptState->getExecutionContext(), v8ThisArg, 3, args, isolate).ToLocal(&result)) {
                exceptionState.rethrowV8Exception(tryCatch.Exception());
                return;
            }
        }
    }
----------------

This code doesn't consider that the callback can change the security characteristics of the object used as a creation context. This may lead to cross-origin object leaks.

VERSION
Chrome 50.0.2661.87 (Stable)
Chrome 51.0.2704.22 (Beta)
Chrome 51.0.2704.19 (Dev)
Chromium 52.0.2716.0 (Release build compiled today)
 
exploit.zip
1.1 KB Download
Comment 1 by vakh@chromium.org, Apr 22 2016
Cc: keishi@chromium.org yhirano@chromium.org yukishiino@chromium.org
Components: Blink>Bindings
Labels: Security_Severity-High Security_Impact-Stable
Comment 2 by vakh@chromium.org, Apr 22 2016
Cc: vakh@chromium.org
Comment 3 by vakh@chromium.org, Apr 22 2016
Owner: dcheng@chromium.org
dcheng@ -- could you please take a look and assign it to the appropriate owner? Thanks!
Comment 4 by dcheng@chromium.org, Apr 22 2016
Cc: dcheng@chromium.org
Labels: M-50 Pri-1
Owner: j...@opera.com
Status: Assigned
Comment 5 by vakh@chromium.org, Apr 22 2016
Cc: leecam@chromium.org
Comment 6 by j...@opera.com, Apr 25 2016
Cc: haraken@chromium.org
Project Member Comment 7 by bugdroid1@chromium.org, Apr 26 2016
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0cd7a9f853e3cb7c035b4ab9e07a503552267f9d

commit 0cd7a9f853e3cb7c035b4ab9e07a503552267f9d
Author: jl <jl@opera.com>
Date: Tue Apr 26 15:22:47 2016

Use correct creation context during Iterable.forEach iteration

Use |thisValue| instead of |scriptState->context()->Global()|, since this
is simpler and since Global() actually returns a WindowProxy object that
may change and become incorrect to use as creation context depending on
what the callback function does.

BUG= 605910 

Review URL: https://codereview.chromium.org/1918763002

Cr-Commit-Position: refs/heads/master@{#389785}

[add] https://crrev.com/0cd7a9f853e3cb7c035b4ab9e07a503552267f9d/third_party/WebKit/LayoutTests/fast/css/fontfaceset-cross-frame.html
[modify] https://crrev.com/0cd7a9f853e3cb7c035b4ab9e07a503552267f9d/third_party/WebKit/Source/bindings/core/v8/Iterable.h

Comment 8 by j...@opera.com, Apr 27 2016
Status: Fixed
This fix ought to be merged to stabilization branches, I assume? It's quite trivial and should be safe to merge.

Note that I'm quite unfamiliar with the proper way of handling such issues, so all advise and/or help is very appreciated.
Labels: Merge-Request-51 Merge-Request-50
We've confirmed that the fix works fine on ToT.
Requesting merge to M50 and M51.
Comment 10 by tin...@google.com, Apr 27 2016
Labels: -Merge-Request-50 Merge-Review-50 Hotlist-Merge-Review
[Automated comment] Request affecting a post-stable build (M50), manual review required.
Comment 11 by tin...@google.com, Apr 27 2016
Labels: -Merge-Request-51 Merge-Approved-51 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M51 (branch: 2704)
Comment 12 by tin...@google.com, Apr 27 2016
Labels: -Merge-Request-51 Merge-Approved-51 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M51 (branch: 2704)
Project Member Comment 13 by clusterf...@chromium.org, Apr 27 2016
Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
jl@, you can now merge the CL to M51.
The branch number is 2704.  Please use the drover to merge the CL.
http://dev.chromium.org/developers/how-tos/drover

Let us know if you have any trouble.

FYI just in case:
http://dev.chromium.org/developers/the-zen-of-merge-requests

Project Member Comment 15 by bugdroid1@chromium.org, Apr 28 2016
Labels: -merge-approved-51 merge-merged-2704
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4fdeff55d9188e4490e71305f38ee0e62d0b50c6

commit 4fdeff55d9188e4490e71305f38ee0e62d0b50c6
Author: Jens Widell <jl@opera.com>
Date: Thu Apr 28 06:12:40 2016

Use correct creation context during Iterable.forEach iteration

Use |thisValue| instead of |scriptState->context()->Global()|, since this
is simpler and since Global() actually returns a WindowProxy object that
may change and become incorrect to use as creation context depending on
what the callback function does.

BUG= 605910 

Review URL: https://codereview.chromium.org/1918763002

Cr-Commit-Position: refs/heads/master@{#389785}
(cherry picked from commit 0cd7a9f853e3cb7c035b4ab9e07a503552267f9d)

Review URL: https://codereview.chromium.org/1927063002 .

Cr-Commit-Position: refs/branch-heads/2704@{#283}
Cr-Branched-From: 6e53600def8f60d8c632fadc70d7c1939ccea347-refs/heads/master@{#386251}

[add] https://crrev.com/4fdeff55d9188e4490e71305f38ee0e62d0b50c6/third_party/WebKit/LayoutTests/fast/css/fontfaceset-cross-frame.html
[modify] https://crrev.com/4fdeff55d9188e4490e71305f38ee0e62d0b50c6/third_party/WebKit/Source/bindings/core/v8/Iterable.h

Cc: tinazh@chromium.org
Labels: -Merge-Review-50 Merge-Approved-50
Approving merge to M50 branch 2661, based on comment #9 and also it is in Beta already. Please merge asap (latest before 1:00 PM PST Monday), so we can take it for next week Stable refresh release. Thank you.
Project Member Comment 17 by bugdroid1@chromium.org, May 9 2016
Labels: -merge-approved-50 merge-merged-2661
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cfb182b8d5cdc104ba1e9c09125c918b00e850bb

commit cfb182b8d5cdc104ba1e9c09125c918b00e850bb
Author: Jens Widell <jl@opera.com>
Date: Mon May 09 06:47:57 2016

Use correct creation context during Iterable.forEach iteration

Use |thisValue| instead of |scriptState->context()->Global()|, since this
is simpler and since Global() actually returns a WindowProxy object that
may change and become incorrect to use as creation context depending on
what the callback function does.

BUG= 605910 

Review URL: https://codereview.chromium.org/1918763002

Cr-Commit-Position: refs/heads/master@{#389785}
(cherry picked from commit 0cd7a9f853e3cb7c035b4ab9e07a503552267f9d)

Review URL: https://codereview.chromium.org/1960013002 .

Cr-Commit-Position: refs/branch-heads/2661@{#671}
Cr-Branched-From: ef6f6ae5e4c96622286b563658d5cd62a6cf1197-refs/heads/master@{#378081}

[add] https://crrev.com/cfb182b8d5cdc104ba1e9c09125c918b00e850bb/third_party/WebKit/LayoutTests/fast/css/fontfaceset-cross-frame.html
[modify] https://crrev.com/cfb182b8d5cdc104ba1e9c09125c918b00e850bb/third_party/WebKit/Source/bindings/core/v8/Iterable.h

Labels: reward-topanel Release-3-M50
Comment 19 by vakh@chromium.org, May 9 2016
Cc: -vakh@chromium.org
Cc: tasak@google.com
Labels: -reward-topanel reward-7500 CVE-2016-1668 reward-unpaid
...and another $7,500 for this report. Thanks again for a great report and POC.
Labels: reward-inprocess
Labels: -reward-unpaid
Project Member Comment 24 by sheriffbot@chromium.org, Aug 3 2016
Labels: -Restrict-View-SecurityNotify
This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member Comment 25 by sheriffbot@chromium.org, Oct 1 2016
This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member Comment 26 by sheriffbot@chromium.org, Oct 2 2016
This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: allpublic
Sign in to add a comment