696662 - HTTP Auth headers cannot be modified after http-on-modify-request got fired

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Georg Koppen]

Attachment: Add-HTTP-auth-headers-before-modify-request-notification

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:6.0) Gecko/20100101 Firefox/6.0

Steps to reproduce:

While trying to defend against user tracking via HTTP Auth (see: http://jeremiahgrossman.blogspot.com/2007/04/tracking-users-without-cookies.html) I investigated the approach to delete the relevant Authorization header via an extension after the http-on-modify-request notification got fired assuming that I can override this header just as the other HTTP headers.


Actual results:

But that did not work and the Authorization header got included though. The problem is that the header gets added after the notification fires.


Expected results:

The header should get added before the http-on-modify-request notification gets fired in order to allow extensions to manipulate HTTP Auth headers as well. 

The Tor Project has already developed a simple patch that accomplishes this. It is deployed in their custom Firefox build successfully and attached to this bug report.

Comment 1

[Jason Duell]

Comment on attachment 568964 [details] [diff] [review]
Add-HTTP-auth-headers-before-modify-request-notification

Honza,

I looked over this patch, and it seems fine to me.  But I think you know auth stuff better than I do.  Can you review?  If not, I'll probably +r it, and hope it's ok :)

Comment 2

[Honza Bambas (:mayhemer)]

(In reply to Jason Duell (:jduell) from comment #1)
> Comment on attachment 568964 [details] [diff] [review]
> Add-HTTP-auth-headers-before-modify-request-notification
> 
> Honza,
> 
> I looked over this patch, and it seems fine to me.  But I think you know
> auth stuff better than I do.  Can you review?  If not, I'll probably +r it,
> and hope it's ok :)

Do you necessarily need it for the next Aurora merge that happens in one week?

Comment 3

[Jason Duell]

No, don't necessarily need it by then,

Comment 4

[Honza Bambas (:mayhemer)]

This also exposes the auth headers to extensions, they can not just delete, but also read, modify, add.  Not sure this is OK at the moment, I have to think a bit about this first, also adding some other folks that might know more.

If the purpose is to drop the auth headers, then we could use LOAD_ANONYMOUS flag for it, somehow.  However, on-modify-request is called after cookies had been added and also the flag cannot be set when on-modify-request is called from DoAuthRetry since we may stick with a connection that is not anonymous (both can be fixed: delete cookies on LOAD_ANONYMOUS and don't allow flag set when already having non-anon connection).

Other way could be to delete/rewrite the cached credentials from the http auth cache, see nsIHttpAuthManager.idl.

Comment 5

[Bjarne (:bjarne)]

I share Honzas concern about exposing all headers to extensions: It has the potential of opening up a Pandoras box of issues we don't see or understand at the moment. A sec-review is at least necessary before doing this, IMO. (Were there no discussion on this topic in the Tor-project? Or maybe their version of Fx doesn't allow 3rd party extensions?)

Now, I don't see why using LOAD_ANONYMOUS wouldn't work. The planned extension must have some rules to decide *when* to clear auth-headers (I assume it doesn't need to *read* auth-headers to figure this out?). If the extension simply apply its rules and set LOAD_ANONYMOUS (nsIRequest.loadFlags) on the channel when necessary instead of clearing the auth-header, this should work nicely with no code-changes.

Preventing cookies to be applied to a request is not part of the problem he wants to solve, afaics? (And the patch doesn't help that situation either, afaics.)

Comment 6

[Georg Koppen]

> I share Honzas concern about exposing all headers to extensions: It has the
> potential of opening up a Pandoras box of issues we don't see or understand
> at the moment. A sec-review is at least necessary before doing this, IMO.

Fair enough. But the user is already lost if she installed a malicious extension, right? Otherwise there would not be the need for having strict AMO editors in the first place.
 
> Now, I don't see why using LOAD_ANONYMOUS wouldn't work. The planned
> extension must have some rules to decide *when* to clear auth-headers (I
> assume it doesn't need to *read* auth-headers to figure this out?). If the
> extension simply apply its rules and set LOAD_ANONYMOUS
> (nsIRequest.loadFlags) on the channel when necessary instead of clearing the
> auth-header, this should work nicely with no code-changes.

Yes, I guess as a first step that would be enough, indeed. However, the purpose of this patch goes further. It is part of a general framework for enhancing the privacy on the web with the main idea to bind identifiers to the URL bar domain (See: e.g. the same idea regarding cookies on https://wiki.mozilla.org/Thirdparty). Now, "just" deleting the Auth headers would not help here. Rather, the extension needs to be able to modify them (i.e. add the proper headers (if there are any) depending on the domain shown in the users' URL bar).
 
> Preventing cookies to be applied to a request is not part of the problem he
> wants to solve, afaics? (And the patch doesn't help that situation either,
> afaics.)

No. Just a "normal" request without (with modified) Auth headers.

Comment 7

[Honza Bambas (:mayhemer)]

What you want is:
- see the auth headers to analyze them
- based on analyzes, modify them
- nothing else should change

Right?

True is that if an extension wants to see the auth headers it can always find some way.  You can always hook e.g. on-examine-response where the headers are already present.  So my concern is probably invalid.

We are already exposing cookie headers in on-modify-request, what could also be considered a problem according my comment, if not even worse then exposing auth headers.

LOAD_ANONYMOUS flag purpose may change in the future and also does other things then just preventing authorization headers get out.


If there are no objections from others, I think we could consider taking the patch in the tree.

Comment 8

[Georg Koppen]

(In reply to Honza Bambas (:mayhemer) from comment #7)
> What you want is:
> - see the auth headers to analyze them
> - based on analyzes, modify them
> - nothing else should change
> 
> Right?

Yes, exactly.

Comment 9

[Bjarne (:bjarne)]

(In reply to Honza Bambas (:mayhemer) from comment #7)
>  You can always hook e.g. on-examine-response where the
> headers are already present.

Good point...  Though at this point the request is made and extensions cannot influence it. (Maybe not a big deal, but it's a difference.)

> We are already exposing cookie headers in on-modify-request, what could also
> be considered a problem according my comment, if not even worse then
> exposing auth headers.

Again: Good point.

> LOAD_ANONYMOUS flag purpose may change in the future and also does other
> things then just preventing authorization headers get out.

LOAD_ANONYMOUS is part of the public API and is not likely to change more frequently than other parts of the API, fwiw. :)

I still think we should ask for a security-review. They might be interested in the fact that extensions already has some access to cookies and headers. This patch is just about increasing that access.

Comment 10

[Jason Duell]

Comment on attachment 568964 [details] [diff] [review]
Add-HTTP-auth-headers-before-modify-request-notification

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

Extensions are not really part of normal security review, IIUC--an extension can already rm -rf / your hard drive, so giving it permission to tweak HTTP auth headers seems like an afterthought after that :)   But let's run it by the sec team just in case they disagree.

In either case the actual code here is fine, so +r.`

Comment 11

[Honza Bambas (:mayhemer)]

Comment on attachment 568964 [details] [diff] [review]
Add-HTTP-auth-headers-before-modify-request-notification

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

r- !

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +4785,5 @@
>      // TODO: save cookies from auth response and send them here (bug 572151).
>      AddCookiesToRequest();
> +
> +    // check to see if authorization headers should be included
> +    mAuthProvider->AddAuthorizationHeaders();

This is redundant.  Here the auth provider already filled header for next retry.  This may even override the headers to previous values or something unexpected.

And to prove it, this breaks test_authentication.js.

Comment 12

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

removed sec-review-needed, secteam is comfortable with this change

Comment 13

[Jason Duell]

Attachment: v2: eliminate call to AddAuthorizationHeaders in DoAuthRetry

Removed the extra call.  Now test_authorization.js passes.

Comment 14

[Honza Bambas (:mayhemer)]

Comment on attachment 587897 [details] [diff] [review]
v2: eliminate call to AddAuthorizationHeaders in DoAuthRetry

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

r=honzab

Comment 15

[Jason Duell]

Georg,

I've landed this on inbound, so it should be in nightly FF releases within a day or two, and will show up in Firefox 12.

Note that the Tor patch had an extra call that was actually a bug (comment 11), in case you want to fix that in the Tor version.

Thanks for the bug report and patch!

Jason

  https://hg.mozilla.org/integration/mozilla-inbound/rev/9c71a5a56681

Comment 16

[Georg Koppen]

Wow, Jason, thanks for the modified patch and your kind advice. I'll see that the Tor version gets fixed accordingly. Keep up the good work!

Comment 17

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/mozilla-central/rev/9c71a5a56681