302263 - XMLHttpRequest allows dangerous request headers to be set

Status: RESOLVED FIXED

(Core :: XML, defect, P1)

Description

[Darin Fisher]

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.

Comment 1

[Darin Fisher]

Attachment: v1 patch

Comment 2

[Jesse Ruderman]

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

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

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 4

[Christian :Biesinger (don't email me, ping me on IRC)]

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?

Comment 5

[Darin Fisher]

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.

Comment 6

[Darin Fisher]

> 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 7

[Johnny Stenback (:jst)]

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.

Comment 8

[Darin Fisher]

Attachment: v1.1 patch: restrict other headers

Comment 9

[Daniel Veditz [:dveditz]]

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

Comment 10

[Darin Fisher]

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.

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

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)

Comment 12

[Darin Fisher]

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

Comment 13

[Daniel Veditz [:dveditz]]

Comment on attachment 190637 [details] [diff] [review]
v1.1 patch: restrict other headers

We probably want this on the branches, too.

Comment 14

[Hixie (not reading bugmail)]

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!

Comment 15

[Christian :Biesinger (don't email me, ping me on IRC)]

(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 16

[Daniel Veditz [:dveditz]]

Comment on attachment 190637 [details] [diff] [review]
v1.1 patch: restrict other headers

a=dveditz
This is required to fully fix bug 297078

Comment 17

[Yutaka OIWA]

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.

Comment 18

[Darin Fisher]

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!

Comment 19

[Yutaka OIWA]

(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.

Comment 20

[Yutaka OIWA]

(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.

Comment 21

[Jesse Ruderman]

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.

Comment 22

[Darin Fisher]

> 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.

Comment 23

[Darin Fisher]

Attachment: v1.2 patch: include Content-Length header

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

Comment 24

[Darin Fisher]

fixed-on-trunk

Comment 25

[Christian :Biesinger (don't email me, ping me on IRC)]

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

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 27

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 28

[chris hofmann]

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