Open Bug 1631047 Opened 5 years ago Updated 4 years ago

Extensions can inject scripts while the debugger is paused, causing .cssRules access failure

Categories

(DevTools :: Debugger, defect, P3)

x86_64
Windows 10
defect

Tracking

(Not tracked)

People

(Reporter: saschanaz, Unassigned)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file)

Attached file cssrules-domexception.html (deleted) —
  1. Install https://darkreader.org/
  2. Open the attachment
  3. You should see .olga and .frau. Now open the DevTools and refresh.
  4. Press F8 twice to pass the debugger statement.

Expected: You should see the same result
Actual: CSSStyleSheet.cssRules getter: Not allowed to access cross-origin stylesheet instead of .olga.

Summary: Attaching debugger causes .cssRules access failure if `media=screen` → Attaching debugger causes .cssRules access failure

Turns out this only happens with Dark Reader extension, I'm not sure which component this should go.

So the error does not happen without debugger? That sounds then possible a debugger issue.

Component: DOM: Core & HTML → Debugger
Flags: needinfo?(krosylight)
Product: Core → DevTools

So the error does not happen without debugger?

Exactly.

Flags: needinfo?(krosylight)

The priority flag is not set for this bug.
:jlast, could you have a look please?

For more information, please visit auto_nag documentation.

Flags: needinfo?(jlaster)

Harald, could you try reproducing this?

Flags: needinfo?(jlaster) → needinfo?(hkirschner)
Flags: needinfo?(hkirschner)

I can easily reproduce using the STR and test case from comment #0
Note that you need the Dark Reader extension installed
https://addons.mozilla.org/en-US/firefox/addon/darkreader/?src=search

The page is showing: CSSStyleSheet.cssRules getter: Not allowed to access cross-origin stylesheet error

Brad, is this something we could fix or it's issue with the add-on?

Honza

Blocks: devtools-csp
Flags: needinfo?(bwerth)

Interesting. I can replicate. The error that we are hitting here is a SecurityError, and it's intentionally thrown. In fact, we have a test to ensure that we throw this exception under these circumstances. But of course this exception isn't being thrown when the debugger isn't involved, so there's something about the debugger that's changing the triggering principal from -- I think -- the document's principal to the debugger's principal.

Edit: Wrong! The principal is being set to the extension principal. See Logan's comment 9 for the correct explanation.

So, some solutions:

  1. Make the debugger use the same principal as the document. This probably breaks a bunch of other things. It's certainly very useful for calls made by the debugger to have different, elevated privileges compared to the document's privileges.
  2. Make the execution of calls from the debugger be trivial pass-throughs to how the code would normally execute. That probably prevents the debugger from acting as a debugger (catching exceptions, etc).
  3. Skip the cross-origin access check for calls made by the debugger. This is the easiest thing to do if it's actually possible. The issue is that what we're calling a SecurityError may actually be something more like an "architecture error". With the pref "fission.autostart.enabled" set, each process is only guaranteed to have access to objects from a single origin. We may not be able to give direct access to cross-origin data, regardless of the privileges of the caller.
  4. If option 3 isn't possible, send a cross-process data packet with the requested data to the caller with appropriate privileges.

A lot going on here. I'd like to get an opinion from Jason on what's going on with the debugger in this situation.

Flags: needinfo?(bwerth) → needinfo?(jlaster)

I'll defer to logan with respect to the debugger's principal.

Flags: needinfo?(jlaster) → needinfo?(loganfsmyth)

Alright I've finished digging into this. This is basically an run-to-completion issue.

The normal flow without the debugger

  1. Page loads
  2. Page skips debugger; since devtools are not open.
  3. Script accessing stylesheets and updating HTML content. Since the stylesheets are inline in the page, they have the page content principal and can be accessed from page JS without throwing.
  4. The extension injects a script into the page using chrome.tabs.executeScript.
  5. That extension assigns .textContent on the stylesheet (https://github.com/darkreader/darkreader/blob/14eb81109bf68e0116efd4f91e1f22c20ffd8ca4/src/inject/dynamic-theme/index.ts#L51), which replaces the internal stylesheet representation, and most importantly, the new stylesheet has the extension principal, meaning that page content can't access the stylesheet anymore. (https://searchfox.org/mozilla-central/rev/3d39d3b7dd1b2be30692d4541ea681614e34c786/layout/style/Loader.cpp#1566)

This means for instance that if the test script waited to update the HTML until after step 5, e.g.

  setTimeout(() => {
    getCSSRules(style1, log1);
    getCSSRules(style2, log2);
  }, 1000);

the page will get the same error, without devtools having to be open.

So now we get to the run-to-completion issue. While we are paused at the debugger; statement, step 3 is blocked from running because we are paused, but step 4 can still run because we're still processing requests from extensions to inject scripts into the page, which means that step 4 and 5 run during step 2, and by the time we reach step 3, the stylesheet has been replaced with one that the page content isn't allowed to access.

Blocks: dbg-r2c
Flags: needinfo?(loganfsmyth)
Summary: Attaching debugger causes .cssRules access failure → Extensions can inject scripts while the debugger is paused, causing .cssRules access failure

That is really useful analysis, thank you. So a way to fix this is to ensure that the calls made from the debugger have a sufficiently elevated principal that they can access stylesheets modified by an extension. Interestingly, we have a test that ensures that this access is prevented when the extension creates the stylesheet.

Kris, should devtools be able to access rules in an inline stylesheet that are modified/overwritten by an extension? Our principal currently prohibits this, and test_ext_contentscript_triggeringPrincipal.js implies that this is intentional when the extension creates the inline stylesheet. Is this a different case that should be allowed?

Flags: needinfo?(kmaglione+bmo)

It's not that devtools can't access the stylesheet, it's that the page content can't access a stylesheet injected by an extension, and devtools causes the script to be injected at a different time. I think all of the principals are correct, but chrome.tabs.executeScript should not inject scripts while the tab is paused.

(In reply to Logan Smyth [:loganfsmyth] from comment #11)

It's not that devtools can't access the stylesheet, it's that the page content can't access a stylesheet injected by an extension, and devtools causes the script to be injected at a different time. I think all of the principals are correct, but chrome.tabs.executeScript should not inject scripts while the tab is paused.

Got it, but your example in comment 9 says that the error would occur without devtools if the page tried to access its inline style rules after the injection. The tab isn't paused in that case.

I was assuming it was expected behavior that page content can't access a stylesheet injected by an extension, but fair question

It is the "ensure that the calls made from the debugger have a sufficiently elevated principal" and "should devtools be able to access rules in an inline stylesheet that are modified/overwritten by an extension" points that I'm contesting because the debugger/devtools is not the issue.

Yes, I'm convinced we're agreeing. The point I'm adding is that it's not required that the devtools use the same principal as the document. We break this intentionally to fix cases like Bug 1391994, and we may want to break it again here. In my mind, the question is "does a user have an expectation that devtools would work in this case?" If so, and if it doesn't expose a security issue, we should make it work.

we may want to break it again here

This is what I'm not following. Devtools code is not the code that is throwing the exception in this case, so I'm not sure how the principal of the devtools comes into play.

(In reply to Logan Smyth [:loganfsmyth] from comment #15)

we may want to break it again here

This is what I'm not following. Devtools code is not the code that is throwing the exception in this case, so I'm not sure how the principal of the devtools comes into play.

We would have to do something like give devtools a special principal for at least the duration of the cssRules getter. Ideally, I'd like to have a devtools principal created when devtools is instantiated, and we use that principal for everything the tools do, but I haven't been able to figure out a good way to do that.

We would have to do something like give devtools a special principal for at least the duration of the cssRules getter.

I feel like we're talking past eachother. The .cssRules access happens in Step 3 in my description, which is after all of the devtools frames have left the stack. There is nothing about that code that involved devtools.

The issue is that the debugger; statement causes the extension-injected script to be executed earlier than it would otherwise have been, meaning that when step 3 runs, the stylesheet has already been replaced by the extension and thus has the extension principal.

(In reply to Logan Smyth [:loganfsmyth] from comment #17)

The issue is that the debugger; statement causes the extension-injected script to be executed earlier than it would otherwise have been, meaning that when step 3 runs, the stylesheet has already been replaced by the extension and thus has the extension principal.

And in that circumstance, if devtools had an elevated principal when invoking the cssRules getter, this would work correctly and not hit the SecurityError. To make that happen, if we change devtools to always have an elevated principal when invoking the cssRules getter, I'm asserting that causes no problems in other use cases. That's the solution I'm proposing.

if devtools had an elevated principal when invoking the cssRules getter

But it doesn't invoke cssRules at all, a normal script does. I agree with you that if it did, giving devtools have a special principal could be useful, but that is not the issue that this bug is addressing.

(In reply to Logan Smyth [:loganfsmyth] from comment #19)

if devtools had an elevated principal when invoking the cssRules getter

But it doesn't invoke cssRules at all, a normal script does. I agree with you that if it did, giving devtools have a special principal could be useful, but that is not the issue that this bug is addressing.

Ah, I see. I was misunderstanding what was going on. I thought this was failing to display the rules when the Rule View was trying to inspect them. I'm sorry for introducing confusion.

Does this mean this is working as intended, barring the run-to-completion issue you identified in comment 9? Do we close this bug? Mark it as a duplicate?

Flags: needinfo?(kmaglione+bmo)

No problem. I think we should leave this open to track the run to completion issue for chrome.tabs.executeScript.

Severity: -- → S3
Priority: -- → P3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: