1777588 - (CVE-2023-1945) Crash in [@ nsTArray_Impl<T>::~nsTArray_Impl | nsUrlClassifierPrefixSet::~nsUrlClassifierPrefixSet]

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect, P2)

Description

[Gabriele Svelto [:gsvelto]]

Crash report: https://crash-stats.mozilla.org/report/index/b24d3d80-faa9-41ef-ae83-1b9f90220630

Reason: EXCEPTION_ACCESS_VIOLATION_READ

Top 10 frames of crashing thread:

0 xul.dll nsTArray_Impl<DXGI_OUTPUT_DESC1, nsTArrayInfallibleAllocator>::~nsTArray_Impl xpcom/ds/nsTArray.h:1030
1 xul.dll nsUrlClassifierPrefixSet::~nsUrlClassifierPrefixSet toolkit/components/url-classifier/nsUrlClassifierPrefixSet.cpp:39
2 xul.dll nsUrlClassifierPrefixSet::Release toolkit/components/url-classifier/nsUrlClassifierPrefixSet.cpp:27
3 xul.dll mozilla::safebrowsing::VariableLengthPrefixSet::Release toolkit/components/url-classifier/VariableLengthPrefixSet.cpp:41
4 xul.dll mozilla::safebrowsing::LookupCacheV2::~LookupCacheV2 toolkit/components/url-classifier/LookupCache.h:332
5 xul.dll nsTArray_Impl<RefPtr<mozilla::safebrowsing::LookupCache>, nsTArrayInfallibleAllocator>::Clear xpcom/ds/nsTArray.h:1934
6 xul.dll mozilla::safebrowsing::Classifier::RemoveUpdateIntermediaries toolkit/components/url-classifier/Classifier.cpp:594
7 xul.dll mozilla::safebrowsing::Classifier::SwapInNewTablesAndCleanup toolkit/components/url-classifier/Classifier.cpp:689
8 xul.dll mozilla::detail::RunnableFunction<`lambda at /builds/worker/checkouts/gecko/toolkit/components/url-classifier/Classifier.cpp:772:13'>::Run xpcom/threads/nsThreadUtils.h:531
9 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1174

Putting this in the networking component but I'm not sure if it's the right one, so feel free to move it where appropriate.

This bug is bad news. It seems that we're accessing a dead object when destroying a nsUrlClassifierPrefixSet instance. Crashes appear to be either use-after-free accesses, double-frees or wild pointer accesses, presumably because the memory we pointed to was already allocated to another object.

I don't know what's the purpose of this code but given it has URL in the name and it's happening in the main process I guess this might be very dangerous if an attacker can trigger it somehow.

Comment 1

[Gabriele Svelto [:gsvelto]]

More signatures.

Comment 2

[Dimi Lee [:dimi]]

(In reply to Gabriele Svelto [:gsvelto] from comment #0)

I don't know what's the purpose of this code but given it has URL in the name and it's happening in the main process I guess this might be very dangerous if an attacker can trigger it somehow.

The code path is triggered by Safe Browsing update, and it's input is coming either from data provided by Google or data provided by us, so theoretically, this shouldn't be something can be triggered by an attacker.

From the crash reports, it seems this crash signature starts to occur after Fx101. Safe Browsing code hasn't changed for quite a long time, and I didn't see any suspicious changeset in 101 (most of the changes are cleanup, testcase refactoring, etc).
If I use nsUrlClassifierPrefixSet::~nsUrlClassifierPrefixSet to search crash signature, I can also see a few crash reports that were reported in Android, so this issue is probably not because of platform dependent changes.

Comment 3

[Gabriele Svelto [:gsvelto]]

It is possible that the bug was already present but living under a different signature (see this one which might be related). That being said it's also possible that some recent change made it spike.

Comment 4

[Dimi Lee [:dimi]]

oops, my bad! Please ignore my previous comment regarding

From the crash reports, it seems this crash signature starts to occur after Fx101`.

I totally forgot that the search result only show report from a week ago...

Comment 5

[Dimi Lee [:dimi]]

(In reply to Dimi Lee [:dimi][:dlee] from comment #2)

(In reply to Gabriele Svelto [:gsvelto] from comment #0)

I don't know what's the purpose of this code but given it has URL in the name and it's happening in the main process I guess this might be very dangerous if an attacker can trigger it somehow.

The code path is triggered by Safe Browsing update, and it's input is coming either from data provided by Google or data provided by us, so theoretically, this shouldn't be something can be triggered by an attacker.

Hi Dan, just want to confirm. Given that this crash cannot be triggered by users (SafeBrowsing update code path and data is from sb list provider), do we still consider this issue as sec-high?

Comment 6

[Daniel Veditz [:dveditz]]

I totally forgot that the search result only show report from a week ago...

... by defaul. You can adjust that by clicking on the "Show Filters" twistie in the upper right.

Given that this crash cannot be triggered by users [...], do we still consider this issue as sec-high?

We shouldn't totally discount the possibility that our trusted SafeBrowsing providers could get hacked, and we know a large number of users are running under a MITM that could be malicious. But yeah, we can lower the rating. The former is an unlikely way to target Firefox users, and the latter likely involved malware on the local machine.

Comment 7

[Gabriele Svelto [:gsvelto]]

I found another signature, this one is a double-free.

Comment 8

[Dimi Lee [:dimi]]

Attachment: Bug 1777588 - Init variable length prefix set in constructor r=timhuang

Comment 9

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

Init variable length prefix set in constructor r=timhuang
https://hg.mozilla.org/integration/autoland/rev/3d45345ab447c451dbe57133268e56b11fcd4fdc
https://hg.mozilla.org/mozilla-central/rev/3d45345ab447

Comment 10

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

The leave-open keyword is there and there is no activity for 6 months.
:dimi, maybe it's time to close this bug?
For more information, please visit auto_nag documentation.

Comment 11

[Dimi Lee [:dimi]]

I think this is fixed

Comment 12

[Daniel Veditz [:dveditz]]

[Tracking Requested - why for this release]:
This patch seems to have worked: it landed in 106, and there are no crashes in versions > 105.x. This still affects ESR-102 and it's a small and safe fix for a security bug. On the other hand the volume is very low (avg 1 crash a day on esr) and it's rated sec-moderate because it's super unreliable and only indirectly triggerable by web content.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for ESR102 approval when you get a chance.

Comment 14

[Dimi Lee [:dimi]]

Comment on attachment 9293452 [details]
Bug 1777588 - Init variable length prefix set in constructor r=timhuang

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration:
• User impact if declined: Firefox might crash when SafeBrowsing runs update in the background
• Fix Landed on Version: 106
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The patch doesn't change any logic or behavior. The patch simply inits a memeber variable (nullptr) in the constructor.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9293452 [details]
Bug 1777588 - Init variable length prefix set in constructor r=timhuang

Approved for 102.10esr.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr102/rev/617c262d3b6f

Comment 17

[Tom Ritter [:tjr]]

Attachment: advisory.txt