New issue
Advanced search Search tips
Starred by 7 users

Issue metadata

Status: Fixed
Owner:
Closed: Dec 17
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 1
Type: Bug-Regression



Sign in to add a comment
link

Issue 912069: Isolated world is incorrectly chosen for an onclick string attribute

Reported by woxxom@gmail.com, Dec 5

Issue description

Chrome: 71

----------------------------

1. install the attached extension
2. open https://www.example.org/
3. click the gray "Click me!" button 

Expected: "SUCCESS" is shown
Observed: "FAILURE" is shown

----------------------------

Bisected to r590972 = 45dedabecb98f1e92ea8cc409ecb68cd4574a7e9 = https://crrev.com/c/1218244 by yukiy@google.com
"Designate world on creating lazy listener"
Landed in 71.0.3552.0

----------------------------

Historically, for the last ~10 years, the HTML attributes had no connection to the isolated world of content scripts in extensions (only DOM properties could be mapped into isolated world) but suddenly it has been changed thus breaking the existing workflows.

Please revert it ASAP.
 
test-ext.zip
750 bytes Download

Comment 1 by haraken@google.com, Dec 5

Owner: yukishiino@chromium.org

Comment 2 by haraken@google.com, Dec 5

Cc: haraken@chromium.org
Components: Blink>Bindings
Status: Assigned (was: Unconfirmed)

Comment 3 by yukishiino@chromium.org, Dec 5

Status: WontFix (was: Assigned)
This is an intended fix.  Isolated worlds are expected to be isolated from the main world (= the web page), and script in setAttribute is expected to run in the world where it was set.

    function myfunc() { alert('myfunc'); }
    let elem = document.createElement('span');
    elem.setAttribute('onclick', 'myfunc()');
    document.body.appendChild(elem);

I think that it's less surprising and more secure that the above code runs |myfunc|.  If the above code didn't run |myfunc|, it would be surprising for extension developers in general, I think.

Comment 4 by woxxom@gmail.com, Dec 5

Just in case, one of the workflows currently broken: augmenting a page by creating additional DOM elements with onXXXX attributes that point to the existing page functions (not in the isolated world). For the past 10 years extensions could do it easily from their content scripts, but now it's possible only by injecting a page-level script just to create these elements, and this is a huge nuisance by itself.

Comment 5 by woxxom@gmail.com, Dec 5

yukishiino@ your assessment seems to be based on a common confusion between DOM properties and HTML attributes.
DOM properties like onclick are isolated because they point to the actual JS code, which is isolated by worlds.
HTML5 attributes are pure strings so they have no connection to the isolated world of content scripts.
Please don't break things that worked for 10 years as expected.

Comment 6 by woxxom@gmail.com, Dec 5

rdevlin.cronin@ you're my last hope.
Please unbork it.

Comment 7 by yukishiino@chromium.org, Dec 5

I understand the difference between IDL attributes and content attributes and intentionally made the change.  IMHO, IDL attributes are expected to be shared among all the worlds as Blink has a single DOM tree among the worlds.  However, scripting is expected to be isolated.  Actually, both of IDL attributes and content attributes are shared among worlds, and only scripting is isolated here.

We apologize for your inconvenience, but I believe this is a better behavior especially from security perspectives (script isolation is pretty important).

Comment 8 by woxxom@gmail.com, Dec 5

I think beliefs shouldn't break 10 years of past experience just like that of a mature feature that wasn't causing problems.
First you need to really investigate it and only then break the things, documenting the change publicly along the way.

Previously content scripts could choose the isolated world:
  element.onclick = runContentScriptFunction;
or the page world:
  element.setAttribute('onclick', 'runPageFunction()');

Now extensions can't as easily augment a page, and injecting a page-level script isn't always easy.
Sometimes it's even impossible due to new bugs that prevent execution of such scripts and that no one fixes and ETA is unknown.

Comment 9 by woxxom@gmail.com, Dec 5

re 7
> scripting is expected to be isolated

That's a string, not code. Extension developers know the difference since the inline code doesn't work in extension pages by default. The new behavior seems to lack integrity even within its own logic - it treats strings in onXXXX attributes in a special way (subverting the standard HTML attribute behavior) under the pretext of enhancing security, but you should see the same logic also applies to <script> elements with code inside textContent property created inside a content script. If you change that too, extensions will be totally unable to augment pages, in other words, it'll kill tons of extensions.

Comment 10 by woxxom@gmail.com, Dec 6

Why is it okay for one programmer to break a feature unilaterally in such a huge project like Chromium, which isn't a personal project, without investigating the impact on the existing extensions, without proving this was indeed a bug, without informing the developer community, without giving a grace period where warnings could be displayed by devtools? Makes me wonder if that was a part of some bounty hunting where "fixing" such "security holes" gives some career points.

Comment 11 by woxxom@gmail.com, Dec 8

I wonder should I open a bunch of new bugs to show how the new behavior (in addition to being conceptually wrong) breaks real extensions?
Like infinite scrolling enablers that append the html of the next page to the end of the current one.

Comment 12 by riv.anti...@gmail.com, Dec 10

Please revert this. This is causing unnecessary headache without having given any warning in advance.

> I think that it's less surprising and more secure that the above code runs |myfunc|.  If the above code didn't run |myfunc|, it would be surprising for extension developers in general, I think.

This is wrong. The function set with setAttribute is actually visible in the source code of the page, if the onclick attribute points to a function that is defined in the content script, this is just confusing. Everything that is visible on the page should point to functions that are defined on the page itself. That's how it was for over a decade and that's what actually makes sense.

I don't see how elements calling functions that are defined on the page itself could constitute a security risk. There is nothing you can do with that behaviour that you can't do with just injecting in-page scripts. So what's the point?

Comment 13 by riv.anti...@gmail.com, Dec 10

Every extension developer knows that if you set the HTML attribute onclick to a function, it points to a function that is defined on the page itself. If you want to call a function that is defined in the content script, you do that with DOM properties. There is nothing surprising about this behaviour, it's just logical.

Comment 14 by riv.anti...@gmail.com, Dec 10

Apologies for the triple post but I don't seem to be able to edit my comments.

So let me get this straight. If I have this on a web page:

    <script type="text/javascript">
      function myfunc() {
        alert("this is myfunc");
      }
    </script>

    <span onclick="myfunc();">myfunc</span>


And I make a content script with this content:

    function myfunc() { alert("this is also myfunc"); }
    let elem = document.createElement('span');
    elem.setAttribute('onclick', 'myfunc()');
    document.body.appendChild(elem);

I will get this as result on the web page:

    <script type="text/javascript">
      function myfunc() {
        alert("this is myfunc");
      }
    </script>

    <span onclick="myfunc();">myfunc</span>
    <span onclick="myfunc();">myfunc</span>

So I will have two identical looking elements that seemingly call the same function, but actually they don't. Is there anyone other than yukishiino@ who thinks that this behaviour is "logical", "not surprising" and that it's the behaviour one would expect?

Comment 15 by yukishiino@chromium.org, Dec 11

I apologize for the lack of a warning in advance and extension developers' inconvenience.  However, IMHO, scripting isolation is very important for security perspectives and I'd like extensions to refrain from script injection via setAttribute.  I'd like to ask a favor to stop the script injection via setAttribute.

Comment 16 by woxxom@gmail.com, Dec 11

It looks like you didn't understand the arguments made above.
Your change doesn't improve anything neither security-wise, nor anything-wise.

Comment 17 by woxxom@gmail.com, Dec 11

In addition to solving nothing, your change *creates* a security breach for extensions that work with the page HTML like the abovementioned infinite scrollers since the web HTML added by such an extension will run in the isolated world of the content script.

Comment 18 by woxxom@gmail.com, Dec 11

Where's Devlin Cronin when the [isolated] world needs him most?
Aren't there any other experts on this topic?
This situation is becoming ridiculous.

Comment 19 by haraken@chromium.org, Dec 11

Status: Started (was: WontFix)
I discussed the issue with yukishiino@.

Here are my takeaways:

- In terms of the semantics, the old behavior and the new behavior both make sense.
- There is no clear security benefit in making the change.
- Therefore, I now think that it would be better to revert the change.

I'd propose the following decision tree.

if (it is easy to revert the change from stable) {  // I'm not sure if it's easy. Stable channel is already shipped. The revert causes conflict.
  revert it from stable;
} else if (this is high priority) {  // I'm not sure about this. The priority of this bug is P3.
  revert it from stable;
} else {
  keep it;
}

Comment 20 by yukishiino@chromium.org, Dec 11

First of all, I apologize for all the things.

About the revert, I think that we can make the behavior reverted in M72, but I don't think we can in M71.  In order to make a stable merge, we need to get people (including release managers but not limited to) convinced that the issue has a huge and/or wide impact on the end users and also it's not easy to workaround the issue.  Otherwise, people don't get convinced that the merge is necessary and never approve a merge.  I think that I'm not capable to do it.  I'm sorry.

Comment 21 by rdevlin....@chromium.org, Dec 11

Sorry for the delay on this, folks.  A few thoughts.

First off, thank you woxxom@ for reporting the issue, and even bisecting it down to the proper revision.

Please also note that we should keep discussions respectful at all times - we're all trying to make the extensions and the web better. :)

haraken@ put it very well that both of these cases, potentially, make sense.  But since one has been existing behavior for many years, we should not suddenly change that.  I think the fact that this happened was a failure of process on our part - *if* we decide the change is worthwhile, we shouldn't make it without first reaching out to developers.  However, since this behavior is important to extensions, it *should* have been tested in extensions code - which it wasn't.  That, I think, is the main point of failure here.  In a codebase as large as Chromium, we rely heavily on test coverage to ensure things don't (unintentionally) break, since it's borderline impossible to ascertain everything that could conceivably be affected.  So it wasn't the fault of the developer making the change. :) (And again, to that point, please make sure to keep discussions focused on the product.)

Moving forward, I agree we should revert this change.  I also think we should try and revert it on M71 stable, if we can.  yukishiino@, it seems like this is a relatively small change - do you think it would be safe to revert on master, or are there dependencies that would complicate it?  If it's clean, I think we can (and should) make the case to the release managers.  Otherwise, this does end up causing a lot of churn for developers and users, who either need to find a workaround or accept broken extensions for the duration of M71.

woxxom@, riv.antigame@, do you know of any popular extensions that are affected by this?  Knowing the scope of impact to end users would be very useful in justifying the merge to 71.

Comment 22 by yukishiino@chromium.org, Dec 11

For safety of behavioral revert, I think that I can create a small patch that wouldn't cause any crash.  I investigated how the change happened and I think I found it.  I'll make a patch tomorrow (it's very late in my timezone now).

Comment 23 by yukishiino@chromium.org, Dec 12

Labels: -Pri-3 ReleaseBlock-Stable Pri-1
As per #c21, I mark this issue as ReleaseBlock-Stable.

Comment 24 by yukishiino@chromium.org, Dec 12

Cc: gov...@chromium.org

Comment 25 by yukishiino@chromium.org, Dec 12

Labels: OS-Chrome OS-Linux OS-Mac OS-Windows

Comment 26 by gov...@google.com, Dec 12

Labels: Target-71 Target-72 Target-73 M-73 M-72 M-71

Comment 27 by yukishiino@chromium.org, Dec 12

Labels: OS-Android

Comment 28 by bugdroid1@chromium.org, Dec 12

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9aa69dc23c484b7364f446e0acebbb32f0f3e3f7

commit 9aa69dc23c484b7364f446e0acebbb32f0f3e3f7
Author: Yuki Shiino <yukishiino@chromium.org>
Date: Wed Dec 12 12:15:10 2018

v8binding: Run content attributes injected by Extensions in the main world

Run script in content attributes that are injected by Chrome
extensions (content scripts) in the main world in order to keep
backward compatibility.

Change-Id: Ie50d62e4a19c050dd2187638b383680bde492e2a
Bug:  912069 
Reviewed-on: https://chromium-review.googlesource.com/c/1373283
Commit-Queue: Yuki Shiino <yukishiino@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#615860}
[modify] https://crrev.com/9aa69dc23c484b7364f446e0acebbb32f0f3e3f7/third_party/blink/renderer/bindings/core/v8/script_event_listener.cc
[modify] https://crrev.com/9aa69dc23c484b7364f446e0acebbb32f0f3e3f7/third_party/blink/web_tests/http/tests/security/isolatedWorld/no-bypass-main-world-csp-for-delayed-execution-expected.txt
[modify] https://crrev.com/9aa69dc23c484b7364f446e0acebbb32f0f3e3f7/third_party/blink/web_tests/http/tests/security/isolatedWorld/resources/no-bypass-main-world-csp-for-delayed-execution.js

Comment 29 by bugdroid1@chromium.org, Dec 12

Project Member
Labels: merge-merged-3638
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c92df47299c1612552f04395abf7209fc930589d

commit c92df47299c1612552f04395abf7209fc930589d
Author: Yuki Shiino <yukishiino@chromium.org>
Date: Wed Dec 12 12:23:23 2018

v8binding: Run content attributes injected by Extensions in the main world

Run script in content attributes that are injected by Chrome
extensions (content scripts) in the main world in order to keep
backward compatibility.

Change-Id: Ie50d62e4a19c050dd2187638b383680bde492e2a
Bug:  912069 
Reviewed-on: https://chromium-review.googlesource.com/c/1373283
Commit-Queue: Yuki Shiino <yukishiino@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#615860}(cherry picked from commit 9aa69dc23c484b7364f446e0acebbb32f0f3e3f7)
Reviewed-on: https://chromium-review.googlesource.com/c/1373933
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Cr-Commit-Position: refs/branch-heads/3638@{#3}
Cr-Branched-From: e515fd072abf3526c45755303acbc9017f30a29d-refs/heads/master@{#615716}
[modify] https://crrev.com/c92df47299c1612552f04395abf7209fc930589d/third_party/blink/renderer/bindings/core/v8/script_event_listener.cc
[modify] https://crrev.com/c92df47299c1612552f04395abf7209fc930589d/third_party/blink/web_tests/http/tests/security/isolatedWorld/no-bypass-main-world-csp-for-delayed-execution-expected.txt
[modify] https://crrev.com/c92df47299c1612552f04395abf7209fc930589d/third_party/blink/web_tests/http/tests/security/isolatedWorld/resources/no-bypass-main-world-csp-for-delayed-execution.js

Comment 30 by yukishiino@chromium.org, Dec 12

Landed a behavioral revert in ToT and Canary.

Comment 31 by gov...@chromium.org, Dec 12

Thank you Yuki for quick fix.

Per offline group chat, we decided to move forward with today's stable release and pick up this change for next M71 respin (if any) after change is well baked/verified in lower channels.

Comment 32 by rob@robwu.nl, Dec 14

Labels: -Type-Bug Type-Bug-Regression

Comment 33 by jmukthavaram@chromium.org, Dec 17

woxxom@,
As per C#29 & 30, could you please confirm on the bug fix?
Thanks..!

Comment 34 by woxxom@gmail.com, Dec 17

re 33, yeah and this is awesome! I confirm. Thanks.

Comment 35 by yukishiino@chromium.org, Dec 17

Labels: Merge-Request-71 Merge-Request-72
Status: Fixed (was: Started)
I request merge the fix at #c28 to M72 and M71.  I've confirmed the fix with 73.0.3642.0 on Mac.

Comment 36 by sheriffbot@chromium.org, Dec 17

Project Member
Labels: -Merge-Request-72 Merge-Review-72 Hotlist-Merge-Review
This bug requires manual review: M72 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: govind@(Android), kariahda@(iOS), djmm@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 37 by gov...@chromium.org, Dec 17

Labels: -Merge-Review-72 Merge-Approved-72
Approving merge to M72 branch 3626 based on comment #35. For M71 merge approval, lets wait until M72 coverage.

Comment 38 by gov...@chromium.org, Dec 17

Pls merge your change to M72 branch 3626 ASAP so we can pick it up for this week beta release, RC cut tomorrow, Tuesday @ 1:00 PM PT.

Comment 39 by bugdroid1@chromium.org, Dec 18

Project Member
Labels: -merge-approved-72 merge-merged-3626
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ca189bffcc0eff3cc3c29040b0e9e674573bc663

commit ca189bffcc0eff3cc3c29040b0e9e674573bc663
Author: Yuki Shiino <yukishiino@chromium.org>
Date: Tue Dec 18 06:47:21 2018

v8binding: Run content attributes injected by Extensions in the main world

Run script in content attributes that are injected by Chrome
extensions (content scripts) in the main world in order to keep
backward compatibility.

(cherry picked from commit 9aa69dc23c484b7364f446e0acebbb32f0f3e3f7)

Change-Id: Ie50d62e4a19c050dd2187638b383680bde492e2a
Bug:  912069 
Reviewed-on: https://chromium-review.googlesource.com/c/1373283
Commit-Queue: Yuki Shiino <yukishiino@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#615860}
Reviewed-on: https://chromium-review.googlesource.com/c/1381361
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Cr-Commit-Position: refs/branch-heads/3626@{#418}
Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}
[modify] https://crrev.com/ca189bffcc0eff3cc3c29040b0e9e674573bc663/third_party/blink/renderer/bindings/core/v8/script_event_listener.cc
[modify] https://crrev.com/ca189bffcc0eff3cc3c29040b0e9e674573bc663/third_party/blink/web_tests/http/tests/security/isolatedWorld/no-bypass-main-world-csp-for-delayed-execution-expected.txt
[modify] https://crrev.com/ca189bffcc0eff3cc3c29040b0e9e674573bc663/third_party/blink/web_tests/http/tests/security/isolatedWorld/resources/no-bypass-main-world-csp-for-delayed-execution.js

Comment 40 by yukishiino@chromium.org, Dec 18

Locally confirmed the fix on branch 3626 (M72).

Comment 41 by cr-audit...@appspot.gserviceaccount.com, Dec 19

Project Member
Labels: Merge-Merged-72-3626
The following revision refers to this bug: 
https://chromium.googlesource.com/chromium/src.git/+/ca189bffcc0eff3cc3c29040b0e9e674573bc663

Commit: ca189bffcc0eff3cc3c29040b0e9e674573bc663
Author: yukishiino@chromium.org
Commiter: yukishiino@chromium.org
Date: 2018-12-18 06:47:21 +0000 UTC

v8binding: Run content attributes injected by Extensions in the main world

Run script in content attributes that are injected by Chrome
extensions (content scripts) in the main world in order to keep
backward compatibility.

(cherry picked from commit 9aa69dc23c484b7364f446e0acebbb32f0f3e3f7)

Change-Id: Ie50d62e4a19c050dd2187638b383680bde492e2a
Bug:  912069 
Reviewed-on: https://chromium-review.googlesource.com/c/1373283
Commit-Queue: Yuki Shiino <yukishiino@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#615860}
Reviewed-on: https://chromium-review.googlesource.com/c/1381361
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Cr-Commit-Position: refs/branch-heads/3626@{#418}
Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}
Cc: phanindra.mandapaka@chromium.org
 Issue 920098  has been merged into this issue.

Comment 43 by woxxom@gmail.com, Jan 12

BTW, this is what I meant by a proper investigation and subsequent announcement about a breaking change:
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-extensions/BQEhuSNVhHw/

Comment 44 by gov...@chromium.org, Jan 24

Labels: -Merge-Request-71 Merge-Rejected-71
We're not planning any further M71 releases, rejecting merge to M71. Please request merge to M72 if not already.

Comment 45 by yukishiino@chromium.org, Jan 25

The fix has already been merged to M72 (3626).  Thanks.

Sign in to add a comment