Screenshot exception with mutation observers
Categories
(Firefox :: Screenshots, defect)
Tracking
()
People
(Reporter: dveditz, Assigned: Gijs)
References
Details
(Keywords: reporter-external, sec-low, Whiteboard: [post-critsmash-triage][adv-main83+])
Attachments
(6 files, 1 obsolete file)
Screenshot 2020-10-19 at 15.44.42.png
Filing this for kaisersoze, WIP for a screenshot bug. Currently if you take a screenshot of the testcase page using one of the two buttons in the top right (whole page or visible window) it will throw an exception in the extension itself. Seems to be trying to grab the screenshot iframe and getting there too early, but could be close to something like a priv-esc into the extension context if he gets the timing right.
|
||
Comment 1•4 years ago
|
||
Here is a revised testcase, that shows in web console that the contents of the screenshot extension become accessible. It's working for me locally on release, right now the screenshot extension doesn't even load for me in nightly.
To see it work, right click choose screenshot, and then open the web console.
|
|
||
Comment 2•4 years ago
|
||
If I would have had time, I planned on hijacking the event listeners possibly gaining access to the browser part of the extension. From there we would see. I believe i may not have the time though.
|
Reporter | |
Comment 3•4 years ago
•
|
||
This only accesses the content that is injected into the page, right? Unfortunately there's no way for extensions to do better so they can't really trust their stuff once it's in the page. See bug 1340930.
Of course, if the extension trusts it's injected content unwisely that can certainly lead to a security bug, but simply accessing the DOM bits aren't that.
[Sidenote: if possible, it would help if your testcases had basic instructions in them. Like in this revised testcase there's a button that says "Click me" but clicking it is not part of the test. If it were part of the test it is not clear if it was to be clicked before or after the screenshot, or really that screenshotting is involved at all.]
I've cc'd you on bug 1414937 that results from that because it seems similar to what you've done here. Also on bug 1389707 which is a spoof built on top of it. Those are known issues. A novel result would be getting the extension code itself to do something unexpected, or injecting script into the extension context (not content scripts).
|
|
||
Comment 4•4 years ago
|
||
The button and elements are there for the fact that the preview iframe will not initialize unless it has something to preview(ie elements).
I'll do more on this if it's not a security issue, then leave it as be and I will follow up and prove it. There's also two other possible attack vectors but I'll leave those for the future.
|
|
||
Comment 5•4 years ago
|
||
Let me clarify, IMHO patching this would be proof of violation of defense in depth. If this was not to be patched(I think that's what I prefer) then I'm sure with more time I can access the execution scope of the extension itself. The two other iframes contain a final check to verify that in fact they have properly loaded the extension URL, the preview iframe does not include this check for some reason(not sure why).
Either way is fine with me, I have a lot going on but will get back to this ASAP. You guys just let me know.
|
|
||
Comment 7•4 years ago
|
||
Some follow up notes, might help people understand what is going on here. A mutation observer is being used to detect when iframes are added to a page for screenshots(nothing new). After this the preview iframe is being selected because for some reason it lacks a final check that the correct extension URI is loaded, then the srcdoc property of that iframe is being set to "<html></html>" which provides a blank document as the extension expects.
When srcdoc and the src of an iframe are both set, srcdoc overwrites the src attribute, and loads the about:srcdoc URL into the window. The window remains content accessible. After this is done to the screenshot preview iframe, it lacks a final check to see that in fact it is at the expected extension URL(which the selection and preselection iframes both have). If in fact there are visible elements in the content page to preview, it initializes the preview iframe, and injects it's content into it, which in this case is completely accessible by content.
What appeared to be the timing issue from before was actually caused by the lack of visible elements. Sorry for the button that says click me, copy pasta rushing through work.
|
Assignee | |
Comment 8•4 years ago
|
||
(In reply to Kaizer Soze from comment #5)
Let me clarify, IMHO patching this would be proof of violation of defense in depth. If this was not to be patched(I think that's what I prefer) then I'm sure with more time I can access the execution scope of the extension itself.
So I'm confused - the execution scope of the extension is mostly in a different process. Short of the extension using eval or similar, I don't see how you'd do this? Even the same-process bits (which do exist, just... there's less of it, and communication is via message passing) I'd expect to be insulated via the usual JS context and wrapper machinery.
It'd surprise me less if you could use this to cajole the extension into creating the "wrong" screenshot, or something. And that's not great, but isn't really the same as accessing the execution scope of the extension, right? Can you elaborate?
The two other iframes contain a final check to verify that in fact they have properly loaded the extension URL, the preview iframe does not include this check for some reason(not sure why).
I agree this is odd.
Emma/Sam: CC'ing you as a heads-up (also CC'd you to the other bugs Dan mentioned).
|
|
||
Comment 9•4 years ago
|
||
Once you have access to the elements, there's always a possibility of code execution within the context based on the fact that there is a lot going on, that depends on text interaction on the elements(style changes considerably). I was aiming for elements that have event listeners, not they're vulnerable per say. I just had a hunch this could be much bigger.
There's @SubstitutedCSS and a few others things that get directly inlaid to HTML(given I have not confirmed they're able to be influenced externally, or apply to the preview iframe.)
There's more there, just not for me at the moment. Take this as is for now. I'll see what I can do in the meantime.
I have in fact looked over and understood the event listeners being mapped with a sort of wrapper function and SendEvent being used to communicate messages. Take it and work on it as is, as my current situation is less than encouraging especially with everything else going on in the world.
|
|
||
Comment 10•4 years ago
|
||
Sorry if anything is off there. sendEvent substitutedCss etc. I was working from memory, back into the code some now. I can now confirm this also works on nightly, but I assume you already knew that. I will follow up with any updates.
|
|
||
Comment 11•4 years ago
|
||
I'm not seeing a way to exploit this right now, but I do see a patch to put in place for this particular case. Simple one liner. Gijs, do you know who maintains this? I need to ask if there's a reason for using an onload handler on the preview iframe with iframe.onload instead of using addEventListener?
The use of assertIsBlankDocument, and using addEventListener instead of iframe.onload would lock down anything I could see to grasp at. I can do the patch, just give me a little time to see if it breaks things(will be later).
Also, is there a reason that assertIsTrusted is using macro expansion on an untrusted event? As in
const exc = new Error(Received untrusted event (type: ${event.type}));
Since the event isn't trusted, then the type might not be what is normally expected.
|
|
Assignee | |
Comment 12•4 years ago
|
||
(In reply to Kaizer Soze from comment #11)
I'm not seeing a way to exploit this right now, but I do see a patch to put in place for this particular case. Simple one liner. Gijs, do you know who maintains this? I need to ask if there's a reason for using an onload handler on the preview iframe with iframe.onload instead of using addEventListener?
I don't know, off-hand I wouldn't have thought so. Equally, I'd expect both the onload setter and calls to addEventListener to be protected by wrapper handling (but I haven't checked). You could manually invoke the onload function, maybe, but then you could also manually dispatch an event with dispatchEvent (and write a custom class MyEvent extends Event { get type() { /* evil things here */ } } ) if so inclined, right?
The use of assertIsBlankDocument, and using addEventListener instead of iframe.onload would lock down anything I could see to grasp at. I can do the patch, just give me a little time to see if it breaks things(will be later).
Also, is there a reason that assertIsTrusted is using macro expansion on an untrusted event? As in
const exc = new Error(
Received untrusted event (type: ${event.type}));Since the event isn't trusted, then the type might not be what is normally expected.
But both event.isTrusted and event.type are properties on the DOM event so wrappers should get us the "real" DOM event property instead of any potential shadowing from a "fake" event. From a quick, non-screenshots-related test, this appears to work correctly from a chrome-privileged calling context and a non-chrome-privileged event dispatcher. It's possible that if the extension's interactions with the page happen in a scope that doesn't have a different privilege level (so same principal associated) that then there's no wrapper protection - but equally, in that case "hijacking" the extension script gives no greater power than what the page script could do itself anyway.
|
|
Assignee | |
Comment 13•4 years ago
|
||
Ah, hm, so I guess the onload setter means that you can then directly invoke the handler via node.onload("foo") with arbitrary arguments, whereas dispatchEvent enforces that the object you pass is an Event, which gets the relevant wrapper.
Though again, I'd expect that wrappers for the different principals enforce that the relevant object we get passed have to be structured-clonable, so custom getters get lost?
|
|
||
Comment 14•4 years ago
|
||
(In reply to :Gijs (he/him) from comment #13)
Though again, I'd expect that wrappers for the different principals enforce that the relevant object we get passed have to be structured-clonable, so custom getters get lost?
This is in fact the case, or at least the function is a wrapper denying all access. I'm just wondering about the choice to use iframe.onload for the preview iframe where the selection, and pre-selection iframes use addEventListener? Is it necessary for screenshots to function properly?
It's sad to say that I am not up on Phabricator, or a properly integrating eslint editor. I am attempting to prepare a patch to stop this specific case here. I will see if I have time to test the difference in event handlers.
As a side note, I'm always weary of anything that involves Error, or an exception as sometimes they're are given to confusion between real objects and wrappers.
Sorry Gijs, I'm more than a bit out of touch with all the changes that have been made in recent times. More testing is needed from my end.
|
|
||
Comment 15•4 years ago
|
||
Here is the most I'm going to have time for at the moment. I have no idea who to flag for review, or what else needs to be done at this point. Sorry for things being so rough, but this is the best I can bring to the table at the moment.
|
|
Assignee | |
Comment 16•4 years ago
|
||
(In reply to Kaizer Soze from comment #15)
Created attachment 9177460 [details] [diff] [review]
ui.js.patchHere is the most I'm going to have time for at the moment. I have no idea who to flag for review, or what else needs to be done at this point. Sorry for things being so rough, but this is the best I can bring to the table at the moment.
No worries, I'll self-needinfo to get this into phab "soon" (it's late here)
|
|
Reporter | |
Updated•4 years ago
|
|
|
||
Comment 17•4 years ago
|
||
There may be more that could be done here in terms of things we would not want to see, but I'll test that later as that patch should stop what I have in mind anyway. If I think of anything else, or stumble onto something I'll update this.
|
|
Assignee | |
Comment 18•4 years ago
|
||
I was a little puzzled by bug 1660651, but now that that's fixed...
|
|
Assignee | |
Comment 19•4 years ago
|
||
|
||
Updated•4 years ago
|
|
|
Assignee | |
Comment 20•4 years ago
|
||
I credited you as patch author as "Kaizer Soze <kaizersoze915@gmail.com>" - is that fine or would you like me to put something else?
|
|
Assignee | |
Comment 21•4 years ago
|
||
Depends on D92589
|
|
Assignee | |
Comment 22•4 years ago
|
||
(In reply to :Gijs (he/him) from comment #20)
I credited you as patch author as "Kaizer Soze <kaizersoze915@gmail.com>" - is that fine or would you like me to put something else?
I'd like to land this by the end of this week to avoid it sitting unaddressed longer than necessary, so I'm going to go ahead with these credits unless I hear otherwise by Friday UTC. I'll try and ping you out-of-band.
|
||
Comment 24•4 years ago
|
||
This needs a sec-rating before landing. Going with sec-low as per a quick skim and Gijs' suggestion.
Nice find, though!
|
|
Assignee | |
Comment 25•4 years ago
|
||
|
|
||
Comment 26•4 years ago
|
||
Comment on attachment 9179884 [details]
Bug 1665820 - tests, r?emalysz
Revision D92606 was moved to bug 1671119. Setting attachment 9179884 [details] to obsolete.
|
|
||
Comment 27•4 years ago
|
||
Could someone put a sec-bounty flag on this. I know it's a low, but it's good work IMHO.
|
|
Assignee | |
Comment 28•4 years ago
|
||
(In reply to Kaizer Soze from comment #27)
Could someone put a sec-bounty flag on this. I know it's a low, but it's good work IMHO.
The flag is already set. :-)
|
||
Comment 29•4 years ago
|
||
https://hg.mozilla.org/integration/autoland/rev/5a4b06d86f52685f2a4b51538f9ac3a7d9be265b
https://hg.mozilla.org/mozilla-central/rev/5a4b06d86f52
|
||
Updated•4 years ago
|
|
||
Comment 30•4 years ago
|
||
Could you please provide some steps to reproduce in order to verify this issue just not to miss any details?
Thanks.
|
|
Assignee | |
Comment 31•4 years ago
|
||
(In reply to Hani Yacoub from comment #30)
Could you please provide some steps to reproduce in order to verify this issue just not to miss any details?
Thanks.
- open https://bugzilla.mozilla.org/attachment.cgi?id=9176739 ("revision" testcase from this bug)
- click the "click me" button
- open the devtools web console
- clear the console
- in the page action menu, click "Take a Screenshot"
- wait a few seconds
- there should not be a console log statement that looks like this:
HTMLAllCollection { 0: html, 1: head, 2: link, 3: style, 4: title, 5: body, 6: div.preview-overlay, 7: div.preview-image, 8: div.preview-buttons, 9: button.highlight-button-cancel, … }
|
|
||
Comment 32•4 years ago
|
||
Thank you for the steps.
What I'm seeing now on the latest Firefox Nightly is a shorter array HTMLAllCollection { 0: html, 1: head, 2: body, length: 3 } and the security error appears in the console after canceling the screenshot. (please see the screenshot attached, on the left is Firefox Nightly 82.0a1 (2020-09-18) and on the right is the latest Nightly)
I'm not sure if this the expected behaviour here.
|
|
Reporter | |
Updated•4 years ago
|
|
|
Assignee | |
Comment 33•4 years ago
|
||
(In reply to Hani Yacoub from comment #32)
Created attachment 9182385 [details]
Screenshot 2020-10-19 at 15.44.42.pngThank you for the steps.
What I'm seeing now on the latest Firefox Nightly is a shorter array HTMLAllCollection { 0: html, 1: head, 2: body, length: 3 } and the security error appears in the console after canceling the screenshot. (please see the screenshot attached, on the left is Firefox Nightly 82.0a1 (2020-09-18) and on the right is the latest Nightly)
I'm not sure if this the expected behaviour here.
Sorry for the delay - yes, this is expected behaviour - the difference is that the screenshots UI has not injected anything into the dummy document created by the exploit/testcase
The other thing you can/should check is that the following error appears in the browser console (need to have "show content messages" turned on):
Error: iframe URL does not match expected blank.html
which also indicates the fix is working.
|
|
||
Comment 34•4 years ago
|
||
The other thing you can/should check is that the following error appears in the browser console (need to have "show content messages" turned on):
Error: iframe URL does not match expected blank.html
which also indicates the fix is working.
I'm not sure for the error received in the console (See attachment from comment 32) where should I see if "show content messages" is turned on?
|
|
Assignee | |
Comment 35•4 years ago
|
||
(In reply to Hani Yacoub from comment #34)
The other thing you can/should check is that the following error appears in the browser console (need to have "show content messages" turned on):
Error: iframe URL does not match expected blank.html
which also indicates the fix is working.
I'm not sure for the error received in the console (See attachment from comment 32) where should I see if "show content messages" is turned on?
This error is in the browser console (open with ctrl/cmd-shift-j), not the regular devtools console. In the browser console there's a "cog"/gear icon in the top right which brings up a menu that has an option to "show content messages".
|
|
||
Comment 36•4 years ago
|
||
Thank you for the details.
Verified as fixed on Firefox Nightly 84.0a1 (2020-10-22) on Windows 10 x64, MacOS 10.14 and on Ubuntu 20.04 x64.
|
||
Updated•4 years ago
|
|
||
Updated•4 years ago
|
|
|
||
Comment 37•4 years ago
|
||
|
|
||
Updated•4 years ago
|
|
|
Reporter | |
Updated•3 years ago
|
|
||
Updated•27 days ago
|
Description
•