Closed Bug 1153295 Opened 5 years ago Closed 4 years ago

NS_NewRunnableMethodWithArgs is security hazard for refcounted objects which don't inherit nsISupports

Categories

(Core :: XPCOM, defect)

36 Branch
x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: smaug, Assigned: gerald)

References

Details

Attachments

(4 files, 4 obsolete files)

Looks like NS_NewRunnableMethodWithArgs deals properly only with objects inheriting nsISupports and keeps them alive by using nsRefPtr. But what if one uses type which just have AddRef/Release methods?
Or what if the type is changed, and doesn't anymore inherit nsISupports.
Everything probably works fine until something unusual happens in event loop which kills the object before its use, and we have UAF.
Summary: NS_NewRunnableMethodWithArgs is security hazard for refcounted objects → NS_NewRunnableMethodWithArgs is security hazard for refcounted objects which don't inherit nsISupports
Whatever we do to fix NS_NewRunnableMethodWithArgs will probably make the lambda version in bug 1152753 usable also.
Blocks: 1152753
bholley asked me some questions about NS_NewRunnableMethodWithArgs recently and how to use it with non-nsISupports AddRef/Release things.  I was confused as to why NS_NewRunnableMethodWithArgs didn't support this, and counseled him to write his own specializations of the relevant templates.  I didn't carry it all the way through to UAF issues. :(

bholley, do you have that code written?  I don't know that it'll be fully general for this case, but it'll at least be useful as an inspiration for what needs to happen.  The fully general solution probably involves some serious template hackery to detect the presence of AddRef/Release...at least those bits would be useful in other cases, too, like bug 977380
Flags: needinfo?(bobbyholley)
I wrote this to Nathan by email. It's actually quite simple, unless I'm missing something:

nsCOMPtr<nsIRunnable> r = NS_NewRunnableMethodWithArg<StorensRefPtrPassByPtr<MyType>>
                            (targetInstance, &MyClass::MyMethod, someInstanceOfMyType);

Am I?
Flags: needinfo?(bobbyholley)
mozilla::AddRvalueReference, needed for DeclVal in next patch.
Attachment #8597692 - Flags: review?(nfroyd)
Attached patch 1153295-p2-declval.patch (obsolete) — Splinter Review
mozilla::DeclVal, needed in next (main) patch.
Attachment #8597694 - Flags: review?(nfroyd)
Main patch, looks for AddRef and Release methods in type to be stored with NS_NewRunnableMethodWithArgs, in which case it uses nsRefPtr to store argument in runnable.
Attachment #8597695 - Flags: review?(nfroyd)
Attached patch 1153295-p4-tests.patch (obsolete) — Splinter Review
Unit tests to check appropriate storage of ref-counted classes.
Attachment #8597696 - Flags: review?(nfroyd)
Attached patch 1153295-p5-typos.patch (obsolete) — Splinter Review
Fixing some old typos, no actual code changes.
Attachment #8597697 - Flags: review?(nfroyd)
Olli, does that address your concerns? (Using nsRefPtr to store any object that has AddRef and Release methods)

Nathan, let me know if you think someone else should review these patches.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d1b1394a9cac
Flags: needinfo?(bugs)
Comment on attachment 8597692 [details] [diff] [review]
1153295-p1-addrvaluereference.patch

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

Thank you for adding tests.

::: mfbt/tests/TestTypeTraits.cpp
@@ +382,5 @@
> +              "not adding && to void* correctly");
> +static_assert(IsSame<AddRvalueReference<void>::Type, void>::value,
> +              "void shouldn't be transformed by AddRvalueReference");
> +static_assert(IsSame<AddRvalueReference<struct S1&>::Type, struct S1&>::value,
> +              "not reference-collapsing struct S1& & to struct S1& correctly");

Shouldn't this be "...struct S1& && to..."?
Attachment #8597692 - Flags: review?(nfroyd) → review+
Attachment #8597694 - Flags: review?(nfroyd) → review+
Comment on attachment 8597695 [details] [diff] [review]
1153295-p3-hasrefcountmethods.patch

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

::: xpcom/glue/nsThreadUtils.h
@@ +540,5 @@
> +static auto HasRefCountMethodsTest(int)
> +    -> SFINAE1True<decltype(mozilla::DeclVal<T>().AddRef(),
> +                            mozilla::DeclVal<T>().Release())>;
> +template<class>
> +static auto HasRefCountMethodsTest(long) -> mozilla::FalseType;

So, just to make sure I understand what's going on here, we're going to try overload resolution with |0|, which is normally an int, so we see if the SFINAETrue template can be instatiated.  If it cannot be, then we'll try the overload with the |long| argument, which requires a conversion, and hence is not chosen right off the bat.

Correct?
Attachment #8597695 - Flags: review?(nfroyd) → review+
Comment on attachment 8597696 [details] [diff] [review]
1153295-p4-tests.patch

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

Thank you for writing tests!
Attachment #8597696 - Flags: review?(nfroyd) → review+
Comment on attachment 8597697 [details] [diff] [review]
1153295-p5-typos.patch

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

Please fold this into part 4 prior to commit.
Attachment #8597697 - Flags: review?(nfroyd) → review+
Comment on attachment 8597695 [details] [diff] [review]
1153295-p3-hasrefcountmethods.patch

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

::: xpcom/glue/nsThreadUtils.h
@@ +540,5 @@
> +static auto HasRefCountMethodsTest(int)
> +    -> SFINAE1True<decltype(mozilla::DeclVal<T>().AddRef(),
> +                            mozilla::DeclVal<T>().Release())>;
> +template<class>
> +static auto HasRefCountMethodsTest(long) -> mozilla::FalseType;

Drive-by comment: I believe so.

Also, I think this is the most concise way of checking for the presence of a member that I've seen thus far. Thanks, Gerald!
(In reply to Botond Ballo [:botond] from comment #14)
> Drive-by comment: I believe so.

That was meant to be a response to:

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #11)
> So, just to make sure I understand what's going on here, we're going to try
> overload resolution with |0|, which is normally an int, so we see if the
> SFINAETrue template can be instatiated.  If it cannot be, then we'll try the
> overload with the |long| argument, which requires a conversion, and hence is
> not chosen right off the bat.
> 
> Correct?
(In reply to Botond Ballo [:botond] from comment #14)
> Drive-by comment: I believe so.
That's right, int first, long second. I suppose I could have put '...' as a more obvious fallback (and it would line up better with 'int' above!)

> Also, I think this is the most concise way of checking for the presence of a
> member that I've seen thus far. Thanks, Gerald!
Thanks, I've tried many options (picked up around the intertubes) but some wouldn't work with MSVC, some wouldn't work with our private-destructor classes, etc.
It looks concise thanks to DeclVal!

Thanks Nathan for the reviews, I'll collapse parts 4 and 5.
Fix typo in AddRvalueReference test text as per comment 10. Carrying r+.
Attachment #8597692 - Attachment is obsolete: true
Attachment #8598353 - Flags: review+
Rebase of 2nd patch due to change in 1st patch. Carrying r+.
Attachment #8597694 - Attachment is obsolete: true
Attachment #8598357 - Flags: review+
Combine patches 4 (unit tests) and 5 (typos in nearby unit tests). Carrying r+.
Attachment #8597696 - Attachment is obsolete: true
Attachment #8597697 - Attachment is obsolete: true
Attachment #8598365 - Flags: review+
Flags: needinfo?(bugs)
Keywords: checkin-needed
Assignee: nobody → gsquelart
You need to log in before you can comment on or make changes to this bug.