283565 - Improper use of Realloc (mlk/crash) and silly return value for OOM

Status: VERIFIED FIXED

(Core Graveyard :: Security: UI, defect)

Description

[timeless]

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/xpcom/build/nsXPCOM.h&rev=1.15&mark=227-246#226
see also bug 248695 / bug 244478 comment 13.

Comment 1

[Daniel de Wildt]

Attachment: Handle realloc correct and return correct error

Comment 2

[Daniel Veditz [:dveditz]]

Comment on attachment 176371 [details] [diff] [review]
Handle realloc correct and return correct error

As long as we're touching this code we should clean it up.
combine the Realloc/Alloc cases by dropping the if (mData) test.

There's also no way for len to be < 0 so the final
else assertion/return is just silly.

  if (len)
    if (mLen < len)
      realloc...
    memcpy
  else if (mData)
      free

Comment 3

[Jesse Ruderman]

*** Bug 334188 has been marked as a duplicate of this bug. ***

Comment 4

[Jesse Ruderman]

When realloc decides to move the data, the existing code creates a dangling-pointer situation, so it could be a security hole.

Comment 5

[timeless]

Attachment: consistently use realloc

Comment 6

[Daniel Veditz [:dveditz]]

Comment on attachment 218583 [details] [diff] [review]
consistently use realloc

sr=dveditz

Comment 7

[Kai Engert (:KaiE:)]

Comment on attachment 218583 [details] [diff] [review]
consistently use realloc

r=kengert

Comment 8

[Daniel Veditz [:dveditz]]

Comment on attachment 218583 [details] [diff] [review]
consistently use realloc

approved for 1.8.0 branch, a=dveditz for drivers

Comment 9

[Daniel Veditz [:dveditz]]

Checked in on trunk and 1.8/1.8.0 branches:
> new revision: 1.9.28.1.2.1; previous revision: 1.9.28.1
> new revision: 1.9.28.2; previous revision: 1.9.28.1
> new revision: 1.12; previous revision: 1.11

Comment 10

[Daniel Veditz [:dveditz]]

The only use of nsNSSASN1PrintableItem::SetData I can find in Mozilla code is from buildASN1ObjectFromDER

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/manager/ssl/src/nsNSSASN1Object.cpp&rev=1.12&mark=204,209#203

Since that creates a fresh new printableItem and calls SetData on it once this must have used the alloc path and not the realloc path, thus this wasn't exploitable in practice. Perhaps in a non-Mozilla PSM client, if there is such a thing.

Kai: am I missing another use of this object?