Closed Bug 1667102 (CVE-2021-43535) Opened 2 years ago Closed 11 months ago

Crash in [@ mozilla::net::Http2Stream::TransmitFrame]

Categories

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

Desktop
Windows
defect

Tracking

()

RESOLVED FIXED
94 Branch
Tracking Status
firefox-esr78 --- wontfix
firefox-esr91 94+ fixed
firefox89 --- wontfix
firefox90 --- wontfix
firefox91 + wontfix
firefox92 + wontfix
firefox93 + fixed
firefox94 + fixed

People

(Reporter: jcristau, Assigned: kershaw)

References

Details

(Keywords: crash, sec-high, testcase-wanted, Whiteboard: [necko-triaged][sec-survey][adv-main93+][adv-esr91.3+])

Crash Data

Attachments

(4 files)

Crash report: https://crash-stats.mozilla.org/report/index/c27d1c1c-2530-4f43-9ce4-c4d000200924

Top 10 frames of crashing thread:

0  @0x858481e6 
1  @0xebeacfc6 
2 xul.dll mozilla::net::Http2Stream::TransmitFrame netwerk/protocol/http/Http2Stream.cpp:971
3 xul.dll mozilla::net::Http2Stream::OnReadSegment netwerk/protocol/http/Http2Stream.cpp:1516
4 xul.dll static mozilla::net::nsHttpTransaction::ReadRequestSegment netwerk/protocol/http/nsHttpTransaction.cpp:725
5 xul.dll nsBufferedInputStream::ReadSegments netwerk/base/nsBufferedStreams.cpp:446
6 xul.dll mozilla::net::nsHttpTransaction::ReadSegments netwerk/protocol/http/nsHttpTransaction.cpp:752
7 xul.dll mozilla::net::Http2Stream::ReadSegments netwerk/protocol/http/Http2Stream.cpp:164
8 xul.dll mozilla::net::Http2Session::ReadSegmentsAgain netwerk/protocol/http/Http2Session.cpp:2815
9 xul.dll mozilla::net::nsHttpConnection::OnSocketWritable netwerk/protocol/http/nsHttpConnection.cpp:1993

Most of these crashes seem to be EXCEPTION_ACCESS_VIOLATION_EXEC which might be scary?

_VIOLATION_EXEC is definitely bad, and there's no good reason for http to be calling into unsymbolized memory (unlike the JIT say). Maybe the stream is gone (UAF), and we shouldn't be using a raw pointer here? Maybe uninitialized, with a predictable pattern that leaves the same value behind from whatever frame was there before?

Very interesting and suspicious that most of the crashes are crashing at the same address 0x858481e6
https://crash-stats.mozilla.org/signature/?product=Firefox&signature=mozilla%3A%3Anet%3A%3AHttp2Stream%3A%3ATransmitFrame&date=%3E%3D2020-09-17T17%3A12%3A00.000Z&date=%3C2020-09-24T17%3A12%3A00.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_columns=install_time&_columns=startup_crash&_sort=-date&page=1#reports

Flags: needinfo?(valentin.gosu)

It seems to me something is going very wrong when jumping through a vtable. The address is indeed very suspicious but it could be an artefact caused by whatever issue leads to this crash. There's a few things worthy of note:

  • All the crashes are on 32-bit Windows
  • The vast majority of the crashes with an URL are on YouTube, with Facebook a distant second so this might have something to do with streaming video
  • The user comments don't contain anything useful at all, it seems to happen out-of-the-blue
  • The vast majority of the crashes are coming from machines with 4GiB of memory or less
  • Many crashes have very low available commit space, so these users were running out of memory. Many more reports don't have memory annotations at all which is something that doesn't happen often. It is possible that this bug is caused - or at least made more likely - by low memory scenarios

Another couple of things worthy of note: this seems to be happening mostly on Windows 7 and there appear to be similar crashes with different signatures:
[@ mozilla::net::nsSocketOutputStream::Write]
https://crash-stats.mozilla.org/report/index/3ff460cb-0666-4ce9-bdc3-6e0e00200924
[@ mozilla::net::nsHttpConnection::OnReadSegment]
https://crash-stats.mozilla.org/report/index/31ab8728-d339-441a-932d-8aa580200924
The stacks for both signatures are eerily similar to this one

Severity: -- → S3
Priority: -- → P1
Assignee: nobody → valentin.gosu
Flags: needinfo?(valentin.gosu)
Whiteboard: [necko-triaged]

I think it's a dupe of bug 1402014 - let's wait for that to land and see if the crashes here go away.

Depends on: 1402014
See Also: 1402014

I have landed:
Bug 1707078
Bug 1705065

Both bugs remove use of raw pointer in the code that is related to this crash. The fixes may help with this crash, but we need to wait and see.

So Bug 1402014 has been re-opened, the patch did not fix the problem, it seems.
Bug 1707078 is fixed in 90, which is our current beta.
Bug 1705065 is not yet fixed.

We should probably un-stall bug 1402014 somehow to move this forward?

Flags: needinfo?(valentin.gosu)

Bug 1705065 is now fixed in 91.
We probably need it to get to beta to know if it worked or not.
Otherwise, we can work either here or in bug 1402014, but the solution is likely a larger refactoring of the code.

Flags: needinfo?(valentin.gosu)

From crashes it looks like Http2Session is destroyed, therefore let's hold refernce to the session.
The pattch alos aadd some diagnostic assertions to make sure that Htttp2Stream holds pointer to the correct (expeced) session.

See Also: → 1711738

Comment on attachment 9228354 [details]
Hold reference to Http2Session during ReadSegment call

Security Approval Request

  • How easily could an exploit be constructed based on the patch?: The patch add some diagnostic asserts and changes a raw pointer to a refPtr. The change tries to guess what is the cause of the bug. The patch does not reveals more that the crash reports already show.
  • 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, this is an old bug
  • If not all supported branches, which bug introduced the flaw?: None
  • Do you have backports for the affected branches?: Yes
  • If not, how different, hard to create, and risky will they be?: The patch should apply cleanly to older branches
  • How likely is this patch to cause regressions; how much testing does it need?: Very unlikely. It only add diagnostic asserts a RefPtr.
Attachment #9228354 - Flags: sec-approval?

Comment on attachment 9228354 [details]
Hold reference to Http2Session during ReadSegment call

sec-approval = dveditz

Attachment #9228354 - Flags: sec-approval? → sec-approval+
Assignee: valentin.gosu → dd.mozilla

I cannot land this his patch in lando, i says that I do not have permissions. Can you land it for me?

Flags: needinfo?(jcristau)

Hold reference to Http2Session during ReadSegment call r=necko-reviewers,valentin
https://hg.mozilla.org/integration/autoland/rev/568cde9bc686ce4ae172331ca1defa25263b5e59

Flags: needinfo?(jcristau)
Group: network-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 91 Branch

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.

Flags: needinfo?(dd.mozilla)
Whiteboard: [necko-triaged] → [necko-triaged][sec-survey]

We're still seeing crashes with this signature in 91.0b1 (i.e. with the fix), e.g. bp-55492aa4-762d-429a-a2de-f6af50210714

QA Whiteboard: [post-critsmash-triage]
Flags: qe-verify-

I forgot to set "leave-open" flag.

Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Dragana, given that 91 is still marked as affected after the patch, do we need to uplift what already landed in the 91 cycle to esr78 or can we delay that to the next 78.x release? Thanks

The patch has no fix he issue. We do not need o uplift it.

Flags: needinfo?(dd.mozilla)
Status: REOPENED → NEW
Target Milestone: 91 Branch → ---

Too late to address this for the current cycle.

Comment on attachment 9241062 [details]
Bug 1667102 - Make sure nsHttpTransaction::mConnection is released on socket thread, r=#necko

Security Approval Request

  • How easily could an exploit be constructed based on the patch?: Not easy, since this patch assumes this crash is caused by a race condition that Http2Session is released on main thread.
  • 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?: None
  • Do you have backports for the affected branches?: Yes
  • If not, how different, hard to create, and risky will they be?: The patch should apply cleanly to older branches.
  • How likely is this patch to cause regressions; how much testing does it need?: Should be low, since this patch only changes the behavior a bit. With this patch, we access Http2Session via a nsWeakPtr, not a raw pointer.
    If an Http2Session is released before accessing, Firefox will be crashed.
Attachment #9241062 - Flags: sec-approval?
Attachment #9239782 - Flags: sec-approval?

Comment on attachment 9241062 [details]
Bug 1667102 - Make sure nsHttpTransaction::mConnection is released on socket thread, r=#necko

Approved to land and request uplift

Attachment #9241062 - Flags: sec-approval? → sec-approval+
Attachment #9239782 - Flags: sec-approval? → sec-approval+
Status: NEW → RESOLVED
Closed: 1 year ago1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 94 Branch

Dragana, do you want to request uplift? Thanks

Flags: needinfo?(dd.mozilla)

(In reply to Pascal Chevrel:pascalc from comment #26)

Dragana, do you want to request uplift? Thanks

Sorry, I should do this.

Assignee: dd.mozilla → kershaw
Flags: needinfo?(dd.mozilla)

Comment on attachment 9241062 [details]
Bug 1667102 - Make sure nsHttpTransaction::mConnection is released on socket thread, r=#necko

Beta/Release Uplift Approval Request

  • User impact if declined: Could be crashed.
  • Is this code covered by automated tests?: Yes
  • 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): Should be low.
    The patch only changed the raw pointer access to a weak pointer and not changes others.
  • String changes made/needed: N/A
Attachment #9241062 - Flags: approval-mozilla-beta?
Attachment #9239782 - Flags: approval-mozilla-beta?

Does it a also need uplifting to 78 and 91 ESR?

Flags: needinfo?(kershaw)

(In reply to Pascal Chevrel:pascalc from comment #29)

Does it a also need uplifting to 78 and 91 ESR?

Probably not, since I am not entirely sure my patches fix the crash.
I'd like to wait a few more cycles.

Flags: needinfo?(kershaw)

Since we don't uplift in 78 ESR this cycle, I am marking 78 as wontfix as this ESR branch will be EOL with 94.

Comment on attachment 9241062 [details]
Bug 1667102 - Make sure nsHttpTransaction::mConnection is released on socket thread, r=#necko

Approved for 93 beta 8, thanks.

Attachment #9241062 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9239782 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Whiteboard: [necko-triaged][sec-survey] → [necko-triaged][sec-survey][adv-main93+r]

(In reply to Kershaw Chang [:kershaw] from comment #30)

Does it a also need uplifting to 78 and 91 ESR?

Probably not, since I am not entirely sure my patches fix the crash.
I'd like to wait a few more cycles.

I'll set this to be tracked for ESR 91.3 ("94+") for now. As we get to the end of it we can see if you still need more evidence, and push it out further if necessary.

Blocks: 1402014
No longer depends on: 1402014

This is looking great on beta/release. I think we're good for an ESR uplift request.

Flags: needinfo?(kershaw)

Comment on attachment 9241062 [details]
Bug 1667102 - Make sure nsHttpTransaction::mConnection is released on socket thread, r=#necko

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is a sec-high bug.
  • User impact if declined: Firefox could crash.
  • Fix Landed on Version: 94
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): This patch is already verified for a few cycles.
  • String or UUID changes made by this patch: N/A
Flags: needinfo?(kershaw)
Attachment #9241062 - Flags: approval-mozilla-esr91?
Attachment #9239782 - Flags: approval-mozilla-esr91?

Comment on attachment 9239782 [details]
Bug 1667102 - Add some diagnostic assertions, r=#necko

Approved for 91.3esr, thanks.

Attachment #9239782 - Flags: approval-mozilla-esr91? → approval-mozilla-esr91+
Attachment #9241062 - Flags: approval-mozilla-esr91? → approval-mozilla-esr91+
Whiteboard: [necko-triaged][sec-survey][adv-main93+r] → [necko-triaged][sec-survey][adv-main93+r][adv-esr91.3+r]
Attached file advisory.txt

Due to this landed in different versions, it's being retroactively assigned an individual CVE. (Which we don't know yet, so it's being given a temporary identifier.) The appropriate advisories will be updated.

Whiteboard: [necko-triaged][sec-survey][adv-main93+r][adv-esr91.3+r] → [necko-triaged][sec-survey][adv-main93+][adv-esr91.3+]
Blocks: 1574971

There are still some crashes on 93, so reopen this bug.
I'll keep working on this.

Status: RESOLVED → REOPENED
Resolution: FIXED → ---

This signature seems related.

Crash Signature: [@ mozilla::net::Http2Stream::TransmitFrame] → [@ mozilla::net::Http2Stream::TransmitFrame] [@ mozilla::net::Http2Stream::ParseHttpRequestHeaders ]

Can we spin any future work to a new bug? This has already had advisories issued for it so tracking is going to get ugly if we keep doing more here.

Flags: needinfo?(kershaw)

(In reply to Ryan VanderMeulen [:RyanVM] from comment #43)

Can we spin any future work to a new bug? This has already had advisories issued for it so tracking is going to get ugly if we keep doing more here.

Sure. I'll close this and file a new one.

Status: REOPENED → RESOLVED
Closed: 1 year ago11 months ago
Flags: needinfo?(kershaw)
Resolution: --- → FIXED
Blocks: 1740274
Alias: CVE-2021-43535
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.