Last Comment Bug 751452 - DNT (DoNotTrack) header can be modified in XMLHttpRequest using setRequestHeader
: DNT (DoNotTrack) header can be modified in XMLHttpRequest using setRequestHeader
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: mozilla17
Assigned To: Alexandre D'Eschambeault
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-02 19:53 PDT by Gregory Fleischer
Modified: 2012-08-15 09:49 PDT (History)
8 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch 1 - Prevent DNT modification in unprivileged mode + test case (1.69 KB, patch)
2012-08-11 19:13 PDT, Alexandre D'Eschambeault
ian.melven: feedback+
Details | Diff | Review
Patch 2 - Prevent DNT modification in unprivileged mode (patch formatted) (1.98 KB, patch)
2012-08-13 19:58 PDT, Alexandre D'Eschambeault
khuey: review+
ian.melven: feedback+
Details | Diff | Review

Description Gregory Fleischer 2012-05-02 19:53:49 PDT
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:12.0) Gecko/20100101 Firefox/12.0
Build ID: 20120420145725

Steps to reproduce:

The DNT header can be modified in XMLHttpRequest requests using setRequestHeader().

For example, to clear the header:

 var xhr = new XMLHttpRequest();
 xhr.open("GET", "/", false);
 if (navigator.doNotTrack) {
   xhr.setRequestHeader("DNT", null);
 }
 xhr.send()



Actual results:

The "DNT: 1" header value would not be sent even if "Tell websites I do not want to be tracked" Privacy preference option is enabled. 

Alternatively, the value could be set for users that want to be tracked.


Expected results:

The "dnt" value is not included in the list of dangerous/invalid headers that are rejected by setRequestHeader.

https://hg.mozilla.org/mozilla-central/file/d4e35005f5a9/content/base/src/nsXMLHttpRequest.cpp#l3229

This is not really a security bug, but affects what some might consider a "security" feature.
Comment 1 Boris Zbarsky [:bz] 2012-05-02 21:32:39 PDT
That list of headers is mandated by the XHR spec.  So adding DNT to it should go through the webapps working group, in general.
Comment 2 Benjamin Smedberg [:bsmedberg] 2012-05-03 05:26:11 PDT
Does not need to remain private, though.
Comment 3 Ian Melven :imelven 2012-05-08 13:09:49 PDT
(In reply to Boris Zbarsky (:bz) from comment #1)
> That list of headers is mandated by the XHR spec.  So adding DNT to it
> should go through the webapps working group, in general.

I proposed adding DNT to the list of restricted headers for XHR on the w3c webapps list today. Looking at the DNT spec it says "A user agent must send the DNT header field on all HTTP requests if (and only if) a tracking preference is enabled", which implies the user's setting should be propagated to their XHR requests, perhaps ? I'm not sure if this is what Gecko does currently.
Comment 4 Ian Melven :imelven 2012-05-14 08:24:12 PDT
Anne van Kesteren posted on the W3C webapps list : "Done: http://dvcs.w3.org/hg/xhr/rev/a4a35861a49d"
Comment 5 Ian Melven :imelven 2012-05-14 08:31:21 PDT
Fixing this bug requires adding the DNT header to the list at https://hg.mozilla.org/mozilla-central/file/d4e35005f5a9/content/base/src/nsXMLHttpRequest.cpp#l3229 

It also requires adding a test to the XHR tests that makes sure the DNT header cannot be set on an XHR request, see http://mxr.mozilla.org/mozilla-central/source/content/base/test/test_xhr_forbidden_headers.html?force=1 which requires a bit of JS work. 

Comment 0 has steps to reproduce - the expected case is that DNT will not be set on the request to the value specified by the JS - i would suggest trying to set it to something other than 0 or 1 (which are the current values set depending on what the user chose in the preference UI) so it's obvious whether the .setRequestHeader was blocked. 

As a bonus, it would be great to remove EnablePrivilege from this test, which already uses SpecialPowers.wrap in one place, but that is a bit orthogonal to the actual bug :)
Comment 6 Ian Melven :imelven 2012-05-14 08:35:09 PDT
(In reply to Ian Melven :imelven from comment #5)
> As a bonus, it would be great to remove EnablePrivilege from this test,
> which already uses SpecialPowers.wrap in one place, but that is a bit
> orthogonal to the actual bug :)

oops, i took a closer read through and this is not necessary - the test explictly tries to set headers in an unprivileged and privileged context - so nothing needs to be done with EnablePrivilege, except adding the DNT header to both sets of tests.
Comment 7 Alexandre D'Eschambeault 2012-08-11 19:13:18 PDT
Created attachment 651158 [details] [diff] [review]
Patch 1 - Prevent DNT modification in unprivileged mode + test case

It's my first patch, but the basics were already present. So, I added the DND in the kInvalidHeaders and add it in the test case (test_xhr_forbidden_headers).

The test still pass.
Comment 8 Ian Melven :imelven 2012-08-13 13:18:32 PDT
Comment on attachment 651158 [details] [diff] [review]
Patch 1 - Prevent DNT modification in unprivileged mode + test case

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

Thanks for working on this ! It looks very close to me.

Please see http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed which talks about some of the things you 
need to set in your patch to get it landed, mostly the author and git format/amount of diff context.

Once you get that and the line length nit cleaned up, you can feedback? to me again if you like or review?
to sicking if you think it's ready to be reviewed and landed. If you have level 1 commit access you might
want to consider a push to Try as well.

Thanks again ! :D

::: content/base/src/nsXMLHttpRequest.cpp
@@ +3224,5 @@
>      // Step 5: Check for dangerous headers.
>      const char *kInvalidHeaders[] = {
>        "accept-charset", "accept-encoding", "access-control-request-headers",
>        "access-control-request-method", "connection", "content-length",
> +      "cookie", "cookie2", "content-transfer-encoding", "date", "dnt", "expect",

Nit : can you keep lines at 80 characters or less please ?

ie. bump "expect" on to the next line and rearrange things to keep each line 
at 80 chars or less.
Comment 9 Alexandre D'Eschambeault 2012-08-13 19:58:45 PDT
Created attachment 651618 [details] [diff] [review]
Patch 2 - Prevent DNT modification in unprivileged mode (patch formatted)
Comment 10 Alexandre D'Eschambeault 2012-08-13 20:15:16 PDT
The line was already 80 characters, but I reformat it to be less than 80. Also, thanks for your blog post reference. I think that my patch is now correctly created.

Finally, just to be sure, is «sicking» the name of a user? :-)
Comment 11 Ian Melven :imelven 2012-08-14 11:06:26 PDT
(In reply to Alexandre D'Eschambeault from comment #10)
> The line was already 80 characters, but I reformat it to be less than 80.
> Also, thanks for your blog post reference. I think that my patch is now
> correctly created.
> 
> Finally, just to be sure, is «sicking» the name of a user? :-)

Oh, thanks for the line reformat, it looked longer for some reason but I was looking at the patch diff and not the actual applied patch.

Thanks also for applying the config in the blog post to the patch, it looks correct to me also.

Yes, Jonas Sicking is a Mozillian who works on content, including WebAPI's and XHR - i've flagged your patch to him for review.
Comment 12 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-08-14 11:10:28 PDT
Comment on attachment 651618 [details] [diff] [review]
Patch 2 - Prevent DNT modification in unprivileged mode (patch formatted)

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

Jonas is busy, so stealing.

r=me
Comment 13 Ian Melven :imelven 2012-08-14 11:21:35 PDT
Thanks for that, Kyle !

Alexandre, you can go ahead and set checkin-needed on this bug and it should get landed to mozilla-inbound the next time the inbound Sheriffs do a sweep. If you like, you can push it to Try (if you have level 1 commit access) if you want to be extra careful, but it seems like a not-so-risky change to me. 

Thank you very much for working on the patch and your contribution to the project :D
Comment 14 Ryan VanderMeulen [:RyanVM] 2012-08-14 17:52:50 PDT
Taking Ian's word for it that this is trivial and can be landed without Try results.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3b7250c5316a
Comment 15 Ed Morley [:emorley] 2012-08-15 09:49:15 PDT
https://hg.mozilla.org/mozilla-central/rev/3b7250c5316a

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