Bug 1176340 (CVE-2016-1976)

DesktopDisplayDevice::operator= uses members after delete on self-assignment

RESOLVED FIXED in Firefox 45

Status

()

defect
P2
normal
Rank:
21
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: q1, Assigned: jesup)

Tracking

({sec-low})

38 Branch
mozilla45
Points:
---
Bug Flags:
sec-bounty -

Firefox Tracking Flags

(firefox45 fixed)

Details

(Whiteboard: [adv-main45+][post-critsmash-triage])

Attachments

(1 attachment)

(Reporter)

Description

4 years ago
On self-assignment, DesktopDisplayDevice::operator= (38.0.1\media\webrtc\trunk\webrtc\modules\desktop_capture\desktop_device_info.cc) deletes its deviceNameUTF8_ and deviceUniqueIdUTF8_ pointers, then uses the deleted pointers to create new objects:

78: DesktopDisplayDevice& DesktopDisplayDevice::operator= (DesktopDisplayDevice& other) {
79:   screenId_ = other.getScreenId();
80:   setUniqueIdName(other.getUniqueIdName());
81:   setDeviceName(other.getDeviceName());
82:
83:   return *this;
84: }

70: const char *DesktopDisplayDevice::getDeviceName() {
71:   return deviceNameUTF8_;
72: }

74: const char *DesktopDisplayDevice::getUniqueIdName() {
75:   return deviceUniqueIdUTF8_;
76: }

58: void DesktopDisplayDevice::setDeviceName(const char *deviceNameUTF8) {
59:   SetStringMember(&deviceNameUTF8_, deviceNameUTF8);
60: }
61:
62: void DesktopDisplayDevice::setUniqueIdName(const char *deviceUniqueIdUTF8) {
63:   SetStringMember(&deviceUniqueIdUTF8_, deviceUniqueIdUTF8);
64: }

16: static inline void SetStringMember(char **member, const char *value) {
17:   if (!value) {
18:     return;
19:   }
20: 
21:   if (*member) {
22:     delete [] *member;
23:     *member = NULL;
24:   }
25: 
26:   size_t  nBufLen = strlen(value) + 1;
27:   char *buffer = new char[nBufLen];
28:   memcpy(buffer, value, nBufLen - 1);
29:   buffer[nBufLen - 1] = '\0';
30:   *member = buffer;
31: }

Lines 80/81 resolve to the calls SetStringMember (&deviceUniqueIdUTF8_, deviceUniqueIdUTF8_) / SetStringMember (&deviceNameUTF8_, deviceNameUTF8_). Line 21 then deletes the member pointers and nulls them out, but the argument value holds a temporary copy of those pointers, which lines 26 et seq use to build new objects and assign them to the member variables.

This bug could leak sensitive information and/or crash, but it seems that it could not overwrite unowned memory.

Updated

4 years ago
Component: Untriaged → WebRTC
Product: Firefox → Core
Assignee: nobody → rjesup
backlog: --- → webRTC+
Rank: 15
Priority: -- → P1
Flags: sec-bounty?
In terms of severity, I believe there's no code in the tree that does (or can) do a self-assignment of this class.  It's a latent bug, but would only become an issue if we wrote code that allowed it to get hit.  I may be even able to just DISABLE_COPY_AND_ASSIGN this class, otherwise I'll implement a more complex fix.
Keywords: sec-low
Flags: sec-bounty? → sec-bounty-
This turns out to not be an issue. Please let us know if it does turn out to be an issue, Ron, and we'll re-examine for bounty.
(Reporter)

Comment 3

4 years ago
(In reply to Al Billings [:abillings] from comment #2)
> This turns out to not be an issue. Please let us know if it does turn out to
> be an issue, Ron, and we'll re-examine for bounty.

Ya. I do suggest fixing it anyway, so it never comes back to bite. The risk of a self-assignment check should be tiny.
(In reply to q1 from comment #3)
> (In reply to Al Billings [:abillings] from comment #2)
> > This turns out to not be an issue. Please let us know if it does turn out to
> > be an issue, Ron, and we'll re-examine for bounty.
> 
> Ya. I do suggest fixing it anyway, so it never comes back to bite. The risk
> of a self-assignment check should be tiny.

Yup.  I plan to land a fix this cycle.

Updated

4 years ago
Group: core-security → media-core-security
Reminder to land a fix, even if it's sec-low
Flags: needinfo?(rjesup)
Rank: 15 → 21
Priority: P1 → P2
Flags: needinfo?(rjesup)
Posted patch desktop.patchSplinter Review
Attachment #8682128 - Flags: review?(jib)
Attachment #8682128 - Flags: review?(jib) → review+
https://hg.mozilla.org/mozilla-central/rev/aff89c5a3121
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Group: media-core-security → core-security-release
Whiteboard: [adv-main45+]
Whiteboard: [adv-main45+] → [adv-main45+][post-critsmash-triage]
Alias: CVE-2016-1976
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.