1176340 - (CVE-2016-1976) DesktopDisplayDevice::operator= uses members after delete on self-assignment

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P2)

Description

[q1]

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.

Comment 1

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

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.

Comment 2

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 3

[q1]

(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.

Comment 4

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

(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.

Comment 5

[Maire Reavy [:mreavy]]

Reminder to land a fix, even if it's sec-low

Comment 6

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

Attachment: desktop.patch

Comment 7

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

 https://hg.mozilla.org/integration/mozilla-inbound/rev/aff89c5a3121

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/aff89c5a3121