1878286 - UndefinedBehaviorSanitizer: /gfx/thebes/gfxPlatformFontList.cpp:3108:33: runtime error: applying non-zero offset 5544 to null pointer

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect)

Description

[Christian Holler (:decoder)]

In experimental IPC fuzzing, we found the following crash on mozilla-central revision 20240127-09a1abcfcfb7 (fuzzing-asan-nyx-opt build):

/gfx/thebes/gfxPlatformFontList.cpp:3108:33: runtime error: applying non-zero offset 5544 to null pointer
    #0 0x7fffdc07b794 in gfxPlatformFontList::SetupFamilyCharMap(unsigned int, mozilla::fontlist::Pointer const&) /gfx/thebes/gfxPlatformFontList.cpp:3108:33
    #1 0x7fffe48f065a in mozilla::dom::ContentParent::RecvSetupFamilyCharMap(unsigned int const&, mozilla::fontlist::Pointer const&) /dom/ipc/ContentParent.cpp:5924:13
    #2 0x7fffe4cfc374 in mozilla::dom::PContentParent::OnMessageReceived(IPC::Message const&) /builds/worker/workspace/obj-build/ipc/ipdl/PContentParent.cpp:10926:81
    #3 0x7fffda4d2e96 in mozilla::ipc::MessageChannel::DispatchAsyncMessage(mozilla::ipc::ActorLifecycleProxy*, IPC::Message const&) /ipc/glue/MessageChannel.cpp:1813:25
    #4 0x7fffda4cd736 in mozilla::ipc::MessageChannel::DispatchMessage(mozilla::ipc::ActorLifecycleProxy*, mozilla::UniquePtr<IPC::Message, mozilla::DefaultDelete<IPC::Message>>) /ipc/glue/MessageChannel.cpp:1732:9
    [...]

The attached testcase can be reproduced using a special build to inject IPC messages.

I'm creating a pernosco session now to investigate this, because it isn't totally obvious to me from the source what is happening.

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 2

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 3

[Christian Holler (:decoder)]

Pernosco session: https://pernos.co/debug/jUQCZAo5FME27iRz5Jg0hA/index.html

I looked briefly and it looks like the sample performs 3 calls:

1. ContentParent::RecvSetupFamilyCharMap
2. ContentParent::RecvSetCharacterMap
3. ContentParent::RecvSetupFamilyCharMap again

I confirmed locally that this reproduces just with messages 2 + 3 but not with 3 only.

Comment 4

[Jonathan Kew [:jfkthame]]

It looks to me like the issue here is that gfxPlatformFontList::SetCharacterMap (called by RecvSetCharacterMap) does not validate that the aFacePtr parameter does in fact refer to a font face record within the list. So if a broken/compromised content process sends a message with an invalid aFacePtr, we can end up overwriting some other piece of the font-list data when trying to set the mCharacterMap field.

To enable robust validation of the face pointer, we should probably extend the message to send both family and face parameters; we can check the family similarly to how SetupFamilyCharMap does it, and then use the family's face array to check the validity of the face param.

So that'll mean updating the signature of the message, and plumbing the appropriate family record through from the various callers.

Comment 5

[Jonathan Kew [:jfkthame]]

So the issue here is that we send Family and Face references from the child to parent as fontlist::Pointer, which is fine as long as the values sent are valid -- the receiver can resolve it to a process-local "real" pointer, and perform the requested operation.

But in the presence of a broken or malicious sender, this becomes problematic because it's hard for the receiver to validate that the resulting pointer does in fact refer to the expected type of data. RecvSetupFamilyCharMap goes to some trouble to try and verify this, but RecvSetCharacterMap currently doesn't -- and making it do so would be considerably more hassle (and costly, perf-wise).

A better way forward is to eliminate this use of fontlist::Pointer altogether, and instead refer to the Family and Face records by their indexes in the font-list and in the family's face-list respectively. These can easily be validated by the receiver, so that it can safely ignore bad messages.

Comment 6

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1878286 - Eliminate usage of fontlist::Pointer in IPDL. r=#gfx-reviewers

No change in behavior.

Comment 7

[Jonathan Kew [:jfkthame]]

Comment on attachment 9378122 [details]
Bug 1878286 - Eliminate usage of fontlist::Pointer in IPDL. r=#gfx-reviewers

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Probably not easily, AFAICS. An attacker would first need to compromise a content process so as to send custom malformed IPC messages to the parent. Such messages could then be used to corrupt the shared font-list data managed by the main process. But it's unclear how to leverage this into a further exploit (other than a crash or broken rendering due to unusable fonts).
• 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?: Esr115 would require a simple backport due to refactored code. (Patch applies unchanged to beta.)
• How likely is this patch to cause regressions; how much testing does it need?: Low risk; no user-visible change in behavior.
• Is Android affected?: Yes

Comment 8

[Jonathan Kew [:jfkthame]]

Christian, I'm reasonably confident from what I see in the pernosco session that the above patch should resolve this; but if you have the cycles to confirm locally with your build, that'd be awesome. Thanks!

Comment 9

[Christian Holler (:decoder)]

(In reply to Jonathan Kew [:jfkthame] from comment #8)

Christian, I'm reasonably confident from what I see in the pernosco session that the above patch should resolve this; but if you have the cycles to confirm locally with your build, that'd be awesome. Thanks!

I confirmed locally that the test doesn't reproduce anymore but that is already caused by the fact that this patch changes the method signatures in IPDL. I'll give this another round of fuzzing soon, but please go ahead and land this meanwhile, thanks!

I also believe your fix is the better generic solution to avoid other confusions like this in the future and should make the code overall more solid, much appreciated!

Comment 10

[Tom Ritter [:tjr]]

Comment on attachment 9378122 [details]
Bug 1878286 - Eliminate usage of fontlist::Pointer in IPDL. r=#gfx-reviewers

This is a pretty big bullseye; but release is only 15 days away, so lets land it and make sure it gets uplifted - could you prep the backport patches if this doesn't apply cleanly?

Comment 11

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1878286 - Eliminate usage of fontlist::Pointer in IPDL. r=#gfx-reviewers

No change in behavior.

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

Comment 12

[Phabricator Automation]

Uplift Approval Request

• User impact if declined: Potential vector for sandbox escape
• Code covered by automated testing: yes
• Risk associated with taking this patch: low
• Fix verified in Nightly: no
• String changes made/needed: none
• Is Android affected?: yes
• Steps to reproduce for manual QE testing: n/a
• Explanation of risk level: No behavior change, just revising IPDL messages to be more easily validated
• Needs manual QE test: no

Comment 13

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1878286 - [rebased to esr115] Eliminate usage of fontlist::Pointer in IPDL.

Comment 14

[Jonathan Kew [:jfkthame]]

Comment on attachment 9378490 [details]
Bug 1878286 - [rebased to esr115] Eliminate usage of fontlist::Pointer in IPDL.

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Potential sandbox-escape vector
• User impact if declined: Malicious content process could corrupt font list data managed by the parent process, via IPC messages causing wild-pointer writes
• Fix Landed on Version: 124
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): No change in behavior; revises IPDL parameters to enable the parent to validate them reliably before use.

Comment 15

[Pulsebot]

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

Comment 16

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/13186e278090

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/40fcc0cdc593
Eliminate usage of fontlist::Pointer in IPDL. r=gfx-reviewers,lsalzman

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/40fcc0cdc593