1820389 - AddressSanitizer: global-buffer-overflow [@ nsRoleMapEntry::IsOfType] with READ of size 4

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Christian Holler (:decoder)]

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

=================================================================
==1960==ERROR: AddressSanitizer: global-buffer-overflow on address 0x7ffff0933c24 at pc 0x7fffe589adfa bp 0x7ffffffd9ba0 sp 0x7ffffffd9b98
READ of size 4 at 0x7ffff0933c24 thread T0
    #0 0x7fffe589adf9 in nsRoleMapEntry::IsOfType(mozilla::a11y::AccGenericType) const accessible/base/ARIAMap.h:152:12
    #1 0x7fffe58da343 in mozilla::a11y::Accessible::HasGenericType(mozilla::a11y::AccGenericType) const accessible/basetypes/Accessible.cpp:110:41
    #2 0x7fffe577240d in mozilla::a11y::Accessible::IsHyperText() const objdir-ff-aflpp/dist/include/mozilla/a11y/Accessible.h:477:37
    #3 0x7fffe576b5ae in GetInterfacesForProxy(mozilla::a11y::RemoteAccessible*) accessible/atk/AccessibleWrap.cpp:956:15
    #4 0x7fffe576b418 in mozilla::a11y::ProxyCreated(mozilla::a11y::RemoteAccessible*) accessible/atk/AccessibleWrap.cpp:997:30
    #5 0x7fffe59dc031 in mozilla::a11y::DocAccessibleParent::AddSubtree(mozilla::a11y::RemoteAccessible*, nsTArray<mozilla::a11y::AccessibleData> const&, unsigned int, unsigned int) accessible/ipc/DocAccessibleParent.cpp:197:5
    #6 0x7fffe59dafb4 in mozilla::a11y::DocAccessibleParent::RecvShowEvent(mozilla::a11y::ShowEventData const&, bool const&) accessible/ipc/DocAccessibleParent.cpp:133:23
    #7 0x7fffe59dca20 in non-virtual thunk to mozilla::a11y::DocAccessibleParent::RecvShowEvent(mozilla::a11y::ShowEventData const&, bool const&) accessible/ipc/DocAccessibleParent.cpp:0:0
    #8 0x7fffe5b1e147 in mozilla::a11y::PDocAccessibleParent::OnMessageReceived(IPC::Message const&) objdir-ff-aflpp/ipc/ipdl/PDocAccessibleParent.cpp:9386:52
    #9 0x7fffda9eff7a in mozilla::dom::PContentParent::OnMessageReceived(IPC::Message const&) objdir-ff-aflpp/ipc/ipdl/PContentParent.cpp:6661:32
    #10 0x7fffcd8540e6 in mozilla::ipc::MessageChannel::DispatchAsyncMessage(mozilla::ipc::ActorLifecycleProxy*, IPC::Message const&) ipc/glue/MessageChannel.cpp:1800:25
    [...]
    #33 0x5555557ea39a in main browser/app/nsBrowserApp.cpp:423:16

This local fuzzing crash is the result of new experimental setups with the IPC fuzzer.

On the experiments we are performing here is targeted fuzzing of a specific actor/method, in this case we are targeting mozilla::a11y::DocAccessibleParent::RecvShowEvent by loading a Mochitest into the fuzzing VM that triggers this method, then snapshot and start fuzzing.

Because of this experimental setup, we cannot provide a testcase yet. But I did some preliminary investigation:

It looks like we are creating a RemoteAccessible here: https://searchfox.org/mozilla-central/rev/7bb059f2e81636e0db20acbf2afe67d49508489b/accessible/ipc/DocAccessibleParent.cpp#200

The roleMapIndex comes from here https://searchfox.org/mozilla-central/rev/7bb059f2e81636e0db20acbf2afe67d49508489b/accessible/ipc/DocAccessibleParent.cpp#189

and AddSubtree is called here https://searchfox.org/mozilla-central/rev/7bb059f2e81636e0db20acbf2afe67d49508489b/accessible/ipc/DocAccessibleParent.cpp#133 in mozilla::a11y::DocAccessibleParent::RecvShowEvent.

Could it be that we just send this index through IPC without bounds-checking it, and then we go out of bounds later in nsRoleMapEntry::IsOfType ?

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 2

[James Teh [:Jamie]]

Yeah, I'd say it's very likely a missing bounds check. We should enforce that before we create the RemoteAccessible.

Comment 3

[Christian Holler (:decoder)]

I also suggest to perform a careful audit of this API. We are passing in untrusted AccessibleData which contains much more than just the roleMapIndex. We should revisit the code with the explicit assumption that all of this data is malicious.

The impact of this specific bug is also not fully clear to me yet. Being able to spoof a different type here through nsRoleMapEntry::IsOfType might lead to other issues later on that require a more complex testcase but might actually grant oob write or worse. So this might well be a sec-high, in which case it should be elevated to S2.

Comment 4

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

The severity field for this bug is set to S3. However, the bug is flagged with the sec-high keyword.
:Jamie, could you consider increasing the severity of this security bug?

For more information, please visit auto_nag documentation.

Comment 5

[James Teh [:Jamie]]

Attachment: Bug 1820389: Gracefully handle unknown ARIA roles.

Comment 6

[Dianna Smith [:diannaS]]

Is the plan to uplift this to 112, or is riding the 113 train?

Comment 7

[James Teh [:Jamie]]

:decoder, do you think this needs an uplift? I'm not really sure how to assess the severity of IPC exploits as far as whether a beta/ESR uplift is required.

Comment 8

[Christian Holler (:decoder)]

I recommend uplifting, if you think the risk of breakage is reasonably low. IPC exploits in general are to be considered severe, though it is unclear how severe this particular instance is. However, in doubt, we should err on the side of caution and fix this if esp. if it is a smaller fix.

Comment 9

[James Teh [:Jamie]]

In that case, :diannaS, I intend to uplift this if we get the review comments sorted out in time.

Comment 10

[James Teh [:Jamie]]

Comment on attachment 9325094 [details]
Bug 1820389: Gracefully handle unknown ARIA roles.

Security Approval Request

• How easily could an exploit be constructed based on the patch?: The addition of a validity check does hint at a bounds issue in the handling of an IPDL message. I'm not sure how easy it'd be to exploit that, though.
• 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?: beta, ESR
• 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?: Should apply cleanly or be easy to create.
• How likely is this patch to cause regressions; how much testing does it need?: Unlikely; just a bounds check.
• Is Android affected?: Yes

Comment 11

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Comment on attachment 9325094 [details]
Bug 1820389: Gracefully handle unknown ARIA roles.

Approved to land, relman indicated they would take this

Comment 12

[Atila Butkovits]

Backed out for causing build bustages at DocAccessibleParent.cpp

Backout link: https://hg.mozilla.org/integration/autoland/rev/f82efcc9e720e3e1670e7897604a0b8c67c2a4a8

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=2a1fc04f9a8cee1868ccd5096ea89317578053eb&selectedTaskRun=NBdlH-nHTvemx6mB4mBMDA.0

Failure log: https://treeherder.mozilla.org/logviewer?job_id=410961506&repo=autoland&lineNumber=6817

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Hybrid builds notably are non-unified. Missing include most likely?

Comment 14

[James Teh [:Jamie]]

Comment on attachment 9325094 [details]
Bug 1820389: Gracefully handle unknown ARIA roles.

Beta/Release Uplift Approval Request

• User impact if declined: Crash, potential IPC bounds exploit.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Just validates an input parameter.
• String changes made/needed:
• Is Android affected?: Yes

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration:
• User impact if declined: Crash, potential IPC bounds exploit.
• Fix Landed on Version: 113 shortly
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Just validates an input parameter.

Comment 15

[James Teh [:Jamie]]

Note that I'm requesting beta/ESR approval before this fully lands on central as asked by RyanVM due to the proximity to rc.

Comment 16

[Dianna Smith [:diannaS]]

Comment on attachment 9325094 [details]
Bug 1820389: Gracefully handle unknown ARIA roles.

Approved for 112.0rc1 and 102.10esr

Comment 17

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

Gracefully handle unknown ARIA roles. r=eeejay,decoder
https://hg.mozilla.org/integration/autoland/rev/d0c10cfe124c46f355177d08196f1e04e437140b
https://hg.mozilla.org/mozilla-central/rev/d0c10cfe124c

Comment 18

[Dianna Smith [:diannaS]]

https://hg.mozilla.org/releases/mozilla-esr102/rev/737a5c36e0f9
https://hg.mozilla.org/releases/mozilla-release/rev/d038fe678958