1300476 - MozPromise's InvokeAsync dangerously stores references as-is

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

InvokeAsync uses automatic template type deduction to store arguments in the dispatched task.
This means that if it is given an argument that is a reference, it will be stored as a reference, and in the worst case the real object could be destroyed by the time the target function is called, causing UAF.

For simplicity we should just drop references when storing arguments.
For cases where a reference really would have worked (e.g. an object that is certain to outlive the dispatch), then it should be easy enough to convert it to a pointer.
There are not that many uses of InvokeAsync (yet), so it should be possible to have a look at all existing calls and decide if they need to be modified.

Alternatively, or in a later bug, we could use a more powerful system, like that used for NewRunnableMethod.

(I'm leaving this bug unassigned for now, if someone else wants to tackle it before I can find the time.)

Comment 1

[Benjamin Smedberg]

Auto-deducing argument types for async methods is intrinsically dangerous. That's why we require explicit template typing for NewRunnableMethod. I think we should require the same here.

Comment 2

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1300476 - Prevent passing references through InvokeAsync -

Passing references to an async call is dangerous because referenced objects
could be destroyed before/during the call, or even be stored by the callee.

Review commit: https://reviewboard.mozilla.org/r/88804/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/88804/

Comment 3

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

(In reply to Benjamin Smedberg [:bsmedberg] from comment #1)
> Auto-deducing argument types for async methods is intrinsically dangerous.
> That's why we require explicit template typing for NewRunnableMethod. I
> think we should require the same here.
I didn't try to import/reuse the whole NewRunnableMethod storage capabilities here, so in the meantime I'm just adding a static_assert that will catch any attempt to pass references.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=770c82d87429452d3b5a79373f4b1c8b52fd8f83

Comment 4

[Nathan Froyd [:froydnj]]

Comment on attachment 8804996 [details]
Bug 1300476 - Prevent passing references through InvokeAsync -

https://reviewboard.mozilla.org/r/88804/#review88178

At first I thought, "yes, of course, r+" and then I thought, "wait, no, let's think about this" and then I thought "well, that'd be a lot of work."  So I'm on the fence, but tilting towards r+; mozreview doesn't give me anyway to say "r+0.5", so I'm setting r+, but I'd like to have a small discussion about this first.

::: xpcom/threads/MozPromise.h:996
(Diff revision 1)
> +  static_assert(!detail::Any(IsReference<ArgTypes>::value...),
> +                "Cannot pass reference types through InvokeAsync");

This seems like an expedient but limiting solution.  It's not technically incorrect for the the method itself to take references, but it would be incorrect for the runnable to have reference-valued member variables.  So I think this solution only works because `MethodCallType` doesn't care about `ActualArgTypes`, but only `ArgTypes`, and since we've verified that `ArgTypes` is reference-free, things effectively get copied and/or moved instead of `MethodCall` storing references?  Is that correct?

There are two ways of doing what we want here.  The first is to duplicate what `NewRunnableMethod` and friends do, and require explicit template types for `ActualArgTypes`, which would then get passed through to `MethodCall`.  The second is to adopt the `WrapRunnable` approach, but to be smarter about it: pass `ActualArgTypes` through to `MethodCall`, but to define `mArgs` thusly:

```
  Tuple<typename Decay<ActualArgTypes>::Type...> mArgs;
```

and thus effectively capture any arguments we need by copying them.

I know some people truly loathe having to provide explicit template argument types.  They do, however, provide some measure of security that implicit argument types can't provide.  OTOH, I think all the stuff `NewRunnableMethod` does under the hood with the template types you supply could be applied to implicit template argument types, so we could get the best of both worlds: safety and terseness.

On the third hand, I'm not keen on requiring you to rewrite a whole bunch of stuff just so we can make this check slightly more correct; reference arguments tend to be rare in our codebase anyway...

Comment 5

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8804996 [details]
Bug 1300476 - Prevent passing references through InvokeAsync -

https://reviewboard.mozilla.org/r/88804/#review88178

> This seems like an expedient but limiting solution.  It's not technically incorrect for the the method itself to take references, but it would be incorrect for the runnable to have reference-valued member variables.  So I think this solution only works because `MethodCallType` doesn't care about `ActualArgTypes`, but only `ArgTypes`, and since we've verified that `ArgTypes` is reference-free, things effectively get copied and/or moved instead of `MethodCall` storing references?  Is that correct?
> 
> There are two ways of doing what we want here.  The first is to duplicate what `NewRunnableMethod` and friends do, and require explicit template types for `ActualArgTypes`, which would then get passed through to `MethodCall`.  The second is to adopt the `WrapRunnable` approach, but to be smarter about it: pass `ActualArgTypes` through to `MethodCall`, but to define `mArgs` thusly:
> 
> ```
>   Tuple<typename Decay<ActualArgTypes>::Type...> mArgs;
> ```
> 
> and thus effectively capture any arguments we need by copying them.
> 
> I know some people truly loathe having to provide explicit template argument types.  They do, however, provide some measure of security that implicit argument types can't provide.  OTOH, I think all the stuff `NewRunnableMethod` does under the hood with the template types you supply could be applied to implicit template argument types, so we could get the best of both worlds: safety and terseness.
> 
> On the third hand, I'm not keen on requiring you to rewrite a whole bunch of stuff just so we can make this check slightly more correct; reference arguments tend to be rare in our codebase anyway...

As I wrote in comment 3, "in the meantime" ;-)

I intend to add more intelligence in there, but since it would be on the more difficult side (and not that urgent by itself), I thought I would take care of the urgent issue right now, that is removing the current risk of using references until we get a better&safer system.

I will open a follow-up bug with Benjamin's and your suggestions. And Jean-Yves' too, who suggested only requiring explicit argument types *if* one of them is a reference.

And since this is a static_assert, it's intended for developers, so I will mention the new bug in the assertion message, in case an annoyed developer really needs reference-passing, then they can go ahead and code it!

Comment 6

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8804996 [details]
Bug 1300476 - Prevent passing references through InvokeAsync -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/88804/diff/1-2/

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/54ad8aaf59c7
Prevent passing references through InvokeAsync - r=froydnj

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/54ad8aaf59c7