1426353 - (CVE-2018-5163) Do not load content process generated alternate data in the parent process.

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect, P3)

Description

[Nicolas B. Pierron [:nbp]]

An attacker could potentially use the JSBC's Alternate Data (Bug 1405738) to escalate from the content process to the parent process.

Today, most of the alternate data content is produced by the content process, and saved in the parent process.  If an attacker has full control over a single content process it might be able to replace the alternate data associated with some JavaScript code.

The problem we face here, is if the parent process were to load the same resources, and get to execute the alternate data content provided by the hacked content process.

From what I heard, while this is not supposed to happen, that we are loading external content in privilege code, it happened in the past, and I think preventing such escalation at the cache level makes more sense too.

[Tracking Requested - why for this release]: Issue introduced by Bug 1405738.

Comment 1

[Valentin Gosu [:valentin] (he/him)]

The parent process (ie. the cache) has to open the alt data when requested by the child process, in order to pass it through IPC to the child.
I guess what we want to do is to add in a check that the only consumer to which we pass OnDataAvailable from an alt-data source is HttpChannelParent. I'm not sure at the moment if that's enough.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Please make sure it still works in non-e10s mode in android, though.  You can use BrowserTabsRemoteAutostart() to determine if we are in e10s or non-e10s:

https://searchfox.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp#5126

Comment 3

[Daniel Veditz [:dveditz]]

nbp: is "Networking" the right component for this bug? You tagged it as a "JavaScript" security bug and the feature bugs that introduced this were also in JavaScript components.

Comment 4

[Nicolas B. Pierron [:nbp]]

(In reply to Daniel Veditz [:dveditz] from comment #3)
> nbp: is "Networking" the right component for this bug?

Yes.

> You tagged it as a
> "JavaScript" security bug and the feature bugs that introduced this were
> also in JavaScript components.

The JS engine is currently the only consumer of this API so far, but this is not an issue from the JS engine, nor something to fix in the JS engine.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

To be clear, this is a defense-in-depth issue, right?  AFAICT it requires another vulnerability to crack the content process or local access to spoof a content process.  Right?

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Ben Kelly [:bkelly] from comment #5)
> To be clear, this is a defense-in-depth issue, right?  AFAICT it requires
> another vulnerability to crack the content process or local access to spoof
> a content process.  Right?

I presume so from comment 0 - however, we have to assume content processes can get owned, and when that happens our defense is all about limiting the exploit to that content process, and avoiding full access to the OS/disk (which allows ransomware, etc).  In general, sandbox escapes are more critical than normal vulnerabilities

Comment 8

[Julien Cristau [:jcristau]]

For 58 it's too late to take anything other than a "disable JSBC" patch.  Hopefully, if this was bad enough that we should do that, it would have been escalated earlier.

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

Ping to ensure the call on this is correct...

Comment 10

[Al Billings [:abillings - ex-MoCo]]

Dan is on PTO. This is too late for the release and I don't think we should re-spin to take it.

Comment 11

[Valentin Gosu [:valentin] (he/him)]

Taking the bug, and I hope to have a patch next week.
My approach here would be to add another field to the cache entry metadata specifying if the alt-data was generated in the content process. If it was, then we assert that the consumer is also the content process. Or even better yet, we could try to act as though we don't have alt-data at all, if the parent process requests a resource with alt-data generated in the child.

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

I'm curious how you will note that it came from a child process.  It would be nice if this is not too coupled to HttpChannelParent.  We'll need the ability to set/read this value for service worker interception cases as well.  Currently that interception is done in the child process, but in the future we are going to do it in the InterceptedHttpChannel in the parent.

Also, how will the nsHttpChannel differentiate between the HttpChannelParent trying to get the alternate data on behalf of the child vs some other nsIChannel consumer in the parent trying to access the alternate data?

Comment 13

[Valentin Gosu [:valentin] (he/him)]

(In reply to Ben Kelly [:bkelly] from comment #12)
> I'm curious how you will note that it came from a child process. [...]

These are very good questions. I really don't know yet :)
I'm aiming for something simple at first, but I'll try to leave room for the future changes you mentioned.

Comment 14

[Daniel Veditz [:dveditz]]

(In reply to Valentin Gosu [:valentin] from comment #11)
> Taking the bug, and I hope to have a patch next week.

How's it going?

Comment 15

[Valentin Gosu [:valentin] (he/him)]

Attachment: bug1426353-alt-data-content-only.patch

Comment 16

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8955387 [details] [diff] [review]
bug1426353-alt-data-content-only.patch

This bug dropped off my radar for a while there :)

I had some time to think about it, and I think this might work:
HttpChannelParent sets a flag in nsHttpChannel that the consumer is in the child process. After opening the cache entry nsHttpChannel checks if the entry has a metadata marking that the alt-data has been generated by the child. If so, it makes sure to only return the alt-data content if the flag matches the metadata.

There are a few things I didn't handle yet:
* are diverting channels a problem?
* other ChannelParent types creating nsHttpChannels
* I also need to pass the flag through redirected channels (current patch doesn't)

Anyway, I thought some feedback would be nice before I proceed with this approach.

Comment 17

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8955387 [details] [diff] [review]
bug1426353-alt-data-content-only.patch

Review of attachment 8955387 [details] [diff] [review]:
-----------------------------------------------------------------

I am not a peer, nor an expert on the HTTP channels.
I think this patch is enough for the issue that I reported here. (except for the HTTP redirection that I would not have noticed if you did not mentioned it)

(In reply to Valentin Gosu [:valentin] from comment #16)
> I had some time to think about it, and I think this might work:
> HttpChannelParent sets a flag in nsHttpChannel that the consumer is in the
> child process. After opening the cache entry nsHttpChannel checks if the
> entry has a metadata marking that the alt-data has been generated by the
> child. If so, it makes sure to only return the alt-data content if the flag
> matches the metadata.
> 
> There are a few things I didn't handle yet:
> * are diverting channels a problem?

What are "diverting channels"?

> * other ChannelParent types creating nsHttpChannels

As far as I know, we do not yet have another ChannelParent providing an AltData.  There is one patch on hold because the author is no longer present.

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ +2274,5 @@
>      return NS_ERROR_NOT_AVAILABLE;
>    }
> +  nsresult rv = mCacheEntry->OpenAlternativeOutputStream(type, _retval);
> +  if (NS_SUCCEEDED(rv)) {
> +    mCacheEntry->SetMetaDataElement("alt-data-from-child", "1");

This might be pointless as we are in the HttpChannelParent anyway, but I would have checked/assert the mAltDataForChild field too, to make it clear that  (mAltDataForChild && OpenAlternativeOutputStream)  implies  "alt-data-from-child: 1".

Comment 18

[Valentin Gosu [:valentin] (he/him)]

(In reply to Nicolas B. Pierron [:nbp] {backlog: ~38} from comment #17)
> Comment on attachment 8955387 [details] [diff] [review]
> bug1426353-alt-data-content-only.patch
>
> > There are a few things I didn't handle yet:
> > * are diverting channels a problem?
> 
> What are "diverting channels"?

When a channel is diverted its listener is replaced with one on the parent side. I think it's usually used for downloading files.

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8955387 [details] [diff] [review]
bug1426353-alt-data-content-only.patch

Sorry, I'm going on parental leave a bit early.  I won't have time to do any reviews here for a while.

Comment 20

[Valentin Gosu [:valentin] (he/him)]

Attachment: Do not allow the parent process to consume alt-data generated in the content process

MozReview-Commit-ID: htQ28muaBI

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/964191c031db7c9b50b6a4f5dad9da1eb63acb49

Comment 22

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/964191c031db

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta approval when you're comfortable doing so.

Comment 24

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8956306 [details] [diff] [review]
Do not allow the parent process to consume alt-data generated in the content process

Approval Request Comment
[Feature/Bug causing the regression]:
Not really a regression, but a flaw in the initial alt-data impl (bug 1231565)

[User impact if declined]:
Provides an attack vector for a compromized content process.

[Is this code covered by automated tests?]:
Yes.

[Has the fix been verified in Nightly?]:
Yes.

[Needs manual test from QE? If yes, steps to reproduce]:
No.
 
[List of other uplifts needed for the feature/fix]:
None.

[Is the change risky?]:
No.

[Why is the change risky/not risky?]:
It marks a cache entry containing alt-data with a flag when the data comes from the content process, and it enforces that we never use that alt-data in the parent process.
This change is relatively simple, and unlikely to cause regressions.

[String changes made/needed]:

Comment 25

[Ben Kelly [:bkelly, not reviewing]]

Do we have any telemetry showing that our alternate data usage is still working in the wild?  I have a service worker probe, but it expired in FF61, so I can't see:

https://mzl.la/2IGZkLu

Comment 26

[Ben Kelly [:bkelly, not reviewing]]

I guess if this is uplifted to beta then I can observe the beta probe that is still active:

https://mzl.la/2ILnbd8

Comment 27

[Nicolas B. Pierron [:nbp]]

(In reply to Ben Kelly [Part-time, not reviewing][:bkelly] from comment #25)
> Do we have any telemetry showing that our alternate data usage is still
> working in the wild?  I have a service worker probe, but it expired in FF61,
> so I can't see:

The following is reporting that the ScriptLoader does load bytecode, thus that the alternate data cache is still being populated and loaded from. As the alternate data is indexed by the build-id, this is literally a daily report of the activity of the JSBC.

https://mzl.la/2DNgFP4

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8956306 [details] [diff] [review]
Do not allow the parent process to consume alt-data generated in the content process

Fixes a sec issue with alt-data. Approved for 60.0b7.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/11c7370ae92e