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

REOPENED
Assigned to

Status

()

Core
Document Navigation
REOPENED
6 years ago
21 days ago

People

(Reporter: bsterne, Assigned: dragana)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, 4 keywords)

Trunk
mozilla40
csectype-spoof, dev-doc-needed, sec-moderate, site-compat
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox40 fixed, relnote-firefox 40+)

Details

(Whiteboard: [parity-Chrome][adv-main40-])

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

6 years ago
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
A major problem here is site compat...
Blocks: 567804
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.
(Reporter)

Comment 3

6 years ago
(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.
echos of bug 580168 ? That bug had been leaning towards WONTFIX, so maybe CSP is a way forward?
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

6 years ago
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.
(Reporter)

Comment 7

6 years ago
(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)
Whiteboard: [sg:low spoof] → [sg:low spoof][parity-Chrome]
(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.
(Reporter)

Updated

6 years ago
Duplicate of this bug: 705501
Duplicate of this bug: 559556
Duplicate of this bug: 714543
See also bug 724182.
See Also: → bug 724182
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.
Component: Networking: HTTP → Document Navigation
QA Contact: networking.http → docshell
Keywords: sec-low
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.
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

4 years ago
(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?
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?)
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.
Keywords: sec-low → sec-high
Whiteboard: [sg:low spoof][parity-Chrome] → [parity-Chrome]
HTTP auth is rarely used on the public web in practice, this is not anything like a general SSL spoof.
Keywords: sec-high → csec-spoof, sec-moderate
Chrome did not stick; see http://crbug.com/174179. They only blocked cross-origin loads AFAICT.
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).
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).
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.
(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.
So we are going to check only hosts? What happens another realm requests a different username/password ?
(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

3 years ago
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.

Updated

3 years ago
See Also: → bug 377496

Updated

3 years ago
Blocks: 411085
Depends on: 979359
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.
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.

Updated

3 years ago
Depends on: 1038756
Duplicate of this bug: 1105944
Tanvi, do you think you could look at this?
Flags: needinfo?(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?
Flags: needinfo?(tanvi) → needinfo?(jduell.mcbugs)
Honza, can you either fix this or teach someone else in Necko (Valentin/Daniel/Dragana) how to?  Not urgent but worth fixing.
Flags: needinfo?(jduell.mcbugs) → needinfo?(honzab.moz)
OK, assigning to me to find an assignee..
Assignee: nobody → honzab.moz
Flags: needinfo?(honzab.moz)
(Assignee)

Comment 35

2 years ago
I will take a look at this.
Please next time update the assignee field.  Thanks.
Assignee: honzab.moz → dd.mozilla
(Assignee)

Updated

2 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 37

2 years ago
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 :)
Attachment #8569912 - Flags: feedback?(honzab.moz)
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
(Assignee)

Updated

2 years ago
Attachment #8569912 - Flags: feedback?(honzab.moz)
(Assignee)

Comment 39

2 years ago
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 :)
(Assignee)

Comment 40

2 years ago
Created attachment 8570139 [details] [diff] [review]
bug_647010_v2.patch
Attachment #8569912 - Attachment is obsolete: true
Attachment #8570139 - Flags: feedback?(tanvi)
Attachment #8570139 - Flags: feedback?(honzab.moz)
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!
Attachment #8570139 - Flags: feedback?(tanvi) → feedback+
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
Attachment #8570139 - Flags: feedback?(honzab.moz) → feedback-
(Assignee)

Comment 43

2 years ago
Created attachment 8574587 [details] [diff] [review]
bug_647010_v3.patch
Attachment #8570139 - Attachment is obsolete: true
Attachment #8574587 - Flags: review?(tanvi)
Attachment #8574587 - Flags: review?(honzab.moz)
(Assignee)

Updated

2 years ago
Attachment #8574587 - Flags: review?(tanvi)
Attachment #8574587 - Flags: review?(honzab.moz)
(Assignee)

Comment 44

2 years ago
Created attachment 8574880 [details] [diff] [review]
bug_647010_v3.patch
Attachment #8574587 - Attachment is obsolete: true
Attachment #8574880 - Flags: review?(tanvi)
Attachment #8574880 - Flags: review?(honzab.moz)
(Assignee)

Comment 45

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5167934020d5
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.
Attachment #8574880 - Flags: review?(tanvi) → review+
(Assignee)

Comment 47

2 years ago
Created attachment 8576046 [details] [diff] [review]
bug_647010_v4.patch

make changes suggested by Tanvi and fixed failing test on Android.
Attachment #8574880 - Attachment is obsolete: true
Attachment #8574880 - Flags: review?(honzab.moz)
Attachment #8576046 - Flags: review?(honzab.moz)
Blocks: 978471
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?
Attachment #8576046 - Flags: review?(honzab.moz) → feedback+
(Assignee)

Comment 49

2 years ago
Created attachment 8586139 [details] [diff] [review]
bug_647010_v5.patch
Attachment #8576046 - Attachment is obsolete: true
Attachment #8586139 - Flags: review?(honzab.moz)
(Assignee)

Comment 50

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3bcb3763af28
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
Attachment #8586139 - Flags: review?(honzab.moz) → review+
(Assignee)

Comment 52

2 years ago
Created attachment 8586752 [details] [diff] [review]
bug_647010_v6.patch
Attachment #8586139 - Attachment is obsolete: true
Attachment #8586752 - Flags: review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e642b4f35c4
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2e642b4f35c4
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Duplicate of this bug: 978471

Updated

2 years ago
Keywords: dev-doc-needed, site-compat

Updated

2 years ago
Blocks: 1159584

Updated

2 years ago
No longer blocks: 1159584
Depends on: 1159584

Comment 56

2 years ago
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)]: ?
relnote-firefox: --- → ?
This is on the site compat doc:
https://developer.mozilla.org/en-US/Firefox/Releases/40/Site_Compatibility#Security
It's more appropriate to relnote this behavior change (point to the above) rather than write a security advisory for it.
Keywords: relnote
Whiteboard: [parity-Chrome] → [parity-Chrome][adv-main40-]
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.
relnote-firefox: ? → 40+
Keywords: relnote

Comment 60

2 years ago
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

2 years ago
This "fix" just broke thousands of sites. It doesn't even appear to allow a user to optionally click the secondary iframe authentication page.
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.
Flags: needinfo?(janderson2000)

Comment 63

2 years ago
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.
Flags: needinfo?(janderson2000)

Comment 64

2 years ago
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

2 years ago
bug report: the HTTP auth dialog does not work since version 40 of Firefox.
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

2 years ago
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

2 years ago
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).
We're not doing a security advisory for this, as previously stated.

Comment 70

2 years ago
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.

Updated

2 years ago
Depends on: 1189268

Comment 71

2 years ago
This update just broke my javascript looping app that worked for like 15 years. Nice....
(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.
Flags: needinfo?(sta)

Comment 73

2 years ago
(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.
Flags: needinfo?(sta)

Comment 74

2 years ago
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

2 years ago
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).
(Assignee)

Updated

2 years ago
Blocks: 1197944

Comment 76

2 years ago
Our entire office (200+ people) have now switched to chrome because of this... GG.
Why don't you use Firefox 38 ESR as I wrote in my comment 66?
(Assignee)

Comment 78

2 years ago
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

2 years ago
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.
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

2 years ago
Interesting. I will suggest it. Thank you.
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

2 years ago
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

2 years ago
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?

Updated

2 years ago
Depends on: 1200247

Updated

2 years ago
Depends on: 1200590

Updated

2 years ago
Depends on: 1201516
As Jesse said, this was reverted so this security risk still exists.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Updated

2 years ago
Depends on: 1230462

Comment 86

a year ago
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?

Updated

11 months ago
Depends on: 1281434

Updated

7 months ago
See Also: → bug 1312243
Depends on: 1357835
You need to log in before you can comment on or make changes to this bug.