Closed Bug 825477 Opened 7 years ago Closed 7 years ago

Fix passing of pointer to stack variable to WrapRunnableRet/NS_DISPATCH_NORMAL

Categories

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

x86
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla20
Tracking Status
firefox19 --- disabled
firefox20 --- fixed
firefox-esr17 --- unaffected
b2g18 --- disabled

People

(Reporter: ekr, Assigned: ekr)

References

Details

(Keywords: csectype-wildptr, sec-high, Whiteboard: [WebRTC] [blocking-webrtc+] [qa-] [adv-main20-])

Attachments

(1 file, 2 obsolete files)

No description provided.
Patch to follow.
Attachment #696579 - Attachment is obsolete: true
Attachment #696580 - Attachment is obsolete: true
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.
Attachment #696581 - Flags: review?(rjesup)
Comment on attachment 696581 [details] [diff] [review]
Don't get return values from async calls

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

Definitely
Attachment #696581 - Flags: review?(rjesup) → review+
Assignee: nobody → ekr
Priority: -- → P1
Whiteboard: [WebRTC] [blocking-webrtc+]
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.
Attachment #696581 - Flags: sec-approval?
Attachment #696581 - Flags: sec-approval? → sec-approval+
Blocks: 825440
https://hg.mozilla.org/mozilla-central/rev/1a7e276db7d9
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Whiteboard: [WebRTC] [blocking-webrtc+] → [WebRTC] [blocking-webrtc+] [qa-]
Whiteboard: [WebRTC] [blocking-webrtc+] [qa-] → [WebRTC] [blocking-webrtc+] [qa-] [adv-main20-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.