1449530 - IPC: global-buffer-overflow crash with PDocAccessible::Msg_StateChangeEvent [@FireStateChangeEvent]

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Christoph Diehl [:posidron]]

Attachment: faulty.txt

The following message was identified to be responsible for this crash and got blacklisted from fuzzing until fixed.

Message: PDocAccessible::Msg_StateChangeEvent

$ hexdump -C /tmp/faulty/message.29834.502
00000000  14 00 00 00 f6 ff ff ff  05 00 32 00 01 00 00 00  |..........2.....|
00000010  00 00 00 00 ff ff ff ff  ff ff ff ff 00 00 00 00  |................|
00000020  00 00 00 00 00 00 00 00  00 00 00 00 04 00 00 00  |................|
00000030  01 00 00 00                                       |....|
$ hexdump -C /tmp/faulty/message.29834.504
00000000  14 00 00 00 f6 ff ff ff  05 00 32 00 01 00 00 00  |..........2.....|
00000010  00 00 00 00 ff ff ff ff  ff ff ff ff 00 00 00 00  |................|
00000020  00 00 00 00 00 00 00 00  00 08 00 00 aa f1 2f 00  |............../.|


==29735==ERROR: AddressSanitizer: global-buffer-overflow on address 0x7f33813484a8 at pc 0x7f337c4dbeba bp 0x7ffdf2002a00 sp 0x7ffdf20029f8
READ of size 4 at 0x7f33813484a8 thread T0
    #0 0x7f337c4dbeb9 in FireStateChangeEvent accessible/atk/AccessibleWrap.cpp:1556:69
    #1 0x7f337c4dbeb9 in mozilla::a11y::ProxyStateChangeEvent(mozilla::a11y::ProxyAccessible*, unsigned long, bool) accessible/atk/AccessibleWrap.cpp:1529
    #2 0x7f337c610feb in mozilla::a11y::DocAccessibleParent::RecvStateChangeEvent(unsigned long const&, unsigned long const&, bool const&) accessible/ipc/DocAccessibleParent.cpp:257:3
    #3 0x7f33711af5c4 in mozilla::a11y::PDocAccessibleParent::OnMessageReceived(IPC::Message const&) obj/ff-asan-release/ipc/ipdl/PDocAccessibleParent.cpp:7009:20
    #4 0x7f3371984dd5 in mozilla::dom::PContentParent::OnMessageReceived(IPC::Message const&) obj/ff-asan-release/ipc/ipdl/PContentParent.cpp:3319:28
    #5 0x7f3370f3fac9 in mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) ipc/glue/MessageChannel.cpp:2133:25
    #6 0x7f3370f3c3b7 in mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) ipc/glue/MessageChannel.cpp:2063:17
    #7 0x7f3370f3df49 in mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask&) ipc/glue/MessageChannel.cpp:1909:5
    #8 0x7f3370f3e898 in mozilla::ipc::MessageChannel::MessageTask::Run() ipc/glue/MessageChannel.cpp:1942:15
    #9 0x7f336fd39526 in nsThread::ProcessNextEvent(bool, bool*) xpcom/threads/nsThread.cpp:1040:14
    #10 0x7f336fd5fd80 in NS_ProcessNextEvent(nsIThread*, bool) xpcom/threads/nsThreadUtils.cpp:517:10
    #11 0x7f3370f480ca in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) ipc/glue/MessagePump.cpp:97:21
    #12 0x7f3370dfacc8 in RunInternal ipc/chromium/src/base/message_loop.cc:326:10
    #13 0x7f3370dfacc8 in RunHandler ipc/chromium/src/base/message_loop.cc:319
    #14 0x7f3370dfacc8 in MessageLoop::Run() ipc/chromium/src/base/message_loop.cc:299
    #15 0x7f3378281f9a in nsBaseAppShell::Run() widget/nsBaseAppShell.cpp:157:27
    #16 0x7f337cd6f7cb in nsAppStartup::Run() toolkit/components/startup/nsAppStartup.cpp:288:30
    #17 0x7f337cfad212 in XREMain::XRE_mainRun() toolkit/xre/nsAppRunner.cpp:4679:22
    #18 0x7f337cfb0129 in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) toolkit/xre/nsAppRunner.cpp:4814:8
    #19 0x7f337cfb1575 in XRE_main(int, char**, mozilla::BootstrapConfig const&) toolkit/xre/nsAppRunner.cpp:4906:21
    #20 0x51bbd5 in do_main browser/app/nsBrowserApp.cpp:231:22
    #21 0x51bbd5 in main browser/app/nsBrowserApp.cpp:304
    #22 0x7f33908381c0 in __libc_start_main /build/glibc-itYbWN/glibc-2.26/csu/../csu/libc-start.c:308
    #23 0x424409 in _start (obj/ff-asan-release/dist/bin/firefox+0x424409)
0x7f33813484a8 is located 24 bytes to the right of global variable 'gAtkStateMap' defined in 'accessible/atk/nsStateMap.h:64:26' (0x7f3381348300) of size 400
SUMMARY: AddressSanitizer: global-buffer-overflow accessible/atk/AccessibleWrap.cpp:1556:69 in FireStateChangeEvent
Shadow bytes around the buggy address:
  0x0fe6f0261040: 00 f9 f9 f9 f9 f9 f9 f9 00 06 f9 f9 f9 f9 f9 f9
  0x0fe6f0261050: 07 f9 f9 f9 f9 f9 f9 f9 00 05 f9 f9 f9 f9 f9 f9
  0x0fe6f0261060: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0fe6f0261070: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0fe6f0261080: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x0fe6f0261090: 00 00 f9 f9 f9[f9]f9 f9 f9 f9 f9 f9 f9 f9 f9 f9
  0x0fe6f02610a0: 00 04 f9 f9 f9 f9 f9 f9 00 00 00 00 00 00 00 00
  0x0fe6f02610b0: 00 f9 f9 f9 f9 f9 f9 f9 05 f9 f9 f9 f9 f9 f9 f9
  0x0fe6f02610c0: 07 f9 f9 f9 f9 f9 f9 f9 00 f9 f9 f9 f9 f9 f9 f9
  0x0fe6f02610d0: 07 f9 f9 f9 f9 f9 f9 f9 00 00 00 01 f9 f9 f9 f9
  0x0fe6f02610e0: 00 00 00 00 00 00 00 00 00 00 00 00 04 f9 f9 f9
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==29735==ABORTING

Comment 1

[Alex Gaynor [:Alex_Gaynor]]

Not super familiar with this code, but the bug is appears to be here: https://searchfox.org/mozilla-central/source/accessible/atk/nsStateMap.h#51 GetStateIndex can return an index beyond the bounds of gAtkStateMap.

Comment 2

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 3

[Eitan Isaacson [:eeejay]]

Comment on attachment 8963586 [details] [diff] [review]
patch

Review of attachment 8963586 [details] [diff] [review]:
-----------------------------------------------------------------

I'm assuming FireStateChangeEvent manages to get a state that is beyond kNoSuchState. I would love to see the fuzzer test that manages to do that.

Aside from that, these changes looks sound. Better to check the bounds and not assume the initial value is within them.

::: accessible/atk/nsStateMap.h
@@ +105,5 @@
>  };
> +
> +static const auto gAtkStateMapLen = std::extent<decltype(gAtkStateMap)>::value;
> +
> +static_assert(((uint64_t) 0x1) << (gAtkStateMapLen - 1) == mozilla::a11y::states::LAST_ENTRY,

I might be wrong, but you declare gAtkStateMapLen as auto, you then cast the '0x1' to uint64_t when shifting bits. So you are assuming that the 'auto' resolves to uint64_t. Might be better not to assume the resolved type when using 'auto'.

Comment 4

[alexander :surkov (:asurkov)]

Comment on attachment 8963586 [details] [diff] [review]
patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch? I suppose no, the crash was found from ASAN fuzzing

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No, I will got with a commit message like "Cleanup states mapping to ATK"

Which older supported branches are affected by this flaw? I don't have data on this, but I would make a guess based on the stack that was 56, where a11y e10s was landed.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? the patch should be backportable as is

How likely is this patch to cause regressions; how much testing does it need? It'd be nice to run Orca screen reader with Firefox, to make sure we are in good shape.

Comment 5

[Al Billings [:abillings - ex-MoCo]]

sec-approval+ for trunk. We'll want a beta branch patch nominated as well.

Comment 6

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a11cb2c5d1f9f9159a62cb3b311f369a32019869
Bug 1449530 - clean up ATK states mapping, r=eeejay

Comment 7

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bd15a594caa8e4b7e491180f90396c1e33b3bbbd
Bug 1449530 - fix bustage of a11cb2c5d1f9c CLOSED TREE

Comment 8

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

https://hg.mozilla.org/mozilla-central/rev/a11cb2c5d1f9
https://hg.mozilla.org/mozilla-central/rev/bd15a594caa8

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on this when you're comfortable doing so.

Comment 10

[alexander :surkov (:asurkov)]

Attachment: beta patch

Comment 11

[alexander :surkov (:asurkov)]

Alex, could you kindly confirm the patch fixes the issue please?

Comment 12

[Alex Gaynor [:Alex_Gaynor]]

Passing off to Christoph -- can you confirm this crasher stopped happening?

Comment 13

[Christoph Diehl [:posidron]]

Yes, this seems fixed! Whop Whop!

Removing 'PDocAccessible::Msg_StateChangeEvent' from the blacklist.

Comment 14

[alexander :surkov (:asurkov)]

(In reply to Christoph Diehl [:posidron] from comment #13)
> Yes, this seems fixed! Whop Whop!
> 
> Removing 'PDocAccessible::Msg_StateChangeEvent' from the blacklist.

thank you for checking it out!

Comment 15

[alexander :surkov (:asurkov)]

Comment on attachment 8965026 [details] [diff] [review]
beta patch

Approval Request Comment
[Feature/Bug causing the regression]:unknown
[User impact if declined]:sec issue
[Is this code covered by automated tests?]:no
[Has the fix been verified in Nightly?]:yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]:n/a
[Is the change risky?]:low risk
[Why is the change risky/not risky?]: just adding more guards/checks
[String changes made/needed]:no

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8965026 [details] [diff] [review]
beta patch

Fix an a11y sec issue. Approved for 60.0b11.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

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