Leak of post-redirect url in error stacktrace when script loaded via importScripts in Web Workers
Categories
(Core :: DOM: Workers, defect, P1)
Tracking
()
People
(Reporter: obmihail, Assigned: asuth)
References
()
Details
(Keywords: csectype-sop, reporter-external, sec-high, Whiteboard: [reporter-external] [client-bounty-form] [sec-survey][adv-main79+][adv-ESR78.1+] [adv-esr68.11+])
Attachments
(6 files, 3 obsolete files)
116.91 KB,
image/png
|
Details | |
47 bytes,
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-beta+
RyanVM
:
approval-mozilla-esr78+
dveditz
:
sec-approval+
|
Details | Review |
290.51 KB,
text/plain
|
Details | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-esr68+
|
Details | Review |
282 bytes,
text/plain
|
Details |
Hello. I found the way how to owner of "a" domain can know the post-redirect URL, loaded from "b" domain.
Here is the example: https://obmi.me/pocs/webworkerexample_5b52f35f-807f-4255-8429-f61a588f404c
The scenario:
- The page in domain 'a.com' has a Web Worker.
- The worker calls importScripts function with the link to the redirector page in domain 'b.com'
- The redirector in 'b.com' domain returns a redirect to the valid javascript script with sensitive data in url, like authorization token, csrf token, etc. (It's popular flow in many websites)
- The worker calls some function from loaded script, which throws an error and catch it via 'try{...}catch(e){}'
AR: The 'e.stack' property contains the full url of the script loaded from 'b.com' and contains the sensitive data.
ER: ??? Maybe 'e.stack' must contain the first url (which was used as argument in importScripts call) or cutted url.
Reproduced in Firefox 75.0
Comment 1•5 years ago
|
||
baku, this looks like it's a repeat of bug 1160890, but via the error's stack property instead of message, or something? Chrome seems to be doing the same thing. Can you take a look?
Reporter: did you also report to chrome/chromium, and if so, can you link the ticket so we can avoid 0-day'ing each other by opening up one of the reports when one of us fixes? :-)
Updated•5 years ago
|
(In reply to :Gijs (he/him) from comment #1)
baku, this looks like it's a repeat of bug 1160890, but via the error's stack property instead of message, or something? Chrome seems to be doing the same thing. Can you take a look?
Reporter: did you also report to chrome/chromium, and if so, can you link the ticket so we can avoid 0-day'ing each other by opening up one of the reports when one of us fixes? :-)
Hello! Yes, that case desribed in this report: https://bugs.chromium.org/p/chromium/issues/detail?id=1074317
Comment 3•5 years ago
|
||
(In reply to Mikhail from comment #2)
Hello! Yes, that case desribed in this report: https://bugs.chromium.org/p/chromium/issues/detail?id=1074317
Thank you!
Comment 4•5 years ago
|
||
asuth, do you have time to work on this?
Assignee | ||
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Assignee | ||
Comment 5•5 years ago
|
||
Updated•5 years ago
|
Assignee | ||
Comment 6•5 years ago
|
||
The test identified a problem in both explicit filename and the stack. Have fix for the filename, fixing the stack issue.
Assignee | ||
Comment 7•5 years ago
|
||
The stack issue turned out to be a test issue that limitations in our test/test reporting infrastructure made more subtle than it would ideally be; I'd fixed the issue. (This is on the known issues list of our Test Improvements Planning effort.)
I'm currently finishing getting full coverage of the worker error reporting space. Right now I'm looking into why the ErrorEvent dispatched on the parent (window) Worker binding loses the error. Pernosco trace for this is https://pernos.co/debug/kuMCB51y5OYK_jNNDCaJtA/index.html
Assignee | ||
Comment 8•5 years ago
|
||
(In reply to Andrew Sutherland [:asuth] (he/him) from comment #7)
Right now I'm looking into why the ErrorEvent dispatched on the parent (window) Worker binding loses the error. Pernosco trace for this is https://pernos.co/debug/kuMCB51y5OYK_jNNDCaJtA/index.html
Ah, and this is of course intentional per spec. Step 1 of https://html.spec.whatwg.org/multipage/workers.html#runtime-script-errors-2 explicitly initializes the error to null. (I got a little thrown off by the fact that we do capture and propagate the stack, but that's for the benefit of the devtools console.) Finishing up test now.
Updated•5 years ago
|
Assignee | ||
Comment 9•5 years ago
|
||
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 10•5 years ago
|
||
Security Analysis
If a worker imports a cross-origin resource that performs a redirect that successfully parses as valid JS and contributes any callable functions to the global during top-level evaluation (which need not fully complete successfully) that can be made to throw (easy), then the worker can call the function and the thrown exception will reveal the final URI of the channel that loaded, potentially exposing private information.
This is most relevant in cases where the cross-origin site uses 1) SameSite=None cookies and 2) exposes a redirector that will act on the cookie. SameSite=Lax and SameSite=Strict cookies will NOT be sent by the script load. HTTP AUTH credentials obtained as a result of a 401 response will probably also be sent. (In other words, we do not perform an anonymous load using the LOAD_ANONYMOUS load flag, but may benefit from other protections that inhibit the credentials from being sent.)
Attempts to import scripts that do not parse as valid JS will not leak redirects.
Attempts to import scripts that parse as valid JS but throw before contributing any function will not leak redirects.
Assignee | ||
Comment 11•5 years ago
|
||
Comment on attachment 9159319 [details]
Bug 1634872 - Correct spec compliance issue. r=baku
Security Approval Request
- How easily could an exploit be constructed based on the patch?: Pretty easily. The fix is minimal and explicitly runs an
IsSameOrigin
check. The fix can be disabled and then the included tests run to better understand what's happening, but I think the fix is so simple/obvious it's not clear the tests are really increasing the risk. (Also, the tests are very dry reading so there is a chance they might put would-be attackers to sleep.)
The question is how much of a concern an exploit would be. See comment 10 for additional context on the risk scenario (in addition to comment 0).
- Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes
- Which older supported branches are affected by this flaw?: all
- If not all supported branches, which bug introduced the flaw?: None
- Do you have backports for the affected branches?: No
- If not, how different, hard to create, and risky will they be?: Easy. Applying the change to older branches is straightforward, but there's been some code drift within range of the fix hunk. Even if we want the tests on trunk, we wouldn't want to uplift them.
I just went to start preparing a beta fix and found that bug 1640982 is preventing me from building beta locally, so I'm going to wait and see what's desired. (In general, this is a recurring problem for me... it frequently requires some kind of manual intervention with rustup overrides to be able to build anything but trunk.)
- How likely is this patch to cause regressions; how much testing does it need?: Very unlikely. Anything that depends on the pre-patch behavior is presumably an attack.
Comment 12•5 years ago
|
||
We're out of betas this cycle, so this would have to go directly into an Fx79 RC if we wanted this fix to ship ASAP, and that makes me nervous. How much downside would there be to letting this ride Fx80/78.2esr/68.12esr instead?
Assignee | ||
Comment 13•5 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM] from comment #12)
We're out of betas this cycle, so this would have to go directly into an Fx79 RC if we wanted this fix to ship ASAP, and that makes me nervous. How much downside would there be to letting this ride Fx80/78.2esr/68.12esr instead?
There isn't a known vulnerable site at this time, but that doesn't rule out the potential existence of vulnerable sites? That said, any potentially vulnerable site would already be pretty close to having a vulnerability accessible from the main thread window/document without concern over redirects because the redirect target has to parse as valid JS and then define a function. (And if you're serving up valid JS containing private information to any site with a script tag, that's already exploitable unless the post-redirect URL is the only leak.) Valid-looking HTML won't. CSV (a common historical concern for muted scripts) isn't much of a concern either unless it literally contains JS that does assignments into the global scope for side-effect because the comma operator puts things in an expression context which means function blah() {} is an expression not a statement that contributes to the enclosing (global) scope.
Which is to say, I'm not sure this is an emergency landing situation, but I'd like the security team's input as much of my understanding of OAuth and similar flows has fallen out of my brain, but I can page it in if needed.
Previously known vulnerable site
The vulnerable site in bug 1160890 (the predecessor of this bug) was https://analytics.twitter.com/ which at that time would issue a redirect in response to the cookie-bearing request to the user's page at https://analytics.twitter.com/user/[USER_NAME]/home
. This is no longer a problem because for twitter because:
- The HTTP redirect is now a valid HTTP 200 load which contains a valid HTML document containing:
<!DOCTYPE html>
<html>
<head>
<meta charset="utf-8">
<meta http-equiv="refresh" content="0; url=/user/[USER_NAME]/home">
</head>
</html>```
- The above document will fail to parse as valid JS. This prevents the worker from learning anything about the redirect because the page will not define any functions in the global scope that can be evaluated and throw.
- All valid-looking HTML will fail to parse as valid JS. Both `<!DOCTYPE html>` and `<html>` as the lead-ins will fail to parse and the page will not define any functions in the global scope.
- Note that on nightly we have SameSite=lax enabled by default for cookies, and that avoids the cookie being sent as part of the load, but that continues to be nightly-only.
Because the targets of privacy-revealing redirects are most likely to be HTML documents, in general we're in good shape. The primary concern ends up being endpoints that would process cookies and redirect the user to a valid JS file that includes private information in the URL. But as noted up top, that ends up potentially being exploitable regardless of the redirect. (Like, if there was an OAUTH flow that only required the user's cookie as a bearer token.)
Comment 14•5 years ago
|
||
[Tracking Requested - why for this release]:
Chrome shipped the fix for their version of this bug on Tuesday (crbug 1074317). It's an easily understood and exploited issue should there be a vulnerable site, though that might not be common given the preconditions.
Will hold off on sec-approval decision until we come to an agreement w/release managers: I don't want the patch checked in now if we're not able to ship it now.
Comment 15•5 years ago
|
||
The patch looks simple enough that if we're reasonably confident that this won't cause any major web compat issues that regular Nightly baking would help suss out, I guess we can try to shoehorn this into the RC builds next week. That said, we're going to need the uplift-ready patches rebased and attached to this bug ASAP to make that possible (not to mention a trunk-ready patch minus the tests assuming we want those to land later). Note that we'd need to land this on Beta, ESR78, and ESR68.
Dan & Andrew, are you both on board with that? I really wish I better understood how bad this would be if it had to wait a couple weeks to land and ride the next cycle instead. It's difficult for me to figure out a risk vs. reward here.
Reporter | ||
Comment 16•5 years ago
|
||
(In reply to Andrew Sutherland [:asuth] (he/him) from comment #13)
There isn't a known vulnerable site at this time, but that doesn't rule out the potential existence of vulnerable sites?
if it'simportant, originally, I've found a CSRF in some google services which allows to hijack user credentials via this bug and the Google VRP team told me to create this issue for browsers. I can't share the details of that bug now, but just want to say that it's not a theoretical\synthetic problem. I didn't check other sites, but the flow when a site redirects client with some secrets in the url to any resource in the same origin is not rare.
Assignee | ||
Comment 17•5 years ago
|
||
(In reply to Mikhail from comment #16)
if it'simportant, originally, I've found a CSRF in some google services which allows to hijack user credentials via this bug and the Google VRP team told me to create this issue for browsers. I can't share the details of that bug now, but just want to say that it's not a theoretical\synthetic problem. I didn't check other sites, but the flow when a site redirects client with some secrets in the url to any resource in the same origin is not rare.
That sounds very bad. Thank you for the extra context! This definitely means it's more urgent to land this sooner.
Can you clarify if the loaded resource was defining functions in the global scope, or was this a slightly different situation that behaved differently in browsers other than Firefox? Also, I'm assuming the resource in question wasn't intended to be loaded by a worker in the first place (as the script would have no way to determine its own post-redirect URL, unless the redirect was a server implementation detail and not intended for use by the client)?
Going to start producing patches sans tests for all branches now.
Reporter | ||
Comment 18•5 years ago
|
||
(In reply to Andrew Sutherland [:asuth] (he/him) from comment #17)
Can you clarify if the loaded resource was defining functions in the global scope
Yes. But I used two points:
- When the loaded resource was defining a function
- When the loaded resource was calling a function, which was defining before (in worker's context).
An example of both cases you can view here: https://obmi.me/pocs/workerff_0c905e45-1222-4727-8801-8326de8719da
*just open the page and you will see the output of both stacks in console
Also, I'm assuming the resource in question wasn't intended to be loaded by a worker in the first place (as the script would have no way to determine its own post-redirect URL, unless the redirect was a server implementation detail and not intended for use by the client)?
Yes, but it's no matter. In most cases I used the standart js libraries, like jquery\angular or parts of them. Any valid javascript can be loaded in the worker's context (the missing objects, like a window, document, etc. can be easiely mocked in the worker's context).
Also the JSONP endpoints can be used instead of JS resources.
Comment 19•5 years ago
|
||
Thanks for the added context. Sounds like this is worth the last-minute uplift into the RC builds this cycle then.
Assignee | ||
Comment 20•5 years ago
|
||
Updated•5 years ago
|
Updated•5 years ago
|
Assignee | ||
Comment 21•5 years ago
|
||
Updated•5 years ago
|
Assignee | ||
Comment 22•5 years ago
|
||
Comment 23•5 years ago
|
||
Can you spell out what the new behavior you actually implemented? I looked at the patch and it not immediately clear to me. Do we just expose the filename instead of the post-redirect (final) URL? For the normal script loader we have been looking into standardizing our behavior of using the pre-redirect URL in bug 1624449.
Assignee | ||
Comment 24•5 years ago
|
||
Assignee | ||
Comment 25•5 years ago
|
||
(In reply to Tom Schuster [:evilpie] from comment #23)
Can you spell out what the new behavior you actually implemented? I looked at the patch and it not immediately clear to me. Do we just expose the filename instead of the post-redirect (final) URL? For the normal script loader we have been looking into standardizing our behavior of using the pre-redirect URL in bug 1624449.
Short answer: Yes, we just expose the filename as passed to importScripts
instead of the post-redirect (final) URL.
More Patch Details
Elaborating on https://phabricator.services.mozilla.com/D81114#2572513:
- Existing behavior was to update the URI to be the final URI in all cases.
- There wasn't a lot of rationale for this from bug archaeology, but https://bugzilla.mozilla.org/show_bug.cgi?id=1340694 and code comments indicates that at some point this became load bearing for the main script. (It may no longer be load bearing.)
- The URI pre-update was the URI from the call to
importScripts
.- The URI is only evaluated relative to the Worker's Base URI just prior to creating the channel. This derived URI doesn't get propagated anywhere (other than the final channel URI logic that the patch changes).
- The primary goal for the fix as a secbug was to eliminate the cross-origin redirect leak and to ensure test coverage across various exception-throwing situations for workers (in Gecko specific tests because of the Gecko-specific fields and their contents on exceptions and out of paranoia for the potential loss of coverage due to automatically upstreamed/downstreamed WPT tests).
- The fix attempts to maintain the status quo for same-origin purposes (show post-redirect value), and falls back to the original filename for cross-origin purposes (with minimal code changes).
Spec Musings
From a spec perspective, fetch a classic worker-imported script, step 3 Let response be response's unsafe response.
, step 6 Let muted errors be true if response was CORS-cross-origin, and false otherwise.
, and step 7 Let script be the result of creating a classic script given source text, settings object, response's url, the default classic script fetch options, and muted errors.
in conjunction with report an error would seem to suggest that the right answer is that the filename is the post-redirect value unless the fetch ever went cross-origin, in which case the script should be muted (but the muted script location is post-redirect, which is problematic given that I think Gecko mutes in fewer cases than the spec calls for and I'm not sure the serializing a request origin logic comes into play).
Relatedly, note that my fix on this patch currently is only evaluating the final URI for cross-origin purposes. So a redirect from Worker origin A to cross-origin B back to origin A would reveal the final URI when per the above spec interpretation, it should not because the response tainting was ratcheted to "opaque" for the cross-origin "no-cors" processing in main fetch. In Necko channel-space, something like nsITimedChannel.allRedirectsSameOrigin might be used.
Script-Loader Un-Forking
For the worker script loader we want to overhaul it in terms of fetch (Internal)Response instances. See https://bugzilla.mozilla.org/show_bug.cgi?id=1613912#c4 for the most concrete plan laid out thus far in terms of implementing bug 1351231 for performance reasons. The general performance idea is that we can avoid pathological IPCStream paths through the system induced by ServiceWorkers doing pass-through fetches of the form fetchEvent.respondWith(fetch(fetchEvent.request))
and even cloned variants through such an approach.
Would it make sense for the normal main-thread script loader to move in such a direction as well, potentially allowing for more unification? (Responses do support alternate data streams.)
(Setting needinfo because it seemed like you meant to needinfo me and given the opaque nature of secbug bugmail, it's probably nice to get clarity that it was a human saying something notable. Also your expertise is appreciated on whether this is the appropriate patch to land/etc. and I should have thought to more directly involve you in the fix planning; I was depending on you already being in the cc list more than I should have.)
Assignee | ||
Comment 26•5 years ago
|
||
Summary of the Patches Up
Fixes:
- https://phabricator.services.mozilla.com/D81114 - For trunk. Reviewed directly by baku.
- Does include a minor test change wherein the ID of a console listener message changed from
"chrome://mochitests/content/browser/dom/workers/test/sharedWorker_console.js",
to"sharedWorker_console.js"
in https://searchfox.org/mozilla-central/rev/b2395478c6adf6e5b241be1610fb1d920ed995ed/dom/workers/test/browser_consoleSharedWorkers.js#45 because the cross-origin check we are using is a cross-origin check and not a principal subsumes check.- The isCrossOrigin check was chosen because it's most conservative/straightforward/analytically simple, but in the case of the test the SharedWorker has the system principal which doesn't have a URI associated with it and so the IsSameOrigin check returns false. Because we are trying to avoid having workers have the system principal (see bug 1163753) and the chrome protocol has a mapping scheme opaque to searchfox, this seemed like an acceptable change.
- Does include a minor test change wherein the ID of a console listener message changed from
- https://phabricator.services.mozilla.com/D84100 - Fx79 / Current beta: This is actually just the trunk fix which cleanly grafts (via automated merge). Feel free to graft that directly as desired.
- https://phabricator.services.mozilla.com/D84099 - ESR78. This is actually just the trunk fix which cleanly grafts (via automated merge). Feel free to graft that directly as desired.
- https://phabricator.services.mozilla.com/D84108 - ESR68. Branch-specific fix. Although the trunk fix grafts syntactically, the result will not build. See https://phabricator.services.mozilla.com/D84108#2602397 for an explanation of differences. But in short this uses the Subsumes check that I avoided using in the trunk fix and is consistent with the application of our muting logic.
Tests:
- https://phabricator.services.mozilla.com/D84096 should land in the future, possibly in another bug (like bug 1624449)?
Comment 27•5 years ago
•
|
||
Thanks for writing up this detailed explanation. I think we should definitely take this patch to get this fixed. From my understanding using the file-name as passed to importScripts should always be safe, because you obviously have to know it :)
It's a bit unfortunate that now we basically have three different muting behaviors, but we should really work on this outside of a sec-bug. I get the feeling there isn't much interest in actually improving the spec here, just because this is such an edge case and it's easy to introduce some new leak.
To summarize:
- The current spec says to use an empty string for url.
- If script's muted errors is true, then set message to "Script error.", urlString to the empty string, line and col to 0, and errorValue to null.
- We use the pre-redirect URL in the normal ScriptLoader. I wrote this comment on the HTML github.
- This patch: Using just the filename instead of a whole URL.
Comment 28•5 years ago
|
||
Comment on attachment 9159319 [details]
Bug 1634872 - Correct spec compliance issue. r=baku
sec-approval+
Comment 29•5 years ago
|
||
Pushed to autoland:
https://hg.mozilla.org/integration/autoland/rev/47e6cde6f609082a9e9616ed7e19a91fdbc51095
Andrew, please nominate for uplift to all the branches when you're able :)
Assignee | ||
Comment 30•5 years ago
|
||
Comment on attachment 9159319 [details]
Bug 1634872 - Correct spec compliance issue. r=baku
Beta/Release Uplift Approval Request
- User impact if declined: Potential credential theft. This is a cross-origin redirect leak which reporter has indicated has had successful exploitation.
- Is this code covered by automated tests?: No
- Has the fix been verified in Nightly?: No
- Needs manual test from QE?: No
- If yes, steps to reproduce:
- List of other uplifts needed: None
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): Straightforward fallback to the filename if the import crossed origins, status quo in same-origin case.
- String changes made/needed: n/a
ESR Uplift Approval Request
- If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-high
- User impact if declined: Potential credential theft. This is a cross-origin redirect leak which reporter has indicated has had successful exploitation.
- Fix Landed on Version:
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): Straightforward fallback to the filename if the import crossed origins, status quo in same-origin case.
- String or UUID changes made by this patch:
Assignee | ||
Comment 31•5 years ago
|
||
Comment on attachment 9164605 [details]
Bug 1634872 - Correct spec compliance issue. ESR68 version. r=baku
ESR Uplift Approval Request
- If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-high
- User impact if declined: Potential credential theft. This is a cross-origin redirect leak which reporter has indicated has had successful exploitation.
- Fix Landed on Version: 80
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): Straightforward fallback to the filename if the import crossed origins, status quo in same-origin case.
- String or UUID changes made by this patch: n/a
Updated•5 years ago
|
Updated•5 years ago
|
Comment 32•5 years ago
|
||
Comment on attachment 9159319 [details]
Bug 1634872 - Correct spec compliance issue. r=baku
Approved for 79.0rc1, 78.1esr, and 68.11esr.
Updated•5 years ago
|
Comment 33•5 years ago
|
||
uplift |
Comment 34•5 years ago
|
||
uplift |
Comment 35•5 years ago
|
||
uplift |
Comment 36•5 years ago
|
||
Comment 37•5 years ago
|
||
As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.
Please visit this google form to reply.
Comment hidden (offtopic) |
Comment hidden (offtopic) |
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Comment 41•5 years ago
|
||
Comment 42•5 years ago
|
||
Thank you for reporting this bug!
Please see attached advisory draft and let us know if you wanted to be credited differently.
A meeting in the following weeks is going to decide on your bounty.
Updated•5 years ago
|
Reporter | ||
Comment 43•5 years ago
|
||
(In reply to Frederik Braun [:freddy] from comment #42)
Thank you for reporting this bug!
Please see attached advisory draft and let us know if you wanted to be credited differently.
A meeting in the following weeks is going to decide on your bounty.
Hello. Could you credited me as "Mikhail Oblozhikhin" ?
Comment 44•5 years ago
|
||
Updated•5 years ago
|
Assignee | ||
Comment 45•4 years ago
|
||
Marking in-testsuite? to track that there's an already-reviewed test improvement on this bug that wants to land when this gets opened up.
Updated•4 years ago
|
Updated•2 years ago
|
Updated•2 years ago
|
Updated•2 years ago
|
Updated•2 years ago
|
Updated•10 months ago
|
Comment 47•10 months ago
|
||
Comment 48•10 months ago
|
||
bugherder |
Assignee | ||
Updated•10 months ago
|
Updated•8 months ago
|
Description
•