334442 - Incorrect use of realloc oom Crash in secmod_ReadPermDB

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[timeless]

found by coverity

Comment 1

[timeless]

please see bug 244478 comment 13 for an explanation of why what this code is doing is very very very wrong.

Comment 2

[timeless]

Attachment: properly use realloc

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 218783 [details] [diff] [review]
properly use realloc

r=nelson

Comment 4

[Daniel Veditz [:dveditz]]

How does this crash rather than just leak?

Comment 5

[Daniel Veditz [:dveditz]]

And who's going to check in the patch?

Comment 6

[Daniel Veditz [:dveditz]]

Timeless points out the code says "if (!moduleList[0])", not the
"if (moduleList)" my brain saw.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

NSS team members will do all checkins.  Want to batch them up, since there 
will apprently be quite a few.  I *expect* (not a promise) that most of 
these will go into 3.11.1 in time for FF 2.0 Beta.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Alexei, please check in the above reviewed fix on both trunk and 3.11 branch.
In the checkin comment, be sure to mention that the patch is 
contributed by [e-mail]
Thanks.

Comment 9

[Alexei Volkov]

Check into the tip:
/cvsroot/mozilla/security/nss/lib/softoken/pk11db.c,v  <--  pk11db.c
new revision: 1.36; previous revision: 1.35

Check into the 3.11 branch:
/cvsroot/mozilla/security/nss/lib/softoken/pk11db.c,v  <--  pk11db.c
new revision: 1.35.2.1; previous revision: 1.35

Comment 10

[Daniel Veditz [:dveditz]]

Comment on attachment 218783 [details] [diff] [review]
properly use realloc

Please check this into the 1.8.0 and 1.8 branches as well, and add "fixed1.8.1" and "fixed1.8.0.3" keywords when you've done that. Thanks!

approved for 1.8.0 branch, a=dveditz for drivers

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Kai, do you have trees for 1.8.0.3 and 1.8.1+?
If so, would you be willing to do the checkins of this bug's patch on those
trees?  They're already approved (see previous comment).

Comment 12

[Kai Engert (:KaiE:)]

done

1.8 branch:
Checking in pk11db.c;
/cvsroot/mozilla/security/nss/lib/softoken/pk11db.c,v  <--  pk11db.c
new revision: 1.32.20.2; previous revision: 1.32.20.1
done

1.8.0 branch:
Checking in pk11db.c;
/cvsroot/mozilla/security/nss/lib/softoken/pk11db.c,v  <--  pk11db.c
new revision: 1.32.30.1; previous revision: 1.32
done

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Many thanks, Kai.

Comment 14

[Alexei Volkov]

thank you, Kai!

Comment 15

[Bob Clary [:bc] (inactive)]

Alexei, any idea on how to test this?

Comment 16

[Daniel Veditz [:dveditz]]

Maybe I'm missing something but I don't see the security issues here -- it looks like the old code is at worse a leak followed immediately by a null deref crash in the OOM case.

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

Daniel, feel free to remove the security flag from this bug as you see fit.
It was set by the reporter.  I don't see how OOM crashes are exploitable,
either.

Comment 18

[Nelson Bolyard (seldom reads bugmail)]

CID 224