Closed Bug 1415158 Opened 2 years ago Closed 2 years ago

Crash in PLDHashTable::Search | nsTHashtable<T>::Contains | mozilla::dom::ContentParent::EnsurePermissionsByKey

Categories

(Core :: Permission Manager, defect, critical)

56 Branch
x86
Windows 7
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox-esr52 --- unaffected
firefox56 --- wontfix
firefox57 --- wontfix
firefox58 + fixed
firefox59 + fixed

People

(Reporter: jesup, Assigned: Nika)

References

Details

(Keywords: crash, csectype-uaf, sec-high, Whiteboard: [adv-main58+][post-critsmash-triage])

Crash Data

Attachments

(1 file)

This bug was filed from the Socorro interface and is 
report bp-c855067e-d713-41e2-b60c-5d06a0171105.
=============================================================

UAF crashes start in 55 (or earlier, not 100% sure).  Unclear if it's fallout from the fix for bug 1349634
Looks like this is happening from this call for Wyciwyg channels:
http://searchfox.org/mozilla-central/rev/7e090b227f7a0ec44d4ded604823d48823158c51/netwerk/protocol/wyciwyg/WyciwygChannelParent.cpp#329

I'm guessing that the problem is related to us not checking mIPCClosed before trying to access our Manager(). IIRC we had a problem like that before which is why there's a check like that in HttpChannelParent (bug 1349634).

It looks like we also don't check this in FTPChannelParent (http://searchfox.org/mozilla-central/rev/7e090b227f7a0ec44d4ded604823d48823158c51/netwerk/protocol/ftp/FTPChannelParent.cpp#465), so I figure I'll add a check to both locations.
MozReview-Commit-ID: 4HBIalVCfno
Attachment #8925993 - Flags: review?(wmccloskey)
Assignee: nobody → nika
Attachment #8925993 - Flags: review?(wmccloskey) → review+
Comment on attachment 8925993 [details] [diff] [review]
Don't notify ContentParent of a load if our IPC channel is closed

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Probably not very easily? This can happen in production as we've seen the crash in the wild, but it seems to be a fairly straightforward null pointer dereference, which is usually not exploitable, and I'm not sure of a reliable setup to pull it off.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
The problem is pretty obvious if you read the code, but I tried to word the commit message such that it didn't paint a bulls-eye.

Which older supported branches are affected by this flaw?
55+ IIRC, so all but ESR.

If not all supported branches, which bug introduced the flaw?
bug 1337056

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
This patch should apply directly to all affected branches - if it doesn't it will be trivial to create a patch which does.

How likely is this patch to cause regressions; how much testing does it need?
This patch is very unlikely to cause regressions.
Attachment #8925993 - Flags: sec-approval?
> [Security approval request comment]
> How easily could an exploit be constructed based on the patch?
> Probably not very easily? This can happen in production as we've seen the
> crash in the wild, but it seems to be a fairly straightforward null pointer
> dereference, which is usually not exploitable, and I'm not sure of a
> reliable setup to pull it off.

The crashes in the field appear to be mostly UAF crashes, with some wildptrs and the like added in.  Few nullptrs.

> Which older supported branches are affected by this flaw?
> 55+ IIRC, so all but ESR.

We'll want an ESR patch and an uplift request for that.
 
Also: we'll want a read on 57.  Push this into RC2?  Hold as a possible 57.0.1 ride-along?  WONTFIX for 57?
Group: core-security → dom-core-security
(In reply to Randell Jesup [:jesup] from comment #4)

> > Which older supported branches are affected by this flaw?
> > 55+ IIRC, so all but ESR.
> 
> We'll want an ESR patch and an uplift request for that.

Oops, misread that as including ESR.  Since ESR is 52, I saw it as "55+, but including ESR" (as ESR wouldn't be in 55+, but you were saying "all active branches 55+ except ESR")
Comment on attachment 8925993 [details] [diff] [review]
Don't notify ContentParent of a load if our IPC channel is closed

Approval Request Comment
[Feature/Bug causing the regression]: bug 1337056
[User impact if declined]: Potential UAF in the chrome process.
[Is this code covered by automated tests?]: Not explicitly.
[Has the fix been verified in Nightly?]: No
[Needs manual test from QE? If yes, steps to reproduce]: No testing needed - fairly straightforward fix. 
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: simple check to ensure that the channel has not been closed before accessing the manager.
[String changes made/needed]: None

--
I doubt this is worth trying to get into 56, but I it's probably worth getting into 57 considering how low risk it is.
Attachment #8925993 - Flags: approval-mozilla-beta?
Hi Dan, could you please comment on whether you think this is a must fix for 57? It doesn't seem easily exploitable which could be a reason to wontfix. But I'll go with your recommendation.
Flags: needinfo?(dveditz)
Ritu, I'm back. I think we don't have to take this now, especially since we're supposed to ship in six days.
Flags: needinfo?(dveditz)
Sec-approval+ for checkin on 11/28, two weeks into the next cycle (for 59).
Whiteboard: [checkin on 11/28]
Attachment #8925993 - Flags: sec-approval? → sec-approval+
Attachment #8925993 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Comment on attachment 8925993 [details] [diff] [review]
Don't notify ContentParent of a load if our IPC channel is closed

resetting beta approval flag so it can be requested for 58 when this lands on trunk.
Attachment #8925993 - Flags: approval-mozilla-beta-
https://hg.mozilla.org/mozilla-central/rev/e09c9404d417
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Please request Beta approval on this when you get a chance. It grafts cleanly as-landed.
Flags: needinfo?(nika)
Comment on attachment 8925993 [details] [diff] [review]
Don't notify ContentParent of a load if our IPC channel is closed

See comment 6 for approval request
Flags: needinfo?(nika)
Attachment #8925993 - Flags: approval-mozilla-beta?
Group: dom-core-security → core-security-release
Duplicate of this bug: 1421529
Comment on attachment 8925993 [details] [diff] [review]
Don't notify ContentParent of a load if our IPC channel is closed

Fix a sec-high. Beta58+.
Attachment #8925993 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Whiteboard: [adv-main58+]
Flags: qe-verify-
Whiteboard: [adv-main58+] → [adv-main58+][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.