Closed Bug 1820389 Opened 1 year ago Closed 1 year ago

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


(Core :: Disability Access APIs, defect)




113 Branch
Tracking Status
firefox-esr102 112+ fixed
firefox111 --- wontfix
firefox112 + fixed
firefox113 + fixed


(Reporter: decoder, Assigned: Jamie)



(4 keywords, Whiteboard: [post-critsmash-triage][adv-main112+r][adv-esr102.10+r])


(2 files)

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:

The roleMapIndex comes from here

and AddSubtree is called here 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 ?

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

Severity: -- → S3

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.

Group: core-security → layout-core-security

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.

Flags: needinfo?(jteh)
Severity: S3 → S2
Flags: needinfo?(jteh)
Assignee: nobody → jteh

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

Flags: needinfo?(jteh)

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

Flags: needinfo?(jteh) → needinfo?(choller)

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.

Flags: needinfo?(choller)

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

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
Attachment #9325094 - Flags: sec-approval?

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

Approved to land, relman indicated they would take this

Attachment #9325094 - Flags: sec-approval? → sec-approval+

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

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.
Flags: needinfo?(jteh)
Attachment #9325094 - Flags: approval-mozilla-esr102?
Attachment #9325094 - Flags: approval-mozilla-beta?

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 on attachment 9325094 [details]
Bug 1820389: Gracefully handle unknown ARIA roles.

Approved for 112.0rc1 and 102.10esr

Attachment #9325094 - Flags: approval-mozilla-release+
Attachment #9325094 - Flags: approval-mozilla-esr102?
Attachment #9325094 - Flags: approval-mozilla-esr102+
Attachment #9325094 - Flags: approval-mozilla-beta?
Group: layout-core-security → core-security-release
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 113 Branch
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main112+r]
Whiteboard: [post-critsmash-triage][adv-main112+r] → [post-critsmash-triage][adv-main112+r][adv-esr102.10+r]
See Also: → 1829249
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.