1160890 - (CVE-2015-7215) Cross-origin information disclosure with error message of Web Workers importScripts()

Status: VERIFIED FIXED

(Core :: DOM: Workers, defect)

Description

[Masato Kinugawa]

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

[Benjamin Smedberg]

Attachment: Testcase HTML

Comment 2

[Masato Kinugawa]

@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.

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Andrea, do you want to take a shot at this?

Comment 4

[Andrea Marchesini [:baku]]

Attachment: crash8.patch

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.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 6

[Andrea Marchesini [:baku]]

Attachment: patch 1 - small fixes

Comment 7

[Andrea Marchesini [:baku]]

Attachment: patch 2 - muted error flag

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 10

[Andrea Marchesini [:baku]]

Attachment: patch 2 - muted error flag

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 12

[Andrea Marchesini [:baku]]

Attachment: patch 1 - small fixes

Comment 13

[Andrea Marchesini [:baku]]

Attachment: patch 2 - muted error flag

Comment 14

[Andrea Marchesini [:baku]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=333361ec56e8

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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 :)

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/22e8c5440405
https://hg.mozilla.org/mozilla-central/rev/37c0bb092ec2

Comment 17

[Andrea Marchesini [:baku]]

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

Comment 18

[Andrea Marchesini [:baku]]

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

See previous comment.

Comment 19

[Sylvestre Ledru [:Sylvestre]]

(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?

Comment 20

[Josh Cheng [:josh]]

Help to NI Andrea per comment 19

Comment 21

[Andrea Marchesini [:baku]]

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.

Comment 22

[Lawrence Mandel [:lmandel] (use needinfo)]

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.

Comment 23

[Andrea Marchesini [:baku]]

Attachment: patch

I have to say that I'm not sure about this approach. bz, feedback? Thanks!

Comment 24

[Boris Zbarsky [: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?

Comment 25

[Boris Zbarsky [:bzbarsky]]

Or possibly better yet in a commit comment....

Comment 26

[Andrea Marchesini [:baku]]

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 27

[Boris Zbarsky [:bzbarsky]]

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...

Comment 28

[Andrea Marchesini [:baku]]

> 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.

Comment 29

[Boris Zbarsky [:bzbarsky]]

> 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?

Comment 30

[Liz Henry (:lizzard) (relman/hg->git project)]

Not tracking for esr since this is sec-moderate. If that changes, please renominate it for tracking.

Comment 31

[Daniel Veditz [:dveditz]]

[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.

Comment 32

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Unless I am told otherwise by sec team, it is too late for 41.

Comment 33

[Andrew McCreight [:mccr8]]

Andrea, now that this is sec-high, could you take a look and finish this bug off? Thanks.

Comment 34

[Andrea Marchesini [:baku]]

Attachment: worker.patch

Comment 35

[Boris Zbarsky [: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...

Comment 36

[Andrea Marchesini [:baku]]

Attachment: worker.patch

I'm planning to do what you are suggesting as a follow up.

Comment 37

[Boris Zbarsky [: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.

Comment 38

[Boris Zbarsky [:bzbarsky]]

> What error events do the two workers and the original patch get?

Original _page_, of course.

Comment 39

[Andrea Marchesini [:baku]]

Attachment: worker.patch

> 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.

Comment 40

[Andrea Marchesini [:baku]]

Follow ups: bug 1211967 and bug 1211970

Comment 41

[Andrea Marchesini [:baku]]

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.

Comment 42

[Boris Zbarsky [:bzbarsky]]

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.

Comment 43

[Andrea Marchesini [:baku]]

> > 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?

Comment 44

[Boris Zbarsky [: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.

Comment 45

[Andrea Marchesini [:baku]]

Attachment: worker.patch

Can you take a look at the test?

Comment 46

[Boris Zbarsky [: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?

Comment 47

[Andrea Marchesini [:baku]]

Attachment: worker.patch

Comment 48

[Andrea Marchesini [:baku]]

> Also, still need an onerror test, right?

onerror on the worker? on the parent we have it.

Comment 49

[Boris Zbarsky [:bzbarsky]]

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.

Comment 50

[Andrea Marchesini [:baku]]

Attachment: worker.patch

See comments 41 and 42.

Comment 51

[Boris Zbarsky [:bzbarsky]]

Thanks, that looks much better!

Comment 52

[Al Billings [:abillings - ex-MoCo]]

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 53

[Al Billings [:abillings - ex-MoCo]]

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?

Comment 54

[Andrea Marchesini [:baku]]

Attachment: patch without tests

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.

Comment 55

[Andrea Marchesini [:baku]]

Attachment: tests

Comment 56

[Al Billings [:abillings - ex-MoCo]]

Is the patch without tests the trunk patch now? If so, carry over the sec-approval+ and get this checked into trunk.

Comment 57

[Andrea Marchesini [:baku]]

(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?

Comment 58

[Al Billings [:abillings - ex-MoCo]]

Yes, that's fine.

Comment 59

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d20b1652e015

Comment 60

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d20b1652e015

Comment 61

[Sylvestre Ledru [:Sylvestre]]

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.

Comment 62

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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?

Comment 63

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 64

[Sylvestre Ledru [:Sylvestre]]

Andrew, do you know if Andrea is around? We are late in the 42 cycle.

Comment 65

[Andrea Marchesini [:baku]]

Yes, I'm here. I'll submit a patch for beta today.

Comment 66

[Andrea Marchesini [:baku]]

Attachment: beta

Here the m-b patch

Comment 67

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/451d4a04dae4

Comment 68

[Boris Zbarsky [:bzbarsky]]

Wait.  What about backporting fixes for the known error-reporting regressions this bug caused?  What is the plan for those?

Comment 69

[Carsten Book [:Tomcat]]

setting the flags for the beta and aurora checkin

Comment 70

[Sylvestre Ledru [:Sylvestre]]

Bz: Don't know. Andrea probably knows better.

Comment 71

[Andrea Marchesini [:baku]]

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?

Comment 72

[Boris Zbarsky [:bzbarsky]]

That sounds perfectly reasonable to me.  Just make sure to request those approvals.

Comment 74

[Andrea Marchesini [:baku]]

I think it's time to land the second patch with the tests. Am I right?

Comment 75

[Liz Henry (:lizzard) (relman/hg->git project)]

We are discussing backing this out instead (in bug 1211970).

Comment 76

[Al Billings [:abillings - ex-MoCo]]

Andrea, even if we weren't backing it out, we wouldn't land tests until *after* a release with the fix ships.

Comment 77

[Liz Henry (:lizzard) (relman/hg->git project)]

Sylvestre would like to back this out from beta.   Let's keep it on trunk and aurora, and uplift the related work to aurora.

Comment 78

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 79

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 81

[Andrea Marchesini [:baku]]

Can we land these patches (and the test) in the new aurora/beta?

Comment 82

[Al Billings [:abillings - ex-MoCo]]

Is this actually fixed in 43 but not 44 or 45?

Comment 83

[Andrea Marchesini [:baku]]

(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.

Comment 84

[Liz Henry (:lizzard) (relman/hg->git project)]

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?

Comment 85

[Liz Henry (:lizzard) (relman/hg->git project)]

Is it https://bugzilla.mozilla.org/attachment.cgi?id=8672622&action=edit that still needs to land on 43, and 44?  (Without tests.)

Comment 87

[Andrea Marchesini [:baku]]

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.

Comment 88

[Al Billings [:abillings - ex-MoCo]]

Once we ship a release with a fix, we'll wait a week for uptake and then you can check the tests in.

Comment 89

[Daniel Veditz [:dveditz]]

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.

Comment 90

[Carsten Book [:Tomcat]]

(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

Comment 91

[Liz Henry (:lizzard) (relman/hg->git project)]

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 92

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 93

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I'm having trouble getting this to cleanly apply to esr38. Can we get a rebased patch?

Comment 94

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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?

Comment 95

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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!

Comment 96

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Per conversation on IRC, this will take longer than an hour.

Comment 97

[Andrea Marchesini [:baku]]

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.

Comment 98

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

This is now tracking for ESR38.6 as ESR38.5 went live earlier today.

Comment 99

[Andrea Marchesini [:baku]]

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).

Comment 100

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

If that's the case, can we mark the firefox-esr38 status flag as 'wontfix'?

Comment 101

[Andrew McCreight [:mccr8]]

It looks like this was fixed a while back, so I'm going to close it.

Comment 102

[Vasilica Tamas, QA [:vtamas]]

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?

Comment 103

[Boris Zbarsky [: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".

Comment 104

[Boris Zbarsky [: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>

Comment 105

[Vasilica Tamas, QA [:vtamas]]

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.