320187 - NSC_WrapKey called with null output returns short length

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Neil Williams]

NSC_WrapKey() when called with an RSA key and a null output pointer sets the output length to two times the input length. It should set the length value to the modulus so that a buffer of appropriate length can be allocated.

Comment 1

[Neil Williams]

I believe the following lines starting at 836 in nss/lib/softokn/pkcs11c.c are
the source of the problem.

 ...
    crv = sftk_GetContext(hSession,&context,SFTK_ENCRYPT,PR_FALSE,&session);
    if (crv != CKR_OK) return crv;

    if (!pEncryptedData) {
	*pulEncryptedDataLen = ulDataLen + 2 * context->blockSize;
	goto finish;
    }
 ...

The output ptr, pEncryptedData, is null so *pulEncryptedDataLen will be set but, since context->blockSize is 0, it will be to the input length which is too small.

Comment 2

[Robert Relyea]

Note: calling NSC_WrapKey is more broken than this, see patch for bug 319619

The patch for bug 319619 does not completely solve the problem of RSA keys, so I'm keeping this bug open and making it dependent on 319619.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

The patch for bug 319619 seems to deal exclusively with private keys.
But this bug concerns wrapping with public keys (as happens when 
encrypting the pre-master secret).  So it's not clear that bug 319619 
blocks this bug, or even has much to do with this one.

The code for computing output buffer size (quoted in a previous comment) 
was written with symmetric ciphers in mind.  It deals with simple stream 
ciphers and with simple block ciphers, but not with ciphers whose block 
size is tha function of the key size (such as RSA).  

Note that is is also an issue for NSC_Encrypt also.  The code in question
appears to be common to those two.

Comment 4

[Robert Relyea]

The patch deals with problems wrapping with symetric keys as well as public keys (though the bug itself only describes wrapping private keys with symetric keys).

The patch is insufficient to fix this problem, but is also required to complete the fix. (without the patch, subsequent calls to NSC_WrapKey will mysteriously fail with 'CKR_OPERATION_ACTIVE').

Comment 5

[Robert Relyea]

Attachment: Return correct length RSA operations when NULL is passed.

Nelson, if you want this in 3.11, add another reviewer.

This patch not only fixes WrapKey, but also other C_ calls that are supposed to return a length.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Bob, I have emailed some review comments.  If your reply persuades me that
this code is correct in the two places where I have questions, then I will
ask julien or wan-teh for SR.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 210396 [details] [diff] [review]
Return correct length RSA operations when NULL is passed.

>Index: pkcs11c.c

In  NSC_DecryptUpdate(), this patch adds:

>@@ -923,6 +942,16 @@
>     crv = sftk_GetContext(hSession,&context,SFTK_DECRYPT,PR_TRUE,NULL);
>     if (crv != CKR_OK) return crv;
> 
>+    if (!pPart) {
>+	if (context->doPad) {
>+	    *pulPartLen = 
>+		ulEncryptedPartLen + context->padDataLength - context->blockSize;
>+	    return CKR_OK;
>+	}

Bob, I think you and I have agreed that the value returned in *pulPartLen 
must always be an integer multiple of the block size.  But the code shown
above doesn't ensure that.  For example, if ulEncryptedPartLen  is 1
and context->padDataLength is zero (no previously buffered data), and 
context->blockSize is 8 (DES), then the value put into *pulPartLen will
be -7, which is not a multiple of context->blockSize.  

Similarly, for any combination of ulEncryptedPartLen + context->padDataLength
that do not sum to a multiple of context->blockSize, this code will return a 
value that is not a multiple of context->blockSize.

Comment 8

[Robert Relyea]

Attachment: Return correct length in C_XXX operations when NULL is passed.

added check for length. One length is completely under control of this application and is an assert. The other length is passed in by the user and returns the appropriate PKCS #11 error code.

Comment 9

[Robert Relyea]

Evidently there was a checkin between these two patches, but the new code should be obvious in the inter patch diff.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 211205 [details] [diff] [review]
Return correct length in C_XXX operations when NULL is passed.


> +    if (!pPart) {
> +	if (context->doPad) {
> +	    /* we can check the data length here because if we are padding,
> +	     * then we must be using a block cipher. In the non-padding case
> +	     * the error will be returned by the underlying decryption
> +	     * function when do do the actual decrypt. We need to do the
> +	     * check here to avoid returning a negative length to the caller.
> + 	     */
> +	    if ((context->padDataLength == 0) ||
> +		(context->padDataLength % context->blockSize) != 0) {
> +		return CKR_ENCRYPTED_DATA_LEN_RANGE;
> +	    }
> +	    *pulPartLen = 
> +		ulEncryptedPartLen + context->padDataLength - context->blockSize;
> +	    return CKR_OK;

I think I see these problems:

1. On the very first call to NSC_DecryptUpdate, context->padDataLength == 0,
so if !pPart on the first call, it will return CKR_ENCRYPTED_DATA_LEN_RANGE.

2. Even if issue #1 above is fixed, it is possible and allowed for 
ulEncryptedPartLen  to be zero.  If that happens on the first call, then
*pulPartLen would get the negative block size.  

3. If ulEncryptedPartLen is not a multiple of the blocksize, this will
set pulPartLen to a strange value, also not a multiple of the blocksize.

Here's a possible alternative:

    if ((ulEncryptedPartLen % context->blockSize) != 0)
        return CKR_ENCRYPTED_DATA_LEN_RANGE;
    if ((context->padDataLength % context->blockSize) != 0) 
        return CKR_ENCRYPTED_DATA_INVALID;
    *pulPartLen = 
        (ulEncryptedPartLen + context->padDataLength > context->blockSize) 
      ? (ulEncryptedPartLen + context->padDataLength - context->blockSize) 
      : 0;
    return CKR_OK;

Comment 11

[Robert Relyea]

Arg! it's not supposed to be context->padDataLength here, it's supposed to be ulEncryptedPartLen (sigh).

bad context->padDataLength is handled by the assert, and cannot be caused by bad user data, so furthur checks should be unnecessary.

bob

I'll return the correct patch

Comment 12

[Robert Relyea]

Attachment: OK, This check should be correct.

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 211236 [details] [diff] [review]
OK, This check should be correct.

r=nelson@bolyard

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

Bob, bug 301394 seems very much like this bug.  
Does the fix for this bug (reviewed patch above) also fix that bug?
If so, please mark one as a dup of the other.

Comment 15

[Wan-Teh Chang]

I just resolved bug 301394 as invalid.

Comment 16

[Robert Relyea]

Comment on attachment 211236 [details] [diff] [review]
OK, This check should be correct.

Need second review for 3.11
checkin

Comment 17

[Robert Relyea]

Tip version has been checked in:
revision 1.74
date: 2006/02/09 19:54:22;  author: rrelyea%redhat.com;  state: Exp;  lines: +68
 -14

Bug 320187 NSC_WrapKey called with null output returns short length
r=nelsonb

Comment 18

[Wan-Teh Chang]

Comment on attachment 211236 [details] [diff] [review]
OK, This check should be correct.

r=wtc.

In NSC_Encrypt, we have:

>     if (!pEncryptedData) {
>-	*pulEncryptedDataLen = ulDataLen + 2 * context->blockSize;
>+	*pulEncryptedDataLen = context->multi ? 
>+		ulDataLen + 2 * context->blockSize : context->maxLen;
> 	goto finish;
>     }

I have a question about the original code.  I understand we need
to add one block size for the padding, but what is the second block
size for?

In NSC_DecryptUpdate, "do do" should be "we do".

The following is a comment, not a suggested change.  In
NSC_EncryptUpdate and NSC_Encrypt, we assume that for multi-part
encryption, the output ciphertext is the same length as the input
plaintext.  This is true for all the symmetric ciphers we support.
If we don't want to depend on this assumption, we will need to change
all the context->update functions to handle a null output buffer
pointer properly.  Moving the buffer length calculation to the
context->update functions is a more robust approach although
it is also a lot more work.

Comment 19

[Robert Relyea]

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

checked into 3.11