1409449 - (CVE-2018-5115) Phishing vector via http auth of background connections

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect, P1)

Description

[Daniel Veditz [:dveditz]]

+++ This bug was initially created as a clone of Bug #1408138 +++

In bug 1408183 comment 4 through bug 1408183 comment 11 Jerry describes a related but different problem. Nothing to do with 'Mixed Content', but the fact that background connections (in that example something to do with the OpenH264 plugin handling media server requests) can pop HTTP Auth. They have to go somewhere so we stick them in the currently active page. They don't get the "WARNING this isn't the top-level site" text we give for sub-resource auth because it's disconnected from the page.

Problem compounded in that case because they were http: requests and could be hijacked locally to trigger auth. The real solution would be to ignore HTTP Auth on background requests not associated with a page.

Comment 1

[Dragana Damjanovic [:dragana]]

Does anyone know how to find out that request is not connected to any tab?

It will not have TopWindow and TopWindowUri.

If I remember correctly some other request do not have it as well, was it xhr?

Comment 2

[Honza Bambas (:mayhemer)]

(In reply to Dragana Damjanovic [:dragana] from comment #1)
> Does anyone know how to find out that request is not connected to any tab?

I think toplevelouterwindowid would be 0 in that case (chrome initiated).  Note that a toplevel load in a new tab will not have this window id as well - there is no content window for it yet.

> 
> It will not have TopWindow and TopWindowUri.
> 
> If I remember correctly some other request do not have it as well, was it
> xhr?

no, xhr invoked from a content script will have the correct window (coming from the load context).

Comment 3

[Dragana Damjanovic [:dragana]]

Tanvi, do you know a reliable way to find out that a load is chrome initiated? something from loadinfo/loadcontext

Probably at the time we want to show an auth dialog we will have windowId for non chrome loads/

Comment 4

[Tanvi Vyas[:tanvi]]

The triggeringPrincipal?  If the triggeringPrincipal is system, it *might* be chrome initiated.  I say "might" because we can't rely on the triggeringPrincipal for top level loads yet.  Christoph, can you think of another way to tell?

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Tanvi Vyas[:tanvi] from comment #4)
> The triggeringPrincipal?  If the triggeringPrincipal is system, it *might*
> be chrome initiated.  I say "might" because we can't rely on the
> triggeringPrincipal for top level loads yet.  Christoph, can you think of
> another way to tell?

TriggeringPrincipal == SystemPrincipal is your best bet to figure out if the load is initiated by chrome. Different question, this is only about background loads, doesn't the flag LOAD_BACKGROUND help to figure out if a channel loads in the background?

Comment 6

[Dragana Damjanovic [:dragana]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #5)
> (In reply to Tanvi Vyas[:tanvi] from comment #4)
> > The triggeringPrincipal?  If the triggeringPrincipal is system, it *might*
> > be chrome initiated.  I say "might" because we can't rely on the
> > triggeringPrincipal for top level loads yet.  Christoph, can you think of
> > another way to tell?
> 
> TriggeringPrincipal == SystemPrincipal is your best bet to figure out if the
> load is initiated by chrome. Different question, this is only about
> background loads, doesn't the flag LOAD_BACKGROUND help to figure out if a
> channel loads in the background?

I am not sure if LOAD_BACKGROUND is set for that case at all. It is usually set on resources on a page that should not count for onload events, e.g. xhr.

How many false positive we get if we use TriggeringPrincipal == SystemPrincipal? I am ok if  we do not block all of them, but I do not want to block something we should not block.

Comment 7

[Jerry Decime]

>I am ok if  we do not block all of them, but I do not want to block something we should not block.

Are there any legitimate cases for an HTTP Auth ask (401 & 407) by a background connection?  I can't recall ever running into this sort of design in any add-ons I've ever used.

Comment 8

[Dragana Damjanovic [:dragana]]

(In reply to Jerry Decime from comment #7)
> >I am ok if  we do not block all of them, but I do not want to block something we should not block.
> 
> Are there any legitimate cases for an HTTP Auth ask (401 & 407) by a
> background connection?  I can't recall ever running into this sort of design
> in any add-ons I've ever used.

From comment 4, this work (setting TriggeringPrincipal to SystemPrincipal for background request) is a work in progress. I just  want to know what are the errors if I only check for TriggeringPrincipal == SystemPrincipal.

Comment 9

[Tanvi Vyas[:tanvi]]

We fall back to systemPrincipal for triggeringPrincipal when we don't know what really initiated the load.  There is a project to fix that that is in progress.  Kate - do you have the list of open bugs for this?  Dragana - we could show you where in the code we do this, but I don't think we know what situations cause this.

Comment 10

[Daniel Veditz [:dveditz]]

(In reply to Jerry Decime from comment #7)
> Are there any legitimate cases for an HTTP Auth ask (401 & 407) by a
> background connection?  I can't recall ever running into this sort of design
> in any add-ons I've ever used.

At the level we're working we probably don't know which loads are initiated by add-ons or not. Background requests with auth are problematic and we'd like to block them. We /tried/ blocking them all, but XHR requests were one that in practice are currently expected to work. If we can distinguish between "background" XHR requests that we can't--for now--break and the true background requests (not associated with any page content) such as the media server connections you found we certainly would want to block the latter.

In the screenshot we are not showing the "WARNING blah blah" text, so we _know_ it's not a 3rd party in-page request. In that case can we do a simple check that "if not '3rd party' then it damn well better match the 1st party" when we're about to attach it to a window?

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Dragana Damjanovic [:dragana] from comment #6)
> How many false positive we get if we use TriggeringPrincipal ==
> SystemPrincipal? I am ok if  we do not block all of them, but I do not want
> to block something we should not block.

Let me summarize:
(a) for all loads that are *not* of TYPE_DOCUMENT, the triggeringPrincipal is always accurate and reflects the web content that triggered the load, or alternatively, if triggered by the system, then SystemPrincipal.

(b) for loads of TYPE_DOCUMENT there is some uncertainty about the triggeringPrincipal (we are about to fix that within Bug 1333030 and dependents): 
* if it's a codebasePrincipal, then we definitely know the load was triggered by web content
* if it's a systemPrincipal, then we are (my guess) 95% sure the load was triggered by the system

The reason for that uncertainty is that we have a fallback mechanism in place that helps us to generate a triggeringPrincipal if not passed explicitly, see:
  https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#1562

Comment 12

[Daniel Veditz [:dveditz]]

(and of course a completely separate question is why is OpenH264 replying to broadcast messages?)

Comment 13

[Jerry Decime]

>(and of course a completely separate question is why is OpenH264 replying to broadcast messages?)

It's part of the SSDP discovery process: https://en.wikipedia.org/wiki/Simple_Service_Discovery_Protocol and I assume was implemented as part of the plugin support for H.264 streaming media.  As an example, maybe a developer would want to integrate with the Silicon Dust HDHomeRun solution within the browser.  In fact I think this might be an actual thing that PLEX supports.

Comment 14

[Dragana Damjanovic [:dragana]]

(In reply to Daniel Veditz [:dveditz] from comment #10)
> (In reply to Jerry Decime from comment #7)  
> In the screenshot we are not showing the "WARNING blah blah" text, so we
> _know_ it's not a 3rd party in-page request. In that case can we do a simple
> check that "if not '3rd party' then it damn well better match the 1st party"
> when we're about to attach it to a window?


I think this is not accurately diagnose now because we do not have windowId that early in some cases, e.g. opening in a new tab.

Comment 15

[Dragana Damjanovic [:dragana]]

Attachment: bug_1409449.patch

If you have other suggestion hot to detect non-web-content triggered resources, please let me know.

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8920337 [details] [diff] [review]
bug_1409449.patch

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

* That seems correct to me;
* Please address my question
* I am not a Necko peer, probably worth having one sign off on it too.

::: netwerk/protocol/http/nsHttpChannelAuthProvider.cpp
@@ +1027,5 @@
>      }
>  
>      if (gHttpHandler->IsTelemetryEnabled()) {
> +        if (nonWebContent) {
> +            Telemetry::Accumulate(Telemetry::HTTP_AUTH_DIALOG_STATS_2,

Don't you have to update all of them to HTTP_AUTH_DIALOG_STATS_3?

Comment 17

[Dragana Damjanovic [:dragana]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #16)
> Comment on attachment 8920337 [details] [diff] [review]
> bug_1409449.patch
> 
> Review of attachment 8920337 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> * That seems correct to me;
> * Please address my question
> * I am not a Necko peer, probably worth having one sign off on it too.
> 

I will have some one look at it, I just wanted your opinion first.

> ::: netwerk/protocol/http/nsHttpChannelAuthProvider.cpp
> @@ +1027,5 @@
> >      }
> >  
> >      if (gHttpHandler->IsTelemetryEnabled()) {
> > +        if (nonWebContent) {
> > +            Telemetry::Accumulate(Telemetry::HTTP_AUTH_DIALOG_STATS_2,
> 
> Don't you have to update all of them to HTTP_AUTH_DIALOG_STATS_3?

Yes, thanks!

Comment 18

[Dragana Damjanovic [:dragana]]

Attachment: bug_1409449.patch

Comment 19

[Dragana Damjanovic [:dragana]]

Comment on attachment 8920613 [details] [diff] [review]
bug_1409449.patch

Francois, can you take a look at the telemetry part. Thanks.

Comment 20

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8920613 [details] [diff] [review]
bug_1409449.patch

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

Looks good to me.

Comment 21

[François Marier [:francois]]

Comment on attachment 8920613 [details] [diff] [review]
bug_1409449.patch

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

This is Category 1 data.

datareview+

::: toolkit/components/telemetry/Histograms.json
@@ +2427,3 @@
>      "record_in_processes": ["main", "content"],
>      "expires_in_version": "61",
>      "alert_emails": ["[e-mail]"],

Can you please add your email address in there?

We now ask that every new probe includes the email of a person who is responsible for or looking at the data.

Comment 22

[Dragana Damjanovic [:dragana]]

(In reply to François Marier [:francois] from comment #21)
> Comment on attachment 8920613 [details] [diff] [review]
> bug_1409449.patch
> 
> Review of attachment 8920613 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is Category 1 data.
> 
> datareview+
> 
> ::: toolkit/components/telemetry/Histograms.json
> @@ +2427,3 @@
> >      "record_in_processes": ["main", "content"],
> >      "expires_in_version": "61",
> >      "alert_emails": ["[e-mail]"],
> 
> Can you please add your email address in there?
> 
> We now ask that every new probe includes the email of a person who is
> responsible for or looking at the data.

I will. Thanks.

Comment 23

[Kate McKinley [:kmckinley, :Kate]]

I see Christoph provided the triggeringPrincipal meta bug in comment 11

Comment 24

[Dragana Damjanovic [:dragana]]

Attachment: bug_1409449.patch

Comment 25

[Ryan VanderMeulen [:RyanVM]]

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

Comment 26

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/df1ad991ade1 for two browser-chrome failures, https://treeherder.mozilla.org/logviewer.html#?job_id=139494407&repo=mozilla-inbound and https://treeherder.mozilla.org/logviewer.html#?job_id=139494489&repo=mozilla-inbound, xpcshell, https://treeherder.mozilla.org/logviewer.html#?job_id=139493765&repo=mozilla-inbound, marionette, https://treeherder.mozilla.org/logviewer.html#?job_id=139493748&repo=mozilla-inbound, and a pair of Win7 (where we still also run non-e10s tests) mochitests, https://treeherder.mozilla.org/logviewer.html#?job_id=139499665&repo=mozilla-inbound and https://treeherder.mozilla.org/logviewer.html#?job_id=139499694&repo=mozilla-inbound

Comment 27

[Dragana Damjanovic [:dragana]]

Thanks.

I really did not expect that I am going to break our own tests :)

I need to take a look if we do the authentication that we want to forbid or is triggeringPrincipal wrong and set to systemPrincipal.

Comment 28

[Dragana Damjanovic [:dragana]]

Attachment: bug_1409449_tests.patch

Comment 29

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8923354 [details] [diff] [review]
bug_1409449_tests.patch

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

::: modules/libpref/init/all.js
@@ +2216,5 @@
>  pref("network.auth.subresource-img-cross-origin-http-auth-allow", true);
>  
>  // Resources that are triggered by some non-web-content:
>  // true - they are allow to present http auth. dialog
>  // false - they are not allow to present http auth. dialog.

nit: they allow / they are allowed

@@ +2217,5 @@
>  
>  // Resources that are triggered by some non-web-content:
>  // true - they are allow to present http auth. dialog
>  // false - they are not allow to present http auth. dialog.
> +pref("network.auth.non-web-content-triggered-resources-http-auth-allow", true);

Should this default to true?

::: toolkit/mozapps/extensions/test/xpinstall/browser_auth.js
@@ +50,5 @@
>    authMgr.clearAll();
>  
>    Services.perms.remove(makeURI("http://example.com"), "install");
>  
> +  var prefs = Cc["@mozilla.org/preferences-service;1"]

We can use Services.prefs here

::: toolkit/mozapps/extensions/test/xpinstall/browser_auth2.js
@@ +9,3 @@
>  function test() {
> +  // Turn off the authentication dialog blocking for this test.
> +  prefs.setBoolPref("network.auth.non-web-content-triggered-resources-http-auth-allow", true);

I think we can use Services.prefs here

@@ +47,5 @@
>    authMgr.clearAll();
>  
>    Services.perms.remove(makeURI("http://example.com"), "install");
>  
> +  prefs.clearUserPref("network.auth.non-web-content-triggered-resources-http-auth-allow");

I think we can use Services.prefs here

::: toolkit/mozapps/extensions/test/xpinstall/browser_auth3.js
@@ +2,5 @@
>  // Test whether an install fails when authentication is required and it is
>  // canceled
>  // This verifies bug 312473
>  
> +var prefs = Cc["@mozilla.org/preferences-service;1"]

I think we can use Services.prefs here

::: toolkit/mozapps/extensions/test/xpinstall/browser_auth4.js
@@ +1,5 @@
>  // Test whether a request for auth for an XPI switches to the appropriate tab
>  var gNewTab;
>  
> +var prefs = Cc["@mozilla.org/preferences-service;1"]
> +              .getService(Ci.nsIPrefBranch);

I think we can use Services.prefs here

Comment 30

[Jerry Decime]

Apple has a similar flaw and would like to coordinate public disclosure if at all possible.  Who do I need to work with on your team to ensure everyone is working on an agreed disclosure timeline?

Comment 31

[Tanvi Vyas[:tanvi]]

Dragana, are you still working on this?

Comment 32

[Dragana Damjanovic [:dragana]]

(In reply to Tanvi Vyas[:tanvi] from comment #31)
> Dragana, are you still working on this?

Yes, there is a stupid test failure that I still do not understand. I will try to fix it in next days. '--verify' mochitest is failing if I only add a empty line, really an empty line, to browser/components/originattributes/test/browser/browser_httpauth.js - very strange. I need to change the test to make it work with this patch, i.e. i need to change the pref introduce here at the beginning of that test.

I will need to take a close look at IsolationTestTools and probably rewrite it. Sorry for the delay.

Comment 33

[Tanvi Vyas[:tanvi]]

Thanks Dragana!

Comment 34

[Dragana Damjanovic [:dragana]]

(In reply to Dragana Damjanovic [:dragana] from comment #32)
> (In reply to Tanvi Vyas[:tanvi] from comment #31)
> > Dragana, are you still working on this?
> 
> Yes, there is a stupid test failure that I still do not understand. I will
> try to fix it in next days. '--verify' mochitest is failing if I only add a
> empty line, really an empty line, to
> browser/components/originattributes/test/browser/browser_httpauth.js - very
> strange. I need to change the test to make it work with this patch, i.e. i
> need to change the pref introduce here at the beginning of that test.
> 
> I will need to take a close look at IsolationTestTools and probably rewrite
> it. Sorry for the delay.

The test failure that I have described in comment 32 has just disappeared. It most have been triggered  by something completely different. I have spend so much time trying to figure it out.
Anyway, now everything works fine.

Comment 35

[Dragana Damjanovic [:dragana]]

Attachment: bug_1409449_tests.patch

Comment 36

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=655aaaeab484bf43967be2871330691ee7b7e8e4

Comment 37

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8934463 [details] [diff] [review]
bug_1409449_tests.patch

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

It looks good. r+ with these changes.
Note that the Android TV test fails in test_ext_proxy_auth.js https://treeherder.mozilla.org/#/jobs?repo=try&revision=655aaaeab484bf43967be2871330691ee7b7e8e4&selectedJob=149595984

::: netwerk/test/unit/test_auth_proxy.js
@@ +235,5 @@
>    prefs.setIntPref("network.proxy.type", 1);
>  
>    // Turn off the authentication dialog blocking for this test.
>    prefs.setIntPref("network.auth.subresource-http-auth-allow", 2);
> +  prefs.setBoolPref("network.auth.non-web-content-triggered-resources-http-auth-allow", true);

does this test require a cleanup/clearPref phase?

::: testing/marionette/harness/marionette_harness/tests/unit/test_modal_dialogs.py
@@ +223,5 @@
>      def tearDown(self):
>          # Ensure to close a possible remaining modal dialog
>          self.close_all_windows()
>  
>          super(TestModalAlerts, self).tearDown()

Do we need to clear the pref here?

::: toolkit/components/extensions/test/xpcshell/test_ext_proxy_auth.js
@@ +19,5 @@
>      response.write("auth required");
>    }
>  });
>  
> +Services.prefs.setBoolPref("network.auth.non-web-content-triggered-resources-http-auth-allow", true);

Same. We might need to clear the pref.

Comment 38

[Dragana Damjanovic [:dragana]]

(In reply to Valentin Gosu [:valentin] from comment #37)
> Comment on attachment 8934463 [details] [diff] [review]
> bug_1409449_tests.patch
> 
> Review of attachment 8934463 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It looks good. r+ with these changes.
> Note that the Android TV test fails in test_ext_proxy_auth.js
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=655aaaeab484bf43967be2871330691ee7b7e8e4&selectedJob=1
> 49595984
> 

This test was added recently. It was not there before. Anyway, since bug 1423522 landed we do not need to change this test, so everything is green.



> ::: netwerk/test/unit/test_auth_proxy.js
> @@ +235,5 @@
> >    prefs.setIntPref("network.proxy.type", 1);
> >  
> >    // Turn off the authentication dialog blocking for this test.
> >    prefs.setIntPref("network.auth.subresource-http-auth-allow", 2);
> > +  prefs.setBoolPref("network.auth.non-web-content-triggered-resources-http-auth-allow", true);
> 
> does this test require a cleanup/clearPref phase?
> 

I have added it and also for 2 others....

Comment 39

[Dragana Damjanovic [:dragana]]

Attachment: bug_1409449_tests.patch

Comment 40

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/09bf615d77d2
https://hg.mozilla.org/mozilla-central/rev/c31b663b4dd2

Comment 41

[Jerry Decime]

Re: 1425307, 1425386.  Luckily there's a solution: either use HTTP for the main site, or switch to using HTTPS for sub-sites and all sub-site resources.  Bad practices on the part of a few solutions should never endanger the wider Internet.  I've been drafting a short statement on this bug but was waiting for Apple to finish some of their patching before publishing.  Maybe it's time to publicly address the flaw which includes suggested guidance?  I'm happy to put something together / coordinate with your communications team.  I understand this change is likely to cause issues for some, but given the severity to everyone else, I think the steps taken were appropriate.

Comment 42

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8921812 [details] [diff] [review]
bug_1409449.patch

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

::: netwerk/protocol/http/nsHttpChannelAuthProvider.cpp
@@ +1046,5 @@
>                                    loadInfo->GetExternalContentPolicyType());
>          }
>      }
>  
> +    if (!sNonWebContentTriggeredAuthAllow && nonWebContent) {

I think this should also check topDoc? I missed it in the original review

Comment 43

[Ryan VanderMeulen [:RyanVM]]

Does this need approval requests?

Comment 44

[Dragana Damjanovic [:dragana]]

(In reply to Valentin Gosu [:valentin] from comment #42)
> Comment on attachment 8921812 [details] [diff] [review]
> bug_1409449.patch
> 
> Review of attachment 8921812 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/protocol/http/nsHttpChannelAuthProvider.cpp
> @@ +1046,5 @@
> >                                    loadInfo->GetExternalContentPolicyType());
> >          }
> >      }
> >  
> > +    if (!sNonWebContentTriggeredAuthAllow && nonWebContent) {
> 
> I think this should also check topDoc? I missed it in the original review

this is fixed by 1425156.

Comment 45

[Dragana Damjanovic [:dragana]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #43)
> Does this need approval requests?

sec-approval?

it is sec-moderate and according to the wiki approval is not needed.

Comment 46

[Ryan VanderMeulen [:RyanVM]]

No, uplifts.

Comment 47

[Dragana Damjanovic [:dragana]]

Christoph, is there a reason not to uplift this? I assume triggringPrincipls is set in the same way on 59 and 58? There are no know broken issues except one  mentioned in comment 11 and comment 9.

Comment 48

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Dragana Damjanovic [:dragana] from comment #47)
> Christoph, is there a reason not to uplift this? I assume triggringPrincipls
> is set in the same way on 59 and 58? There are no know broken issues except
> one  mentioned in comment 11 and comment 9.

Triggeringprincipal has been stable for a while. Save to uplift - please request uplift approval.

Comment 49

[Dragana Damjanovic [:dragana]]

Attachment: bug_1409449_beta.patch

Comment 50

[Dragana Damjanovic [:dragana]]

Attachment: bug_1409449_tests_beta.patch

Comment 51

[Dragana Damjanovic [:dragana]]

Comment on attachment 8938403 [details] [diff] [review]
bug_1409449_beta.patch

Approval Request Comment
[Feature/Bug causing the regression]: It is not a regression the code exists for a very long time.
[User impact if declined]:This bug disallow a http authentication dialog to be triggered by the non-web content. It can be used for phishing.
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: bug 1425156 and also patch bug_1409449_tests_beta.patch from this bug.
[Is the change risky?]: low risk
[Why is the change risky/not risky?]: the code change is not risky.  triggeringPrincipal is not set always set properly, in most of the cases it is, but I am not the right person to make statement on this. According to comment 48 it is a safe to uplift.
[String changes made/needed]:none

Comment 52

[Gerry Chang [:gchang]]

Hi Dan,
Given this is sec-moderate and it's late in 58, do you think we need to take this in 58?

Comment 53

[Daniel Veditz [:dveditz]]

(In reply to Gerry Chang [:gchang] from comment #52)
> Given this is sec-moderate and it's late in 58, do you think we need to take this in 58?

We don't "need" to, but since it's blocking an abuse technique that's actively being used against our users in bad ads it would be very nice to take. The guts of the patch are actually quite simple, and it's got tons of telemetry and adds a pref so we can easily flip off the change if we have to.

Comment 54

[Gerry Chang [:gchang]]

Comment on attachment 8938403 [details] [diff] [review]
bug_1409449_beta.patch

Fix a potential sec issue which can be used for phishing. Beta58+.

Comment 55

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/1bf31fdbfa8e

Comment 56

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing xpcshell's netwerk/test/unit/test_auth_proxy.js and toolkit/components/extensions/test/xpcshell/test_ext_proxy_auth.js:

https://hg.mozilla.org/releases/mozilla-beta/rev/60e4ebb5f15082a790d840a4dc51514252ce3fab

Non-busted push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta&revision=1b3d0019f65e91d5ec62760f9ba0104cc98ea052&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable

Failure log netwerk/test/unit/test_auth_proxy.js: https://treeherder.mozilla.org/logviewer.html#?job_id=154752534&repo=mozilla-beta

[task 2018-01-08T14:05:01.332Z] 14:05:01     INFO -  TEST-PASS | netwerk/test/unit/test_auth_proxy.js | test_onStopR - [test_onStopR : 194] 2147500036 == 2147500036
[task 2018-01-08T14:05:01.332Z] 14:05:01     INFO -  PID 7049 | test: proxy wrongpw
[task 2018-01-08T14:05:01.333Z] 14:05:01     INFO -  (xpcshell/head.js) | test pending (2)
[task 2018-01-08T14:05:01.333Z] 14:05:01     INFO -  (xpcshell/head.js) | test finished (2)
[task 2018-01-08T14:05:01.334Z] 14:05:01     INFO -  "CONSOLE_MESSAGE: (error) [JavaScript Error: "uncaught exception: 2147500036" {file: "/builds/worker/workspace/build/tests/xpcshell/head.js" line: 221}]"
[task 2018-01-08T14:05:01.334Z] 14:05:01     INFO -  PID 7049 | proxy password required
[task 2018-01-08T14:05:01.335Z] 14:05:01     INFO -  PID 7049 | [7049, Cache2 I/O] WARNING: Forcing memory-only entry since OpenFile failed: file /builds/worker/workspace/build/src/netwerk/cache2/CacheFile.cpp, line 527
[task 2018-01-08T14:05:01.337Z] 14:05:01  WARNING -  TEST-UNEXPECTED-FAIL | netwerk/test/unit/test_auth_proxy.js | test_onStartR - [test_onStartR : 172] 200 == 407
[task 2018-01-08T14:05:01.338Z] 14:05:01     INFO -  /builds/worker/workspace/build/tests/xpcshell/tests/netwerk/test/unit/test_auth_proxy.js:test_onStartR:172
[task 2018-01-08T14:05:01.339Z] 14:05:01     INFO -  /builds/worker/workspace/build/tests/xpcshell/head.js:_do_main:221
[task 2018-01-08T14:05:01.341Z] 14:05:01     INFO -  /builds/worker/workspace/build/tests/xpcshell/head.js:_execute_test:543
[task 2018-01-08T14:05:01.342Z] 14:05:01     INFO -  -e:null:1
[task 2018-01-08T14:05:01.343Z] 14:05:01     INFO -  exiting test

Comment 57

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Also fails in browser-chrome: https://treeherder.mozilla.org/logviewer.html#?job_id=154751654&repo=mozilla-beta

[task 2018-01-08T14:08:28.485Z] 14:08:28     INFO - TEST-PASS | toolkit/mozapps/extensions/test/xpinstall/browser_auth.js | Should have seen the expected number of installs started - 
[task 2018-01-08T14:08:28.487Z] 14:08:28     INFO - Console message: [JavaScript Error: "The character encoding of the HTML document was not declared. The document will render with garbled text in some browser configurations if the document contains characters from outside the US-ASCII range. The character encoding of the page must be declared in the document or in the transfer protocol." {file: "http://example.com/browser/toolkit/mozapps/extensions/test/xpinstall/installtrigger.html?%7B%22Unsigned%20XPI%22%3A%22http%3A%2F%2Fexample.com%2Fbrowser%2Ftoolkit%2Fmozapps%2Fextensions%2Ftest%2Fxpinstall%2FauthRedirect.sjs%3Fhttp%3A%2F%2Fexample.com%2Fbrowser%2Ftoolkit%2Fmozapps%2Fextensions%2Ftest%2Fxpinstall%2Famosigned.xpi%22%7D" line: 0}]
[task 2018-01-08T14:08:28.488Z] 14:08:28     INFO - TEST-PASS | toolkit/mozapps/extensions/test/xpinstall/browser_auth.js | Should only see failures for started installs - 
[task 2018-01-08T14:08:28.489Z] 14:08:28     INFO - TEST-PASS | toolkit/mozapps/extensions/test/xpinstall/browser_auth.js | Failed installs should have an error or be appDisabled - 
[task 2018-01-08T14:08:28.494Z] 14:08:28     INFO - Buffered messages finished
[task 2018-01-08T14:08:28.497Z] 14:08:28     INFO - TEST-UNEXPECTED-FAIL | toolkit/mozapps/extensions/test/xpinstall/browser_auth.js | Install should not have failed - 
[task 2018-01-08T14:08:28.502Z] 14:08:28     INFO - Stack trace:
[task 2018-01-08T14:08:28.503Z] 14:08:28     INFO - chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/xpinstall/browser_auth.js:download_failed:30
[task 2018-01-08T14:08:28.506Z] 14:08:28     INFO - chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/xpinstall/head.js:onDownloadFailed:351
[task 2018-01-08T14:08:28.508Z] 14:08:28     INFO - resource://gre/modules/AddonManager.jsm:callInstallListeners:1637
[task 2018-01-08T14:08:28.509Z] 14:08:28     INFO - resource://gre/modules/AddonManager.jsm:callInstallListeners:3165
[task 2018-01-08T14:08:28.510Z] 14:08:28     INFO - resource://gre/modules/addons/XPIInstall.jsm:downloadFailed:2468
[task 2018-01-08T14:08:28.513Z] 14:08:28     INFO - resource://gre/modules/addons/XPIInstall.jsm:onStopRequest:2444

Comment 58

[Dragana Damjanovic [:dragana]]

(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #56)
> Backed out for failing xpcshell's netwerk/test/unit/test_auth_proxy.js and
> toolkit/components/extensions/test/xpcshell/test_ext_proxy_auth.js:
> 


you will need to push bug_1409449_tests_beta.patch (this one only changes tests) as well as patch from bug 1425156.

Comment 59

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/f75756056fc4274c57a074a6f1e6430e809a0d0b
https://hg.mozilla.org/releases/mozilla-beta/rev/6130add50170cca8fccd472ada196c93f1ef8ecd

Comment 60

[Jerry Decime]

FYI, Apple has released info on CVE-2017-7153 which addresses the mitigation of this vulnerability in Safari 11.0.2:

https://support.apple.com/en-us/HT208324

Related, they've also released information on CVE-2017-7164 related to similar vulnerabilities in the App Store:

https://support.apple.com/en-us/HT208334

Each of these CVE's also applies to a number of other Apple platforms which they've further acknowledged.

Comment 61

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

(In reply to Jerry Decime from comment #60)
> FYI, Apple has released info on CVE-2017-7153 which addresses the mitigation
> of this vulnerability in Safari 11.0.2:
> 
> https://support.apple.com/en-us/HT208324
> 
> Related, they've also released information on CVE-2017-7164 related to
> similar vulnerabilities in the App Store:
> 
> https://support.apple.com/en-us/HT208334
> 
> Each of these CVE's also applies to a number of other Apple platforms which
> they've further acknowledged.

Does this mean that we can remove the embargo now? Is this disclosure from Apple enough to enable our own disclosure for Firefox 58?

Comment 62

[Daniel Veditz [:dveditz]]

Removing embargo comment