326144 - softoken leaks in nsc_pbe_key_gen

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Julien Pierre]

This was reported by an internal customer. nsc_pbe_key_gen doesn't free the iv returned by nsspkcs5_ComputeKeyAndIV after it has copied the value to pbeParams->pInitVector . I will attach a patch.

Comment 1

[Julien Pierre]

Attachment: free iv after it is copied

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 210928 [details] [diff] [review]
free iv after it is copied

r=nelson.bolyard

>+    if (iv.data) {
>+        if (pbe_params->pInitVector != NULL) {
>+	    PORT_Memcpy(pbe_params->pInitVector, iv.data, iv.len);
>+        }
>+        PORT_Free(iv.data);
>     }

Curious that struct CK_PBE_PARAMS has no member for the length of 
the IV buffer.  There's no way for the PKCS#11 module to verify that
it's not about to overflow the caller's buffer.  :-/

Comment 3

[Robert Relyea]

Comment on attachment 210928 [details] [diff] [review]
free iv after it is copied

r+ rrelyea

Comment 4

[Julien Pierre]

Nelson, Bob,

Thanks for the quick reviews. I checked this in to the tip :

Checking in pkcs11c.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11c.c,v  <--  pkcs11c.c
new revision: 1.73; previous revision: 1.72

And to NSS_3_11_BRANCH :
Checking in pkcs11c.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11c.c,v  <--  pkcs11c.c
new revision: 1.68.2.3; previous revision: 1.68.2.2
done

Comment 5

[Wan-Teh Chang]

Comment on attachment 210928 [details] [diff] [review]
free iv after it is copied

The error code path in nsspkcs5_ComputeKeyAndIV uses
PORT_ZFree to free iv.data.  I'm wondering if it is
necessary to zero iv.data before freeing.  It would
be good to free iv.data the same way in the two
places.

Comment 6

[Julien Pierre]

Nelson,

Re: comment #2, I believe there is such a field in the NSSPKCS5PBEParameter structure : ivLen .

Wan-Teh,

Re: comment #5,
Good catch, Wan-Teh. We can use the ivLen field to pass as 2nd argument to PORT_ZFree .

Comment 7

[Wan-Teh Chang]

Comment on attachment 210928 [details] [diff] [review]
free iv after it is copied

Julien, if we should use PORT_ZFree to free
iv.data, we can just use iv.len as the second
argument.  But I'm not sure if we need to zero
iv.data before freeing.  In CBC mode of block
ciphers at least, IV can be made public because
it is the first block of ciphertext.  I'm not
familiar with the code in question, so I don't
know if iv.data contains any sensitive data.

Comment 8

[Julien Pierre]

Attachment: incremental patch to use PORT_ZFree

Comment 9

[Wan-Teh Chang]

Comment on attachment 210954 [details] [diff] [review]
incremental patch to use PORT_ZFree

>-        PORT_Free(iv.data);
>+        PORT_ZFree(iv.data, pkcs5_pbe->ivLen);

Please use iv.len as the second argument when
you check this in.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Comments, 

IVs are generally not regarded as plaintext or keys, but rather as ciphertext.
Consequently, they don't need to be zero'd before being freed.

The NSSPKCS5PBEParameter structure is not available to the patched function, 
is it?

Comment 11

[Julien Pierre]

Nelson,

Re: comment #10, this structure is available to the patched function - it passes it on the following line as the first argument :
pbe_key = nsspkcs5_ComputeKeyAndIV(pkcs5_pbe, &pwitem, &iv, faulty3DES);
But as Wan-Teh pointed out, iv->len is also available.

Glen, Wan-Teh,
What does FIPS140-2 say about the requirement for clearing IVs ? If there is none, then I think the fix can just be left as-is, clearing it will just be an unnecessary inefficiency. Otherwise, I will check in the patch that uses iv->len as the 2nd argument to PORT_ZFree.

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

iv.len is the source length.  
I'm concerned about the length of the destination buffer, to avoid overflows.
What assurance do we have that iv.len does not exceed the length of 
pbe_params->pInitVector ?

Comment 13

[Julien Pierre]

Nelson,

Check the following assignment in nsspkcs5_ComputeKeyAndIV in lowpbe.c, line 587 :
	iv->len = pbe_param->ivLen;

Comment 14

[Wan-Teh Chang]

FIPS 140-2 requires zeroization of plaintext keys and CSPs, where
CSPs (critical security parameters) are security-related information
(e.g., secret and private cryptographic keys, and authentication
data such as passwords and PINs) whose disclosure or modification
can compromise the security of a cryptographic module.

The IV in PKCS #5 is the IV for CBC mode.  So this IV will become
the first ciphertext block, and therefore shoudn't need to be
zeroized.  However, I found that we zeroize the PKCS #5 IV in
nsspkcs5_ComputeKeyAndIV (error path) and nsspkcs5_CipherData.
It would be good to be consistent (either always zeroize, or
never zeroize).

But I found that in nsspkcs5_ComputeKeyAndIV we don't zeroize
the 'hash' SECItem, which contains a copy of the key!

Comment 15

[Wan-Teh Chang]

Attachment: Zeroize 'hash' in nsspkcs5_ComputeKeyAndIV

'hash' should be zeroized because it contains a copy
of the key, as the PORT_Memcpy calls.

Under 'loser', 'key' cannot possibly contain any key
bytes.  So we don't need to zeroize 'key'.  I can omit
this change if you find it not worth the risk (of new
code breaking this invariant).

Comment 16

[Robert Relyea]

Comment on attachment 211096 [details] [diff] [review]
Zeroize 'hash' in nsspkcs5_ComputeKeyAndIV

r+ relyea.

I wonder if we shouldn't do late allocation of the key secitem, since we don't use it until the last step.

Comment 17

[Julien Pierre]

Comment on attachment 211096 [details] [diff] [review]
Zeroize 'hash' in nsspkcs5_ComputeKeyAndIV

I'm fine with the first part of this patch, but I think the second part should be left alone. As you say, there is risk of this invariant in the code changing over time. The cost is probably small. If you are worried about performance of keygen, I would be OK with this change, but otherwise it is best left out.

Re: Bob's comment, unless you are worried about keygen performance, it's probably not worth making the change to delay allocation.

Comment 18

[Robert Relyea]

Actually, my comment was more for code readability. You would only get the peformance win on the error path (not exactly the case I would optimize for:).

Comment 19

[Wan-Teh Chang]

Attachment: Zeroize 'hash' in nsspkcs5_ComputeKeyAndIV (as checked in)

I listened to Julien and removed the change to
not zeroize the SECItem 'key' on the error path.
I checked in this patch on the NSS trunk (3.12)
and NSS_3_11_BRANCH (3.11.1).

Checking in lowpbe.c;
/cvsroot/mozilla/security/nss/lib/softoken/lowpbe.c,v  <--  lowpbe.c
new revision: 1.15; previous revision: 1.14
done

Checking in lowpbe.c;
/cvsroot/mozilla/security/nss/lib/softoken/lowpbe.c,v  <--  lowpbe.c
new revision: 1.14.2.1; previous revision: 1.14
done