1402871 - AddressSanitizer: heap-buffer-overflow /builds/worker/workspace/build/src/obj-firefox/dist/include/nsCOMPtr.h:796:45 in operator bool

Status: RESOLVED FIXED

(Core :: WebVR, defect)

Description

[Jason Kratzer [:jkratzer]]

Found while fuzzing mozilla-central rev 7e962631ba42.  I am currently reducing the testcase and will update once complete.

==29098==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x619001d29b40 at pc 0x7f3522f1aed9 bp 0x7fff6b8b5280 sp 0x7fff6b8b5278
READ of size 8 at 0x619001d29b40 thread T0
    #0 0x7f3522f1aed8 in operator bool /builds/worker/workspace/build/src/obj-firefox/dist/include/nsCOMPtr.h:796:45
    #1 0x7f3522f1aed8 in IsCurrentInnerWindow /builds/worker/workspace/build/src/obj-firefox/dist/include/nsPIDOMWindowInlines.h:90
    #2 0x7f3522f1aed8 in mozilla::dom::VREventObserver::NotifyVRDisplayConnect(unsigned int) /builds/worker/workspace/build/src/dom/vr/VREventObserver.cpp:113
    #3 0x7f351f1ab06e in mozilla::gfx::VRManagerChild::FireDOMVRDisplayConnectEventInternal(unsigned int) /builds/worker/workspace/build/src/gfx/vr/ipc/VRManagerChild.cpp:625:15
    #4 0x7f351f1b43e2 in applyImpl<mozilla::gfx::VRManagerChild, void (mozilla::gfx::VRManagerChild::*)(unsigned int), StoreCopyPassByConstLRef<unsigned int> , 0> /builds/worker/workspace/build/src/obj-firefox/dist/include/nsThreadUtils.h:1142:12
    #5 0x7f351f1b43e2 in apply<mozilla::gfx::VRManagerChild, void (mozilla::gfx::VRManagerChild::*)(unsigned int)> /builds/worker/workspace/build/src/obj-firefox/dist/include/nsThreadUtils.h:1148
    #6 0x7f351f1b43e2 in mozilla::detail::RunnableMethodImpl<mozilla::gfx::VRManagerChild*, void (mozilla::gfx::VRManagerChild::*)(unsigned int), true, (mozilla::RunnableKind)0, unsigned int>::Run() /builds/worker/workspace/build/src/obj-firefox/dist/include/nsThreadUtils.h:1192
    #7 0x7f351f443afe in nsContentUtils::AddScriptRunner(already_AddRefed<nsIRunnable>) /builds/worker/workspace/build/src/dom/base/nsContentUtils.cpp:5716:13
    #8 0x7f351f1a84f7 in mozilla::gfx::VRManagerChild::FireDOMVRDisplayConnectEvent(unsigned int) /builds/worker/workspace/build/src/gfx/vr/ipc/VRManagerChild.cpp:569:3
    #9 0x7f351f1a78f7 in mozilla::gfx::VRManagerChild::UpdateDisplayInfo(nsTArray<mozilla::gfx::VRDisplayInfo>&) /builds/worker/workspace/build/src/gfx/vr/ipc/VRManagerChild.cpp:261:5
    #10 0x7f351f1a863d in mozilla::gfx::VRManagerChild::RecvUpdateDisplayInfo(nsTArray<mozilla::gfx::VRDisplayInfo>&&) /builds/worker/workspace/build/src/gfx/vr/ipc/VRManagerChild.cpp:270:3
    #11 0x7f351deaf4a2 in mozilla::gfx::PVRManagerChild::OnMessageReceived(IPC::Message const&) /builds/worker/workspace/build/src/obj-firefox/ipc/ipdl/PVRManagerChild.cpp:744:20
    #12 0x7f351da869c9 in mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) /builds/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:2115:25
    #13 0x7f351da83a0f in mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) /builds/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:2045:17
    #14 0x7f351da85144 in mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask&) /builds/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:1891:5
    #15 0x7f351da85798 in mozilla::ipc::MessageChannel::MessageTask::Run() /builds/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:1924:15
    #16 0x7f351cce2bec in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1039:14
    #17 0x7f351cce8a0c in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:521:10
    #18 0x7f351da8e571 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/worker/workspace/build/src/ipc/glue/MessagePump.cpp:97:21
    #19 0x7f351d9f044b in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:326:10
    #20 0x7f351d9f044b in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:319
    #21 0x7f351d9f044b in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:299
    #22 0x7f352318fbff in nsBaseAppShell::Run() /builds/worker/workspace/build/src/widget/nsBaseAppShell.cpp:158:27
    #23 0x7f35272f77b1 in nsAppStartup::Run() /builds/worker/workspace/build/src/toolkit/components/startup/nsAppStartup.cpp:288:30
    #24 0x7f35274d81bb in XREMain::XRE_mainRun() /builds/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4701:22
    #25 0x7f35274d9dd8 in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) /builds/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4865:8
    #26 0x7f35274db20b in XRE_main(int, char**, mozilla::BootstrapConfig const&) /builds/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4960:21
    #27 0x4ebfe3 in do_main /builds/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:236:22
    #28 0x4ebfe3 in main /builds/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:309
    #29 0x7f353aa3c82f in __libc_start_main /build/glibc-bfm8X4/glibc-2.23/csu/../csu/libc-start.c:291
    #30 0x41db38 in _start (/home/forb1dden/builds/mc-asan/firefox+0x41db38)

AddressSanitizer can not describe address in more detail (wild memory access suspected).
SUMMARY: AddressSanitizer: heap-buffer-overflow /builds/worker/workspace/build/src/obj-firefox/dist/include/nsCOMPtr.h:796:45 in operator bool
Shadow bytes around the buggy address:
  0x0c328039d310: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c328039d320: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c328039d330: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c328039d340: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c328039d350: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
=>0x0c328039d360: fa fa fa fa fa fa fa fa[fa]fa fa fa fa fa fa fa
  0x0c328039d370: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c328039d380: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c328039d390: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c328039d3a0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c328039d3b0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
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
  Heap right redzone:      fb
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack partial redzone:   f4
  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
==29098==ABORTING

Comment 1

[Jason Kratzer [:jkratzer]]

Attachment: trigger_part_1.html

Comment 2

[Jason Kratzer [:jkratzer]]

Attachment: trigger_part_2.html

Comment 3

[Jason Kratzer [:jkratzer]]

Both attached testcases are required to trigger.  Launch firefox with trigger_part_1.html as the starting location.  Testcases may require several "iterations" to trigger the issue.

Comment 4

[:kip (Kearwood Gilbert)]

Am investigating now, thanks for the reduced test case.

Comment 5

[Daniel Veditz [:dveditz]]

Probably a UAF of a window (weak pointer is used!) rather than a buffer overflow as reported by ASAN.

Comment 6

[Jason Kratzer [:jkratzer]]

(In reply to Daniel Veditz [:dveditz] from comment #5)
> Probably a UAF of a window (weak pointer is used!) rather than a buffer
> overflow as reported by ASAN.

ASAN would routinely switch from buffer-overflow to UAF during reduction so you're probably correct.

Comment 7

[:kip (Kearwood Gilbert)]

I am trying to reproduce this, but haven't succeeded so far.

My config:

Ubuntu 17.04 64-bit
Current mozilla-central head (As of 2017-10-04)
64-bit ASAN built locally
Locally built LLVM with revision 314736 from https://llvm.org/svn/llvm-project/llvm/trunk
Set "dom.vr.puppet.enabled" pref to "True" in domfuzz's dom/automation/constant-prefs.js

Ran test case with:

python ./domfuzz/dom/automation/domInteresting.py ~/dev/mozilla-unified/obj-asan-x86_64-pc-linux-gnu ~/Desktop/trigger_part_1.html

Were any other prefs set other than "dom.vr.puppet.enabled"?  Also, any other steps I may have missed?

Comment 8

[Christoph Diehl [:posidron]]

// WebVR
user_pref("dom.vr.enabled", true);
user_pref("dom.vr.test.enabled", true);
user_pref("dom.vr.puppet.enabled", true);
user_pref("dom.vr.require-gesture", false);
user_pref("dom.vr.poseprediction.enabled", false);
user_pref("full-screen-api.allow-trusted-requests-only", false);

Comment 9

[Jason Kratzer [:jkratzer]]

(In reply to :kip (Kearwood Gilbert) from comment #7)
> I am trying to reproduce this, but haven't succeeded so far.
> 
> My config:
> 
> Ubuntu 17.04 64-bit
> Current mozilla-central head (As of 2017-10-04)
> 64-bit ASAN built locally
> Locally built LLVM with revision 314736 from
> https://llvm.org/svn/llvm-project/llvm/trunk
> Set "dom.vr.puppet.enabled" pref to "True" in domfuzz's
> dom/automation/constant-prefs.js
> 
> Ran test case with:
> 
> python ./domfuzz/dom/automation/domInteresting.py
> ~/dev/mozilla-unified/obj-asan-x86_64-pc-linux-gnu
> ~/Desktop/trigger_part_1.html
> 
> Were any other prefs set other than "dom.vr.puppet.enabled"?  Also, any
> other steps I may have missed?

Part 2 of this testcase also requires the fuzzpriv extension which can be found at:
https://github.com/MozillaSecurity/domfuzz/tree/master/dom/extension

Comment 10

[Jason Kratzer [:jkratzer]]

Additionally, you'll need to set the prefs to allow legacy extensions:

user_pref("extensions.allow-non-mpc-extensions", true); // Required to load fuzzPriv
user_pref("extensions.legacy.enabled", true); // Required to load fuzzPriv

Comment 11

[:kip (Kearwood Gilbert)]

(In reply to Jason Kratzer [:jkratzer] from comment #10)
> Additionally, you'll need to set the prefs to allow legacy extensions:
> 
> user_pref("extensions.allow-non-mpc-extensions", true); // Required to load
> fuzzPriv
> user_pref("extensions.legacy.enabled", true); // Required to load fuzzPriv

Thanks!  I managed to reproduce it now.

I'm working on a patch now and should be able to verify the result.

Comment 12

[:kip (Kearwood Gilbert)]

Attachment: Bug 1402871 - Change VREventObserver::mWindow to a RefPtr

Changing VREventObserver::mWindow to a RefPtr appears to fix the crash in the STR.

With some manual testing using real VR hardware (Oculus Rift CV1), this change does not appear to have any negative effect.  I was able to confirm that the VREventObserver was deallocated without cyclic dependencies.

Comment 13

[Daniel Veditz [:dveditz]]

Comment on attachment 8917619 [details] [diff] [review]
Bug 1402871 - Change VREventObserver::mWindow to a RefPtr

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

Am I the best reviewer for this? r=dveditz to the extent this should fix the UAF, but I don't know the WebVR code at all and can't say whether this creates leaks or not. (I prefer leaks to UAFs anyway.)

Comment 14

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

Is this needed for firefox 57?

Comment 15

[:kip (Kearwood Gilbert)]

(In reply to Emma Humphries ☕️ (she/her) [:emceeaich] (UTC-8) +needinfo me from comment #14)
> Is this needed for firefox 57?

Firefox 57 would be affected as well.  I would suggest uplift after some testing in Nightly and with the rebased patch to ensure that there are no combinational effects (such as memory leaks).

NI?'ing myself to follow up after this has been in Nightly for a few days.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/84ff58454e22

Please request Beta approval on this when you get a chance.

Comment 17

[:kip (Kearwood Gilbert)]

Comment on attachment 8917619 [details] [diff] [review]
Bug 1402871 - Change VREventObserver::mWindow to a RefPtr

Approval Request Comment
[Feature/Bug causing the regression]:
N/A - New Feature
[User impact if declined]:
There is a possibility of browser crashes or UAF when closing active WebVR windows.
[Is this code covered by automated tests?]:
Yes, the issue affects the "puppet" VR device used in our reftests and mochitests that run without physical hardware.
[Has the fix been verified in Nightly?]:
The fix has landed in Nightly for a week, without any negative effects.
[Needs manual test from QE? If yes, steps to reproduce]: 
N/A - Unable to reproduce manually outside of ASAN builds with non-default preferences.

[List of other uplifts needed for the feature/fix]:
N/A
[Is the change risky?]:
Low to medium risk.
[Why is the change risky/not risky?]:
The change is a single line, affecting only WebVR sites.  Unexpected side-effects would be most likely impacting clean shutdown or causing memory leaks after viewing WebVR content.  These cases are included by our automated mochitests.

[String changes made/needed]:
N/A

Comment 18

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Dan, Al, does this need a sec-approval+? I'd like to uplift it to beta57. Will do so after sec-approval is granted.

Comment 19

[Daniel Veditz [:dveditz]]

It needed sec-approval before landing on nightly, so that we didn't miss it and end up 0-daying the 57 release. At this point it's post-facto approval which is more about damage assessment and control. Please take it on beta.

Comment 20

[Daniel Veditz [:dveditz]]

Comment on attachment 8917619 [details] [diff] [review]
Bug 1402871 - Change VREventObserver::mWindow to a RefPtr

sec-approval=dveditz

Comment 21

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

It would be good if the assigned dev answered the sec-approval? template questions though.

Comment 22

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8917619 [details] [diff] [review]
Bug 1402871 - Change VREventObserver::mWindow to a RefPtr

Sec-high, beta57+

Comment 23

[Ryan VanderMeulen [:RyanVM]]

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

Comment 24

[:kip (Kearwood Gilbert)]

Comment on attachment 8917619 [details] [diff] [review]
Bug 1402871 - Change VREventObserver::mWindow to a RefPtr

[Security approval request comment] 
How easily can the security issue be deduced from the patch?
- To identify the security issue, someone would have to be very familiar with the code base.  It is not obvious how to create a payload that would replicate the issue.  Very precise timing requirements, affected by garbage collection would need to be replicated.  The STR in this bug relies on "puppet" VR API's which disabled by a pref which is turned only only be VR reftests and mochitests.
Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
- There is no mention of UAF or vulnerabilities in the comment.
Which older supported branches are affected by this flaw? 
- This would affect all branches that enable WebVR by default
If not all supported branches, which bug introduced the flaw? 
Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? 
- This patch applies to beta as-is.
How likely is this patch to cause regressions; how much testing does it need? 
- Regressions would manifest most likely as memory leaks or failure to shutdown cleanly.  Such issues were not seen in Nightly which has already landed this patch.