Closed Bug 1369533 Opened 3 years ago Closed 2 years ago

Callback-to-JS conversions can generally produce null

Categories

(Core :: DOM: Core & HTML, defect)

53 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr45 --- unaffected
firefox-esr52 --- unaffected
firefox53 --- wontfix
firefox54 - wontfix
firefox55 + wontfix
firefox56 + wontfix
firefox57 + fixed

People

(Reporter: bzbarsky, Assigned: kmag)

References

Details

(Keywords: sec-audit, Whiteboard: [adv-main57-][post-critsmash-triage])

Attachments

(1 file, 1 obsolete file)

After bug 1273251, callback-to-JS conversions can generally produce null.

We have a few options for handling this:

1)  Disallow to-JS conversion for non-nullable callbacks.  For nullable callbacks, things will already work correctly once bug 1369367 is fixed.

2)  Allow non-nullable callbacks to still convert to null.  This will require at least the following things to be fixed:

  * getJSReturnTypeTag for callbacks.  Right now we claim to the jit that a callback return type will never be null.  This is why this is security-sensitive.

  * getMaybeWrapValueFuncForType for callbacks; it will need to return MaybeWrapObjectOrNullValue

but maybe there are others too.  This is a lot more fragile than option 1, because we have to go through and audit all the various places that might depend on the Value produced from a callback not being null.

Option 1, of course, has the benefit of not needing to deviate from spec IDL.
Flags: needinfo?(peterv)
Flags: needinfo?
Depends on: 1369367
Flags: needinfo? → needinfo?(kmaglione+bmo)
Gah.  Bugzilla nicely lost my comment...

Summary: Option 1 is not viable because there are specced APIs (e.g. a bunch of webrtc stuff) that use non-nullable callbacks, and making them nullable is observably not spec-compliant.

Also, the consumers we have right now that do getJSReturnTypeTag on a non-nullable callback are either test-only or chromeonly, so the security exposure is somewhat mitigated...
Regression from 53. From the discussion in bug 1369367 sounds like 54 and 55 are also affected.
[Tracking Requested - why for this release]:

[Tracking Requested - why for this release]:

[Tracking Requested - why for this release]:

I guess the real question is who will do the auditing we need for approach 2 above.  I can do it next week if no one gets to it before that....
We're spinning the Fx54 RC build today, so this isn't going to make that release.
There's also another option:

3) Replace the callback with a dead wrapper rather than setting it to null.

That would give us more or less the same behavior as we have most other places, where we explicitly wrap the value for the compartment of the object we're storing it on.

But my preference would be to disallow storing callback functions that might eventually be nuked in any location where the value would be accessible. The most obvious example is an extension setting an event listener property on a content document, which aside from having namespacing conflicts (until bug 1330113 is fixed), gives the content scripts access to it.

But maybe there are other places where there's no viable workaround.
Flags: needinfo?(kmaglione+bmo)
> 3) Replace the callback with a dead wrapper rather than setting it to null.

Hmm.  Is there a reason we didn't do that to start with, instead of changing invariants?

> my preference would be to disallow storing callback functions that might eventually
> be nuked in any location where the value would be accessible.

That's pretty hard to do, because determining "would be accessible" is a pretty non-local decision.  How would addEventListener() be able to tell that the value is not "accessible"?  The fact that it's not is kind of an implementation detail of EventListenerManager::GetListenerInfo, right?

Did you have a concrete proposal for how this would work?
Flags: needinfo?(kmaglione+bmo)
(In reply to Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-) from comment #6)
> > 3) Replace the callback with a dead wrapper rather than setting it to null.
> 
> Hmm.  Is there a reason we didn't do that to start with, instead of changing
> invariants?

Well, mainly because I went with the implementation that you suggested. But also because smaug was working on a web-facing version of this with similar semantics, and because it's easier said than done. But it should be doable.

> > my preference would be to disallow storing callback functions that might eventually
> > be nuked in any location where the value would be accessible.
> 
> That's pretty hard to do, because determining "would be accessible" is a
> pretty non-local decision.  How would addEventListener() be able to tell
> that the value is not "accessible"?  The fact that it's not is kind of an
> implementation detail of EventListenerManager::GetListenerInfo, right?
> 
> Did you have a concrete proposal for how this would work?

No. I think it would probably have to be done on a case-by-case basis, unfortunately.
Flags: needinfo?(kmaglione+bmo)
OK.  If there's any complication at all in doing option 3, I think we should just do option 2.  It just needs auditing of codegen for all the isCallback and isCallbackInterface bits and changing them all accordingly.  Pretty straightforward; just tedious.
It should be easier since bug 1354733, which lets us create dead wrappers directly when we don't have a wrapper to nuke. If we want the dead wrapper to report itself as callable, though, it will need a bit more work.

After that, I think we should be able to change the deferred finalizer to null out the incumbent global and replace the JS callback with a dead wrapper, rather than nulling it out and calling DropJSObjects.
I don't think we care about the dead wrapper reporting itself as callable.
Track 54- as we've already built 54 RC and the security exposure is somewhat mitigated according to comment #1. Feel free to nominate again if the security level comes to critical/high.
I'm marking this audit because it sounds like you aren't sure if there's an issue. Please clear the keyword or adjust the rating if that is incorrect.
Keywords: sec-audit
(In reply to Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-) from comment #10)
> I don't think we care about the dead wrapper reporting itself as callable.

I wouldn't think so either, but based on bug 1354294, I'm not sure.
Flags: needinfo?(shu)
(In reply to Kris Maglione [:kmag] (busy; behind on reviews) from comment #13)
> (In reply to Boris Zbarsky [:bz] (if a patch has no decent message,
> automatic r-) from comment #10)
> > I don't think we care about the dead wrapper reporting itself as callable.
> 
> I wouldn't think so either, but based on bug 1354294, I'm not sure.

We don't care whether a dead wrapper is callable, but we do care about it preserving the callability. (For context: I ran into crashes from bug 1354294 when I tried to make dead wrappers always callable, even if the previously live thing was not callable. Since it's more semantically intuitive that dead wrappers don't change IsCallable and IsConstructor, I opted to not spend the time debugging and preserve those properties.)

The current behavior is when constructing a DeadObjectProxy, it retains the constructibility and callability of the nuked target.

(In reply to Kris Maglione [:kmag] (busy; behind on reviews) from comment #9)
> It should be easier since bug 1354733, which lets us create dead wrappers
> directly when we don't have a wrapper to nuke. If we want the dead wrapper
> to report itself as callable, though, it will need a bit more work.
> 

You can get a callable DeadProxyObject handler directly with |DeadObjectProxy<DeadProxyIsCallable{Not,Is}Constructor>::singleton()|. Does that help?
Flags: needinfo?(shu)
(In reply to Shu-yu Guo [:shu] from comment #14)
> (In reply to Kris Maglione [:kmag] (busy; behind on reviews) from comment
> #13)
> > (In reply to Boris Zbarsky [:bz] (if a patch has no decent message,
> > automatic r-) from comment #10)
> > > I don't think we care about the dead wrapper reporting itself as callable.
> > 
> > I wouldn't think so either, but based on bug 1354294, I'm not sure.
> 
> We don't care whether a dead wrapper is callable, but we do care about it
> preserving the callability.

That probably doesn't matter for this case, then. We'd be creating a new wrapper entirely, and no code in the JS engine should expect the getter to return the same object, or the same type of object, that it returned previously.

Although... that might change if we wind up marking any of those attributes Pure at some point.

> (In reply to Kris Maglione [:kmag] (busy; behind on reviews) from comment #9)
> You can get a callable DeadProxyObject handler directly with
> |DeadObjectProxy<DeadProxyIsCallable{Not,Is}Constructor>::singleton()|. Does
> that help?

The only real issue at this point is that those are non-public APIs, and the only jsfriend API for creating new dead wrappers can only create them based on an existing wrapper, or as non-callable/non-constructable. It probably wouldn't be too hard to change that to allow explicitly specifying those properties, or to get them from a non-wrapper object, but it would be much easier not to.
Do you intend to make changes for the 55 or 56 timeframe? 
If so, can you help figure out who's actually going to take the bug?
Flags: needinfo?(shu)
Flags: needinfo?(bzbarsky)
Either me or Kris, depending on the solution we pick.  Kris, do you think you can do #3 reasonably?  If not, I will make the time to go through and do #2...
Flags: needinfo?(bzbarsky) → needinfo?(kmaglione+bmo)
Flags: needinfo?(shu)
Yes, I think #3 is doable at this point.
Assignee: nobody → kmaglione+bmo
Flags: needinfo?(kmaglione+bmo)
Kris, are you still planning to get to this?  It looks like it missed 55 now...
Flags: needinfo?(kmaglione+bmo)
Yes. Sorry, other urgent bugs came up in the mean time. I'll put it on my todo list for this week.
Flags: needinfo?(kmaglione+bmo)
Boris, are you willing to review this?
Flags: needinfo?(bzbarsky)
Comment on attachment 8904383 [details] [diff] [review]
Return dead wrappers rather than null for dead CallbackObject values

This looks good as far as bindings go, thank you!

I think we still have a few places with not-obviously-ok CallbackOrNull/CallableOrNull bits:

1) CustomElementRegistry::Define will crash if the callable is null.
   Can it not be null there?
2) CustomElementRegistry::Get should arguably hand back a dead object
   wrapper, not null.
3) DOMEventTargetHelper::GetEventHandler should arguably hand back
   a dead object wrapper.
4) I dunno whether EventListenerInfo::GetJSVal is even used...
5) The CallbackObject overload of ToJSValue over in ToJSValue.h should
   hand back a dead object, I'd think.

Please make sure a followup or followups is reported on those, and let me know whether I should pick the followups up or whether you have the time.
Flags: needinfo?(bzbarsky)
Attachment #8904383 - Flags: review+
Wontfix for 56 at this point as the release will be next week. 
kmag, bz, if you want to land this for 57, it should be soon.
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(bzbarsky)
It's still on my priority list for this week. I would have had it done yesterday, but I had to spend half the day recovering from a power failure that outlasted my UPS and laptop batteries :(
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] (still digging out from vacation mail) from comment #22)
> I think we still have a few places with not-obviously-ok
> CallbackOrNull/CallableOrNull bits:
>
> 1) CustomElementRegistry::Define will crash if the callable is null.
>    Can it not be null there?

It can't, though possibly for non-obvious reasons. The only way a callback
object can ever wind up being null is if we nuke the compartment it belongs to,
and then run the cycle collector's forget-skippable phase, and it calls the
CallbackObject's CanSkip.

Normally the second part is prevented in binding methods by the fact that we use
RootedCallback<>s, which means the CC never sees them before the binding method
does something with them.

In this case, though, there's just no opportunity for code to run that could
nuke the owner compartment or trigger the cycle collector. Perhaps some comments
in nsDocument::RegisterElement and nsDocument::RegisterElement to ensure that
continues to be the case would make sense.

> 2) CustomElementRegistry::Get should arguably hand back a dead object
>    wrapper, not null.
> 3) DOMEventTargetHelper::GetEventHandler should arguably hand back
>    a dead object wrapper.

Hm. Quite. I'd have to find a place where we still use XPIDL event targets to
test that...

> 4) I dunno whether EventListenerInfo::GetJSVal is even used...

It is. I'm not sure why Searchfox doesn't index it properly.

The event listener service filters out dead callbacks when it's asked to iterate
event listeners, though, so this isn't an issue.
Flags: needinfo?(peterv)
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #25)
> > 3) DOMEventTargetHelper::GetEventHandler should arguably hand back
> >    a dead object wrapper.
> 
> Hm. Quite. I'd have to find a place where we still use XPIDL event targets to
> test that...

It seems like everywhere we currently use NS_IMPL_EVENT_HANDLER, we're already using WebIDL bindings. So we should probably just remove the XPIDL versions. But that's really another bug. For now, I'll just make the change but not add any tests for it.
Attachment #8904383 - Attachment is obsolete: true
> For now, I'll just make the change but not add any tests for it.

Sounds good.  I filed bug 1401848 on removing NS_IMPL_EVENT_HANDLER.
Comment on attachment 8910515 [details] [diff] [review]
Return dead wrappers rather than null for dead CallbackObject values

r=me
Attachment #8910515 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/8af47160570c05088128428bae1eb69bf48b0dc4
Bug 1369533: Return dead wrappers rather than null for dead CallbackObject values. r=bz
https://hg.mozilla.org/mozilla-central/rev/8af47160570c
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Group: dom-core-security → core-security-release
Whiteboard: [adv-main57-]
Flags: qe-verify-
Whiteboard: [adv-main57-] → [adv-main57-][post-critsmash-triage]
Group: core-security-release
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.