1193389 - (CVE-2024-5702) crash in mozilla::net::nsHttpTransaction::OnTransportStatus(nsITransport*, nsresult, __int64) PR_Write

Status: RESOLVED FIXED

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

Description

[Valentin Gosu [:valentin] (he/him)]

This bug was filed from the Socorro interface and is 
report bp-650c34bd-66fb-461f-b1a4-ad5eb2150810.
=============================================================

May be related to bug 1153929. 
Small number of crashes. 
Stack traces indicate connection with Http2.

Comment 1

[Patrick McManus [:mcmanus]]

could be related to the various pr_write bugs.. that has just happened in this stack trace

Comment 2

[Patrick McManus [:mcmanus]]

in the last 28 days there have been 0 of these on >=46 ..

Comment 3

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 4

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

low-frequency continuing wildptr crashes. Clearing priority for re-triage

Comment 6

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

The severity field for this bug is set to S3. However, the bug is flagged with the sec-high keyword.
:valentin, could you consider increasing the severity of this security bug?

For more information, please visit auto_nag documentation.

Comment 7

[Daniel Veditz [:dveditz]]

In the last 3 months there have been only 23 crashes, mostly on Windows. A lot are on ESR-115 builds, but those crashes are unthrottled compared to Windows Release crashes. There are similar crashes before and after 115.

There are two crashes on Linux, both a clearly UAFs: calling a virtual function on a deleted mTransportSink at https://searchfox.org/mozilla-esr115/rev/b412646630c31691852911babfa95fd60bfe1005/netwerk/protocol/http/nsHttpTransaction.cpp#693
The calling stacks above that are different though. One was Http2 and the other was DNS.
bp-40f5da92-b485-4219-b90b-16bf90230905
bp-82270723-1c9b-4bdc-9fd0-f59aa0230713

mTransportSink is a nsComPtr that's set from net_NewTransportEventSinkProxy() in nsHttpTransaction::Init() and then only used on the line where the crash is. Can the TransportSink free itself even though there's a strong reference to it? Or is it more likely that the nsHttpTransaction itself has been freed?
I include these because they're a bit different from the "wildptr"-looking Windows ones.

The Windows crashes look like a bunch of different things. EXCEPTION_STACK_BUFFER_OVERRUN / FAST_FAIL_STACK_COOKIE_CHECK_FAILURE means the stack canary check failed when it was about to return from the function. All the local variables look like fixed-size things like integers so I'm not seeing how the canary corruption could be due to a buffer overflow. Maybe some other function corrupted the stack?

Similarly mysterious is EXCEPTION_STACK_BUFFER_OVERRUN / FAST_FAIL_GUARD_ICALL_CHECK_FAILURE which always comes from the tellable->Tell(&prog); line.

Comment 8

[Tom Ritter [:tjr]]

FAST_FAIL_GUARD_ICALL_CHECK_FAILURE is indicating that CFI icall protection prevented the function call. Presumably this is because whatever tellable[offset of Tell()] points to is not a valid function entry point. ni to Valentin for Comment 7

Comment 9

[Valentin Gosu [:valentin] (he/him)]

Kershaw, can you have a quick look next week and unstall if needed?

Comment 10

[Kershaw Chang [:kershaw]]

(In reply to Valentin Gosu [:valentin] (he/him) {{ PTO until Jan 5th }} from comment #9)

Kershaw, can you have a quick look next week and unstall if needed?

Assign this to myself.

Comment 11

[Kershaw Chang [:kershaw]]

Attachment: Bug 1193389 - nsTransportEventSinkProxy clean up, r=#necko

Comment 12

[Kershaw Chang [:kershaw]]

Remove stalled keyword since there is a patch in review.

Comment 13

[Valentin Gosu [:valentin] (he/him)]

I was trying to figure out if the attached patch is correct. I think the cleanup makes sense, but it's unlikely to fix the bug.
I can't make much sense of the issue though. The points Dan brought up in comment 7 seem promising but I couldn't find a way TransportSink could be freed in nsHttpTransaction. Most of those member variables are set and never again changed, so thread safety should not be a problem.
My best guess is that we have an A::F1 holds a reference to B and B::F2 calls A::Close which releases B scenario - but I can't see where that could happen.

Comment 14

[Kershaw Chang [:kershaw]]

Comment on attachment 9377145 [details]
Bug 1193389 - nsTransportEventSinkProxy clean up, r=#necko

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Unknown. This is a cleanup patch to avoid accessing raw pointers.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• 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?: No
• If not, how different, hard to create, and risky will they be?: No need to backport
• How likely is this patch to cause regressions; how much testing does it need?: Should be low risk.
• Is Android affected?: Yes

Comment 15

[Tom Ritter [:tjr]]

Comment on attachment 9377145 [details]
Bug 1193389 - nsTransportEventSinkProxy clean up, r=#necko

Approved to land and uplift if desired

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/28503602f196
nsTransportEventSinkProxy clean up, r=necko-reviewers,valentin

Comment 17

[Kershaw Chang [:kershaw]]

Let's monitor if the patch helps for 2 months.

Comment 18

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

https://hg.mozilla.org/mozilla-central/rev/28503602f196

Comment 19

[Daniel Veditz [:dveditz]]

This landed at the beginning of Fx125 and so far no crashes yet. But it's extremely low volume: one crash in 120, none in 121 or 122, five crashes in 123.0.1 (from four installs), and one nightly 124.0a1 (but no beta or release crashes).

Comment 20

[Kershaw Chang [:kershaw]]

I've checked again and so far no new crash yet.
Given that the crash rate is really low, I'd suggest to monitor this for another month.

Comment 21

[Kershaw Chang [:kershaw]]

(In reply to Daniel Veditz [:dveditz] from comment #19)

This landed at the beginning of Fx125 and so far no crashes yet. But it's extremely low volume: one crash in 120, none in 121 or 122, five crashes in 123.0.1 (from four installs), and one nightly 124.0a1 (but no beta or release crashes).

It appears there have been no crashes since the patch landed. I think we can close this bug.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for ESR uplift when you get a chance.

Comment 23

[Kershaw Chang [:kershaw]]

Attachment: Bug 1193389 - nsTransportEventSinkProxy clean up, r=#necko

This patch might not fix this issue, but we should try to avoid accessing raw pointer.

Original Revision: https://phabricator.services.mozilla.com/D199976

Comment 24

[Phabricator Automation]

esr115 Uplift Approval Request

• User impact if declined: Possible crash
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: N/A
• Risk associated with taking this patch: Low
• Explanation of risk level: This patch is straightforward.
• String changes made/needed: N/A
• Is Android affected?: yes

Comment 25

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/8b2b8eb760f4

Comment 26

[Tom Ritter [:tjr]]

Attachment: advisory.txt