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

RESOLVED FIXED in mozilla17

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Gregory Fleischer, Assigned: Alexandre D'Eschambeault)

Tracking

Trunk
mozilla17
x86
Mac OS X
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

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

Comment 3

5 years ago
(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

5 years ago
Anne van Kesteren posted on the W3C webapps list : "Done: http://dvcs.w3.org/hg/xhr/rev/a4a35861a49d"

Comment 5

5 years ago
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++]

Comment 6

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

Comment 7

5 years ago
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.
Attachment #651158 - Flags: review+
(Assignee)

Updated

5 years ago
Attachment #651158 - Flags: review+ → review?(imelven)

Updated

5 years ago
Assignee: yboily → xel1045

Comment 8

5 years ago
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+
(Assignee)

Comment 9

5 years ago
Created attachment 651618 [details] [diff] [review]
Patch 2 - Prevent DNT modification in unprivileged mode (patch formatted)
Attachment #651158 - Attachment is obsolete: true
Attachment #651618 - Flags: feedback?(imelven)
(Assignee)

Comment 10

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

5 years ago
(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.

Updated

5 years ago
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+

Comment 13

5 years ago
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++]
(Assignee)

Updated

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.