572809 - PK11_GenerateKeyPair doesn't null check parameters, dereferences and crashes

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Kai Engert (:KaiE:)]

Call PK11_GenerateKeyPair with type CKM_DSA_KEY_PAIR_GEN, use NULL for parameters, and it will crash.

Should crashes be security sensitive? Please open if you think it's not necessary. I'll also mark the related bug 104103 as sensitive, because I will add a way to trigger this crash from web content.

Comment 1

[Kai Engert (:KaiE:)]

Please see bug 104103 comment 13 for further details about the crash.

Comment 2

[Kai Engert (:KaiE:)]

My reading of the code is, PK11_GenerateKeyPairWithOpFlags will always typecast and dereference "params".

I propose that a null check (and error return) is added to function PK11_GenerateKeyPairWithOpFlags.

Comment 3

[Kai Engert (:KaiE:)]

Attachment: Patch v1

like this?

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 452033 [details] [diff] [review]
Patch v1

r=nelson
I agree with your analysis, Kai.

Comment 5

[Kai Engert (:KaiE:)]

Checking in lib/pk11wrap/pk11akey.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11akey.c,v  <--  pk11akey.c
new revision: 1.30; previous revision: 1.29
done

Comment 6

[Daniel Veditz [:dveditz]]

A null-deref is generally a "safe" crash and doesn't normally need to be security-sensitive in client code (execution jumping to null is another story, but just trying to read it isn't bad). Writing to null needs investigation to make sure null is the only possible bad value (e.g. you forgot to check the return value from malloc before writing to it) and that there isn't some alternate path that could cause you to write to arbitrary values.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

This is a duplicate of 4-year-old public Bug 343868.
It is already public.  It need not be security sensitive any more.