Closed Bug 751452 Opened 12 years ago Closed 12 years ago

DNT (DoNotTrack) header can be modified in XMLHttpRequest using setRequestHeader

Categories

(Core :: DOM: Core & HTML, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla17

People

(Reporter: gfleischer+bugzilla, Assigned: xel1045)

Details

Attachments

(1 file, 1 obsolete file)

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.
That list of headers is mandated by the XHR spec.  So adding DNT to it should go through the webapps working group, in general.
Does not need to remain private, though.
Group: core-security
Component: General → DOM
QA Contact: general → general
(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.
Anne van Kesteren posted on the W3C webapps list : "Done: http://dvcs.w3.org/hg/xhr/rev/a4a35861a49d"
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 :)
Whiteboard: [mentor=imelven][lang=c++]
(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.
Assignee: nobody → yboily
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.
Attachment #651158 - Flags: review+
Attachment #651158 - Flags: review+ → review?(imelven)
Assignee: yboily → xel1045
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.
Attachment #651158 - Flags: review?(imelven) → feedback+
Attachment #651158 - Attachment is obsolete: true
Attachment #651618 - Flags: feedback?(imelven)
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? :-)
(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.
Attachment #651618 - Flags: review?(jonas)
Attachment #651618 - Flags: feedback?(imelven)
Attachment #651618 - Flags: feedback+
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
Attachment #651618 - Flags: review?(jonas) → review+
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
Whiteboard: [mentor=imelven][lang=c++]
Keywords: checkin-needed
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
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3b7250c5316a
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.