Closed Bug 1258231 Opened 9 years ago Closed 9 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
Status: ASSIGNED → RESOLVED
Closed: 9 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.

Attachment

General

Created:
Updated:
Size: