647010 - Only present HTTP authentication dialogs if it is the top-level document initiating the auth

Status: NEW

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

Description

[Brandon Sterne (:bsterne)]

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 [:bzbarsky]]

A major problem here is site compat...

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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

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

echos of bug 580168 ? That bug had been leaning towards WONTFIX, so maybe CSP is a way forward?

Comment 5

[YGN Ethical Hacker Group]

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]

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

(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: [e-mail]]]

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

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

See also bug 724182.

Comment 13

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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

[Tom S [:evilpie]]

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

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]

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

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

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

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

Chrome did not stick; see http://crbug.com/174179. They only blocked cross-origin loads AFAICT.

Comment 21

[Justin Dolske [:Dolske]]

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

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

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

(In reply to Johnny Stenback (:jst, [e-mail]) 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)]

So we are going to check only hosts? What happens another realm requests a different username/password ?

Comment 26

[Honza Bambas (:mayhemer)]

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

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

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

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 31

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

Tanvi, do you think you could look at this?

Comment 32

[Tanvi Vyas[:tanvi]]

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

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

OK, assigning to me to find an assignee..

Comment 35

[Dragana Damjanovic [:dragana]]

I will take a look at this.

Comment 36

[Honza Bambas (:mayhemer)]

Please next time update the assignee field.  Thanks.

Comment 37

[Dragana Damjanovic [:dragana]]

Attachment: 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]]

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

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

Attachment: bug_647010_v2.patch

Comment 41

[Tanvi Vyas[:tanvi]]

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

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

Attachment: bug_647010_v3.patch

Comment 44

[Dragana Damjanovic [:dragana]]

Attachment: bug_647010_v3.patch

Comment 45

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5167934020d5

Comment 46

[Tanvi Vyas[:tanvi]]

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

Attachment: bug_647010_v4.patch

make changes suggested by Tanvi and fixed failing test on Android.

Comment 48

[Honza Bambas (:mayhemer)]

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

Attachment: bug_647010_v5.patch

Comment 50

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3bcb3763af28

Comment 51

[Honza Bambas (:mayhemer)]

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

Attachment: bug_647010_v6.patch

Comment 53

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2e642b4f35c4

Comment 54

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

https://hg.mozilla.org/mozilla-central/rev/2e642b4f35c4

Comment 56

[Florian Bender]

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

This is on the site compat doc:
https://developer.mozilla.org/en-US/Firefox/Releases/40/Site_Compatibility#Security

Comment 58

[Daniel Veditz [:dveditz]]

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

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]

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]

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

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]

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]

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]

bug report: the HTTP auth dialog does not work since version 40 of Firefox.

Comment 66

[Kohei Yoshino [:kohei]]

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]

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]

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 - ex-MoCo]]

We're not doing a security advisory for this, as previously stated.

Comment 70

[Simon Peeters]

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]

This update just broke my javascript looping app that worked for like 15 years. Nice....

Comment 72

[Kohei Yoshino [:kohei]]

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

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

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]

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]

Our entire office (200+ people) have now switched to chrome because of this... GG.

Comment 77

[Kohei Yoshino [:kohei]]

Why don't you use Firefox 38 ESR as I wrote in my comment 66?

Comment 78

[Dragana Damjanovic [:dragana]]

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]

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

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]

Interesting. I will suggest it. Thank you.

Comment 82

[Justin Dolske [:Dolske]]

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]

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]

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

As Jesse said, this was reverted so this security risk still exists.

Comment 86

[Florian Bender]

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?

Comment 87

[angeloliao16]

The current Chromium behavior is that it suppresses auth prompt requested by an image resource loaded from cross-origin : https://bugs.chromium.org/p/chromium/issues/detail?id=400380&q=image%20xhr%20prompt&colspec=ID%20Pri%20M%20Stars%20ReleaseBlock%20Component%20Status%20Owner%20Summary%20OS%20Modified. This behavior has been around for three years. So it may best to change to that to better protect the user. We are thinking about similar behavior on the Edge side as well.

Comment 88

[Andrew Overholt [:overholt]]

(In reply to angeloliao16 from comment #87)
> The current Chromium behavior is that it suppresses auth prompt requested by
> an image resource loaded from cross-origin :
> https://bugs.chromium.org/p/chromium/issues/
> detail?id=400380&q=image%20xhr%20prompt&colspec=ID%20Pri%20M%20Stars%20Releas
> eBlock%20Component%20Status%20Owner%20Summary%20OS%20Modified. This behavior
> has been around for three years. So it may best to change to that to better
> protect the user. We are thinking about similar behavior on the Edge side as
> well.

Thanks for the info! It looks like we ran into a bunch of web breakage when we tried to ship this (see https://bugzilla.mozilla.org/show_bug.cgi?id=1189268#c15). Dragana, do you know if we have plans to revisit things here? If not, maybe Wennie is aware of plans?

Anne, is there a spec change required here?

Comment 89

[Anne (:annevk)]

Yeah, it would require changes to https://fetch.spec.whatwg.org/. (Search for "prompt". I don't think Chrome has documented that; they've proposed related changes though: https://github.com/whatwg/fetch/pull/465.)

Comment 90

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Andrew Overholt [:overholt] from comment #88)
> Dragana, do you
> know if we have plans to revisit things here? If not, maybe Wennie is aware
> of plans?

Yes, we definitely should revisit that. I'll add to the Dom:Security roadmap.

Comment 91

[Dragana Damjanovic [:dragana]]

(In reply to Andrew Overholt [:overholt] from comment #88)
> (In reply to angeloliao16 from comment #87)
> > The current Chromium behavior is that it suppresses auth prompt requested by
> > an image resource loaded from cross-origin :
> > https://bugs.chromium.org/p/chromium/issues/
> > detail?id=400380&q=image%20xhr%20prompt&colspec=ID%20Pri%20M%20Stars%20Releas
> > eBlock%20Component%20Status%20Owner%20Summary%20OS%20Modified. This behavior
> > has been around for three years. So it may best to change to that to better
> > protect the user. We are thinking about similar behavior on the Edge side as
> > well.
> 
> Thanks for the info! It looks like we ran into a bunch of web breakage when
> we tried to ship this (see
> https://bugzilla.mozilla.org/show_bug.cgi?id=1189268#c15). Dragana, do you
> know if we have plans to revisit things here? If not, maybe Wennie is aware
> of plans?
> 
> Anne, is there a spec change required here?

We do, there are already some bugs that are touching this. We need to be careful and turn off http-auth piece by piece.

Comment 92

[Dragana Damjanovic [:dragana]]

(In reply to angeloliao16 from comment #87)
> The current Chromium behavior is that it suppresses auth prompt requested by
> an image resource loaded from cross-origin :
> https://bugs.chromium.org/p/chromium/issues/
> detail?id=400380&q=image%20xhr%20prompt&colspec=ID%20Pri%20M%20Stars%20Releas
> eBlock%20Component%20Status%20Owner%20Summary%20OS%20Modified. This behavior
> has been around for three years. So it may best to change to that to better
> protect the user. We are thinking about similar behavior on the Edge side as
> well.

Bug 1423146 is open to flip  the pref.

Comment 93

[Jonathan Watt [:jwatt]]

Mass bug change to replace various 'parity' whiteboard flags with the new canonical keywords. (See bug 1443764 comment 13.)

Comment 94

[MustLive]

Hello Mozilla! I sent you a letter 21.05.2018, but without an answer, so write here.

In March 2011 I informed you about URL Spoofing vulnerability in Mozilla Firefox and in other browsers (for attacks via auth dialogs from external sites). 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.

My advisory: Bug 647010. You fixed it in 2015, but quickly reversed the fix. And I notices, that you fixed it in Firefox 59 in January. But now you fixed only partly - just protection from images vector and not from other vectors of attack.

You should add my Bug 647010 to References for 2017, e.g. at fxsitecompat.com, since these later bugzilla entries (after March 2011) just repeat my entry. And by myself I wrote about more vectors of attack (all of them), not only images, because it's primitive to talk only about attack by injecting images, while there are many other vectors.

This is not good that you limited the fix, but it's good, that after 2,5 years you tried again to fix it. Tell me, why do you limited yourself only to images and do you plan to fix other vectors? Do you consider them as a vulnerability at all or images vector of attack was the only vulnerability on your opinion?

Comment 95

[BugBot [:suhaib / :marco/ :calixte]]

The severity field for this bug is relatively low, S3. However, the bug has 5 duplicates, 12 votes, 72 CCs and 9 See Also bugs.
:dragana, could you consider increasing the bug severity?

For more information, please visit auto_nag documentation.

Comment 96

[Dragana Damjanovic [:dragana]]

The severity field is appropriate.