Closed Bug 1426353 (CVE-2018-5163) Opened 2 years ago Closed 2 years ago

Do not load content process generated alternate data in the parent process.

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox-esr52 --- unaffected
firefox57 --- unaffected
firefox58 - wontfix
firefox59 --- wontfix
firefox60 --- fixed
firefox61 --- fixed

People

(Reporter: nbp, Assigned: valentin)

References

Details

(Keywords: csectype-priv-escalation, regression, sec-moderate, Whiteboard: [necko-triaged][adv-main60+][post-critsmash-triage])

Attachments

(1 file, 1 obsolete file)

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.
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.
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
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.
Blocks: 1405738
Group: core-security
Flags: needinfo?(nicolas.b.pierron)
(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.
Flags: needinfo?(nicolas.b.pierron)
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?
(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
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.
Ping to ensure the call on this is correct...
Flags: needinfo?(dveditz)
Flags: needinfo?(abillings)
Dan is on PTO. This is too late for the release and I don't think we should re-spin to take it.
Flags: needinfo?(abillings)
Whiteboard: ni?2018-01-18 ni?developer
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.
Assignee: nobody → valentin.gosu
Whiteboard: ni?2018-01-18 ni?developer → [necko-triaged] ni?2018-01-18 ni?developer
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?
(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.
Priority: -- → P3
(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?
Group: javascript-core-security → network-core-security
Flags: needinfo?(dveditz) → needinfo?(valentin.gosu)
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.
Flags: needinfo?(valentin.gosu)
Attachment #8955387 - Flags: feedback?(nicolas.b.pierron)
Attachment #8955387 - Flags: feedback?(bkelly)
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".
Attachment #8955387 - Flags: feedback?(nicolas.b.pierron) → feedback+
(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 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.
Attachment #8955387 - Flags: feedback?(bkelly)
MozReview-Commit-ID: htQ28muaBI
Attachment #8956306 - Flags: review?(michal.novotny)
Attachment #8955387 - Attachment is obsolete: true
Attachment #8956306 - Flags: review?(michal.novotny) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/964191c031db7c9b50b6a4f5dad9da1eb63acb49
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [necko-triaged] ni?2018-01-18 ni?developer → [necko-triaged]
https://hg.mozilla.org/mozilla-central/rev/964191c031db
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Please nominate this for Beta approval when you're comfortable doing so.
Flags: needinfo?(valentin.gosu)
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]:
Flags: needinfo?(valentin.gosu)
Attachment #8956306 - Flags: approval-mozilla-beta?
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
Flags: needinfo?(nicolas.b.pierron)
I guess if this is uplifted to beta then I can observe the beta probe that is still active:

https://mzl.la/2ILnbd8
(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
Flags: needinfo?(nicolas.b.pierron)
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.
Attachment #8956306 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Group: network-core-security → core-security-release
Whiteboard: [necko-triaged] → [necko-triaged][adv-main60+]
Alias: CVE-2018-5163
Flags: qe-verify-
Whiteboard: [necko-triaged][adv-main60+] → [necko-triaged][adv-main60+][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.