Bug 1160890 (CVE-2015-7215)

Cross-origin information disclosure with error message of Web Workers importScripts()

VERIFIED FIXED in Firefox 43

Status

()

defect
VERIFIED FIXED
4 years ago
2 years ago

People

(Reporter: masatokinugawa, Assigned: baku)

Tracking

({csectype-disclosure, csectype-sop, sec-high})

37 Branch
mozilla43
Points:
---
Dependency tree / graph
Bug Flags:
sec-bounty +
in-testsuite ?

Firefox Tracking Flags

(firefox40- wontfix, firefox41- wontfix, firefox42+ wontfix, firefox43+ fixed, firefox46 verified, firefox47 verified, firefox-esr31 wontfix, firefox-esr3844+ wontfix, firefox-esr45 verified, b2g-v2.0 wontfix, b2g-v2.0M wontfix, b2g-v2.1 wontfix, b2g-v2.1S wontfix, b2g-v2.2 affected, b2g-v2.2r affected, b2g-master fixed)

Details

(Whiteboard: [b2g-adv-main2.5?][adv-main43+])

Attachments

(3 attachments, 14 obsolete attachments)

17.75 KB, patch
Details | Diff | Splinter Review
7.48 KB, patch
Details | Diff | Splinter Review
17.69 KB, text/plain
Details
Reporter

Description

4 years ago
User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:37.0) Gecko/20100101 Firefox/37.0
Build ID: 20150415140819

Steps to reproduce:

1. Go to http://vulnerabledoma.in/fx_worker_importScripts.html and click the "go" button.
2. If you are Twitter login user, you can see the following alert:

https://analytics.twitter.com/user/[USER_NAME]/home

If not, you will see "https://analytics.twitter.com/about".

This means that Firefox leaks redirect information to unrelated site.


Actual results:

Firefox includes redirect information in error message.


Expected results:

Firefox should not include redirect information in error message.

Comment 1

4 years ago
Posted file Testcase HTML (obsolete) —

Updated

4 years ago
Status: UNCONFIRMED → NEW
Component: Untriaged → DOM: Workers
Ever confirmed: true
Flags: needinfo?(khuey)
Product: Firefox → Core
Reporter

Comment 2

4 years ago
@Benjamin
This bug is similar to https://bugzilla.mozilla.org/show_bug.cgi?id=947592 .
I think proper severity is sec-high because Bug 947592 severity is it.
Flags: needinfo?(khuey) → needinfo?(bent.mozilla)
Andrea, do you want to take a shot at this?
Flags: needinfo?(bent.mozilla) → needinfo?(amarchesini)
Assignee

Comment 4

4 years ago
Posted patch crash8.patch (obsolete) — Splinter Review
We don't check if the principal is able to load the URI. This seems required from the spec. Wondering why CheckLoadURIWithPrincipal doesn't do this.

Probably we should start the merging of the Fetch() code with this.
Flags: needinfo?(amarchesini)
Attachment #8632744 - Flags: review?(bugs)
Comment on attachment 8632744 [details] [diff] [review]
crash8.patch

Unrelated changes make this a but harder to review.


> using mozilla::dom::cache::Cache;
> using mozilla::dom::cache::CacheStorage;
> using mozilla::dom::Promise;
> using mozilla::dom::PromiseNativeHandler;
>-using mozilla::dom::workers::exceptions::ThrowDOMExceptionForNSResult;
this is easy 

>   } else if (aIsMainScript) {
>-    // If this script loader is being used to make a new worker then we need
>-    // to do a same-origin check. Otherwise we need to clear the load with the
>-    // security manager.
>-    nsCString scheme;
>-    rv = uri->GetScheme(scheme);
>-    NS_ENSURE_SUCCESS(rv, rv);
>-
...but here I wondered if there was some reason to have GetScheme call.
GetScheme was added in bug 699857, but
CheckMayLoad was modified in bug 341604, so GetScheme isn't needed anymore.



>     // We pass true as the 3rd argument to checkMayLoad here.
>     // This allows workers in sandboxed documents to load data URLs
>     // (and other URLs that inherit their principal from their
>     // creator.)
>     rv = principal->CheckMayLoad(uri, false, true);
>     NS_ENSURE_SUCCESS(rv, NS_ERROR_DOM_SECURITY_ERR);
>   }
>   else {
>+    rv = principal->CheckMayLoad(uri, false, false);
>+    NS_ENSURE_SUCCESS(rv, NS_ERROR_DOM_SECURITY_ERR);
So you pass false as the 3rd param. That breaks data: urls, right?


>+
>     rv = secMan->CheckLoadURIWithPrincipal(principal, uri, 0);
>     NS_ENSURE_SUCCESS(rv, NS_ERROR_DOM_SECURITY_ERR);

So the comment you remove before GetScheme says we shouldn't do same origin check


I don't see how this follows https://html.spec.whatwg.org/multipage/workers.html#dom-workerglobalscope-importscripts, which doesn't 
say there is requirement for same origin.
Attachment #8632744 - Flags: review?(bugs) → review-
Assignee

Comment 6

4 years ago
Posted patch patch 1 - small fixes (obsolete) — Splinter Review
Attachment #8632744 - Attachment is obsolete: true
Attachment #8632844 - Flags: review?(bugs)
Assignee

Comment 7

4 years ago
Posted patch patch 2 - muted error flag (obsolete) — Splinter Review
Attachment #8632845 - Flags: review?(bugs)
Comment on attachment 8632844 [details] [diff] [review]
patch 1 - small fixes

This really should go to some other bug. Otherwise tracking what should land from this bug to which all branches becomes hard. But r+ for the patch.
Attachment #8632844 - Flags: review?(bugs) → review+
Assignee

Updated

4 years ago
Assignee: nobody → amarchesini
Comment on attachment 8632845 [details] [diff] [review]
patch 2 - muted error flag

Feels a bit odd to rely on CheckMayLoad here, and not use
similar setup as what nsScriptLoader does
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsScriptLoader.cpp#1059

Also, doesn't nsScriptLoader (the one used for <script>) end up setting the flag later, right before executing so that we don't leak data even if same origin request was redirected to some other url.

The spec says 
"If the script came from a resource whose URL does not have the same origin as the origin", and that is after we've downloaded the script, not before we do the request.
Attachment #8632845 - Flags: review?(bugs) → review-
Assignee

Comment 10

4 years ago
Posted patch patch 2 - muted error flag (obsolete) — Splinter Review
Attachment #8632845 - Attachment is obsolete: true
Attachment #8632920 - Flags: review?(bugs)
Comment on attachment 8632920 [details] [diff] [review]
patch 2 - muted error flag

>+    nsCOMPtr<nsIChannel> channel = do_QueryInterface(request);
>+    MOZ_ASSERT(channel);
>+
>+    nsIScriptSecurityManager* ssm = nsContentUtils::GetSecurityManager();
>+    NS_ASSERTION(ssm, "Should never be null!");
>+
>+    nsCOMPtr<nsIPrincipal> channelPrincipal;
>+    rv = ssm->GetChannelResultPrincipal(channel, getter_AddRefs(channelPrincipal));
>+    if (NS_WARN_IF(NS_FAILED(rv))) {
>+      return rv;
>+    }
>+
>+    nsIPrincipal* principal = mWorkerPrivate->GetPrincipal();
>+    if (!principal) {
>+      WorkerPrivate* parentWorker = mWorkerPrivate->GetParent();
>+      MOZ_ASSERT(parentWorker, "Must have a parent!");
>+      principal = parentWorker->GetPrincipal();
>+    }
>+
>+    aLoadInfo.mMutedErrorFlag = channelPrincipal->Subsumes(principal);
you want principal->Subsumes(channelPrincipal); right? and that is what nsScriptLoader does.

>                         const mozilla::dom::ChannelInfo& aChannelInfo,
>                         UniquePtr<PrincipalInfo> aPrincipalInfo)
>   {
>     AssertIsOnMainThread();
>     MOZ_ASSERT(aIndex < mLoadInfos.Length());
>     ScriptLoadInfo& loadInfo = mLoadInfos[aIndex];
>     MOZ_ASSERT(loadInfo.mCacheStatus == ScriptLoadInfo::Cached);
> 
>+    nsCOMPtr<nsIPrincipal> responsePrincipal =
>+      PrincipalInfoToPrincipal(*aPrincipalInfo);
>+
>+    nsIPrincipal* principal = mWorkerPrivate->GetPrincipal();
>+    if (!principal) {
>+      WorkerPrivate* parentWorker = mWorkerPrivate->GetParent();
>+      MOZ_ASSERT(parentWorker, "Must have a parent!");
>+      principal = parentWorker->GetPrincipal();
>+    }
>+
>+    loadInfo.mMutedErrorFlag = responsePrincipal->Subsumes(principal);
ditto.



I would make mMutedErrorFlag Maybe<bool>, and then explicitly set it to true or false, and then in 
ScriptExecutorRunnable::WorkerRun assert that the value actually has been set, or default to true.
Attachment #8632920 - Flags: review?(bugs) → review+
Assignee

Comment 12

4 years ago
Posted patch patch 1 - small fixes (obsolete) — Splinter Review
Attachment #8632844 - Attachment is obsolete: true
Assignee

Comment 13

4 years ago
Posted patch patch 2 - muted error flag (obsolete) — Splinter Review
Attachment #8632920 - Attachment is obsolete: true
Assignee

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/22e8c5440405
https://hg.mozilla.org/integration/mozilla-inbound/rev/37c0bb092ec2

Andrea says this goes back a long ways and that we should backport it. Andrea, please nominate for Aurora/Beta/esr38/b2g37/b2g34 approval when you get a chance :)
Flags: needinfo?(amarchesini)
https://hg.mozilla.org/mozilla-central/rev/22e8c5440405
https://hg.mozilla.org/mozilla-central/rev/37c0bb092ec2
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Assignee

Comment 17

4 years ago
Comment on attachment 8633507 [details] [diff] [review]
patch 2 - muted error flag

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Workers
User impact if declined: user data is shared with 3rd party scripts
Testing completed: none
Risk to taking this patch (and alternatives if risky): none
String or UUID changes made by this patch: none
Flags: needinfo?(amarchesini)
Attachment #8633507 - Flags: approval-mozilla-esr38?
Attachment #8633507 - Flags: approval-mozilla-beta?
Attachment #8633507 - Flags: approval-mozilla-b2g37?
Attachment #8633507 - Flags: approval-mozilla-b2g34?
Attachment #8633507 - Flags: approval-mozilla-aurora?
Assignee

Comment 18

4 years ago
Comment on attachment 8633506 [details] [diff] [review]
patch 1 - small fixes

See previous comment.
Attachment #8633506 - Flags: approval-mozilla-esr38?
Attachment #8633506 - Flags: approval-mozilla-beta?
Attachment #8633506 - Flags: approval-mozilla-b2g37?
Attachment #8633506 - Flags: approval-mozilla-b2g34?
Attachment #8633506 - Flags: approval-mozilla-aurora?
(In reply to Andrea Marchesini (:baku) from comment #17)
> Risk to taking this patch (and alternatives if risky): none
To be honest, I don't see how you can state there is no risk in a 5.7k+2.4k patches without test?!
Could you clarify why you think there is no risk?
Help to NI Andrea per comment 19
Flags: needinfo?(amarchesini)
Assignee

Comment 21

4 years ago
This bug is not fixed correctly. I have to do more work and the spec has to be updated too:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=28961

I'll submit a new patch in the next 1 or 2 days.
Status: RESOLVED → REOPENED
Flags: needinfo?(amarchesini)
Resolution: FIXED → ---
Assignee

Updated

4 years ago
Attachment #8633506 - Flags: approval-mozilla-esr38?
Attachment #8633506 - Flags: approval-mozilla-beta?
Attachment #8633506 - Flags: approval-mozilla-b2g37?
Attachment #8633506 - Flags: approval-mozilla-b2g34?
Attachment #8633506 - Flags: approval-mozilla-aurora?
Assignee

Updated

4 years ago
Attachment #8633507 - Flags: approval-mozilla-esr38?
Attachment #8633507 - Flags: approval-mozilla-beta?
Attachment #8633507 - Flags: approval-mozilla-b2g37?
Attachment #8633507 - Flags: approval-mozilla-b2g34?
Attachment #8633507 - Flags: approval-mozilla-aurora?
This being a sec-moderate bug I don't think we need to track it. We can consider for uplift when a fix is ready. Note that we are already up to 40 beta7 so we should likely target a fix at 41.
Assignee

Comment 23

4 years ago
Posted patch patch (obsolete) — Splinter Review
I have to say that I'm not sure about this approach. bz, feedback? Thanks!
Attachment #8600885 - Attachment is obsolete: true
Attachment #8633506 - Attachment is obsolete: true
Attachment #8633507 - Attachment is obsolete: true
Attachment #8636611 - Flags: review?(bzbarsky)
Andrea, do you have the 4-item list of things we decided needed fixing here handy?  If so, can you paste it in here?
Or possibly better yet in a commit comment....
Assignee

Comment 26

4 years ago
These 4 points are from an IRC chat between me and bz:

1)  The filename should be set to the URI we started loading, not whatever the result of the various redirects was.
2)  The muted errors flag should be set if the channel result origin of the _final_ channel is not subsumed by the worker.
3)  The worker error reporter needs to do things correctly if the muted flag is set (does it already?)
4)  importScripts needs to examine the muted flag if the script throws and nerf the exception.
Comment on attachment 8636611 [details] [diff] [review]
patch

>+    // If this error has to be muted, we has to do clean the pending exception

"If this error has to be muted, we have to clear the pending exception, if any, and ..."

I assume we're guaranteed that if !loadInfos[index].mExecutionResult then the exception on the JSContext came from that loadinfo?

>+      rv.SuppressException();

This looks wrong.  Say we had an exception on aCx which we muted.  This got turned into an error on the ErrorResult and we cleared the exception, right?  But here we're just clearing the info on the ErrorResult and returning false, which will turn into an uncatchable exception.

One viable option here is to just go ahead and report the ErrorResult exception to the JSContext after we clear the previous exception.  But if not, we need to report here.

Similar issue in the debugger bits.

>+      message = NS_ConvertUTF8toUTF16(aMessage);

  CopyUTF8toUTF16(aMessage, message);

The error reporter bit doesn't look right to me.  It will change what's reported to the console as well, not just what the page sees in onerror events.  We do NOT want to sanitize what the console sees, just what the error event handlers see.

>+  postMessage(fileName2 == undefined);

Probably worth doing a bit more checking here (e.g. that the .message is what we expect).

Need to add a test for error event handlers seeing sanitized exceptions when the muted flag is set...
Attachment #8636611 - Flags: review?(bzbarsky) → review-
Assignee

Comment 28

4 years ago
> I assume we're guaranteed that if !loadInfos[index].mExecutionResult then
> the exception on the JSContext came from that loadinfo?

mExecutionResult is set to true if JS::Evaluate() succeeds.

I'm applied your comments.
> mExecutionResult is set to true if JS::Evaluate() succeeds.

No, what I meant is that we're guaranteed that once one JS::Evaluate fails on one of those loadinfos the other ones won't get executed at all?

Updated

4 years ago
Group: core-security → dom-core-security
Not tracking for esr since this is sec-moderate. If that changes, please renominate it for tracking.
[Tracking Requested - why for this release]:
Masato is right in comment 2--we typically consider redirect leaks as sec-high because of the prevalence of highly sensitive information you can gather that way. De-anonymizing users as in his testcase, or collecting single-signon tokens if those are passed via URL.
Flags: sec-bounty?
Unless I am told otherwise by sec team, it is too late for 41.
Andrea, now that this is sec-high, could you take a look and finish this bug off? Thanks.
Flags: needinfo?(amarchesini)
Assignee

Comment 34

4 years ago
Posted patch worker.patch (obsolete) — Splinter Review
Attachment #8636611 - Attachment is obsolete: true
Flags: needinfo?(amarchesini)
Attachment #8667996 - Flags: review?(bzbarsky)
Comment on attachment 8667996 [details] [diff] [review]
worker.patch

>+      JS_ClearPendingException(aCx);
>+      mScriptLoader.mRv.Throw(NS_ERROR_FAILURE);

Somewhere here we should report things to the console, no?  Otherwise there's absolutely nothing an author debugging via devtools can do to figure out what's going on.  A followup is probably OK if it happens expeditiously.

>   WorkerRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate) override
>+    if (NS_WARN_IF(rv.Failed())) {
>+      JS_ClearPendingException(aCx);

Why are we clearing the exception here?  Please document.  And again, shouldn't we at least report this exception to console?

>@@ -519,18 +522,25 @@ private:
>+      JS_ClearPendingException(aCx);

Same here.

>@@ -5615,46 +5625,48 @@ WorkerPrivate::ReportError(JSContext* aC

These changes are still wrong, just like the last time I looked at them.  The sanitization needs to happen for the thing in the error event, but NOT what we report to the console.  That means it needs to happen in ReportErrorRunnable::ReportError, not here.  r- for this part...
Attachment #8667996 - Flags: review?(bzbarsky) → review-
Assignee

Comment 36

4 years ago
Posted patch worker.patch (obsolete) — Splinter Review
I'm planning to do what you are suggesting as a follow up.
Attachment #8667996 - Attachment is obsolete: true
Attachment #8669587 - Flags: review?(bzbarsky)
Comment on attachment 8669587 [details] [diff] [review]
worker.patch

So I just tried a basic testcase:

data:text/html,<script>var%20w%20=%20new%20Worker("data:application/javascript,%&%^&%^");</script>

and this patch causes the syntax error to not be reported.  That's because in ScriptLoaderRunnable::OnStreamCompleteInternal we have an nsNullPrincipal as channelPrincipal, so it's not subsumed by principal.

Do we not inherit the principal for data: loads in workers?  If not, why not?  In any case, we should fix this, though a followup is probably OK because data: workers are not interoperably implemented.  Perhaps the simplest thing here is to never mute for toplevel worker loads, since those are already subject to same-origin policy.  Or something.  Inheriting the principal for data: seems cleaner, in case we ever introduce cross-origin workers.  In any case, this needs tests.

Also, it seems like test coverage here is generally poor.  Because this is precisely the error that I would have expected to get (incorrectly) suppressed by the JS_ClearPendingException calls you used to have in WorkerPrivate.cpp that are now removed; if I reinstate them then this modified testcase:

  <script>
  var url = URL.createObjectURL(new Blob(["%&%^&%^"]));
  var w = new Worker(url);
  </script>

no longer reports an error to the console or fires an "error" event at "w".  Please add tests for this situaton of a syntax error in a toplevel worker, since apparently we don't have any.

Speaking of test coverage, the other thing I'd like to see is what happens with error events in the following cases:

1)  The setup as in your test, but the error event handler is on the WorkerGlobalScope inside the worker, not on the worker in the parent page.  They should see the same thing, of course.

2)  A setup as follows: Page at origin A loads worker at origin A, which loads a worker at origin B (assuming that's allowed) which imports script at origin 
B which has a syntax error.  What error events do the two workers and the original patch get?
>     filename = NS_ConvertUTF8toUTF16(aReport->filename);
>+
>     line = aReport->uclinebuf;

Why the added blank line?

>+  var url1 = new URL(a.data.url);

Document that this is meant to be the "same origin" URL.

>+  var url2 = new URL(a.data.url);

And this one is the "cross origin" one.  In fact, maybe just name those variables sameOriginURL and crossOriginURL?

r=me with the nits picked, tests added, and the two followups (one to fix error reporting for data: workers and one to deal with reporting to console when we mute an error) filed.
Attachment #8669587 - Flags: review?(bzbarsky) → review+
> What error events do the two workers and the original patch get?

Original _page_, of course.
Assignee

Comment 39

4 years ago
Posted patch worker.patch (obsolete) — Splinter Review
> 2)  A setup as follows: Page at origin A loads worker at origin A, which
> loads a worker at origin B (assuming that's allowed) which imports script at
> origin 
> B which has a syntax error.  What error events do the two workers and the
> original patch get?

This is not allowed. we check if the worker script can be loaded by the 'parent' principal and this scenario fails.
Attachment #8669587 - Attachment is obsolete: true
Assignee

Comment 40

4 years ago
Follow ups: bug 1211967 and bug 1211970
Assignee

Comment 41

4 years ago
Comment on attachment 8670340 [details] [diff] [review]
worker.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

This bug is not about a crash, it's about exposing 3rd party data via error events in workers.

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

Yes: we don't mute errors when needed. Tests are included.

Which older supported branches are affected by this flaw?

All the branches.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

We can easily backport this patch.

How likely is this patch to cause regressions; how much testing does it need?

The patch touches how we report errors in workers but it doesn't break existing tests and it has a good coverage with mochitests.
Attachment #8670340 - Flags: sec-approval?
Whiteboard: [b2g-adv-main2.5?]
I somewhat disagree with parts of comment 41.  Specifically, this patch is known to cause at least two regressions: bug 1211967 and bug 1211970.  We left fixing those for followups, but if we plan to backport this we'd need to backport those fixes too.

I somewhat disagree about the extent of existing test coverage as well, given comment 37.  We added some more tests here, but I don't think anyone has sat down and actually made sure we have tests for all these error codepaths in a systematic way...

Andrea, you don't seem to have added a test for this situation:

> but the error event handler is on the WorkerGlobalScope inside the worker

in the latest attached patch.
Depends on: 1211967, 1211970
Flags: needinfo?(amarchesini)
Assignee

Comment 43

4 years ago
> > but the error event handler is on the WorkerGlobalScope inside the worker

I think I do. When "nested" is true, the worker create a sub-worker and it passes all the incoming messages to the new one. Then it propagates all that is received to the parent context (the window in this case).
All the tests are executed with nested to false and then to true.

Is this what you meant with handling the WorkerGlobalScope inside the worker?
Flags: needinfo?(amarchesini) → needinfo?(bzbarsky)
> Is this what you meant with handling the WorkerGlobalScope inside the worker?

No, I meant something like this, inside a worker loaded from the page:

  addEventListener("error", function(e) {
    e.preventDefault();
    postMessage(/* whatever, based on examining e */);
  });
  importScripts(stuff);  // or load a sub-worker and importScripts in it, 
                         // or whatever else we want to test.

and the same thing with setting onerror at toplevel instead of using an "error" event listener, to make sure we check both codepaths.  That would actually check the behavior of error events on the WorkerGlobalScope.  The testcase in the attached patch only looks at error events on Worker objects.
Flags: needinfo?(bzbarsky)
Assignee

Comment 45

4 years ago
Posted patch worker.patch (obsolete) — Splinter Review
Can you take a look at the test?
Attachment #8670340 - Attachment is obsolete: true
Attachment #8670340 - Flags: sec-approval?
Attachment #8670392 - Flags: feedback?(bzbarsky)
Comment on attachment 8670392 [details] [diff] [review]
worker.patch

I'm not sure what the "eventListener" parts of the message are used for, and it seems like instead of a "try" tristate that's a boolean-but-not we should just have a string indicating what we're trying to test or something.

Also, still need an onerror test, right?
Attachment #8670392 - Flags: feedback?(bzbarsky) → feedback+
Assignee

Comment 47

4 years ago
Posted patch worker.patch (obsolete) — Splinter Review
Attachment #8670392 - Attachment is obsolete: true
Assignee

Comment 48

4 years ago
> Also, still need an onerror test, right?

onerror on the worker? on the parent we have it.
onerror on the WorkerGlobalScope.  Specifically, note that inside a worker:

  addEventListener("error", function(e) {
    // Single argument is an ErrorEvent
  });

  onerror = function(...args) {
    // Args are various things like the error type, filename, line number, etc
  }

and we want to make sure that both the ErrorEvent and the arguments that get passed to onerror are properly sanitized or not sanitized.
Assignee

Comment 50

4 years ago
Posted patch worker.patch (obsolete) — Splinter Review
See comments 41 and 42.
Attachment #8670946 - Attachment is obsolete: true
Attachment #8671913 - Flags: sec-approval?
Thanks, that looks much better!
sec-approval+. We'll want this on branches, including ESR38, after it is on trunk. Please nominate branch patches for approval (for branches, not security).
Comment on attachment 8671913 [details] [diff] [review]
worker.patch

Oh, and can we not land the tests until after we ship a release with the fix?
Attachment #8671913 - Flags: sec-approval? → sec-approval+
Assignee

Comment 54

4 years ago
Approval Request Comment
[Feature/regressing bug #]: Workers
[User impact if declined]: cross-origin script errors will be received my 'main' origin.
[Describe test coverage new/current, TreeHerder]: tests are in a separate patch.
[Risks and why]: This patch touches how errors are handled in workers. I hope this will not generate regressions and it should not as far as we treeherder push can tell.
[String/UUID change made/needed]: none.
Attachment #8671913 - Attachment is obsolete: true
Attachment #8672622 - Flags: approval-mozilla-esr38?
Attachment #8672622 - Flags: approval-mozilla-beta?
Attachment #8672622 - Flags: approval-mozilla-aurora?
Assignee

Comment 55

4 years ago
Posted patch testsSplinter Review
Is the patch without tests the trunk patch now? If so, carry over the sec-approval+ and get this checked into trunk.
Flags: needinfo?(amarchesini)
Assignee

Comment 57

4 years ago
(In reply to Al Billings [:abillings] from comment #56)
> Is the patch without tests the trunk patch now? If so, carry over the
> sec-approval+ and get this checked into trunk.

Can I actually land it to m-i?
Flags: needinfo?(amarchesini) → needinfo?(abillings)
Yes, that's fine.
Flags: needinfo?(abillings)
Keywords: leave-open
Comment on attachment 8672622 [details] [diff] [review]
patch without tests

I am going to take that in aurora & beta (should be in 42 beta 8) as it is a sec-high and we have one beta and a rc after that. However, as you don't feel very confident about the potential regression, I wonder what we should do about esr.
Attachment #8672622 - Flags: approval-mozilla-beta?
Attachment #8672622 - Flags: approval-mozilla-beta+
Attachment #8672622 - Flags: approval-mozilla-aurora?
Attachment #8672622 - Flags: approval-mozilla-aurora+
I'm hitting conflicts on beta because bug 1180861 isn't on beta (so any line using mExnType or aExnType that this patch also touches). Is that okay for me to rebase around?
Flags: needinfo?(amarchesini)
Andrew, do you know if Andrea is around? We are late in the 42 cycle.
Flags: needinfo?(overholt)
Assignee

Comment 65

4 years ago
Yes, I'm here. I'll submit a patch for beta today.
Flags: needinfo?(overholt)
Flags: needinfo?(amarchesini)
Assignee

Comment 66

4 years ago
Posted file beta
Here the m-b patch
Wait.  What about backporting fixes for the known error-reporting regressions this bug caused?  What is the plan for those?
Flags: needinfo?(sledru)
setting the flags for the beta and aurora checkin
Bz: Don't know. Andrea probably knows better.
Flags: needinfo?(sledru) → needinfo?(amarchesini)
Assignee

Comment 71

4 years ago
This patch breaks a few things in order to fix others. There are 2 dependences: the first (bug 1211970) is already landed and it's easy to port. About the second one (bug 1211967) I'm still working on it.

I would like to have bug 1211970 backported because it's a big improvement for developers having errors correctly reported into the web console. Plus, the patch is small and simple and I doubt it will introduce regressions. For the other one I would wait until I have a working patch.

How does it sound?
Flags: needinfo?(amarchesini)
That sounds perfectly reasonable to me.  Just make sure to request those approvals.
Duplicate of this bug: 1218110
Assignee

Comment 74

4 years ago
I think it's time to land the second patch with the tests. Am I right?
Flags: needinfo?(abillings)
We are discussing backing this out instead (in bug 1211970).
Andrea, even if we weren't backing it out, we wouldn't land tests until *after* a release with the fix ships.
Flags: needinfo?(abillings)
Sylvestre would like to back this out from beta.   Let's keep it on trunk and aurora, and uplift the related work to aurora.
Flags: needinfo?(wkocher)
The merge to m-r already happened.

Backed out from both beta and release for consistency's sake:
https://hg.mozilla.org/releases/mozilla-release/rev/de3239af77ff
https://hg.mozilla.org/releases/mozilla-beta/rev/da2fdb1f0eca
Flags: needinfo?(wkocher)
Comment on attachment 8672622 [details] [diff] [review]
patch without tests

Since we ended up backing the original patch out of beta, I think we should leave this out of esr-38 for now.
Attachment #8672622 - Flags: approval-mozilla-esr38? → approval-mozilla-esr38-
Assignee

Updated

4 years ago
Duplicate of this bug: 1184310
Assignee

Comment 81

4 years ago
Can we land these patches (and the test) in the new aurora/beta?
Flags: needinfo?(lhenry)
Is this actually fixed in 43 but not 44 or 45?
Flags: needinfo?(amarchesini)
Flags: sec-bounty? → sec-bounty+
Assignee

Comment 83

4 years ago
(In reply to Al Billings [:abillings] from comment #82)
> Is this actually fixed in 43 but not 44 or 45?

Correct. We land land these patches in m-i and aurora.
Flags: needinfo?(amarchesini)
baku, Sorry I missed the needinfo.  Which patches?  This work landed in 43 and 44. We backed the patches out of 42 (which was on both m-b and m-r at that moment). 
  
Is there anything else that needs to land in 43 or 44?  

Can you check to see this is fixed?

Kamil or Matt, can you verify the fix in 43 and 44?
Is it https://bugzilla.mozilla.org/attachment.cgi?id=8672622&action=edit that still needs to land on 43, and 44?  (Without tests.)
Flags: needinfo?(lhenry)
Duplicate of this bug: 1230821
Flags: needinfo?(amarchesini)
Assignee

Comment 87

4 years ago
The patch (without test) is already in beta and aurora:

https://hg.mozilla.org/releases/mozilla-beta/rev/2e69beaa47cd
https://hg.mozilla.org/releases/mozilla-aurora/rev/2e69beaa47cd

I want to know if we can land the second patch (the tests) to m-i.
Flags: needinfo?(amarchesini)
Once we ship a release with a fix, we'll wait a week for uptake and then you can check the tests in.
Whiteboard: [b2g-adv-main2.5?] → [b2g-adv-main2.5?][adv-main43+]
Alias: CVE-2015-7215
Carsten: what was the reason for the "leave-open" keyword? We're releasing a fix in Firefox 43 so can we resolve this FIXED now?

Why hasn't this landed for the corresponding ESR 38.5 release? This sec-high bug is obviously easily rediscoverable since there have been two dupes so far.
Flags: needinfo?(cbook)
Target Milestone: mozilla42 → mozilla43
(In reply to Daniel Veditz [:dveditz] from comment #89)
> Carsten: what was the reason for the "leave-open" keyword? We're releasing a
> fix in Firefox 43 so can we resolve this FIXED now?
> 
> Why hasn't this landed for the corresponding ESR 38.5 release? This sec-high
> bug is obviously easily rediscoverable since there have been two dupes so
> far.

Hey Dan,

the reason for the leave open was the patch landed without the tests, but i guess we can mark this as fixed now. 

For esr38, this has approval minus here
Flags: needinfo?(cbook)
We should accept this now to esr38.
I didn't realize the previous a- would remove it from tracking and thought Sylvestre was going to catch it for this next esr.
Comment on attachment 8672622 [details] [diff] [review]
patch without tests

Approved for esr38. We should land this before we build ESR, it's sec-high and looks OK on other branches.
Attachment #8672622 - Flags: approval-mozilla-esr38- → approval-mozilla-esr38+
I'm having trouble getting this to cleanly apply to esr38. Can we get a rebased patch?
Flags: needinfo?(amarchesini)
Andrew, it would be nice if we can get a rebased patch for this one. ESR38.5 is due to go live tomorrow. Would it be possible for someone to provide a rebased patch to the Sheriffs today (PST) so we can still release esr38.5 as per schedule?
Flags: needinfo?(overholt)
Kyle, Andrew suggested I try pinging you in case you could help with this in the next hour or so. Please let me know. Thanks!
Flags: needinfo?(khuey)
Per conversation on IRC, this will take longer than an hour.
Flags: needinfo?(khuey)
Flags: needinfo?(overholt)
Assignee

Comment 97

4 years ago
I agree with khuey: the main issue is that we don't have the support for WorkerDebugger in ESR38. This means that we have to 'rewrite' the patch for ESR38. I am ok to do it... but it's not just a rebase.
Flags: needinfo?(amarchesini)
This is now tracking for ESR38.6 as ESR38.5 went live earlier today.
Assignee

Comment 99

3 years ago
We are not going to fix this for ESR38 because it will require too much work. The ESR38 repo doesn't have a lot of patches needed to reuse the same patch (or something similar).
If that's the case, can we mark the firefox-esr38 status flag as 'wontfix'?
Flags: needinfo?(amarchesini)
Assignee

Updated

3 years ago
Flags: needinfo?(amarchesini)
It looks like this was fixed a while back, so I'm going to close it.
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago3 years ago
Resolution: --- → FIXED
Group: dom-core-security → core-security-release
I was able to reproduce the initial issue on Firefox 40.0a1 (2015-05-03) under Windows 10 64-bit.

> >+  postMessage(fileName2 == undefined);
> 
> Probably worth doing a bit more checking here (e.g. that the .message is
> what we expect).

Based on Comment 27, I’m not sure which message should be displayed, but I can confirm that on Firefox 45 ESR (20160301172432) and Firefox 46.0a2 (2016-03-01) appears “undefined” message. Tested under Windows 10 64-bit, Ubuntu 12.04 32-bit and Mac OS X 10.11. 

Boris, is this the expected message? 


Also tested on Firefox 47.0a1 (2016-03-01) and noticed that there appears only a blank pop-up and no message is displayed:  http://i.imgur.com/xktayn1.jpg

Performed a regression range and there are 2 potential issues that could have caused this regression : Bug 1249673 and Bug 1249652

Last good revision: 00cda66bd40083d8e2e14980a50090cc7cd8ab02
First bad revision: ee07c6a974bccea9ccf6b3bdcf4874b79308b188
Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=00cda66bd40083d8e2e14980a50090cc7cd8ab02&tochange=ee07c6a974bccea9ccf6b3bdcf4874b79308b188

Boris, any thoughts? Should I file a new bug for this regression?
Flags: needinfo?(bzbarsky)
> but I can confirm that on Firefox 45 ESR (20160301172432) and Firefox 46.0a2 (2016-03-01) appears “undefined” message.

You mean after clicking "go" on http://vulnerabledoma.in/fx_worker_importScripts.html ?

Comment 27 is about the .message property of error events inside the worker, which that testcase is not testing.

In terms of the testcase, there is no spec that defines the behavior, because the filename/fileName property is not standard.  That said, the exception we were throwing was in fact incorrect: it was a nonstandard NS_ERROR_FAILURE XPConnect Exception, whereas the spec says to throw a NetworkError DOMException.  We fixed that in bug 1249673.  DOMException, unlike Exception, inherits from Error, and Error.prototype has a "fileName" property on it (still non-standard, note), defaulting to empty string.

So if this bug is fixed I would expect an alert showing "undefined" before bug 1249673 is fixed and an empty alert otherwise.  If this bug is NOT fixed, I would expect an alert showing "https://analytics.twitter.com/about".
Flags: needinfo?(bzbarsky)
Oh, and just in case the linked testcase from comment 0 ever goes away, it is:

  <script>
  function go(){
	var worker = new Worker(`data:,
		try{
			importScripts("https://analytics.twitter.com/")
		}catch(e){
			postMessage(e.fileName)
		}
	`);
	worker.onmessage = function(msg){alert(msg.data)}
  }
  </script>
  <button onclick=go()>go</button>
Thanks Boris for the explanations! It seems that I encountered the expected result across all versions and platforms.

Based on Comment 102 and Comment 103 I am marking this bug as Verified Fixed.
Status: RESOLVED → VERIFIED
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.