Open Bug 827853 Opened 12 years ago Updated 2 years ago

Parent side of HTTP channel implementation does not do input validation of child process's requests

Categories

(Core :: Networking, defect, P3)

Product:
Core
Component:
Networking
Type:
defect

Tracking

()

Project Flags:
blocking-basecamp -
Tracking Flags:
Tracking Status
b2g18 + ---

People

(Reporter: briansmith, Unassigned, NeedInfo)

References

(Blocks 1 open bug)

Details

(Keywords: csectype-priv-escalation, sec-low, Whiteboard: [necko-backlog]) 

+++ This bug was initially created as a clone of Bug #827847 +++

The child can modify the headers of the new request (the target of the redirect). We need to verify, in the parent process, that the headers that the child set are reasonable. For example, we need to ensure that the child didn't set a header value or name with "\r\n" or "\r\n\r\n" in it to cause a header injection of a security-sensitive header or request splitting, and we need to ensure the child didn't set a security-sensitive header that we shouldn't allow it to set (if any). Also, we need to ensure that we don't pass any headers into the child process that we shouldn't (e.g. Set-Cookie).

See also bug 827847.

I suggest we fix this by removing the ability of the child process to have redirect handlers. In that case, all redirect handlers should be in the parent process.

See also bug 827269.
Soft-blocking on this
blocking-basecamp: ? → -
tracking-b2g18: --- → +
+1

It means to audit (and change) all places where content can use it.

I would also suggest to remove OMR and others to not be triggered on content if there is not really a strong need to do it.  It would make things simpler for many of us.

These all are hooked by extensions or our chrome.  On content process those should not be used at all.  It IMO breaks the idea of isolation and sand-boxing.
CC'ing Honza (from Firebug) to let him know about these intentions.  I know about a content light version of Firebug that could use these APIs also on content.

Honza, can you confirm or reject this?  (I might be out here, since I don't know how the light version of Firebug works.)
Why is this sec-hidden? This is defense in depth, since you'd have to exploit the content process anyway. Content does have to be aware of redirects in all sorts of circumstances... why shouldn't that be allowed?
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #4)
> Why is this sec-hidden? This is defense in depth, since you'd have to
> exploit the content process anyway. Content does have to be aware of
> redirects in all sorts of circumstances... why shouldn't that be allowed?

Can you please give examples?
(In reply to Honza Bambas (:mayhemer) from comment #5)
> (In reply to Benjamin Smedberg  [:bsmedberg] from comment #4)
> > Why is this sec-hidden? This is defense in depth, since you'd have to
> > exploit the content process anyway. Content does have to be aware of
> > redirects in all sorts of circumstances... why shouldn't that be allowed?
> 
> Can you please give examples?

Ah.. Now I realize, on the content process we run all the docshell machinery and all around...

If we don't propagate to child we will break all of this.  This is IMO WONTFIX.
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #4)
> Why is this sec-hidden? This is defense in depth, since you'd have to
> exploit the content process anyway.

This is sec-hidden because it is a break in B2G content sandboxing that is probably not going to be fixed before B2G 1.0.

> Content does have to be aware of
> redirects in all sorts of circumstances... why shouldn't that be allowed?

It is one thing to notify content of the redirect. It is another thing to allow content processes to do request splitting and other things that affect other applications. Note that multiple sandboxed applications in B2G can send/receive responses on a shared connection so request splitting is one way for an application to attack another application (they can add a request that we did not expect, so that the server sends a response that we did not expect, and then we return that response to another application that is using the same connection, instead of returning the response that that application was actually expecting.)
(In reply to Honza Bambas (:mayhemer) from comment #6)
> If we don't propagate to child we will break all of this.  This is IMO
> WONTFIX.

Please read the whole description in comment 0. This is a bug that must be fixed. I think you and bsmedberg are objecting to my proposed fix (removing redirect handlers in the child process) and I agree that my proposed fixed is likely not going to be something we can implement. But, that just means we must find an alternative fix (e.g. stronger input validation in nsHttpChannelParent).

Jonas pointed out that we also need to make sure that, if the child process sets the request's Content-Length, then we need to ensure that the request length actually matches the Content-Length.

We also need to make sure that the content process doesn't try to set a header multiple times that should only be set once (e.g. Content-Length).

In general, most/all of the security checks we do to sanity check responses from servers need to be done on request properties set by child processes.
Summary: Parent side of HTTP channel implementation does not validate child process's modifications of a request done in its nsIAsyncVerifyRedirectCallback::OnRedirectVerifyCallback implementation → Parent side of HTTP channel implementation does not do input validation of child process's requests
Not WONTFIX, just LESSFIX :)

So we'll allow redirect observers on the child (and, I suspect on-modify-request--see bug 827269).  But there are still some things we should do to vet child HTTP requests: 

1) filter out \r\n from header values set by child.  We really don't want to allow compromised children to start splitting HTTP requests into 2 (could allow creative keep-alive attacks where next request on HTTP connection gets unexpected response).

2) blacklist any header types that we think are suspect: a good start here is the list that XHR prevents clients from setting (see step 5):

   http://www.w3.org/TR/XMLHttpRequest/#the-setrequestheader()-method

3) Whitelist allowed HTTP methods: again XHR is good start here: steps 5+6 of

   http://www.w3.org/TR/XMLHttpRequest/#the-open()-method

4) Probably some other things we haven't thought of yet.  bsmith, it's probably worth putting your scheming mind to use parsing all the P*.ipdl files in /netwerk to see if there's stuff that sticks out to you.  (I don't understand how nsIAssociatedContentSecurity works, so if someone can sanity check the PHttpChannel::UpdateAssociatedContentSecurity that would be swell).

I'll fork bugs and write patches for 1-3.  Let's keep this as a meta-bug for discussion about what else we may need to fix.
(In reply to Jason Duell (:jduell) from comment #9)
> So we'll allow redirect observers on the child (and, I suspect
> on-modify-request--see bug 827269).

Let's debate on-modify-request in its bug. :)

I agree about allowing redirect observers in the child, at least in the short term. BUT, I think yesterday jduell, sicking, and I also agreed that we should really not be calling into the child process for redirect observers in any case that we can help it, at least for performance reasons. I am hoping that we could use jduell's proposed scheme (add a HANDLE_DIRECTS_LIKE_XHR_IS_SUPPOSED_TO flag to nsIHttpChannel, so that HttpChannelParent could implement most/all redirect logic internally without needing to call into the parent) to solve both that performance problem AND part of this security problem. But, as jduell also noted in private conversation, we still have to do all this input validation anyway, because child processes set request headers on the initial requests, not just in redirect handlers.

So, IMO we should still have a bug to investigate completely removing redirect handlers from the child process.

> 1) filter out \r\n from header values set by child.

To be precise, I would say that we should reject any setting of a header where the name is not a valid name (according to the HTTP grammar), or where the value contains a control character other than '\t': {0x00-0x0a, 0x0c-0x1f, 0x7f}. This is because some servers interpret single '\r' or '\n' characters as equivalent to '\r\n' (just as we do when parsing responses, IIRC).

> 2) blacklist any header types that we think are suspect: a good start here
> is the list that XHR prevents clients from setting (see step 5):
> 
>    http://www.w3.org/TR/XMLHttpRequest/#the-setrequestheader()-method

Yes, I agree, but we have to be careful that we don't break non-XHR code that is setting those headers (e.g. Content-Length), if any.

> 3) Whitelist allowed HTTP methods: again XHR is good start here: steps 5+6 of
> 
>    http://www.w3.org/TR/XMLHttpRequest/#the-open()-method

+1.

> 4) Probably some other things we haven't thought of yet.  bsmith, it's
> probably worth putting your scheming mind to use parsing all the P*.ipdl
> files in /netwerk to see if there's stuff that sticks out to you.  (I don't
> understand how nsIAssociatedContentSecurity works, so if someone can sanity
> check the PHttpChannel::UpdateAssociatedContentSecurity that would be swell).
> 
> I'll fork bugs and write patches for 1-3.  Let's keep this as a meta-bug for
> discussion about what else we may need to fix.

Sounds like a good plan to me.
4) we should ensure that cookie values and HTTP auth credentials are never sent into the child process
4) we should ensure that */HTTP-only/* cookie values and HTTP auth credentials are never sent into the child process.

Important clarification :)
> IMO we should still have a bug to investigate completely removing redirect handlers 
> from the child process.

Sure, we'd get a perf win (an IPDL round trip, only for redirects, but still), so let's investigate it.  I don't know what other code besides XHR is needing redirect notifications in the child.  Go ahead and open a bug.
Depends on: 828221
(In reply to Honza Bambas (:mayhemer) from comment #3)
> CC'ing Honza (from Firebug) to let him know about these intentions.  I know
> about a content light version of Firebug that could use these APIs also on
> content.
Firebug Lite is pure web app running inside a page (e.g. as a bookmarklet) and so limited to content API. If I understand this bug properly, it would be related to the Net panel (which is displaying HTTP traffic). This panel is not implemented by Firebug Lite (only by the chrome/extension Firebug full version) since content doesn't provide API to intercept HTTP traffic (e.g. http-on-modify-request).

I am not sure about this bug, bug in any case, having such API available on content would definitely help Firebug Lite to grow and offer more features.

Honza
Depends on: 828958
> 1) filter out \r\n from header values set by child
> ...or where the value contains a control character other than '\t'

Nothing to do here IMO:  we do the same checks that SetHeader does normally, i.e. check that header name passes nsHttp::IsValidToken(), and reject values that contain '\r' or \n'.  bsmith, if you want to file a (non-e10s, across the browser) bug for filtering out more control chars, you're welcome to, but a quick read of RFC 2616 seems to indicate that HTTP header values adhere to RFC 822's definition of header values, which says that "The field-body may be composed of any ASCII characters, except CR or LF".
(In reply to Jason Duell (:jduell) from comment #16)
> > 1) filter out \r\n from header values set by child
> > ...or where the value contains a control character other than '\t'
> 
> Nothing to do here IMO:  we do the same checks that SetHeader does normally,
> i.e. check that header name passes nsHttp::IsValidToken(), and reject values
> that contain '\r' or \n'.  bsmith, if you want to file a (non-e10s, across
> the browser) bug for filtering out more control chars, you're welcome to,
> but a quick read of RFC 2616 seems to indicate that HTTP header values
> adhere to RFC 822's definition of header values, which says that "The
> field-body may be composed of any ASCII characters, except CR or LF".

That sounds like good news.

HTTPbis (and I think RFC 2616) defines its own rules for valid characters in headers:
http://tools.ietf.org/html/draft-ietf-httpbis-p1-messaging-21#section-3.2
We need an owner here.
Assignee: nobody → jduell.mcbugs
Opening this up, checked with Brian. This does not cover any protection we currently offer.
Group: core-security
Changing security rating to reflect the fact this is not an issue with current security features, it's a future request.
Keywords: sec-high → sec-low
Whiteboard: [necko-backlog]
Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258
Priority: -- → P1
Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258
Priority: P1 → P3
Assignee: jduell.mcbugs → nobody

With all the intervening changes, is this still applicable?

Blocks: fission-site-sandbox
Flags: needinfo?(valentin.gosu)
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.