1218437 - Firefox crash by checking x instanceof Components.Exception when x is not an object

Status: VERIFIED FIXED

(Core :: XPConnect, defect)

Description

[tabmix.onemen]

Firefox crash by checking x instanceof Components.Exception when x is not an object

Comment 1

[:Gijs (he/him)]

This is crashing somewhere in JS land. Odd. Jan, any idea what's up here?

Sample crashreports:

53: https://crash-stats.mozilla.com/report/index/cb0e4ce9-1b8b-4792-abf3-f59d52170325

55: https://crash-stats.mozilla.com/report/index/70eb054a-6f02-4643-a2ed-748672170325

(latter looks like it has a more useful stack)

STR:

1. eval in browser toolbox: null instanceof Components.Exception

Comment 2

[Jan de Mooij [:jandem]]

We end up in nsXPCComponents_Exception::HasInstance. There we pass v.toObjectOrNull() to UNWRAP_OBJECT:

http://searchfox.org/mozilla-central/rev/7419b368156a6efa24777b21b0e5706be89a9c2f/js/xpconnect/src/XPCComponents.cpp#1713

Then in UnwrapObject we call GetDOMClass and assume |obj| is nullptr.

http://searchfox.org/mozilla-central/rev/7419b368156a6efa24777b21b0e5706be89a9c2f/dom/bindings/BindingUtils.h#203

Hiding this as I really don't like that unchecked toObjectOrNull() call.

Comment 3

[Jan de Mooij [:jandem]]

bz, any thoughts on this? Is this code available to content?

Comment 4

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #2)
> Then in UnwrapObject we call GetDOMClass and assume |obj| is nullptr.

I meant non-nullptr, obviously.

Comment 5

[:Gijs (he/him)]

Components.Exception is not available to content, I think, so I'm not sure this is reachable from public web code, which might affect security-severity?

Comment 6

[Boris Zbarsky [:bzbarsky]]

This should not be available to content, no.

This is a clear bug that was introduced in <https://hg.mozilla.org/mozilla-central/rev/16cf9da0d24282153c9ea53e49439dcb3aac194f>: The UNWRAP_OBJECT things should only happen if isObject().

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Need to check that the valie is an object before we try to UNWRAP_OBJECT it to check whether it's an Exception

MozReview-Commit-ID: 8lwCoilRz49

Comment 8

[Andrew McCreight [:mccr8]]

I'll mark this sec-other because it isn't clear this is really a problem, but we'd like to leave it hidden in case it is.

Comment 9

[Daniel Veditz [:dveditz]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #7)
> Created attachment 8851622 [details] [diff] [review]

I assume the CanvasRenderingContext2D.cpp fontMetrics stuff in the patch is part of some other fix?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Need to check that the value is an object before we try to UNWRAP_OBJECT it to check whether it's an Exception

Er... yes, it is!

Comment 11

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1ca1ed5ebd7e3c6b1908969c370255cc6efdfc14

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8851697 [details] [diff] [review]
Need to check that the value is an object before we try to UNWRAP_OBJECT it to check whether it's an Exception

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
   Simple crash fix for a possibly-exploitable crash, if it can be triggered.
User impact if declined: Crashes if extensions do the wrong thing.
Fix Landed on Version: 55
Risk to taking this patch (and alternatives if risky): Very low risk.
String or UUID changes made by this patch: None.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 911258
[User impact if declined]:  Possibly-exploitable crash, if it can be triggered.
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes, on my local machine.
[Needs manual test from QE? If yes, steps to reproduce]: See comment 1
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Just adds a missing type of value check.
[String changes made/needed]: None.

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1ca1ed5ebd7e

Comment 14

[Julien Cristau [:jcristau]]

Comment on attachment 8851697 [details] [diff] [review]
Need to check that the value is an object before we try to UNWRAP_OBJECT it to check whether it's an Exception

crash fix for aurora and beta

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3e5bbabd5279
https://hg.mozilla.org/releases/mozilla-beta/rev/4409827e9906

Comment 16

[Andrei Vaida [:avaida]]

Flagging this for manual testing, str available in Comment 1.

Comment 17

[Julien Cristau [:jcristau]]

Comment on attachment 8851697 [details] [diff] [review]
Need to check that the value is an object before we try to UNWRAP_OBJECT it to check whether it's an Exception

let's take this in esr52 as well now

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/e13692bfd5f5

Comment 19

[Bogdan Maris, Desktop QA]

Reproduced the initial crash using 53 beta 5, verified that the crash no longer happens using Firefox 53 beta 9, latest Developer Edition 54.0a2 and latest Nightly 55.0a1 on macOS 10.12.4, Windows 10 64bit and Ubuntu 16.04 32bit.

Comment 20

[Andrei Vaida [:avaida]]

Bogdan, could you please take a look at this on 52.1esr as well?

Comment 21

[Bogdan Maris, Desktop QA]

(In reply to Andrei Vaida, QA [:avaida] – please ni? me from comment #20)
> Bogdan, could you please take a look at this on 52.1esr as well?

I can also confirm that Fx 52.1.0esr-build3 does not crash across platforms (macOS 10.12.4, Windows 10 64bit and Ubuntu 16.04 32bit).

Comment 22

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8851697 [details] [diff] [review]
Need to check that the value is an object before we try to UNWRAP_OBJECT it to check whether it's an Exception

esr45 is gone.