Closed Bug 1424474 Opened 6 years ago Closed 6 years ago

Changing attributes using the web inspector seems to ignore security checks

Categories

(DevTools :: Inspector, defect)

58 Branch
defect
Not set
normal

Tracking

(firefox-esr52 unaffected, firefox57 unaffected, firefox58+ fixed, firefox59+ fixed)

RESOLVED FIXED
Firefox 59
Tracking Status
firefox-esr52 --- unaffected
firefox57 --- unaffected
firefox58 + fixed
firefox59 + fixed

People

(Reporter: qab, Assigned: bzbarsky)

References

(Depends on 1 open bug)

Details

(Keywords: csectype-priv-escalation, regression, sec-low, Whiteboard: [post-critsmash-triage])

Attachments

(5 files)

Attached file svger.html
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/62.0.3202.94 Safari/537.36

Steps to reproduce:

View attached PoC and follow instructions.

Video: https://www.youtube.com/watch?v=9WvOCYI4FZc

Tested on nightly 59.0a1 (2017-12-08) (64-bit)


Actual results:

If we change say the SRC attribute of an iframe using web inspector, it will load it without security checks. For example, try to change a src of an iframe to a file: uri within web content (using inspector), it will load the file uri.

The attached PoC shows that this can also be used to perform an XSS on framable cross origin website.


Expected results:

Security checks should be applied.
Patrick, can you (suggest someone working on devtools) take a look at this? :-)
Component: Untriaged → Developer Tools
Flags: needinfo?(pbrosset)
Here is where attributes are changed when users edit them in the inspector: https://searchfox.org/mozilla-central/rev/281d99b3d342b71d0653b872345761372d38f5c1/devtools/server/actors/inspector.js#740-757

This is literally just doing node.setAttribute. I don't know what it should be doing instead.

I'll spend some more time looking into this. Keeping the needinfo for now.
Status: UNCONFIRMED → NEW
Component: Developer Tools → Developer Tools: Inspector
Ever confirmed: true
I think its because the inspector has chrome privilege and somehow the command inherits this priv?

For example, if you open about:about (chrome page) and then insert an iframe pointing to normal web content (say //localhost/poc.html) and within poc.html contains an empty frame. If you attempt to change the src of the inner most iframe (the one inside poc.html) you can make it point to anything because the command is coming from chrome priv page.
If you try to do it from within //localhost/ whilst its framed in about:about it will not allow it.
(In reply to Abdulrahman Alqabandi from comment #3)
> I think its because the inspector has chrome privilege and somehow the
> command inherits this priv?

I suspect that this is right and we somehow end up checking script privilege levels when there's JS on the stack, or something like that. Boris, any idea if that could be right, and if we could stop doing that in DOM land rather than trying to fix all the devtools consumers to never touch webpages without jumping through some extra "deprivilege-me" hoops? Compartments mostly fixed this, I thought, but apparently not completely, which I'm finding a bit worrying...
Flags: needinfo?(bzbarsky)
[Tracking Requested - why for this release]: Security regression

Ugh.  This is fallout from bug 1406278.  In particular, 57 is _not_ affected.

> we somehow end up checking script privilege levels when there's JS on the stack, or something like that.

Right.

> and if we could stop doing that in DOM

I think we could change nsContentUtils::GetAttrTriggeringPrincipal to ignore aSubjectPrincipal if it's system.  That shouldn't affect the use cases of bug 1406278 while fixing this bug as filed...  But it's pretty annoying that we have _some_ places where we want to use the caller principal but not others, effectively.

Past that, we should at least think about the bits added in bug 1415352 and what should happen there for system callers.  Probably we want to ignore the caller principal if system there too.

Kris, are there any other bits we need to worry about?

Is it perhaps worth adding a webidl annotation like NeedsCallerPrincipal but that only passes through non-system principals, or only pass through expanded principals, or something else, to make it harder to mess these things up?
Blocks: 1406278
Flags: needinfo?(bzbarsky) → needinfo?(kmaglione+bmo)
Oh, and compartments are not quite relevant here.  The whole point is that inspector is accessing the DOM node via Xray, which has the system code as the subject principal if you ask for the subject principal.  It's just that DOM used to not consider the subject principal for these loads and now it does.
sec regression in 58, tracking.
I'm not sure this is really a security issue. It's not really much different than we allow for content script callers, and the only way it's really exploitable is via self-XSS.

But I do think we should probably fix it, if only because it's unexpected, and likely to be confusing.

Ignoring system principals seems like a good solution. I don't particularly care whether we do it in GetAttrTriggeringPrincipal or at the WebIDL layer.
Flags: needinfo?(kmaglione+bmo)
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #8)
> I'm not sure this is really a security issue. It's not really much different
> than we allow for content script callers, and the only way it's really
> exploitable is via self-XSS.
> ..

Self XSSing on attacker controlled website that results in XSS on facebook.com is a security issue. Also, there are already security measures in place to prevent normal self XSS (not allowing paste in web console, remove javascript: on paste inside address bar) which this bug 'bypasses'.
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
I'm still thinking about a sane way to write a test for this, but I did test it manually with <iframe src> bits.
Attachment #8937541 - Flags: review?(kmaglione+bmo)
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #11)
> I'm still thinking about a sane way to write a test for this, but I did test
> it manually with <iframe src> bits.

If you used a browser chrome test you could presumably load a page and then use ContentTask to modify an attribute from the relevant system-privileged content script, in much the same way devtools do here, and verify that it throws and/or produces a security error in the console (which you could verify with a console listener) ?
Comment on attachment 8937540 [details] [diff] [review]
part 1.  Add a way to request only non-system subject principals in webidl bindings

Review of attachment 8937540 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bindings/Codegen.py
@@ +7458,1 @@
>                      // Initializing a nonnull is pretty darn annoying...

This comment doesn't feel super appropriate anymore.

::: dom/bindings/Configuration.py
@@ +615,5 @@
>  
>          def maybeAppendNeedsSubjectPrincipalToAttrs(attrs, needsSubjectPrincipal):
> +            if (needsSubjectPrincipal is not None and
> +                needsSubjectPrincipal is not True and
> +                needsSubjectPrincipal[0] != "NonSystem"):

We should check that the length is 1 here too. We can probably do that with needsSubjectPrincipal == ["NonSystem"]

@@ +623,3 @@
>              if needsSubjectPrincipal is not None:
>                  attrs.append("needsSubjectPrincipal")
> +                if needsSubjectPrincipal is not True:

This isn't super clear. I think it would be nicer if you wrote it as "needsSubjectPrincipal == ["NonSystem"]".
Attachment #8937540 - Flags: review?(nika) → review+
> you could presumably load a page and then use ContentTask to modify
> an attribute from the relevant system-privileged content script

Sure.  The problem is to what.  On Mac, for example, I had to turn off content sandboxing to get a file:// URL to load without my patches...  (I didn't actually see any error messages in the console with or without this patch, which surprised me a bit.)

> This comment doesn't feel super appropriate anymore.

Good point.  Will nix.

> We should check that the length is 1 here too

Yep, done using your suggestion.

> This isn't super clear. I think it would be nicer if you wrote it as "needsSubjectPrincipal == ["NonSystem"]".

Done.
Flags: needinfo?(pbrosset)
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #14)
> > you could presumably load a page and then use ContentTask to modify
> > an attribute from the relevant system-privileged content script
> 
> Sure.  The problem is to what.  On Mac, for example, I had to turn off
> content sandboxing to get a file:// URL to load without my patches... 

A chrome: test URI? If that doesn't work, you could potentially open a new file: tab (which will load in the file content process, which has access), then load the http URI in that tab afterwards (IIRC we don't flip processes), then frame the file: thing. But maybe that's starting to get into diminishing returns...
Comment on attachment 8937541 [details] [diff] [review]
part 2.  Make sure that we only pass non-system subject principals to setters/methods that later use that principal for loading security checks

Review of attachment 8937541 [details] [diff] [review]:
-----------------------------------------------------------------

If I were writing the tests, I'd probably change the existing triggering principal tests to run a third time, in a system-privileged sandbox, and expect the page principal as the triggering principal.
Attachment #8937541 - Flags: review?(kmaglione+bmo) → review+
OK, I spent a while looking into the solution from comment 16, and I can't figure out how to create the sandbox in the process where the actual window it needs to poke lives....

I'll poke about some more, but for now would like to get this landed and a followup filed on test.
Comment on attachment 8937541 [details] [diff] [review]
part 2.  Make sure that we only pass non-system subject principals to setters/methods that later use that principal for loading security checks

[Security approval request comment]
How easily could an exploit be constructed based on the patch?  Not very easily; you have to figure out what system code would be modifying DOM bits and then get it to do that.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?  No.

Which older supported branches are affected by this flaw? 58

If not all supported branches, which bug introduced the flaw? Bug 1406278.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?  I expect this to backport cleanly to 58.

How likely is this patch to cause regressions; how much testing does it need?  Regression risk is low, but bitrot risk is high; I'd like to get this landed ASAP.
Attachment #8937541 - Flags: sec-approval?
The PoC cleverly hides the fact that the iframe contents are facebook by time you paste the js URL, but it's still a convoluted and unlikely self-XSS/social-engineering attack.
Depends on: 1426531
I filed bug 1426531 on adding a test.
Flags: in-testsuite?
(In reply to Daniel Veditz [:dveditz] from comment #19)
> The PoC cleverly hides the fact that the iframe contents are facebook by
> time you paste the js URL, but it's still a convoluted and unlikely
> self-XSS/social-engineering attack.

Shouldn't sec-moderate be more appropriate given "Client bugs that might have high or critical results but require the user perform unusual or complex actions to trigger." from https://wiki.mozilla.org/Security_Severity_Ratings ? I feel this bug fits within that description.
Flags: needinfo?(dveditz)
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #17)
> OK, I spent a while looking into the solution from comment 16, and I can't
> figure out how to create the sandbox in the process where the actual window
> it needs to poke lives....
> 
> I'll poke about some more, but for now would like to get this landed and a
> followup filed on test.

Something like SpecialPowers.Cu.Sandbox(...) from either the page script or the extension content script should do it.

Oh, right, but it's an xpcshell test... that makes it trickier...
> This was backed out for assertions in test_bug494328.html.

Fun, so this is interaction with the fix for bug 1222924.

We're landing in FeedWriter.js doing an attribute set on an about:feeds page.  An image is getting its "src" attribute set to a moz-icon: URI.

Before fixing either bug 1222924 or bug 1406278, we landed in CheckLoadURIFlags, entered the URI_IS_UI_RESOURCE, branch, saw that we have ALLOW_CHROME, saw that targetScheme wasn't "chrome" or "resource", and allowed the load;

Then we fixed bug 1406278, and never even got to CheckLoadURIFlags, because the triggering principal became system.

Then we fixed bug 1222924 and removed the "allow if not chrome or resource" bit.

Then the patch in this bug made us reach CheckLoadURIFlags again.  We reach the same codepath, discover "targetScheme" is not "resource", so skip checking with the resource protocol handler, but _do_ call into the chrome registry to check whether access is allowed.  And then we get the asserts, because our URI is not "chrome".

I think the right fix here is to deny if the scheme is not "resource" or "chrome" in the security manager code.  Will post a patch to that effect and run it all through try.
MozReview-Commit-ID: I3DyrYGpGC2
Attachment #8938234 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8938234 [details] [diff] [review]
part 1.  Make sure we don't call into the chrome registry's AllowContentToAccess with non-chrome URLs

Review of attachment 8938234 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the detailed explanation. Also, ugh, this code.
Attachment #8938234 - Flags: review?(gijskruitbosch+bugs) → review+
Flags: needinfo?(dveditz)
Attachment #8937541 - Flags: sec-approval? → sec-approval+
Comment on attachment 8937540 [details] [diff] [review]
part 1.  Add a way to request only non-system subject principals in webidl bindings

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1406278
[User impact if declined]: Possible to give sites extra privileges
   accidentally by using the inspector.
[Is this code covered by automated tests?]: Not yet.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: Yes.  See comment 0.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: Not really.
[Why is the change risky/not risky?]: It just restores, for system callers,
   the behavior we had in 57.  There's a lot of code changing around, but
   that's what it comes down to.
[String changes made/needed]: None.
Attachment #8937540 - Flags: approval-mozilla-beta?
Hi Dan,
I would like to get your opinion on this one. It's sec-low and the size of the patch is huge. Do we have to take this in 58 or let it ride the train on 59?
Flags: needinfo?(dveditz)
I would like to just note that the huge patch is pretty mechanical: it just changes the signatures of a bunch of methods and adds the new annotation this bug implements in all the places bug 1406278 touched.
Comment on attachment 8937540 [details] [diff] [review]
part 1.  Add a way to request only non-system subject principals in webidl bindings

Fix a sec regression in 58. Beta58+.
Attachment #8937540 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Part 2 has conflicts due to bug 1419270. Please post a rebased patch or request approval on that bug also.
Flags: needinfo?(dveditz) → needinfo?(bzbarsky)
(In reply to Ryan VanderMeulen [:RyanVM] from comment #34)
> Part 2 has conflicts due to bug 1419270. Please post a rebased patch or
> request approval on that bug also.

It would be nice to have this uplifted to avoid other merge conflicts. It should be pretty low-risk.
I requested approval for part 1 of bug 1419270, which is what part 2 here depends on.  Backporting that is safer than rewriting the part 2 patches here to work without that infrastructure.

I will post a rebased part 3 which is on top of just part 1 of bug 1419270, without part 2 of that bug.
Though backporting part 2 of bug 1419270 is definitely also an option that is reasonably safe.
Flags: needinfo?(bzbarsky)
(In reply to Gerry Chang [:gchang] from comment #31)
> Hi Dan, I would like to get your opinion on this one.

As a regression fix it would be nice to take in 58 even though it is sec-low. The size of the patch is misleading: a lot of it are mechanical changes.
Whiteboard: [post-critsmash-triage]
Group: firefox-core-security → core-security-release
Group: core-security-release
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.