Bug 1409449 (CVE-2018-5115)

Phishing vector via http auth of background connections

RESOLVED FIXED in Firefox 58

Status

()

P1
normal
RESOLVED FIXED
a year ago
21 days ago

People

(Reporter: dveditz, Assigned: dragana)

Tracking

(Depends on: 2 bugs, {csectype-spoof, sec-moderate})

unspecified
mozilla59
csectype-spoof, sec-moderate
Points:
---
Dependency tree / graph
Bug Flags:
sec-bounty +
qe-verify -

Firefox Tracking Flags

(firefox57 wontfix, firefox58 fixed, firefox59 fixed)

Details

(Whiteboard: [reporter-external][necko-triaged][adv-main58+][post-critsmash-triage])

Attachments

(4 attachments, 4 obsolete attachments)

(Reporter)

Description

a year ago
+++ 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.
(Assignee)

Comment 1

a year ago
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?
Flags: needinfo?(honzab.moz)
(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).
Flags: needinfo?(honzab.moz)
(Assignee)

Comment 3

a year ago
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/
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?
Flags: needinfo?(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?
Flags: needinfo?(ckerschb)
(Assignee)

Comment 6

a year ago
(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.
Flags: needinfo?(ckerschb)

Comment 7

a year ago
>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.
(Assignee)

Comment 8

a year ago
(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.
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.
Flags: needinfo?(kmckinley)
(Reporter)

Comment 10

a year ago
(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?
(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
Flags: needinfo?(ckerschb)
(Reporter)

Comment 12

a year ago
(and of course a completely separate question is why is OpenH264 replying to broadcast messages?)
Keywords: csectype-spoof, sec-moderate

Comment 13

a year ago
>(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.
(Assignee)

Comment 14

a year ago
(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.
(Assignee)

Comment 15

a year ago
Posted patch bug_1409449.patch (obsolete) — Splinter Review
If you have other suggestion hot to detect non-web-content triggered resources, please let me know.
Assignee: nobody → dd.mozilla
Status: NEW → ASSIGNED
Attachment #8920337 - Flags: review?(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?
Attachment #8920337 - Flags: review?(ckerschb) → review+
(Assignee)

Comment 17

a year ago
(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!
(Assignee)

Comment 18

a year ago
Posted patch bug_1409449.patch (obsolete) — Splinter Review
Attachment #8920613 - Flags: review?(valentin.gosu)
(Assignee)

Comment 19

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

Francois, can you take a look at the telemetry part. Thanks.
Attachment #8920613 - Flags: review?(francois)
Comment on attachment 8920613 [details] [diff] [review]
bug_1409449.patch

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

Looks good to me.
Attachment #8920613 - Flags: review?(valentin.gosu) → review+
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": ["necko@mozilla.com"],

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.
Attachment #8920613 - Flags: review?(francois) → review+
(Assignee)

Comment 22

a year ago
(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": ["necko@mozilla.com"],
> 
> 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.
I see Christoph provided the triggeringPrincipal meta bug in comment 11
Flags: needinfo?(kmckinley)
(Assignee)

Comment 24

a year ago
Attachment #8920337 - Attachment is obsolete: true
Attachment #8920613 - Attachment is obsolete: true
Attachment #8921812 - Flags: review+
(Assignee)

Updated

a year ago
Keywords: checkin-needed
(Assignee)

Updated

a year ago
Priority: -- → P1
Whiteboard: Embargo while not fixed in other browsers [reporter-external] → Embargo while not fixed in other browsers [reporter-external][necko-triaged]
(Assignee)

Comment 27

a year ago
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.
(Assignee)

Comment 28

a year ago
Posted patch bug_1409449_tests.patch (obsolete) — Splinter Review
Attachment #8923354 - Flags: review?(valentin.gosu)
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
Attachment #8923354 - Flags: review?(valentin.gosu)

Comment 30

a year ago
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?
Dragana, are you still working on this?
(Assignee)

Comment 32

a year ago
(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.
Thanks Dragana!
(Assignee)

Comment 34

a year ago
(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.
(Assignee)

Comment 35

a year ago
Posted patch bug_1409449_tests.patch (obsolete) — Splinter Review
Attachment #8923354 - Attachment is obsolete: true
Attachment #8934463 - Flags: review?(valentin.gosu)
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.
Attachment #8934463 - Flags: review?(valentin.gosu) → review+
(Assignee)

Comment 38

a year ago
(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....
(Assignee)

Comment 39

a year ago
Attachment #8934463 - Attachment is obsolete: true
Attachment #8936350 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/09bf615d77d2
https://hg.mozilla.org/mozilla-central/rev/c31b663b4dd2
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
(Reporter)

Updated

a year ago
Flags: sec-bounty?

Updated

a year ago
Depends on: 1425307
Depends on: 1425386

Comment 41

a year ago
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.
(Reporter)

Updated

a year ago
Group: network-core-security → core-security-release
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
(Reporter)

Updated

a year ago
Flags: sec-bounty? → sec-bounty+
Does this need approval requests?
Flags: needinfo?(dd.mozilla)
(Assignee)

Comment 44

a year ago
(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.
(Assignee)

Comment 45

a year ago
(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.
Flags: needinfo?(dd.mozilla)
No, uplifts.
Flags: needinfo?(dd.mozilla)
(Assignee)

Comment 47

a year ago
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.
Flags: needinfo?(dd.mozilla) → needinfo?(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.
Flags: needinfo?(ckerschb)
(Assignee)

Comment 51

a year ago
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
Attachment #8938403 - Flags: approval-mozilla-beta?
Hi Dan,
Given this is sec-moderate and it's late in 58, do you think we need to take this in 58?
Flags: needinfo?(dveditz)
(Reporter)

Comment 53

a year ago
(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.
Flags: needinfo?(dveditz)
Comment on attachment 8938403 [details] [diff] [review]
bug_1409449_beta.patch

Fix a potential sec issue which can be used for phishing. Beta58+.
Attachment #8938403 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
status-firefox58: --- → affected
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
Flags: needinfo?(dd.mozilla)
status-firefox58: fixed → affected
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
(Assignee)

Comment 58

a year ago
(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.
Flags: needinfo?(dd.mozilla)

Comment 60

a year ago
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.
(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?
Flags: needinfo?(dveditz)
(Reporter)

Comment 62

a year ago
Removing embargo comment
Flags: needinfo?(dveditz)
Whiteboard: Embargo while not fixed in other browsers [reporter-external][necko-triaged] → [reporter-external][necko-triaged]
Whiteboard: [reporter-external][necko-triaged] → Embargo while not fixed in other browsers [reporter-external][necko-triaged][adv-main58+]
Whiteboard: Embargo while not fixed in other browsers [reporter-external][necko-triaged][adv-main58+] → [reporter-external][necko-triaged][adv-main58+]
status-firefox57: --- → wontfix
Alias: CVE-2018-5115
Flags: qe-verify-
Whiteboard: [reporter-external][necko-triaged][adv-main58+] → [reporter-external][necko-triaged][adv-main58+][post-critsmash-triage]
(Reporter)

Updated

5 months ago
Group: core-security-release

Updated

21 days ago
Depends on: 1529901
You need to log in before you can comment on or make changes to this bug.