XMLHttpRequest allows request headers to be set that could be used to subvert security checks. We need to prevent XMLHttpRequest clients from modifying the Host and Transfer-Encoding request headers. The WHATWG spec at http://whatwg.org/specs/web-apps/current-work/#setrequestheader places restrictions on other request headers, but Host and Transfer-Encoding are the only ones that seem problematic to me. I see no harm in permitting modification to Accept-Charset, Accept-Encoding, If-Modified-Since, If-None-Match, If-Range, Range, Connection, Keep-Alive, and User-Agent headers. At least, changes to those headers should not impact Mozilla in any negative way that I can foresee. This bug is a spin-off from bug 297078.

Bug 297078 comment 12 lists some more headers that Yutaka Oiwa believes should be blocked for security reasons.

I think Content-Length needs to be added too... couldn't the page otherwise smuggle a second request in, in what necko believes to be the POST data? (to another vhost on the same host)

Comment on attachment 190629 [details] [diff] [review] v1 patch also, what about content-transfer-encoding? is there a special reason to remove the IsASCII check?

Right, he blacklists the following headers: Content-Length Host Connection Proxy-connection (non-standard) Referer Date Upgrade Via Hmm, let's review each one of these headers: Content-Length This header is overridden by XMLHttpRequest.send when a body is specified, so it is not possible to change the value of this header when a non-empty body is specified. Host Agreed; this should be blocked. Connection / Proxy-Connection These headers pose no real harm. By modifying these headers, the consumer can simply control whether or not the connection is keep-alive or not. Our HTTP implementation will behave properly if these headers are overridden, and I actually think it is very important that we allow people the ability to set 'Connection: close' since that can be useful as a way to free up a persistent connection when it is anticipated that a XMLHttpRequest query will take a long time to complete. Referer Since XMLHttpRequest is restricted to same-origin, what is the harm of lying about the referrer? I think there is none. Date This header may optionally be used as a request header for a POST or PUT response. There is no harm in allowing the page author the ability to tweak this header. Again, they are communicating with their own server. Upgrade The response to an Upgrade command must be a valid HTTP response, so our HTTP implementation would do the right thing. It would return whatever message body and headers were returned with the Upgrade query. It would not have any side-effects. I suppose the only danger is that the upgraded connection may go into the HTTP connection pool and be reused as a HTTP connection when in fact it may have been "upgraded" to a different protocol. That could lead to problems I suppose, but I don't know of any specific examples. Via I'm not sure what the exploit is here, but I suppose it could confuse some server infrastructure into thinking that a request from the browser actually passed through some proxy server. My take on this is that it probably would be good to add Upgrade and Via to the set of blacklisted request headers. I might add Content-Length too just for kicks, but I don't think it's really necessary.

> also, what about content-transfer-encoding? Is that a HTTP header? (See section 19.4.5 of RFC 2616.) > is there a special reason to remove the IsASCII check? nsHttpChannel::SetRequestHeader now has a more restrictive check, so there is no reason to duplicate effort here.

Comment on attachment 190629 [details] [diff] [review] v1 patch sr=jst for the change in general. Feel free to add headers to the list if there's agreement that they're needed.

r=dveditz fwiw. I'll leave the request flag in case you specifically want biesi

Comment on attachment 190637 [details] [diff] [review] v1.1 patch: restrict other headers marking r=dveditz, sr=jst. biesi: please chim in if you think something should be revised. requesting check-in approval.

looks fine to me, though per http://people.redhat.com/drepper/dsohowto.pdf 2.4.1/2.4.3 that should maybe not be a pointer but something like const char kInvalidHeaders[][18] (although that'd waste some space, so, feel free not to do this; I'd just think I should mention that)

biesi: I think I prefer to optimize for space instead of speed since this is not performance critical code. Sound good?

Comment on attachment 190637 [details] [diff] [review] v1.1 patch: restrict other headers We probably want this on the branches, too.

I'll update the spec taking your conclusions into account, fwiw, so please be as detailed as possible regarding your conclusions in comments in this bug! :-) Thanks!

(In reply to comment #12) > biesi: I think I prefer to optimize for space instead of speed since this is not > performance critical code. Sound good? ok, fine with me.

Comment on attachment 190637 [details] [diff] [review] v1.1 patch: restrict other headers a=dveditz This is required to fully fix bug 297078

Mozilla overwrite Content-Length fields only when datum for request body is given. If there is not, the Content-Length header is sent as is. Content-Length: insertion can be used for request splitting, if web server/proxy sends positive response early, or client performs request pipelining. Early response seems not be prohibitted in HTTP/1.1 spec. (early negative response is explicitly allowed.) example: >> GET /a HTTP/1.0 >> Host: foo:80 >> Content-length: 4lines >> >> GET /b HTTP/1.0 >> Host: foo:80 >> Content-length: 8lines >> >> GET /c HTTP/1.0 >> Host: foo:80 >> Content-length: 0 >> >> POST /intruder HTTP/1.0 >> Host: bar:80 >> Content-length: 4lines >> >> GET /victim HTTP/1.0 >> Host: foo:80 >> Authorization: ... >> The browser sends requests for /a, /b (contains /c and /intruder as a request body), and /victim, but the server receives that for /a (contains /b), /c and /intruder (contains /victim). Apache2 (at least mod_core and mod_cgi) seems not sending any early positive response, (it sends early negative response), but it may (or may not) occur with other servers or modules. So, it is better prohibitting now. My suggestions for other headers are for general security/safety precaution. So I agree not discussing this issue in bug 302263. In my opinion, XMLHTTP should not be treated as a wget or telnet-equivalent, because it sends secret information (passwords and cookies) automatically. Intra-site TRACE is a such kind of problem. I suggest that request should be legitimate as possible, by not allowing to modify any existing valuable headers unless there is strong need to do so. Anyway, it might be better splitted to a non-critical issue.

Yutaka, Do origin servers actually look at the Content-Length header for GET requests? I suppose if they did, then the problem you describe could definitely happen. OK, I will add CL to the blacklist. > My suggestions for other headers are for general security/safety precaution. Do you have any explicit exploits in mind for the other headers? Can you explain why TRACE is a problem? Please see my comments regarding TRACE in bug 297078. Thanks!

(In reply to comment #18) > Yutaka, > > Do origin servers actually look at the Content-Length header for GET requests? > I suppose if they did, then the problem you describe could definitely happen. > OK, I will add CL to the blacklist. Yes.

(In reply to comment #18) > Do you have any explicit exploits in mind for the other headers? For a Referer, a sanity check performed by web servers can be circumvented. (A Referer: value itself is untrustful (possible forging), but when combined with other credential (password or cookie), it might be more or less trustful, because wget-using attackers can not send Referer-forged request with correct passwords.) It also confuses log files and other facilities. However, I am currently thinking about how much extent XSS can do a similar effect without XMLHTTP. So, please consider the Referer header, which is contained in the second list of original advisory, as a non-critical suggestion. To this purpose I have separated blacklist and "graylist" in the advisory. I cannot still find a good reason to allow forging Referer: and Date: values which the browser always knows the correct value. For the Connection: header, I included it to "blacklist" because it is a connection-controlling header, not a content metadata. It seems "unnatural" to allow such headers in XMLHTTP request. However, if you are 100% sure Mozilla implementation do not cause any problem under any settings (for example, setting "Connection: keep-alive" under keepalive disabled, or setting "Connection: close" under pipelining enabled), and you consider this as a "feature", It will be OK.

What does IE blacklist? Can we use a whitelist instead of a blacklist? What are the use cases for setRequestHeader? There could be an entire whitelisted namespace (e.g. xmlhttp-foo would be allowed for all foo) along with allowing overrides of safe common headers.

> What does IE blacklist? Good question. I'd really like to know this too. > Can we use a whitelist instead of a blacklist? We cannot use a whitelist. Part of the power of XMLHttpRequest is the ability to set custom headers when communicating to your server backend.

This is the version of the patch that I checked in on the trunk.

fixed-on-trunk

*** Bug 303672 has been marked as a duplicate of this bug. ***

Fix (via cvs up -j from what landed on the trunk) checked in to MOZILLA_1_7_BRANCH and AVIARY_1_0_1_20050124_BRANCH.

I had to make this change to get it to compile: - if (header.LowerCaseEqualsASCII(kInvalidHeaders[i])) { + if (header.Equals(kInvalidHeaders[i], nsCaseInsensitiveStringComparator())) {

bunch of samples and test cases put into https://bugzilla.mozilla.org/show_bug.cgi?id=297078