825477 - Fix passing of pointer to stack variable to WrapRunnableRet/NS_DISPATCH_NORMAL

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect, P1)

Comment 1

[Eric Rescorla (:ekr)]

Patch to follow.

Comment 2

[Eric Rescorla (:ekr)]

Attachment: Don't get return values from async calls

Comment 3

[Eric Rescorla (:ekr)]

Attachment: Don't get return values from async calls

Comment 4

[Eric Rescorla (:ekr)]

Attachment: Don't get return values from async calls

Comment 5

[Eric Rescorla (:ekr)]

Comment on attachment 696581 [details] [diff] [review]
Don't get return values from async calls

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

WrapRunnableRet assigns the return value to the last value. This is obviously unsafe if that is a pointer
to a stack variable and you are NS_DISPATCH_NORMAL.

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 696581 [details] [diff] [review]
Don't get return values from async calls

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

Definitely

Comment 7

[Eric Rescorla (:ekr)]

Comment on attachment 696581 [details] [diff] [review]
Don't get return values from async calls

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

The underlying issue is that we are assigning the return value
of the dispatched function to *ret. But since the stack
has returned, this is just some random stack frame depending
on thread timing. Generally, we will be setting *ret = 0
and I don't *think* the attacker can make it fail with
an arbitrary value. Currently, it is causing random crashes.

* How easily can the security issue be deduced from the patch?

With a modest amount of thought the defect can be seen,
though it's not obvious how to exploit it.

*  Do comments in the patch, the check-in comment, or tests included
   in the patch paint a bulls-eye on the security problem?

No.

* Which older supported branches are affected by this flaw?

None. Though we have not scrubbed for related problems in
older branches (all of which are preffed off).


* If not all supported branches, which bug introduced the flaw?

Bug 820102 (Landed 12/21)

*  Do you have backports for the affected branches? If not, how different,
   hard to create, and risky will they be?

N/A

* How likely is this patch to cause regressions; how much testing does
  it need?

This is clearly a defect and has a very low chance of making things
worse.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

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