Closed Bug 1258231 Opened 8 years ago Closed 8 years ago

crash in nsConsoleService::ClearMessagesForWindowID

Categories

(Core :: XPCOM, defect)

x86
Windows NT
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox45 --- wontfix
firefox46 + fixed
firefox47 + fixed
firefox48 + fixed
firefox-esr38 --- unaffected
firefox-esr45 46+ fixed

People

(Reporter: jesup, Assigned: erahm)

References

Details

(5 keywords, Whiteboard: [post-critsmash-triage][adv-main46+][adv-esr45.1+])

Crash Data

Attachments

(1 file, 1 obsolete file)

This bug was filed from the Socorro interface and is 
report bp-7db23744-0e54-43f8-b1bf-0b2b52160315.
=============================================================

Crash is on a line last touched by froyd in bug 1189829; the bug might pre-exist that change however.

UAF signature, with ~1000 crashes in the last week.  Likely a sec-crit; certainly a sec-high
So we assert that we're on the main thread [1] which is good enough if anything else accessing the message array is main thread only as well, but that's not the case [2].

I think we need to lock |mLock| to actually guarantee thread safety.

[1] http://hg.mozilla.org/releases/mozilla-beta/annotate/fb3494d06dfb/xpcom/base/nsConsoleService.cpp#l77
[2] http://hg.mozilla.org/releases/mozilla-beta/annotate/fb3494d06dfb/xpcom/base/nsConsoleService.cpp#l206
Attachment #8732966 - Flags: review?(nfroyd)
Assignee: nobody → erahm
Status: NEW → ASSIGNED
Also just to be clear, this was a latent issue existing prior to bug 1189829. It was most likely introduced in bug 814497.
Comment on attachment 8732966 [details] [diff] [review]
Lock while iterating console messages

Nathan's out, mccr8 I think you've touched this code before, mind rs'ing?
Attachment #8732966 - Flags: review?(nfroyd) → review?(continuation)
Comment on attachment 8732966 [details] [diff] [review]
Lock while iterating console messages

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

Nice. Does nsConsoleService::ClearMessages() need this treatment, too? It also accesses mMessages without a lock. I guess it is only called from Reset() where the lock is held and from the dtor which hopefully can't happen when anything else holds it.
Attachment #8732966 - Flags: review?(continuation) → review+
(In reply to Andrew McCreight [:mccr8] from comment #5)
> Comment on attachment 8732966 [details] [diff] [review]
> Lock while iterating console messages
> 
> Review of attachment 8732966 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice. Does nsConsoleService::ClearMessages() need this treatment, too? It
> also accesses mMessages without a lock. I guess it is only called from
> Reset() where the lock is held and from the dtor which hopefully can't
> happen when anything else holds it.

Yeah I checked that as well and came to the same conclusions.
Maybe add a comment?
(In reply to Andrew McCreight [:mccr8] from comment #7)
> Maybe add a comment?

Sure.
Update reviewer, added requested comment.
Attachment #8732966 - Attachment is obsolete: true
Comment on attachment 8732986 [details] [diff] [review]
Lock while iterating console messages

Carrying forward r+.

> [Security approval request comment]
> How easily could an exploit be constructed based on the patch?

Hard for me to assess, I would guess given the prevalence of crashes it would be pretty easy.

> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

Nothing explicit, but it's pretty clear what we're fixing.

> Which older supported branches are affected by this flaw?

This goes back to 42, so all.

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

It affects all, was introduced in bug 814497.

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

Patch should apply cleanly to all supported branches.

> How likely is this patch to cause regressions; how much testing does it need?

Not likely to regress, fixes crashes.
Attachment #8732986 - Flags: sec-approval?
Attachment #8732986 - Flags: review+
Group: core-security → dom-core-security
Keywords: sec-high
sec-approval+ for trunk. We'll want this on branches too.
Attachment #8732986 - Flags: sec-approval? → sec-approval+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1ad759d658a3
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Group: dom-core-security → core-security-release
Can you request uplift to aurora and beta? Thanks.
Flags: needinfo?(erahm)
Comment on attachment 8732986 [details] [diff] [review]
Lock while iterating console messages

Approval Request Comment
[Feature/regressing bug #]:
bug 814497

[User impact if declined]:
Crashes, UAF.

[Describe test coverage new/current, TreeHerder]:
Baked on Nightly for a few days.

[Risks and why]: 
None, fixes multithreaded access of console messages.

[String/UUID change made/needed]:
None.
Flags: needinfo?(erahm)
Attachment #8732986 - Flags: approval-mozilla-beta?
Attachment #8732986 - Flags: approval-mozilla-aurora?
Comment on attachment 8732986 [details] [diff] [review]
Lock while iterating console messages

Crash fix, sec-high, let's take this for aurora and for beta 6
Attachment #8732986 - Flags: approval-mozilla-beta?
Attachment #8732986 - Flags: approval-mozilla-beta+
Attachment #8732986 - Flags: approval-mozilla-aurora?
Attachment #8732986 - Flags: approval-mozilla-aurora+
Whiteboard: [post-critsmash-triage]
Comment on attachment 8732986 [details] [diff] [review]
Lock while iterating console messages

sec-high, taking it in esr45 too.
Attachment #8732986 - Flags: approval-mozilla-esr45+
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main46+][adv-esr45.1+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.