Last Comment Bug 647010 - Only present HTTP authentication dialogs if it is the top-level document initiating the auth
: Only present HTTP authentication dialogs if it is the top-level document init...
Status: REOPENED
[parity-Chrome][adv-main40-]
: csectype-spoof, dev-doc-needed, sec-moderate, site-compat
Product: Core
Classification: Components
Component: Document Navigation (show other bugs)
: Trunk
: All All
: -- normal with 6 votes (vote)
: mozilla40
Assigned To: Dragana Damjanovic [:dragana]
:
:
Mentors:
: 559556 705501 714543 978471 1105944 (view as bug list)
Depends on: 1281434 979359 1038756 1159584 1189268 1200247 1200590 1201516 1230462
Blocks: 411085 567804 978471 1197944
  Show dependency treegraph
 
Reported: 2011-03-31 15:16 PDT by Brandon Sterne (:bsterne)
Modified: 2016-06-22 05:09 PDT (History)
55 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
40+


Attachments
bug_647010_v1.patch (6.04 KB, patch)
2015-02-26 07:36 PST, Dragana Damjanovic [:dragana]
no flags Details | Diff | Splinter Review
bug_647010_v2.patch (5.93 KB, patch)
2015-02-26 13:52 PST, Dragana Damjanovic [:dragana]
honzab.moz: feedback-
tanvi: feedback+
Details | Diff | Splinter Review
bug_647010_v3.patch (10.77 KB, patch)
2015-03-09 02:45 PDT, Dragana Damjanovic [:dragana]
no flags Details | Diff | Splinter Review
bug_647010_v3.patch (26.99 KB, patch)
2015-03-09 14:48 PDT, Dragana Damjanovic [:dragana]
tanvi: review+
Details | Diff | Splinter Review
bug_647010_v4.patch (28.19 KB, patch)
2015-03-11 08:50 PDT, Dragana Damjanovic [:dragana]
honzab.moz: feedback+
Details | Diff | Splinter Review
bug_647010_v5.patch (25.16 KB, patch)
2015-03-31 08:08 PDT, Dragana Damjanovic [:dragana]
honzab.moz: review+
Details | Diff | Splinter Review
bug_647010_v6.patch (25.15 KB, patch)
2015-04-01 06:56 PDT, Dragana Damjanovic [:dragana]
dd.mozilla: review+
Details | Diff | Splinter Review

Description Brandon Sterne (:bsterne) 2011-03-31 15:16:37 PDT
MustLive reported this today to security@m.o and I was unable to find an existing bug in Bugzilla, so I've opened this.  Feel free to DUP it if you find the appropriate bug.  I'll write the short summary and include MustLive's full message below:

Summary:

Users can be fooled into typing their credentials into HTTP authentication dialogs from other (potentially attacker-controlled) web sites if an attacker can inject content protected by HTTP auth into a legitimate site.  Sub-document resources like images, scripts, iframes, etc. should not be able to cause this dialog, possibly in any case, but certainly in cases where the resource is in a different origin.

Full Message:

Hello Mozilla!
 
I want to warn you about URL Spoofing vulnerability in Mozilla Firefox (and in other browsers). I found it long time ago, at 6th of February 2008, and at last Saturday I made announcement of it at my site (just found time for it). Vulnerability is low risk and can be considered by you and all browser vendors as small one, but I think it worth mentioning.
 
And it has enough danger for users of browsers (of almost all browsers, because all browsers which support Basic/Digest Authentication are vulnerable) to not just write small post about it, but to inform all browser vendors (which browsers I have at my computer and in which I've tested). Affected are Mozilla Firefox (and Mozilla and all other Gecko-based browsers), Internet Explorer, Google Chrome and Opera.
 
This is better to call attack, then vulnerability, because it's using built-in browsers functionality (and its intended behavior) to attack users of web sites. This attack allows to conduct phishing attacks on users of web sites - in this case phishing is doing not at other (phishing) sites, not with using of holes of target sites (like reflected XSS or persistent XSS), but with using of browsers functionality (and allowed functionality of target sites to place external content).
 
I called this attack as Onsite phishing (or Inline phishing). It can be used (including by phishers) for stealing of logins and passwords of users of web sites.
 
As I've tested, a lot of different methods (with using of tags and CSS), which allow to make cross-site requests, can be used to conduct this attack. Except prefetching (in all Gecko-based browsers which support prefetching functionality), which doesn't show Authentication window at receiving of 401 response from web server. The next methods can be used:
 
Tags img, script, iframe, frame, embed, link (css) - Mozilla, Firefox, IE, Google Chrome and Opera.
Tag object - Internet Explorer, Google Chrome and Opera.
CSS (inline, in html files, in external css files): such as -moz-binding:url - Mozilla and Firefox < 3.0, such as background-image:url - in all browsers.
 
Here are screenshots of the attack in different browsers (in Firefox 3.0.19, 3.5.x, 3.6.x. 4.0b2 the dialog window looks almost equally):

http://websecurity.com.ua/uploads/2011/03/Attack%20on%20Mozilla.png
http://websecurity.com.ua/uploads/2011/03/Attack%20on%20Firefox.png
http://websecurity.com.ua/uploads/2011/03/Attack%20on%20IE6.png
http://websecurity.com.ua/uploads/2011/03/Attack%20on%20IE7.png
http://websecurity.com.ua/uploads/2011/03/Attack%20on%20IE8.png
http://websecurity.com.ua/uploads/2011/03/Attack%20on%20Chrome.png
http://websecurity.com.ua/uploads/2011/03/Attack%20on%20Opera.png
 
The attack can be made as reflected at target site, as persistent (with using of allowed functionality at target site, which allows to put some tags, like img tag). The persistent attack is more dangerous (and such type of attack is showed on screenshots). And there are millions of web sites which allow such user generated content (like img tags) which can lead to such persistent attacks. If you'll need some additional explanation of this attack to understand it better, then tell me and I'll write you common attack scenario for it.
 
Vulnerable are Mozilla Firefox 3.0.19, Firefox 3.5.11, Firefox 3.6.8, Firefox 4.0b2 and previous and next versions (and other browsers).
 
Attend to security of all of yours web sites, web software, browsers and to security audit.
 
I mentioned about this vulnerability at my site (http://websecurity.com.ua/5038/). For now I don't post any concrete information about vulnerability, in order to inform you first and give you time to fix them.
 
Best wishes & regards,
MustLive
Administrator of Websecurity web site
http://websecurity.com.ua
Comment 1 Boris Zbarsky [:bz] (still a bit busy) 2011-04-01 09:04:05 PDT
A major problem here is site compat...
Comment 2 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-04-01 12:40:44 PDT
Like the reporter suggested, it might be OK to allow sub-resources with the same origin as the main document to cause the auth dialog to pop up. At least then the plan to make the auth dialog a doorhanger would continue to make sense.

Perhaps this could also be made a setting that is controlled by CSP or a CSP-like mechanism? If so, the default should be to restrict to the same origin.
Comment 3 Brandon Sterne (:bsterne) 2011-04-01 13:50:55 PDT
(In reply to comment #2)
> Perhaps this could also be made a setting that is controlled by CSP or a
> CSP-like mechanism? If so, the default should be to restrict to the same
> origin.

Funny you mention that.  A http-auth-src directive (perhaps differently named) has already been proposed as a new CSP feature.
Comment 4 Patrick McManus [:mcmanus] 2011-04-02 08:37:40 PDT
echos of bug 580168 ? That bug had been leaning towards WONTFIX, so maybe CSP is a way forward?
Comment 5 YGN Ethical Hacker Group 2011-04-06 19:39:02 PDT
I think so. The http-auth-src in CSP can be a good solution to this. Limiting to http-auth-src to the same domain by default might break the web.
Comment 6 MustLive 2011-04-09 12:23:14 PDT
Dear Mozilla developers!

As I already wrote to Brandon, it's quite possible, that I was not first who have though about this attack on users of different web browsers. And I though so in February 2008, when found this hole, and because I've never heard about this attack from that time, so I decided to inform four browsers vendors about it. And if there are no earlier entries in Bugzilla about this issue, then this will be such one.

To mentioned in my letter I'll add, that my position is the next: browsers vendors need to fix it. If they begun long time ago to fixing phishing attacks via phishing sites (by using of blacklists of sites - which is built-in feature in most modern browsers), then this attack is another phishing attack vector and it also need to be fixed (and efficiently).

There are two ways which you and other browsers vendors can go:

1. Fix it completely.
2. Made some interface improvements to reduce the risk.

It's possible to combine both of them, but if you'll make #1, then there will be no need in #2.

#1 - it's to fix issue completely, to remove possibility for phishing attacks (it can be done in different ways).

#2 - it's to improve user interface. Like to add highlighting to domain name (to make it harder to fool people) and to add possibility to get information about the domain which shows authentication window. The last feature is already implemented in Opera - in only one from all browsers tested by me (see on my screenshots or open authentication window in Opera 10 or 11). In Opera users have more chances to identified such phishing attacks, but even this browsers needs to fix this issue. So #1 is very recommended for all browsers vendors.

Concerning CSP, which many of you have mentioned. It's a good way to solve this issue. But it has two shortcomings:

1. CSP is supported only in Firefox 4.0 (and I've recommended to fix this issue in Firefox 3.x too).
2. CSP will require actions from admins of web sites, so it'll take a long time, when all sites will be protected against this attack. So during a long time there will be sites, where users of Gecko-based browsers (even of last versions) will be under risk of this attack.

So take it into account when you'll be working on the fix.
Comment 7 Brandon Sterne (:bsterne) 2011-07-20 08:40:26 PDT
(In reply to comment #5)
> I think so. The http-auth-src in CSP can be a good solution to this.
> Limiting to http-auth-src to the same domain by default might break the web.

Well, Chromium just announced this exact fix, so we don't have the breaking-the-web excuse anymore:
http://blog.chromium.org/2011/06/new-chromium-security-features-june.html

(they're blocking cross-origin HTTP auth prompts)
Comment 8 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-07-20 14:01:58 PDT
(In reply to comment #7)
> Well, Chromium just announced this exact fix, so we don't have the
> breaking-the-web excuse anymore:

That's not quite how that works - that Chromium's going to try something doesn't mean concerns about Web compat just go away :) They have good in-the-field experimentation data (and the ability to revert changes quickly), but they also still have a smaller (and different in composition) user base than we do. It's great that they're being aggressive - I think we should be more aggressive too - but we still need to be careful and avoid just blindly following their lead.
Comment 9 Brandon Sterne (:bsterne) 2011-11-28 20:02:38 PST
*** Bug 705501 has been marked as a duplicate of this bug. ***
Comment 10 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-01-24 14:42:03 PST
*** Bug 559556 has been marked as a duplicate of this bug. ***
Comment 11 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-01-24 14:42:09 PST
*** Bug 714543 has been marked as a duplicate of this bug. ***
Comment 12 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-02-03 17:57:15 PST
See also bug 724182.
Comment 13 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-02-03 18:01:25 PST
Changing components because, while I think this is a good idea, but this shouldn't be implemented in Necko, but rather in some higher, UI-related component. Necko shouldn't be displaying or controlling UI at all (I know it does participate in UI-related things currently, but there is a bug already on file to have us stop doing so.)

Bug 724182 describes a way to limit the compatibility impact by enforcing CORS for these requests. Presumably, we could prompt for HTTP credentials if the CORS preflight request tells us to do so.
Comment 14 AWAY Tom Schuster [:evilpie] 2013-05-02 13:51:54 PDT
This tweet (https://twitter.com/RSnake/status/328908680097574912), which was going around yesterday is somewhat related. I tried implementing some hackish switch to requesting channel's tab logic in ModalPrompert.promptAuth, but couldn't get it to work.
Comment 15 Honza Bambas (:mayhemer) 2013-05-02 15:33:13 PDT
We could easily bypass prompts at nsHttpChannelAuthProvider::PromptForIdentity for channels that are subrequests, however that is not that easy to recognize what should actually bypass.  We cannot use !LOAD_INITIAL_DOCUMENT_URI, since we must allow XHR and download to prompt.
Comment 16 Florian Bender 2013-05-17 02:15:16 PDT
(In reply to Tom Schuster [:evilpie] from comment #14)
> This tweet (https://twitter.com/RSnake/status/328908680097574912)

Is there a bug covering the "delayed reload and auth" attack?
Comment 17 Justin Dolske [:Dolske] 2013-05-18 17:31:35 PDT
Comments 7/8 imply Chrome was experimenting with this -- did it stick for them? Also, are they only allowing auth for the top-level doc load itself (as this bug imples), or only cross-origin loads? (Eg, if an unprotected index.html loads ./subdir/protected.foo, will Chrome prompt?)
Comment 18 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2013-05-18 19:37:18 PDT
I changed this to sec-high because it meets the criteria "Spoofing of full URL bar or bypass of SSL integrity checks". I think it is fair to say that users are unlikely to notice when the origin in the HTTP auth dialog box jibberish is different than the origin in the address bar. Note that even with mixed content blocker enabled, SSL integrity checks can be bypassed because we still allow non-HTTPS images/audio/video in HTTPS pages.
Comment 19 Daniel Veditz [:dveditz] 2013-06-04 12:16:26 PDT
HTTP auth is rarely used on the public web in practice, this is not anything like a general SSL spoof.
Comment 20 Allan Gardner (:Mathnerd314) 2013-06-14 16:35:05 PDT
Chrome did not stick; see http://crbug.com/174179. They only blocked cross-origin loads AFAICT.
Comment 21 Justin Dolske [:Dolske] 2013-06-14 18:06:41 PDT
Thanks for the pointer. From a quick skim, looks like the actual undoing happened in http://crbug.com/123150... But it's not quite clear to me exactly _why_ it got reverted; the cross-origin blocking from http://crbug.com/81251 shipped for a year before being backed out (apparently not quite following their process, and now 174179 et al are back to figuring out how to fix things).

I'm curious about the rationale for their backout. Seems like there was some user grumbling, but also maybe some unexpected breakage of authenticated XHR (see 123150 #68/69).
Comment 22 Allan Gardner (:Mathnerd314) 2013-06-19 09:36:02 PDT
The worry is that someone could load a million http://user:pw@host url's and conduct a relatively silent brute-force password attack on e.g. a home router. They addressed it in 94578 by disabling the user:pw syntax, but that got reverted due to user complaints (123150). So instead they added back the auth dialogs (174129).
Comment 23 Johnny Stenback (:jst, jst@mozilla.com) 2013-07-11 13:38:56 PDT
Honza, is this something you could take? We could likely apply a fix much like the fix the chrome guys took for this same problem.
Comment 24 Honza Bambas (:mayhemer) 2013-07-12 03:48:32 PDT
(In reply to Johnny Stenback (:jst, jst@mozilla.com) from comment #23)
> Honza, is this something you could take? We could likely apply a fix much
> like the fix the chrome guys took for this same problem.

Yes, but not very soon.
Comment 25 O. Atsushi (Torisugari) 2013-08-10 10:56:43 PDT
So we are going to check only hosts? What happens another realm requests a different username/password ?
Comment 26 Honza Bambas (:mayhemer) 2013-08-13 05:09:18 PDT
(In reply to O. Atsushi (Torisugari) from comment #25)
> So we are going to check only hosts? What happens another realm requests a
> different username/password ?

I'm not sure I understand tour questions fully, can you be more detailed please?
Comment 27 Florian Bender 2014-02-26 07:07:14 PST
What's the plan here now? Disable all subresource HTTP-auth, or just cross-origin? I'd add both options and a pref, e.g.: 

network.auth.allow-subresource-auth = (AUTH_ALLOW_SUBRESOURCE = 1) | (AUTH_ALLOW_CROSS_ORIGIN = 2)

Default is 1 (for web compat). Only allowing cross-origin (i.e. value of 2) wouldn't make any sense, of course.

This may also warrant a toggle in the security prefs panel (not just an about:config value). This can be tri-state (allow all, allow same-origin, disallow) or binary (with any of the three possible combinations) though I'd only offer a checkbox that toggles the first bit (i.e. allow subresource loading or not) and leave the cross-origin option to the power users.
Comment 28 Tanvi Vyas [:tanvi] 2014-09-22 17:03:53 PDT
Does anyone know of any legitimate websites that need cross-site http auth prompts?  Looking at the chrome bugs on the issue, they ran into problems with xhr and iframes.  Chrome decided to block http auth prompts via images.

For this bug, it would be great if we could block all cross-site http auth requests and add an about:config pref to restore to previous functionality.  Since this might cause site compatibility issues, we could at a minimum deny cross site prompts from non-xhr and non-iframe loads.

There is a bug open to gather telemetry (https://bugzilla.mozilla.org/show_bug.cgi?id=979359).  We could gather some data to determine how frequently cross site auth prompts occur, and when they do occur we can check if they are xhr, frame, or other.  In order to gather telemetry, we will have to do the engineering work to implement this feature anyway.  Perhaps the implementation will be simpler now that we have loadInfo on channels (bug 1038756).  Before we show the prompt, we can do a same origin check with the loadInfo->loadingNode and check the content policy type of the load.

Adding more options (allow everything, allow subresource same origin, deny subresource same origin), adding user visible options to preferences, and changing the UI to more prominently display the website requesting credentials would all also be nice.  But I think we can do those as followup bugs.
Comment 29 Tanvi Vyas [:tanvi] 2014-09-25 16:30:00 PDT
We have to be wary of a potential attack vector when turning cross-orign http auth off.  An attacker could try and brute force a password with something like:
<img src="http://username:password@ipaddress">

They could look for onerror events and cycle through passwords until they find one that doesn't invoke the onerror.

I think we should try and turn off cross-origin http auth for everything but xhr and frames to start with.  And also disable sending credentials in the url when we block the auth prompt.
Comment 30 Boris Zbarsky [:bz] (still a bit busy) 2014-11-28 20:50:31 PST
*** Bug 1105944 has been marked as a duplicate of this bug. ***
Comment 31 Olli Pettay [:smaug] (reviewing overload) 2014-12-03 14:36:53 PST
Tanvi, do you think you could look at this?
Comment 32 Tanvi Vyas [:tanvi] 2014-12-04 14:40:50 PST
(In reply to Olli Pettay [:smaug] from comment #31)
> Tanvi, do you think you could look at this?

Last I talked to jduell, he was going to add this to the networking teams backlog and see if he could assign someone from his team to this bug.  jduell, any update?
Comment 33 Jason Duell [:jduell] (needinfo me) 2014-12-04 16:29:47 PST
Honza, can you either fix this or teach someone else in Necko (Valentin/Daniel/Dragana) how to?  Not urgent but worth fixing.
Comment 34 Honza Bambas (:mayhemer) 2015-01-13 13:55:51 PST
OK, assigning to me to find an assignee..
Comment 35 Dragana Damjanovic [:dragana] 2015-02-25 05:14:33 PST
I will take a look at this.
Comment 36 Honza Bambas (:mayhemer) 2015-02-25 05:23:15 PST
Please next time update the assignee field.  Thanks.
Comment 37 Dragana Damjanovic [:dragana] 2015-02-26 07:36:47 PST
Created attachment 8569912 [details] [diff] [review]
bug_647010_v1.patch

I am not sure if I am doing the right thing with cross-origin check - is it ok as it is implemented or i will need to look for top document and its uri?
It should be the top-document uri that i have to check against, but i am not sure i am doing it correctly. Probably it is not correct :)
Comment 38 Tanvi Vyas [:tanvi] 2015-02-26 11:03:40 PST
Comment on attachment 8569912 [details] [diff] [review]
bug_647010_v1.patch

Hi Dragana,

Thank you for picking up this bug!

Why are iframes whitelisted?  iframes should only be allow to cause the HTTP Auth prompt if they are same origin.

Also, we should use the loadingPrincipal from loadInfo instead of the triggeringPrincipal.  There are cases where the triggeringPrincipal is a css stylesheet (for example) and what we are looking for is the origin of the top level document.
https://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsILoadInfo.idl#57
Comment 39 Dragana Damjanovic [:dragana] 2015-02-26 11:25:49 PST
Thank you for the feedback.

(In reply to Tanvi Vyas [:tanvi] from comment #38)
> Comment on attachment 8569912 [details] [diff] [review]
> bug_647010_v1.patch
> 
> Hi Dragana,
> 
> Thank you for picking up this bug!
> 
> Why are iframes whitelisted?  iframes should only be allow to cause the HTTP
> Auth prompt if they are same origin.
> 

Sorry I meant to change that..

> Also, we should use the loadingPrincipal from loadInfo instead of the
> triggeringPrincipal.  There are cases where the triggeringPrincipal is a css
> stylesheet (for example) and what we are looking for is the origin of the
> top level document.
> https://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsILoadInfo.
> idl#57

I have read that and just mixed them up. I have read today a lot of stuff about all loadInfo, loadingNode, content types... (the names are a bit confusing, at least when you first look at them)
I will update it soon and ping you for feedback :)
Comment 40 Dragana Damjanovic [:dragana] 2015-02-26 13:52:20 PST
Created attachment 8570139 [details] [diff] [review]
bug_647010_v2.patch
Comment 41 Tanvi Vyas [:tanvi] 2015-02-26 16:21:40 PST
Comment on attachment 8570139 [details] [diff] [review]
bug_647010_v2.patch

+    nsCOMPtr<nsIChannel> chan = do_QueryInterface(mAuthChannel);
+    nsCOMPtr<nsILoadInfo> loadInfo;
+    chan->GetLoadInfo(getter_AddRefs(loadInfo));
+    if (!loadInfo) {
+        return true;
+    }

Unfortunately, I think we have to return false if there is no loadInfo.  addon created channels won't have a loadInfo attached to them.  Most Gecko channels should, so 99% of the time we can make a good decision about whether or not we should allow the auth prompt.  It is not ideal, but at least this patch gets us in a better place than we are today.

Jonas, Christoph - if you feel differently and feel that we can fail closed if there is no loadInfo in this case, please chime in!
Comment 42 Honza Bambas (:mayhemer) 2015-02-27 08:08:05 PST
Comment on attachment 8570139 [details] [diff] [review]
bug_647010_v2.patch

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

::: modules/libpref/init/all.js
@@ +1682,5 @@
>  pref("network.automatic-ntlm-auth.allow-proxies", true);
>  pref("network.automatic-ntlm-auth.allow-non-fqdn", false);
>  pref("network.automatic-ntlm-auth.trusted-uris", "");
>  
> +// Subresource HTTP-auth: 0 - do not allow suresources HTTP-auth

don't allow sub-resources to open WWW authentication credentials dialogs

update the option 1 comment as well

::: netwerk/protocol/http/nsHttpChannelAuthProvider.cpp
@@ +736,5 @@
>                  level = nsIAuthPrompt2::LEVEL_SECURE;
>              else if (authFlags & nsIHttpAuthenticator::IDENTITY_ENCRYPTED)
>                  level = nsIAuthPrompt2::LEVEL_PW_ENCRYPTED;
>  
> +            // Prompt the use?

user

but this comment should rather say what the BlockPromp() method does.

@@ +803,5 @@
> +    nsCOMPtr<nsIPrefBranch> prefService =
> +        do_GetService(NS_PREFSERVICE_CONTRACTID);
> +    int32_t authAllowOpt;
> +    nsresult rv = prefService->GetIntPref("network.auth.allow-subresource-auth",
> +                                          &authAllowOpt);

you may use mozilla::Preferences

is this ensured to be called on the main thread only if you want to read the prefs?  maybe better would be to have a cache for the pref if MT-only cannot be enforced here.  It would also be faster.   See mozilla::Preferences::Add*VarCache methods.

@@ +807,5 @@
> +                                          &authAllowOpt);
> +
> +    // On error just allow everything.
> +    // 2 - allow everything.
> +    if (NS_FAILED(rv) || (authAllowOpt == 2)) {

have #defines for the values
I'd slightly more prefer a switch here
split the conditions (have two return false; for each side of ||)

@@ +816,5 @@
> +    if (authAllowOpt == 0) {
> +        return true;
> +    }
> +
> +    // 1 - only allow for subresources that are not cross-origin.

this is actually != 0 && != 2..

::: netwerk/protocol/http/nsHttpChannelAuthProvider.h
@@ +110,5 @@
>       * This is called from ProcessResponse.
>       */
>      nsresult ProcessSTSHeader();
>  
> +    bool BlockPrompt();

some comments please
Comment 43 Dragana Damjanovic [:dragana] 2015-03-09 02:45:41 PDT
Created attachment 8574587 [details] [diff] [review]
bug_647010_v3.patch
Comment 44 Dragana Damjanovic [:dragana] 2015-03-09 14:48:52 PDT
Created attachment 8574880 [details] [diff] [review]
bug_647010_v3.patch
Comment 45 Dragana Damjanovic [:dragana] 2015-03-09 14:49:18 PDT
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5167934020d5
Comment 46 Tanvi Vyas [:tanvi] 2015-03-10 12:19:04 PDT
Comment on attachment 8574880 [details] [diff] [review]
bug_647010_v3.patch

+            // Prompt the user for HTTP-authentication dialog? Depending on the
+            // pref, we may suppress the authentication dialog for all
+            // sub-resources, only for cross-origin sub-resources or the dialog
+            // will be displayed for all sub-resources.
Depending on the pref setting, the authentication dialog may be blocked for all sub-resources, blocked for cross-origin sub-resources, or always allowed for sub-resources.
(And the same comment change in nsHttpChannelAuthProvider.h)

+            // For more details look at the bug 647010.
+            if (BlockPrompt()) {
+              return NS_ERROR_ABORT;
+            }
+


+            nsCOMPtr<nsIURI> loadingURI;
+            loadInfo->LoadingPrincipal()->GetURI(getter_AddRefs(loadingURI));
You should add a null check for LoadingPrincipal.

Leaving the test for Honza.
Comment 47 Dragana Damjanovic [:dragana] 2015-03-11 08:50:02 PDT
Created attachment 8576046 [details] [diff] [review]
bug_647010_v4.patch

make changes suggested by Tanvi and fixed failing test on Android.
Comment 48 Honza Bambas (:mayhemer) 2015-03-24 11:00:08 PDT
Comment on attachment 8576046 [details] [diff] [review]
bug_647010_v4.patch

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

Any try run?

I'd like to check the next version.

::: netwerk/protocol/http/nsHttpChannelAuthProvider.cpp
@@ +28,5 @@
>  namespace mozilla {
>  namespace net {
>  
> +#define DO_NOT_ALLOW_AUTH_DIALOG_FOR_SUB_RESOURCES 0
> +#define ALLOW_AUTH_DIALOG_FOR_NOT_CROSS_ORIGIN_SUB_RESOURCES 1

this should strictly reflect the description in all.js.

I'd see the names rather as:

SUBRESOURCE_AUTH_DIALOG_DISALLOW_ALL 0
SUBRESOURCE_AUTH_DIALOG_DISALLOW_CROSS_ORIGIN 1
SUBRESOURCE_AUTH_DIALOG_ALLOW_ALL 2

@@ +64,5 @@
>  {
>      MOZ_ASSERT(!mAuthChannel, "Disconnect wasn't called");
>  }
>  
> +uint32_t nsHttpChannelAuthProvider::sAuthAllowOpt = 1;

use #define

@@ +71,5 @@
> +nsHttpChannelAuthProvider::InitializePrefs()
> +{
> +  mozilla::Preferences::AddUintVarCache(&sAuthAllowOpt,
> +                                        "network.auth.allow-subresource-auth",
> +                                        1);

nit: maybe assert main thread here.

@@ +817,5 @@
> +        (loadInfo->GetContentPolicyType() == nsIContentPolicy::TYPE_XMLHTTPREQUEST)) {
> +        return false;
> +    }
> +
> +    bool block = false;

maybe just directly return the result where applicable instead of playing with this local var?

@@ +828,5 @@
> +    case ALLOW_AUTH_DIALOG_FOR_NOT_CROSS_ORIGIN_SUB_RESOURCES:
> +        // Do not open the http-authentication credentials dialog for
> +        // the sub-resources only if they are not cross-origin.
> +        {
> +            nsCOMPtr<nsIURI> loadingURI;

declare before use

@@ +830,5 @@
> +        // the sub-resources only if they are not cross-origin.
> +        {
> +            nsCOMPtr<nsIURI> loadingURI;
> +            nsCOMPtr<nsIPrincipal> loadingPrincipal = loadInfo->LoadingPrincipal();
> +            if (loadingPrincipal) {

if (!loadingProncipal) 
  return false

?

etc..

@@ +834,5 @@
> +            if (loadingPrincipal) {
> +                loadingPrincipal->GetURI(getter_AddRefs(loadingURI));
> +
> +                nsIScriptSecurityManager* ssm = nsContentUtils::GetSecurityManager();
> +                if (NS_FAILED(ssm->CheckSameOriginURI(loadingURI, mURI, false))) {

isn't there some CheckMayLoad method/function taking loading principal instead of URL?

@@ +843,5 @@
> +            }
> +        }
> +        break;
> +    default:
> +        // Allow the http-authentication dialog. 

nit: ws

::: netwerk/protocol/http/nsHttpChannelAuthProvider.h
@@ +114,5 @@
> +    // Depending on the pref setting, the authentication dialog may be blocked
> +    // for all sub-resources, blocked for cross-origin sub-resources, or
> +    // always allowed for sub-resources.
> +    // For more details look at the bug 647010.
> +    bool BlockPrompt();

blank line after this line please

@@ +154,5 @@
>      uint32_t                          mSuppressDefensiveAuth    : 1;
>  
>      nsRefPtr<nsHttpHandler>           mHttpHandler;  // keep gHttpHandler alive
> +
> +    static uint32_t                   sAuthAllowOpt;

comment what this means

s/Opt/Pref/ ?

::: netwerk/test/unit/test_auth_dialog_permission.js
@@ +113,5 @@
> +
> +  onStopRequest: function test_onStopR(request, ctx, status) {
> +    do_check_eq(status, Components.results.NS_ERROR_ABORT);
> +
> +    if (current_test < (tests.length - 1)) {

I personally prefer test.shift() semantics ;)

and should have some next_test() function

@@ +126,5 @@
> +      do_test_pending();
> +      httpserv.stop(do_test_finished);
> +    }
> +
> +    do_test_finished();

please explain why you call this here.

@@ +201,5 @@
> +  let uri = make_uri("https://example.com");
> +  let principal = Components.classes["@mozilla.org/scriptsecuritymanager;1"]
> +                    .getService(Ci.nsIScriptSecurityManager)
> +                    .getNoAppCodebasePrincipal(uri);
> +  let chan = makeChan(URL + "/auth", principal, Ci.nsIContentPolicy.TYPE_OTHER);

would be nice to hide call to make_uri and principal creation into the makeChannel function.  You always do the same thing anyway.  Or have a make_loading_principal(URI) function?

@@ +258,5 @@
> +}
> +
> +function test_no_cross_origin_aurh_request_not_cross_origin() {
> +  prefs.setIntPref("network.auth.allow-subresource-auth", 1);
> +  let uri = make_uri(URL + "/auth");

nit: maybe use URL + "/" for the sameorigin loading principals

@@ +289,5 @@
> +}
> +
> +function test_no_sub_resources_auth_request_cross_origin() {
> +  prefs.setIntPref("network.auth.allow-subresource-auth", 0);
> +  let uri = make_uri("https://exeption.com");

what is this host name?
Comment 49 Dragana Damjanovic [:dragana] 2015-03-31 08:08:47 PDT
Created attachment 8586139 [details] [diff] [review]
bug_647010_v5.patch
Comment 50 Dragana Damjanovic [:dragana] 2015-03-31 08:15:13 PDT
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3bcb3763af28
Comment 51 Honza Bambas (:mayhemer) 2015-03-31 12:40:10 PDT
Comment on attachment 8586139 [details] [diff] [review]
bug_647010_v5.patch

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

The test looks good!  thanks.

::: modules/libpref/init/all.js
@@ +1684,5 @@
> +// Sub-resources HTTP-authentication:
> +//   0 - don't allow sub-resources to open HTTP authentication credentials
> +//       dialogs
> +//   1 - allow sub-resources to open HTTP authentication credentials dialogs,
> +//       but don't allow it for cross-origin sub-resources

don't allow cross-origin subresources to open HTTP ... ?

::: netwerk/protocol/http/nsHttpChannelAuthProvider.cpp
@@ +30,5 @@
>  
> +#define SUBRESOURCE_AUTH_DIALOG_DISALLOW_ALL 0
> +#define SUBRESOURCE_AUTH_DIALOG_DISALLOW_CROSS_ORIGIN 1
> +#define SUBRESOURCE_AUTH_DIALOG_ALLOW_ALL 2
> +#define SUBRESOURCE_AUTH_DIALOG_DEFAULT 1

nit: rather then 1 use one of the defines here

but I kinda don't follow why you are trying to hide this under a DEFAULT define..

@@ +72,5 @@
> +
> +void
> +nsHttpChannelAuthProvider::InitializePrefs()
> +{
> +  MOZ_ASSERT(NS_IsMainThread(), "This must be on the main thread.");

The message is redundant.  What the assertion says is clear from the first arg.  If you want a message here, make it meaningful or just omit (preferred).

@@ +75,5 @@
> +{
> +  MOZ_ASSERT(NS_IsMainThread(), "This must be on the main thread.");
> +  mozilla::Preferences::AddUintVarCache(&sAuthAllowPref,
> +                                        "network.auth.allow-subresource-auth",
> +                                        1);

if you want the _DEFAULT define then why don't you use it here?

::: netwerk/test/unit/test_auth_dialog_permission.js
@@ +19,5 @@
> +  if (metadata.hasHeader("Authorization") &&
> +      metadata.getHeader("Authorization") == expectedHeader) {
> +
> +    response.setStatusLine(metadata.httpVersion, 200, "OK, authorized");
> +    response.setHeader("WWW-Authenticate", 'Basic realm="secret"', false);

are you sure this header should be set on a 200 response?

@@ +110,5 @@
> +  return ios.newURI(url, null, null);
> +}
> +
> +function makeChan(loadingUrl, url, contentPolicy) {
> +  var uri = make_uri(loadingUrl);

nit: maybe s/uri/loadingURI/ ?

@@ +179,5 @@
> +    Components.classes["@mozilla.org/network/http-auth-manager;1"]
> +              .getService(Components.interfaces.nsIHttpAuthManager)
> +              .clearAll();
> +
> +    do_timeout(0, run_next_test);

nit: there is do_execute_soon()

@@ +198,5 @@
> +  }
> +};
> +
> +var tests = [
> +  // For the next 3 test the preference is set to 2 - allow the cross-origin

3 tests
Comment 52 Dragana Damjanovic [:dragana] 2015-04-01 06:56:01 PDT
Created attachment 8586752 [details] [diff] [review]
bug_647010_v6.patch
Comment 53 Ryan VanderMeulen [:RyanVM] 2015-04-01 07:44:53 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e642b4f35c4
Comment 54 Wes Kocher (:KWierso) 2015-04-01 18:07:38 PDT
https://hg.mozilla.org/mozilla-central/rev/2e642b4f35c4
Comment 55 Daniel Veditz [:dveditz] 2015-04-22 11:28:56 PDT
*** Bug 978471 has been marked as a duplicate of this bug. ***
Comment 56 Florian Bender 2015-07-16 13:46:49 PDT
Release Note Request (optional, but appreciated)
[Why is this notable]: less user annoyance & higher security
[Suggested wording]: Sub-resources can no longer request HTTP authentication, thus protecting users from inadvertently disclosing login data.
[Links (documentation, blog post, etc)]: ?
Comment 57 Kohei Yoshino [:kohei] 2015-07-16 13:48:19 PDT
This is on the site compat doc:
https://developer.mozilla.org/en-US/Firefox/Releases/40/Site_Compatibility#Security
Comment 58 Daniel Veditz [:dveditz] 2015-07-29 17:14:17 PDT
It's more appropriate to relnote this behavior change (point to the above) rather than write a security advisory for it.
Comment 59 Sylvestre Ledru [:sylvestre] 2015-08-03 06:07:31 PDT
relnote-firefox is the proper way to propose a release note item.

Anyway, added to the release notes with "Sub-resources can no longer request HTTP authentication, thus protecting users from inadvertently disclosing login data" as wording. I also added a link against https://developer.mozilla.org/en-US/Firefox/Releases/40/Site_Compatibility#Security for more details.

I guess this change also impact our Android release.
Comment 60 MustLive 2015-08-11 11:05:26 PDT
Daniel and guys!

Relnote for this behavior change is not appropriate. Since it's a vulnerability (and Mozilla developers agreed with me), so there must be security advisory for it.

Make MFSA as you always do. So everyone will see that after many years of thinking and working on a fix, you made it and your browser will be safe from this vulnerability. As Google did in Chrome in 2011 (4 months after I informed them, without answering and thanking me, but with no doubts they agreed with me concerning it).
Comment 61 janderson2000 2015-08-11 15:54:34 PDT
This "fix" just broke thousands of sites. It doesn't even appear to allow a user to optionally click the secondary iframe authentication page.
Comment 62 Daniel Veditz [:dveditz] 2015-08-11 20:15:11 PDT
janderson: probably best to file a separate bug giving examples of sites that regressed from this fix, otherwise it will get lost at the end of this "resolved" bug.

I don't understand what you mean by "click the secondary iframe authentication page". It might be clearer with a specific example site and the steps you expect to work.
Comment 63 janderson2000 2015-08-11 20:58:37 PDT
Many internal business sites use embedded iframes from more than one source/domain.  This "bug fix" has eliminated what many consider a useful browser behavior in a trusted environment with the mindset that it was just a security hole waiting to be exploited with the help of a careless user who doesn't notice the source of a secondary iframe authentication.  

While the above scenario can happen, legitimate secondary source authentication is also in many sites, and they are all now instantly broken.

A better solution to this potential problem would be to show a warning to the user that another source is seeking authentication and then allow the user to accept or reject.  

As this "fix" was implemented, it has been very disruptive.
Comment 64 janderson2000 2015-08-11 21:01:10 PDT
Also, I know about the configuration fix, but you can't have all your users change their browser configuration to allow more than one authentication source.
Comment 65 zorgos 2015-08-12 05:49:25 PDT
bug report: the HTTP auth dialog does not work since version 40 of Firefox.
Comment 66 Kohei Yoshino [:kohei] 2015-08-12 06:31:51 PDT
Enterprise users can deploy Firefox 38 ESR [1] which is not affected by this change, and they can also centrally configure [2] Firefox 45 ESR when it's out. This is already stated in the site compatibility doc [3]. Nothing should be problem here.

[1] https://www.mozilla.org/en-US/firefox/organizations/
[2] https://developer.mozilla.org/en-US/docs/MCD,_Mission_Control_Desktop_AKA_AutoConfig
[3] https://developer.mozilla.org/en-US/Firefox/Releases/40/Site_Compatibility#Security
Comment 67 janderson2000 2015-08-12 07:27:57 PDT
Deploying an entirely different version isn't exactly a simple solution.

This was a terrible fix....a lazy fix....  a "we don't give a F" fix.  Just shut the whole thing down instead of implementing a smart embedded warning/exception storing fix.  Another blow to the usability and utility of Firefox.
Comment 68 MustLive 2015-08-12 12:03:30 PDT
Daniel, Kohei and Mozilla in the whole.

Firefox 40 was released yesterday and this vulnerability was fixed in it. But there was no MSFA for this issue, even you made 14 security advisories for FF 40.

In Release Notes in Security section you mentioned about it, but without MFSA (
https://developer.mozilla.org/en-US/Firefox/Releases/40/Site_Compatibility#Security). Even for vulnerability MFSA 2015-91 you made both - wrote about the bug 1086999 in Release Notes after description of this bug 647010 and made security advisory. The same you need to do for my bug 647010. If you made relnote for it, then OK, but also make MFSA (as you did with MFSA 2015-91).

This vulnerability is more important then some MFSA from the last release (such as 2015-86 and 2015-91) and many others from previous releases. And if you made security advisory for them, then you need to do it for this vulnerability (it deserves for SA).
Comment 69 Al Billings [:abillings] 2015-08-12 14:47:23 PDT
We're not doing a security advisory for this, as previously stated.
Comment 70 Simon Peeters 2015-08-13 01:04:03 PDT
This 'feature' broke a lot of usefull sites, please revert or change the default back to something usefull.

example: 'static' frontends for api's behind basic auth.
These are static javascript powered frontends which query remote api's trough xhr or jsonp.
practical example: we have a kanban style viewer for our redmine tickes, this is just a static page, and relies on the basic auth on the redmine server api to get the data for the right user. (and to get any access at all)

I could live with an added yellow, or even red warning, saying that this isn't for the main site. but disabling this is just wrong.
Comment 71 Mike 2015-08-13 11:41:49 PDT
This update just broke my javascript looping app that worked for like 15 years. Nice....
Comment 72 Kohei Yoshino [:kohei] 2015-08-16 11:59:04 PDT
(In reply to zorgos from comment #65)
> bug report: the HTTP auth dialog does not work since version 40 of Firefox.

Hi zorgos, is your auth dialog triggered by <iframe>, <img>, <script>, XMLHttpRequest or CSS background-image? If not, it's another issue currently tracked in Bug 1195091.
Comment 73 zorgos 2015-08-16 23:40:03 PDT
(In reply to Kohei Yoshino [:kohei] from comment #72)
> (In reply to zorgos from comment #65)
> > bug report: the HTTP auth dialog does not work since version 40 of Firefox.
> 
> Hi zorgos, is your auth dialog triggered by <iframe>, <img>, <script>,
> XMLHttpRequest or CSS background-image? If not, it's another issue currently
> tracked in Bug 1195091.

Hi Kohei,
By <script src="https://another-domain.com/"></script>
The top domain is cached by cloudflare so for security reason, authentication is done through another domain. For me, this patch reduces my security.
Comment 74 Daniel B. 2015-08-17 03:57:30 PDT
Was this perhaps implemented in such a way that embedded resources cannot even request saved credentials?

I have FoxyProxy set to redirect YouTube over a specific proxy that required credentials. These credentials are saved in FoxyProxy. That means I don't get a dialog asking for credentials.

After upgrading to Firefox 40, embedded YouTube videos (iframe) now show "The proxy server is refusing connections" (which is a completely wrong message btw) until I load YouTube.com in a top-level context. After that, the credentials are cached and everything works as expected. After closing Firefox, the cache is lost, of course.
Comment 75 Bob 2015-08-18 17:51:34 PDT
This is marked as parity-Chrome, but as far as I can tell it does not match the behaviour of the current version (44).

The end result is quite a few sites are effectively broken in Firefox, effectively forcing users not advanced enough to hunt for the config option to use a different browser instead. Perhaps this change would have been better as a warning *first* (perhaps with telemetry to count occurrences) and then blocked in a later version, when site authors have had more warning?

Also, the HTTPS version of the site is considered a different origin. This is probably intended, and not doing so would enable a similar attack by anyone in a position to MitM, but there's some sites in the wild that are affected (top-level is a frameset over plain HTTP, some content frames on the same domain over HTTPS).
Comment 76 Kevin 2015-08-24 19:41:36 PDT
Our entire office (200+ people) have now switched to chrome because of this... GG.
Comment 77 Kohei Yoshino [:kohei] 2015-08-24 21:23:06 PDT
Why don't you use Firefox 38 ESR as I wrote in my comment 66?
Comment 78 Dragana Damjanovic [:dragana] 2015-08-24 22:01:29 PDT
Changing pref will return to behavior before this bug has been introduces:
go to about:config 
search for network.auth.allow-subresource-auth and set it to 2

firefox will behave as before.
Comment 79 Kevin 2015-08-24 22:24:06 PDT
Kohei,

Do you think we should need to do that?

As a single user doing this or changing the config is an easy option yes. Being IT in an office with hundreds of people is a different story. My intent is not to be mean, but to express the problem. I understand the seriousness of security risks, but the other major browsers do not have this issue.
Comment 80 Kohei Yoshino [:kohei] 2015-08-24 22:49:17 PDT
Regardless of this specific problem, enterprise users may want to use Firefox ESR (and the centralized configuration control system) to avoid such compatibility issues. Many companies, governments, universities and other organizations are choosing ESR. It should make your life easier :)
Comment 81 Kevin 2015-08-24 22:59:53 PDT
Interesting. I will suggest it. Thank you.
Comment 82 Justin Dolske [:Dolske] 2015-08-25 12:40:20 PDT
So, while I support broader disabling of the HTTP Auth prompt, I'm not really thrilled with the course this bug took...

Note comment 17-21 where the parity-chrome claim was clarified, as the fix they landed actually bounced due to site compatibility around XHR/iframes. So it's no surprise that we're seeing reports like bug 1195091 and bug 1189268.

Nor was the telemetry patch in bug 979359 landed, so it's not clear if what landed here was supported by any actual data on usage. If it's still in relatively widespread use, we'll probably need to figure out a different or more gradual way to deprecate/discourage its use.
Comment 83 schwarz 2015-08-26 04:29:56 PDT
This does not simply use existing homograph mitigation in the auth dialog because … ?

This protection does not need to be requested by the server by setting, e.g., Content Security Policy headers because … ?

In fact, from a cursory look at the patch, this does not even seem to allow sources that are explicitly whitelisted by server Content Security Policy?!
Comment 84 Jesse Ruderman 2015-08-31 15:14:56 PDT
In bug 1197944, the pref was flipped to revert the behavior change.

Is there a plan or metabug for fixing it again, after we address some of the compatibility issues?
Comment 85 Daniel Veditz [:dveditz] 2015-10-29 22:16:47 PDT
As Jesse said, this was reverted so this security risk still exists.
Comment 86 Florian Bender 2016-06-11 14:37:03 PDT
So Bug 1230462 is now fixed, however I'd say there's more that can be done. 

As a first measure, I propose to disable the "OK" button for a second or two for cross-origin requests (similar to the behaviour of the download window). A more drastic measure could be to add a checkbox à la "Are you sure you want to send this data?" but this could annoy some (esp. corp) users – an advanced variant of this could track if a user activated the checkbox for this origin-cross-origin-combination more than 3 times and remember this preference.

Any other ideas?

Note You need to log in before you can comment on or make changes to this bug.