Closed Bug 856978 Opened 8 years ago Closed 8 years ago

Removing authentication headers via extension is broken

Categories

(Core :: Networking: HTTP, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: gk, Unassigned)

References

Details

(Keywords: regression)

Attachments

(2 files, 2 obsolete files)

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

Steps to reproduce:

We have an extensions that defends against tracking with (third party) authentication requests. See: http://jeremiahgrossman.blogspot.fr/2007/04/tracking-users-without-cookies.html for the details. It basically removes the Authorization header on http-on-modify-request.


Actual results:

This worked until bug 761479 landed. After that the defense is broken if the user has JavaScript allowed.


Expected results:

The defense should still work regardless JavaScript on/off.
Blocks: 761479
OS: Windows 7 → All
I just realized that there is more to it as the fix in bug 696662 is broken as well (i have not bisected that yet but the change occurs somewhere after Firefox 17 got released), ugh. Maybe that is the root cause here although I still don't understand why only users with JS seem to be affected.
Blocks: 696662
Keywords: regression
1. I'm not sure what you exactly mean by "users with JS" ; today every browser is by default JS-enabled

2. Do I understand well that your bisection might be wrong?  If so, I'll wait for a better regression window.  Otherwise I may take a look if you arm me with ref to the extension.
I also suspect the culprit here could be a change in how the on-modify-request event is fired.  ni? on jason who remembers this better then me.
Flags: needinfo?(jduell.mcbugs)
(In reply to Honza Bambas (:mayhemer) from comment #2)
> 1. I'm not sure what you exactly mean by "users with JS" ; today every
> browser is by default JS-enabled

Sure. But as our users have often/by default JsvaScript disabled due to tracking risks we have developed a test that tries to take that into account. You find it on http://ip-check.info.
 
> 2. Do I understand well that your bisection might be wrong?

Well, there are two issues here which maybe deserve two different bug entries (I am not sure about that yet) but both show that removing the authentication headers via extensions is "broken".
One is that, given JS is enabled, the defense developed in the extension is not working anymore since bug 761479 landed. You might test that with the minimal test extension I attached to this bug. Just go to http://ip-check.org and watch the Authentication row in the test results. It is green without JS enabled and red with JS enabled in current Firefox versions but always green with the latest ESR.
The other thing is that it seems that the authentication headers are again added _after_ http-on-modify-request is fired a thing bug 696662 "fixed" (I just looked at the logs and have not bisected that yet). Thus, we can't reliably stop the tracking anymore I mentioned in my initial comment. I have added that to this bug as IMO there is some hope that issue one might not be an issue any longer if the second one get fixed (which we need anyway).
Attached file Test extension
Attachment #732773 - Attachment mime type: application/octet-stream → application/x-xpinstall
This patch fixes the regression. I ran the xpcshell tests locally and this patch did not cause a test failure. Given that the auth headers are currently not removable at all and this patch fixes this even with bug 761479, the latter should not be blocked anymore by this bug.
Attachment #748763 - Flags: review?(jduell.mcbugs)
No longer blocks: 761479
Comment on attachment 748763 [details] [diff] [review]
This patch makes the auth headers removable again

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

I'd like to take a look at this patch as well.
Attachment #748763 - Flags: review?(honzab.moz)
Comment on attachment 748763 [details] [diff] [review]
This patch makes the auth headers removable again

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

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +4419,4 @@
>      // If mTimingEnabled flag is not set after OnModifyRequest() then
>      // clear the already recorded AsyncOpen value for consistency.
>      if (!mTimingEnabled)
>          mAsyncOpenTime = TimeStamp();

Also move this bit after CallOnModifyRequestObservers();

Otherwise looks good to me, but needs a good testing...
Attachment #748763 - Flags: review?(honzab.moz) → review+
Comment on attachment 748763 [details] [diff] [review]
This patch makes the auth headers removable again

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

honza already reviewed this and knows auth code better than me.  Sounds like we just need a test here.
Attachment #748763 - Flags: review?(jduell.mcbugs)
Flags: needinfo?(jduell.mcbugs)
This improved patch takes the review comment into account add contains the needed unit test.
Attachment #748763 - Attachment is obsolete: true
Attachment #758698 - Flags: review?(honzab.moz)
Comment on attachment 758698 [details] [diff] [review]
This patch makes the auth headers removable again -- second shot

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

r=honzab

please check how the test behaves (if actually fails) when the change in http channel is not in the tree.  As well please run the whole xpcshell test set in /netwerk/test (mach xpcshell-test netwerk/test).

::: netwerk/test/unit/test_bug856978.js
@@ +94,5 @@
> +    } else {
> +      do_test_pending();
> +      httpServer.stop(do_test_finished);
> +    }
> +    do_test_finished();

The second do_test_finished(); call is redundant?
Attachment #758698 - Flags: review?(honzab.moz) → review+
> please check how the test behaves (if actually fails) when the change in
> http channel is not in the tree.  As well please run the whole xpcshell test
> set in /netwerk/test (mach xpcshell-test netwerk/test).

I had done the first request already and the test is failing as there are is no auth header available on http-on-modify-request without the patch. The whole test set is passing with one exception but that is due to a test failing locally and tracked in bug 833723.
 
> ::: netwerk/test/unit/test_bug856978.js
> @@ +94,5 @@
> > +    } else {
> > +      do_test_pending();
> > +      httpServer.stop(do_test_finished);
> > +    }
> > +    do_test_finished();
> 
> The second do_test_finished(); call is redundant?

Actually, no. The test is not passing without it. See: https://mxr.mozilla.org/mozilla-central/source/netwerk/test/unit/test_authentication.js#248ff. for a similar code path.
I made the patch ready for check-in. Could somebody with the necessary privs do that for me? Thanks!
Attachment #758698 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/1e9c7600208a
Status: UNCONFIRMED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.