1667102 - (CVE-2021-43535) Crash in [@ mozilla::net::Http2Stream::TransmitFrame]

Status: RESOLVED FIXED

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

Description

[Julien Cristau [:jcristau]]

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?

Comment 1

[Daniel Veditz [:dveditz]]

_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

Comment 2

[Gabriele Svelto [:gsvelto]]

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

Comment 3

[Gabriele Svelto [:gsvelto]]

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

Comment 4

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

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

Comment 5

[Dragana Damjanovic [:dragana]]

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.

Comment 6

[Jens Stutte [:jstutte]]

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?

Comment 7

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

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.

Comment 8

[Dragana Damjanovic [:dragana]]

Attachment: Hold reference to Http2Session during ReadSegment call

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.

Comment 9

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=684a0e2dd1e07861a98ec6660d557a06c16dcf18

Comment 10

[Dragana Damjanovic [:dragana]]

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.

Comment 11

[Daniel Veditz [:dveditz]]

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

sec-approval = dveditz

Comment 12

[Dragana Damjanovic [:dragana]]

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

Comment 13

[Julien Cristau [:jcristau]]

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

Comment 14

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

https://hg.mozilla.org/mozilla-central/rev/568cde9bc686

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

[Julien Cristau [:jcristau]]

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

Comment 17

[Dragana Damjanovic [:dragana]]

I forgot to set "leave-open" flag.

Comment 18

[Pascal Chevrel:pascalc]

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

Comment 19

[Dragana Damjanovic [:dragana]]

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

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Too late to address this for the current cycle.

Comment 21

[Kershaw Chang [:kershaw]]

Attachment: Bug 1667102 - Add some diagnostic assertions, r=#necko

Comment 22

[Kershaw Chang [:kershaw]]

Attachment: Bug 1667102 - Make sure nsHttpTransaction::mConnection is released on socket thread, r=#necko

Depends on D124753

Comment 23

[Kershaw Chang [:kershaw]]

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.

Comment 24

[Tom Ritter [:tjr]]

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

Approved to land and request uplift

Comment 25

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

Add some diagnostic assertions, r=necko-reviewers,dragana
https://hg.mozilla.org/integration/autoland/rev/b933ec9d0dc387d348229cf8d2d07e82103972ea
Make sure nsHttpTransaction::mConnection is released on socket thread, r=necko-reviewers,dragana
https://hg.mozilla.org/integration/autoland/rev/624ecfecb03527c8f36daf59e48bfa9118647919

https://hg.mozilla.org/mozilla-central/rev/b933ec9d0dc3
https://hg.mozilla.org/mozilla-central/rev/624ecfecb035

Comment 26

[Pascal Chevrel:pascalc]

Dragana, do you want to request uplift? Thanks

Comment 27

[Kershaw Chang [:kershaw]]

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

Dragana, do you want to request uplift? Thanks

Sorry, I should do this.

Comment 28

[Kershaw Chang [:kershaw]]

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

Comment 29

[Pascal Chevrel:pascalc]

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

Comment 30

[Kershaw Chang [: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.

Comment 31

[Pascal Chevrel:pascalc]

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 32

[Pascal Chevrel:pascalc]

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

Approved for 93 beta 8, thanks.

Comment 33

[Pascal Chevrel:pascalc]

https://hg.mozilla.org/releases/mozilla-beta/rev/9f8eed9137f0
https://hg.mozilla.org/releases/mozilla-beta/rev/a4237f33cc63

Comment 34

[Daniel Veditz [:dveditz]]

(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.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

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

Comment 36

[Kershaw Chang [: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

Comment 37

[Ryan VanderMeulen [:RyanVM]]

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

Approved for 91.3esr, thanks.

Comment 38

[Dianna Smith [:diannaS]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/fff749a15d5b
https://hg.mozilla.org/releases/mozilla-esr91/rev/c1741d6eb4f0

Comment 39

[Tom Ritter [:tjr]]

Attachment: advisory.txt

Comment 40

[Tom Ritter [:tjr]]

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.

Comment 41

[Kershaw Chang [:kershaw]]

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

Comment 42

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

This signature seems related.

Comment 43

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 44

[Kershaw Chang [: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.