1748210 - Hit MOZ_CRASH(nsSupportsPRUint32 not thread-safe) at xpcom/base/nsISupportsImpl.cpp:43

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect, P2)

Description

[Christian Holler (:decoder)]

The attached testcase crashes on mozilla-central revision 20211231-d0a94b1f309b (fuzzing build required).

For detailed crash information, see attachment.

I was not able to reproduce this issue so far, but it reproduces fairly often in our fuzzing infra. I guess this is a data race of some sort, but I also tried a TSan build with no luck. Maybe the stack is enough to figure out what is going on.

Normally, the issue would be reproducible with the following steps:

1. Download the attached testcase, save as "test.bin".
   2a. Build with --enable-fuzzing (requires Clang and ASan, also build gtests using ./mach gtest dontruntests).
   2b. Alternatively you can download builds from TC using python -mfuzzfetch -a --fuzzing --gtest (see https://github.com/MozillaSecurity/fuzzfetch).
2. Run FUZZER=NetworkHttpProxyPlain objdir/dist/bin/firefox test.bin

Marking s-s until investigated.

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 2

[Christian Holler (:decoder)]

(Missing testcase for comment 0).

Comment 3

[Christian Holler (:decoder)]

Attachment: Testcase for comment 4

Comment 4

[Kershaw Chang [:kershaw]]

Attachment: Bug 1748210 - Always release GetNextTokenCompleteEvent on main thread, r=#necko

Comment 5

[Andrew McCreight [:mccr8]]

Doing refcounting on the wrong thread for a non-threadsafe object can cause a UAF from a race. This is probably difficult to actually exploit, but I'll conservatively mark it sec-high.

Comment 6

[Kershaw Chang [:kershaw]]

Comment on attachment 9257321 [details]
Bug 1748210 - Always release GetNextTokenCompleteEvent on main thread, r=#necko

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Probably not easy, since an exploit can be only constructed by race.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes
• Which older supported branches are affected by this flaw?: all
• If not all supported branches, which bug introduced the flaw?: n/a
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?:
• How likely is this patch to cause regressions; how much testing does it need?: Unlikely. Forcing GetNextTokenCompleteEvent to be released on the right thread should not cause any problem.

Comment 7

[Tom Ritter [:tjr]]

Comment on attachment 9257321 [details]
Bug 1748210 - Always release GetNextTokenCompleteEvent on main thread, r=#necko

Approved to land and uplift

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Always release GetNextTokenCompleteEvent on main thread, r=necko-reviewers,valentin
https://hg.mozilla.org/integration/autoland/rev/d0dc81c342c08e99cfd73b61d13138cc2ecec010
https://hg.mozilla.org/mozilla-central/rev/d0dc81c342c0

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta & ESR91 approval when you get a chance.

Comment 11

[Kershaw Chang [:kershaw]]

Comment on attachment 9257321 [details]
Bug 1748210 - Always release GetNextTokenCompleteEvent on main thread, r=#necko

Beta/Release Uplift Approval Request

• User impact if declined: Possible UAF
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: N/A
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The patch is straightforward.
• String changes made/needed: N/A

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration:
• User impact if declined: Possible UAF
• Fix Landed on Version: 98
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The patch is straightforward.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9257321 [details]
Bug 1748210 - Always release GetNextTokenCompleteEvent on main thread, r=#necko

Approved for 97.0b3 and 91.6esr.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/9861140acd38

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/5a44dfe6261b

Comment 15

[BugBot [:suhaib / :marco/ :calixte]]

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.

Comment 16

[Kershaw Chang [:kershaw]]

(In reply to Release mgmt bot [:marco/ :calixte] from comment #15)

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.

Done.