Be less trusting of data we receive from the content process over IPC
Categories
(Core :: DOM: Content Processes, defect)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox71 | --- | fixed |
People
(Reporter: ehsan.akhgari, Assigned: barret)
Details
Attachments
(1 file)
|
47 bytes,
text/x-phabricator-request
|
lizzard
:
approval-mozilla-beta-
|
Details | Review |
In the new nsIWebProgressListener helpers we've added to PBrowser there are a number of scenarios where we receive Maybe arguments from the content process and trust them to be a some based on other data that we've received. Such assumptions aren't valid for a compromised content process which may be sending us garbage to play a DoS attack.
|
Assignee | |
Comment 1•5 years ago
|
||
Previously we assumed that we could trust the OnLocationChange,
OnStateChange, and OnSecurityChange IPC messages from the BrowserChild to
BrowserParent to always contain their optional data if the message was
top-level. However, if a content process becomes compromised, this could be
used to DOS the parent process by maliciously sending these messages to the
parent process with invalid data.
Now we explicitly check that the data is valid before derefing.
Pushed by brennie@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8de293399b5a Ensure we have optional data before derefing in BrowserParent r=Ehsan
|
||
Comment 3•5 years ago
|
||
| bugherder | ||
|
|
Assignee | |
Comment 4•5 years ago
|
||
Comment on attachment 9099367 [details]
Bug 1586871 - Ensure we have optional data before derefing in BrowserParent r?Ehsan
Beta/Release Uplift Approval Request
- User impact if declined: If a content process is compromised, it could DOS the parent process from ever being able to ever load a page.
- Is this code covered by automated tests?: No
- Has the fix been verified in Nightly?: No
- Needs manual test from QE?: No
- If yes, steps to reproduce:
- List of other uplifts needed: None
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): Content processes always send this information when it is expected so there will be no change in behaviour for non-compromised content processes.
- String changes made/needed:
|
|
Assignee | |
Comment 5•5 years ago
|
||
Note: this hasn't been verified on nightly in the sense that it would require compromising a content process to send invalid data. However, pages continue to load properly on nightly.
|
Reporter | |
Comment 6•5 years ago
•
|
||
In theory OSS-fuzz should (have) be(en) able to verify (find) this bug...
|
||
Comment 7•5 years ago
|
||
Ehsan do you think we should report that to oss-fuzz ? I could go ahead and do that.
|
|
||
Comment 8•5 years ago
|
||
Barret how important do you think it is to ship this to 70? We are quite late in beta and it hasn't had a whole lot of time on nightly to catch possible problems. We can uplift it if you think it is worth some risk. There is tomorrow's beta build which releases Friday, and then the release candidate build will be on Monday morning. So time is tight.
|
|
Reporter | |
Comment 9•5 years ago
|
||
(In reply to Liz Henry (:lizzard) from comment #7)
Ehsan do you think we should report that to oss-fuzz ? I could go ahead and do that.
I'm not familiar with it really, posidron may know.
|
|
Assignee | |
Comment 10•5 years ago
|
||
:lizzard I don't think it is super important to uplift this. It should be very low risk but the we hit a case where this would prevent an issue, something else has gone wrong (ie someone found a way to compromise a content proc).
|
||
Comment 11•5 years ago
|
||
(In reply to Liz Henry (:lizzard) from comment #7)
Ehsan do you think we should report that to oss-fuzz ? I could go ahead and do that.
We will investigate this, as we maintain those targets and have regular meetings with the Google fuzzing people, but thanks!
Ehsan, which target on oss-fuzz would that have been? The ContentParentIPC one? Unfortunately I don't enough about our IPC to know how these classes work together, so figuring out if our IPC fuzzing targets could have reached this code would be the first step.
|
|
||
Comment 12•5 years ago
|
||
Comment on attachment 9099367 [details]
Bug 1586871 - Ensure we have optional data before derefing in BrowserParent r?Ehsan
We're so close to 70 launch, let's let this ride with 71.
|
|
Reporter | |
Comment 13•5 years ago
|
||
(In reply to Christian Holler (:decoder) from comment #11)
(In reply to Liz Henry (:lizzard) from comment #7)
Ehsan do you think we should report that to oss-fuzz ? I could go ahead and do that.
We will investigate this, as we maintain those targets and have regular meetings with the Google fuzzing people, but thanks!
Thanks!
Ehsan, which target on oss-fuzz would that have been? The ContentParentIPC one?
yes, ContentParentIPC is what I'm thinking.
Unfortunately I don't enough about our IPC to know how these classes work together, so figuring out if our IPC fuzzing targets could have reached this code would be the first step.
Yeah me neither. I'm working off of the fact that I've recently seen oss fuzz discover some other similar bugs, for example see bug 1577558, bug 1577563 and bug 1578458.
|
||
Comment 14•5 years ago
|
||
(In reply to :ehsan akhgari from comment #9)
(In reply to Liz Henry (:lizzard) from comment #7)
Ehsan do you think we should report that to oss-fuzz ? I could go ahead and do that.
I'm not familiar with it really, posidron may know.
Sorry for the late NI but I was on honeymoon. Not entirely sure I have understood the above correctly but this shouldn't have any affect for any of the IPC fuzzing runs because they unaware of the state and structure of the IPC messages.
|
|
Reporter | |
Comment 15•5 years ago
|
||
(In reply to Christoph Diehl [:posidron] from comment #14)
(In reply to :ehsan akhgari from comment #9)
(In reply to Liz Henry (:lizzard) from comment #7)
Ehsan do you think we should report that to oss-fuzz ? I could go ahead and do that.
I'm not familiar with it really, posidron may know.
Sorry for the late NI but I was on honeymoon.
No worries; congratulations on your wedding. :-)
Not entirely sure I have understood the above correctly but this shouldn't have any affect for any of the IPC fuzzing runs because they unaware of the state and structure of the IPC messages.
My main question here was whether the ContentParentIPC fuzzing we run through oss-fuzz should have been able to find this bug (which we found through code inspection) by fuzzing this protocol?
Description
•