The default bug view has changed. See this FAQ.

XMLHttpRequest allows dangerous request headers to be set

RESOLVED FIXED in mozilla1.8beta4

Status

()

Core
XML
P1
critical
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: Darin Fisher, Assigned: Darin Fisher)

Tracking

({fixed-aviary1.0.7, fixed1.7.12})

Trunk
mozilla1.8beta4
fixed-aviary1.0.7, fixed1.7.12
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.7.11 -
blocking1.7.12 +
blocking-aviary1.0.7 +
blocking1.8b5 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:fix])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

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

Updated

12 years ago
Severity: normal → critical
Status: NEW → ASSIGNED
Flags: blocking1.8b4?
Flags: blocking1.7.11?
Flags: blocking-aviary1.0.7?
Priority: -- → P1
Target Milestone: --- → mozilla1.8beta4
(Assignee)

Updated

12 years ago
OS: Linux → All
Hardware: PC → All
(Assignee)

Comment 1

12 years ago
Created attachment 190629 [details] [diff] [review]
v1 patch
Attachment #190629 - Flags: superreview?(jst)
Attachment #190629 - Flags: review?(cbiesinger)

Comment 2

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

Comment 5

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

Comment 6

12 years ago
> 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.
Attachment #190629 - Flags: superreview?(jst) → superreview+
(Assignee)

Comment 8

12 years ago
Created attachment 190637 [details] [diff] [review]
v1.1 patch: restrict other headers
Attachment #190629 - Attachment is obsolete: true
Attachment #190637 - Flags: review?(cbiesinger)
(Assignee)

Updated

12 years ago
Attachment #190629 - Flags: review?(cbiesinger)
r=dveditz fwiw. I'll leave the request flag in case you specifically want biesi
Flags: blocking1.8b4?
Flags: blocking1.8b4+
Flags: blocking1.7.12?
Flags: blocking1.7.11?
Flags: blocking1.7.11-
(Assignee)

Comment 10

12 years ago
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.
Attachment #190637 - Flags: superreview+
Attachment #190637 - Flags: review?(cbiesinger)
Attachment #190637 - Flags: review+
Attachment #190637 - Flags: approval1.8b4?
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)
(Assignee)

Comment 12

12 years ago
biesi: I think I prefer to optimize for space instead of speed since this is not
performance critical code.  Sound good?
Whiteboard: [sg:fix]
Comment on attachment 190637 [details] [diff] [review]
v1.1 patch: restrict other headers

We probably want this on the branches, too.
Attachment #190637 - Flags: approval1.7.12?
Attachment #190637 - Flags: approval-aviary1.0.7?
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.
Blocks: 297078
Comment on attachment 190637 [details] [diff] [review]
v1.1 patch: restrict other headers

a=dveditz
This is required to fully fix bug 297078
Attachment #190637 - Flags: approval1.8b4? → approval1.8b4+

Comment 17

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

Comment 18

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

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

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

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

Comment 22

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

Comment 23

12 years ago
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.
Attachment #190637 - Attachment is obsolete: true
Attachment #190858 - Flags: approval1.7.12?
Attachment #190858 - Flags: approval-aviary1.0.7?
(Assignee)

Comment 24

12 years ago
fixed-on-trunk
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Assignee)

Updated

12 years ago
Attachment #190637 - Flags: approval1.7.12?
Attachment #190637 - Flags: approval-aviary1.0.7?
(Assignee)

Updated

12 years ago
Depends on: 302809
*** Bug 303672 has been marked as a duplicate of this bug. ***
Flags: blocking1.7.12?
Flags: blocking-aviary1.0.7?

Updated

12 years ago
Flags: blocking1.7.13?
Flags: blocking1.7.12?
Flags: blocking1.7.12+
Flags: blocking-aviary1.0.8?
Flags: blocking-aviary1.0.7?
Flags: blocking-aviary1.0.7+

Updated

12 years ago
Attachment #190858 - Flags: approval1.7.13?
Attachment #190858 - Flags: approval1.7.12+
Attachment #190858 - Flags: approval-aviary1.0.8?
Attachment #190858 - Flags: approval-aviary1.0.7+
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.
Keywords: fixed-aviary1.0.7, fixed1.7.12
I had to make this change to get it to compile:
-  if (header.LowerCaseEqualsASCII(kInvalidHeaders[i])) {
+  if (header.Equals(kInvalidHeaders[i], nsCaseInsensitiveStringComparator())) {

Comment 28

12 years ago
bunch of samples and test cases put into
https://bugzilla.mozilla.org/show_bug.cgi?id=297078
Group: security
You need to log in before you can comment on or make changes to this bug.