Closed Bug 1218437 Opened 4 years ago Closed 3 years ago

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

Categories

(Core :: XPConnect, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla55
Tracking Status
firefox-esr45 - wontfix
firefox52 --- wontfix
firefox-esr52 53+ verified
firefox53 + verified
firefox54 + verified
firefox55 + verified

People

(Reporter: tabmix.onemen, Assigned: bzbarsky)

References

Details

(Keywords: crash, regression, sec-other, Whiteboard: [adv-main53-][adv-esr52.1-])

Attachments

(1 file, 1 obsolete file)

Firefox crash by checking x instanceof Components.Exception when x is not an object
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
Component: General → JavaScript Engine
Flags: needinfo?(jdemooij)
Product: Firefox → Core
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.
Group: core-security
Component: JavaScript Engine → XPConnect
Flags: needinfo?(jdemooij)
bz, any thoughts on this? Is this code available to content?
Flags: needinfo?(bzbarsky)
(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.
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?
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().
Flags: needinfo?(bzbarsky)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
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.
Keywords: sec-other
(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?
Flags: needinfo?(bzbarsky)
Attachment #8851622 - Attachment is obsolete: true
Attachment #8851622 - Flags: review?(kyle)
Flags: needinfo?(bzbarsky)
Attachment #8851697 - Flags: review?(kyle) → review+
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.
Attachment #8851697 - Flags: approval-mozilla-esr52?
Attachment #8851697 - Flags: approval-mozilla-esr45?
Attachment #8851697 - Flags: approval-mozilla-beta?
Attachment #8851697 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/1ca1ed5ebd7e
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
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
Attachment #8851697 - Flags: approval-mozilla-beta?
Attachment #8851697 - Flags: approval-mozilla-beta+
Attachment #8851697 - Flags: approval-mozilla-aurora?
Attachment #8851697 - Flags: approval-mozilla-aurora+
Group: core-security → core-security-release
Flagging this for manual testing, str available in Comment 1.
Flags: qe-verify+
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
Attachment #8851697 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Whiteboard: [adv-main53-][adv-esr52.1-]
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Bogdan, could you please take a look at this on 52.1esr as well?
Flags: needinfo?(bogdan.maris)
(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).
Flags: needinfo?(bogdan.maris)
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.
Attachment #8851697 - Flags: approval-mozilla-esr45?
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.