Last Comment Bug 302263 - XMLHttpRequest allows dangerous request headers to be set
: XMLHttpRequest allows dangerous request headers to be set
Status: RESOLVED FIXED
[sg:fix]
: fixed-aviary1.0.7, fixed1.7.12
Product: Core
Classification: Components
Component: XML (show other bugs)
: Trunk
: All All
: P1 critical (vote)
: mozilla1.8beta4
Assigned To: Darin Fisher
: Ashish Bhatt
Mentors:
: 303672 (view as bug list)
Depends on: 302809
Blocks: 297078
  Show dependency treegraph
 
Reported: 2005-07-26 15:28 PDT by Darin Fisher
Modified: 2006-08-20 18:16 PDT (History)
13 users (show)
dveditz: blocking1.7.11-
asa: blocking1.7.12+
asa: blocking‑aviary1.0.7+
dveditz: blocking1.8b5+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 patch (1.30 KB, patch)
2005-07-26 15:30 PDT, Darin Fisher
jst: superreview+
Details | Diff | Splinter Review
v1.1 patch: restrict other headers (1.42 KB, patch)
2005-07-26 16:16 PDT, Darin Fisher
darin.moz: review+
darin.moz: superreview+
dveditz: approval1.8b4+
Details | Diff | Splinter Review
v1.2 patch: include Content-Length header (1.44 KB, patch)
2005-07-28 10:25 PDT, Darin Fisher
asa: approval‑aviary1.0.7+
asa: approval1.7.12+
Details | Diff | Splinter Review

Description Darin Fisher 2005-07-26 15:28:04 PDT
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 2005-07-26 15:30:19 PDT
Created attachment 190629 [details] [diff] [review]
v1 patch
Comment 2 Jesse Ruderman 2005-07-26 15:39:22 PDT
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) 2005-07-26 15:42:46 PDT
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) 2005-07-26 15:45:01 PDT
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 2005-07-26 16:00:18 PDT
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 2005-07-26 16:02:32 PDT
> 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, jst@mozilla.com) 2005-07-26 16:05:46 PDT
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 2005-07-26 16:16:32 PDT
Created attachment 190637 [details] [diff] [review]
v1.1 patch: restrict other headers
Comment 9 Daniel Veditz [:dveditz] 2005-07-26 18:07:55 PDT
r=dveditz fwiw. I'll leave the request flag in case you specifically want biesi
Comment 10 Darin Fisher 2005-07-26 19:19:45 PDT
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) 2005-07-27 05:53:02 PDT
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 2005-07-27 08:49:07 PDT
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] 2005-07-27 14:12:30 PDT
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) 2005-07-27 14:20:08 PDT
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) 2005-07-27 14:38:09 PDT
(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] 2005-07-27 17:21:46 PDT
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 2005-07-27 19:56:03 PDT
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 2005-07-27 20:05:10 PDT
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 2005-07-27 20:13:17 PDT
(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 2005-07-27 21:28:07 PDT
(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 2005-07-27 21:31:31 PDT
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 2005-07-28 09:53:45 PDT
> 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 2005-07-28 10:25:08 PDT
Created attachment 190858 [details] [diff] [review]
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 2005-07-28 10:26:01 PDT
fixed-on-trunk
Comment 25 Christian :Biesinger (don't email me, ping me on IRC) 2005-08-06 13:31:35 PDT
*** Bug 303672 has been marked as a duplicate of this bug. ***
Comment 26 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2005-09-12 18:24:55 PDT
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+8 (review requests must explain patch) 2005-09-12 18:50:12 PDT
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 2005-09-15 16:54:33 PDT
bunch of samples and test cases put into
https://bugzilla.mozilla.org/show_bug.cgi?id=297078

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