1699835 - (CVE-2021-23995) AddressSanitizer: heap-use-after-free [@ EntryCount] using toggle responsive design mode (ctrl+shift+m)

Status: VERIFIED FIXED

(Core :: Graphics: Text, defect)

Description

[Irvan Kurniawan (:sourc7)]

Attachment: testcase.html

After visit the testcase, when toggling the responsive design mode, the tab crashed with summary: AddressSanitizer: heap-use-after-free /builds/worker/workspace/obj-build/dist/include/PLDHashTable.h:434:40 in EntryCount.

Affected version:

• Firefox Nightly 88.0a1 (2021-03-19) (64-bit)
• Firefox Release 86.0.1 (64-bit)
• Firefox ESR 78.8.0esr (64-bit)

Steps to Reproduce:

1. Visit attached testcase.html
2. Press Ctrl + Shift + M to switch to Responsive Design Mode
3. After ~5 seconds the page will reload itself
4. After the page reload, press Ctrl + Shift + M to exit the Responsive Design Mode
5. The tab crashed

Comment 1

[Irvan Kurniawan (:sourc7)]

Attachment: asan.txt

Comment 2

[Irvan Kurniawan (:sourc7)]

Attachment: Firefox - UAF Canvas2D PLDHashTable.mkv

Comment 3

[:Gijs (he/him)]

Moving to gfx given the stack trace suggesting it's a hashtable accessed from #3 0x7f9ebfb82930 in gfxFontGroup::BuildFontList() /builds/worker/checkouts/gecko/gfx/thebes/gfxTextRun.cpp:1898:44

Comment 4

[Andrew McCreight [:mccr8]]

The free and allocation stacks are the same as in 1700038 (something in the FontMatchingStats ctor) so it might be a dupe. It looks like gfxFontGroup has a weak reference to a FontMatchingStats, mFontMatchingStats, and the use stacks in both bugs look like they are accessing that field.

Comment 5

[Jonathan Kew [:jfkthame]]

The FontMatchingStats belongs to the presContext, and a weak reference gets passed on to the nsFontMetrics and hence gfxFontGroup instances that it creates. The assumption here is that the presContext will outlive these objects that it's using to do its layout. But then we have the nsFontCache, which caches nsFontMetrics objects so that we don't always have to create them afresh. And nsFontCache does not check that the instance it returns refers to the same FontMatchingStats as its caller intends to use.

I think what's happening here, then, is that when responsive design mode is toggled, a new presContext is created that uses the same nsDeviceContext and hence nsFontCache, so it gets already-existing nsFontMetrics instances from the cache; but these refer to the previous context's FontMatchingStats. That previous context is gone, and so those references are not valid.

The simple fix, assuming this is roughly what's happening, will be to include fontStats in the fields that nsFontCache::GetMetricsFor checks before returning an existing cache entry. I'll see if I can confirm this with a local ASAN build.

Comment 6

[Jonathan Kew [:jfkthame]]

Hmm, so that's not sufficient to fix it. Need to look a bit deeper...

Comment 7

[Jonathan Kew [:jfkthame]]

The problem here is indeed a gfxFontGroup with a FontMatchingStats reference that came from a now-deleted presContext, but the font group isn't coming from the nsFontCache as I first suspected. (However, I still intend to add a check there, as it looks like a potential source of a similar bug even though it's not what this testcase hits.)

In this case, though, the problem comes from CanvasRenderingContext2D, which holds on to gfxFontGroup references in its ContextState stack. We need to check that the FontMatchingStats reference (and similarly TextPerfMetrics, actually) is still valid, and if not, recreate the gfxFontGroup when it is accessed via GetCurrentFontStyle().

Comment 8

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1699835 - Check that references in cached fontGroup match current presContext. r=lsalzman

Comment 10

[Andrew McCreight [:mccr8]]

I marked this bug sec-high, but this does require some slightly weird user interaction which does mitigate it a bit.

Comment 11

[Jonathan Kew [:jfkthame]]

I agree the kind of interaction required by the STR here seems to offer some mitigation; however, I think there's a fair chance there could be other situations, possibly not requiring user interaction, that end up hitting the same underlying problem.

Comment 12

[Jonathan Kew [:jfkthame]]

Comment on attachment 9211258 [details]
Bug 1699835 - Check that references in cached fontGroup match current presContext. r=lsalzman

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Unclear; the flaw itself (cached fontgroup refers to a now-deleted prescontext) is fairly easy to understand from the code change, but currently the only known STR to hit this issue depends on relatively obscure user interaction which mitigates any potential exploit.
• 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?: No
• If not, how different, hard to create, and risky will they be?: Backports should be trivial.
• How likely is this patch to cause regressions; how much testing does it need?: Low risk, patch just validates/refreshes the font style object before using it.

Comment 13

[Tom Ritter [:tjr] (OOTO until mid-August)]

Comment on attachment 9211258 [details]
Bug 1699835 - Check that references in cached fontGroup match current presContext. r=lsalzman

Approved to land and uplift

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/autoland/rev/6a4e853e5e7a203e4815bd7f6de9770ee0beae07

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a29179f9160a

This'll need rebasing for ESR78, however.

Comment 16

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

https://hg.mozilla.org/mozilla-central/rev/6a4e853e5e7a

Comment 17

[Jonathan Kew [:jfkthame]]

Attachment: Bug1699835-esr78.patch

Patch rebased to current esr78 tip.

Comment 18

[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 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/106fff71bae2

Comment 20

[Tom Ritter [:tjr] (OOTO until mid-August)]

Attachment: advisory.txt