338352 - Coverity Leak and OOM crash in PK11_PubDeriveWithKDF (security/nss/lib/pk11wrap/pk11skey.c)

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Kenneth Herron]

This is coverity CID 317. Please see the sample URL. The allocation into |mechParams| at line 1710 leaks if the test on the next line succeeds. 

Additionally, it looks like |PORT_ZNew| is just a wrapper around |PR_Calloc| which can fail and return null, but there's no check for that here.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

To be consistent with the other coverity bugs about NULL pointer dereferences
this bug should be critical.

Taking.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Attachment: plug leaks, eiminate unnecessary creation+freeing of symkey, reduce scope of numerous variables

This patch:
a) plugs the leak
b) eliminates the OOM crash
c) eliminates the unnecessary creation and immediate destruction of a 
   symkey that occurred for all cases except ecKey.  (Did that serve a 
   useful purpose?)
d) moves all the variable declarations and initializations that are ONLY 
   used for case ecKey into case ecKey.  

Bob, please review

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Add requested reviewer to cc list

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 222478 [details] [diff] [review]
plug leaks, eiminate unnecessary creation+freeing of symkey, reduce scope of numerous variables

Seeking additional reviewers.  Just need one (I think)

Comment 5

[Wan-Teh Chang]

Comment on attachment 222478 [details] [diff] [review]
plug leaks, eiminate unnecessary creation+freeing of symkey, reduce scope of numerous variables

r=wtc.

Remark 1: This patch reformats all the variable
declarations to a different coding style.  I
implore Nelson to resist the urge to make this
kind of changes.  It leaves the file in a mixture
of coding styles.

Remark 2: This function can be further improved by
rewriting the switch statement as an if statement:

    /* variable declarations in the original ecKey case */
    ...

    if (privKey->keyType != ecKey) {
        return PK11_PubDerive(privKey, pubKey, isSender, randomA, randomB,
                derive, target, operation, keySize, wincx);
    }

    /*
     * the original ecKey case, replacing 'break' with
     * 'return NULL'
     */
    ...

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Alternative patch 2, factor EC case out into separate function

Maybe this patch is clearer than the previous one.  
Bob? what do you think?

Comment 7

[Robert Relyea]

Comment on attachment 222987 [details] [diff] [review]
Alternative patch 2, factor EC case out into separate function

r+ = relyea
both patches are acceptable. This one is preferred.

bob

Comment 8

[Robert Relyea]

Comment on attachment 222478 [details] [diff] [review]
plug leaks, eiminate unnecessary creation+freeing of symkey, reduce scope of numerous variables

r+

Though the other patch is preferred.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Checked in on branch.

Fix leak and OOM crash.  Bug 338352. Coverity.  r=rrelyea

Checking in pk11skey.c; new revision: 1.108.2.1; previous revision: 1.108

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Fix leak and OOM crash.  Bug 338352 and 338356. Coverity.  
r=rrelyea, alexei.volkov

Checking in pk11skey.c; new revision: 1.109;     previous revision: 1.108
Checking in pk11skey.c; new revision: 1.108.2.1; previous revision: 1.108

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

CID 317