226285 - regression: certs with odd RSA key sizes don't work

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Johnny Stenback (:jst)]

The above URL stopped working in Mozilla sometime between 20031229 and 20031110.
Now when you load that URL you get an alert saying "Error establishing an
encrypted connection to support.dell.com. Error code: -8152.". This error comes
from pk11_InitGeneric():

    /* find the key */
    if (keyPtr) {
	unsigned int size;
        key = pk11_ObjectFromHandle(hKey,session);
        if (key == NULL) {
-->	    return CKR_KEY_HANDLE_INVALID;
    	}

Comment 1

[Johnny Stenback (:jst)]

blocking1.6 and 1.6b per discussion with Asa.

Comment 2

[Stephane Saux]

Note: I will not look at this bug. If it blocks 1.6 someone must look at it.
I've requested that [e-mail] not be the default owner for such bugs.

Comment 3

[Darin Fisher]

could this be related to the NSS 3.9 beta 3 landing?

Comment 4

[Darin Fisher]

jst: by 20031229, you maybe meant 20031029? ;-)

Comment 5

[Wan-Teh Chang]

Bob, Nelson, could you take a look at this bug?

Comment 6

[Wan-Teh Chang]

Assigned the bug to Bob.  Changed product to NSS for investigation.

Error code -8152 is SEC_ERROR_INVALID_KEY: The key does not support
the requested operation.

NSS 3.9 Beta 2 landed on Monday, 2003-11-17.  If this bug was
introduced between 20031029 and 20031110 (Johnny, please confirm),
it must be caused by a change on the NSS_3_8_BRANCH in that time
interval.  Bob, I suggest that you look at the checkins on the
NSS 3.8 branch after NSS_3_8_2_RTM.  You can search in both Bugzilla
and Bugscape for bugs with target milestone 3.8.3 or [3.8.3] in
the status whiteboard.

Comment 7

[Johnny Stenback (:jst)]

Duh, yes, I meant 20031029. And the testing I did yesterday did show that this
was introduced before 11/17.

Comment 8

[Johnny Stenback (:jst)]

Further testing shows that this problem showed up between the builds 2003110709
and 2003110909. From looking at bonsai, there are no changes to nss in that time
window, but there are some from the day before. I'm not sure exactly when the
code for the earlier build was pulled...

Comment 9

[Radosław Szkodziński]

It does happen only with Dell site, not with either my Apache server,
SourceForge, GRC or Hushmail.
Seems to only happen with this site. What is so special about it?

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

I am not at all certain this is a regression.  It may be that this is 
another case where NSS is now properly enforcing PKI rules about which it
was lax before.  But we won't know for certain until we find the real issue.

Today, I was able to reproduce this error with a fresh trunk tip build, 
using tstclnt.  I set a breakpoint on the line of code indicated above
by the bug reporter.  The stack at that point is:

pk11_InitGeneric(0x005403a0, 0x0012e760, 0, 0x0012e764, 0, 0x0012e778, 2, 262)
line 355
pk11_CryptInit(10, 0x0012e7f8, 0, 262, 0, 1) line 448 + 44 bytes
NSC_WrapKey(10, 0x0012e7f8, 0, 2, 0x00540308, 0x0012e7f4) line 3630 + 26 bytes
PK11_PubWrapSymKey(1, 0x0053df20, 0x0053de28, 0x0012e844) line 2434 + 41 bytes
sendRSAClientKeyExchange(0x00533070, 0x0053df20) line 3593 + 19 bytes
ssl3_SendClientKeyExchange(0x00533070) line 4281 + 13 bytes
ssl3_HandleServerHelloDone(0x00533070) line 5319 + 9 bytes
ssl3_HandleHandshakeMessage(0x00533070, 0x0053779d, 0) line 8068 + 9 bytes
ssl3_HandleHandshake(0x00533070, 0x00533218) line 8156 + 25 bytes
ssl3_HandleRecord(0x00533070, 0x0012e9cc, 0x00533218) line 8424 + 13 bytes
ssl3_GatherCompleteHandshake(0x00533070, int 0) line 203 + 22 bytes
ssl_GatherRecord1stHandshake(0x00533070) line 1260 + 11 bytes
ssl_Do1stHandshake(0x00533070) line 149 + 13 bytes
ssl_SecureSend(0x00533070, 0x0012eab4, 16, 0) line 1033 + 9 bytes
ssl_SecureWrite(0x00533070, 0x0012eab4, 16) line 1067 + 19 bytes
ssl_Write(0x00533008, 0x0012eab4, 16) line 1282 + 21 bytes
PR_Write(0x00533008, 0x0012eab4, int 16) line 143 + 20 bytes
main(7, 0x00502800) line 857 + 24 bytes

So, the problem is with the public key that is imported from the server's
cert.  More as I find it.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

There are two different issues here:

1. This cert has a 1023-bit public key, not a 1024-bit public key.
We recently added code that checks RSA public keys for certain validity 
constraints, one of which is that the key length in bits is an even number.
The test that actually fails is this one in lib/softoken/pkcs11.c

    switch (key_type) {
    case CKK_RSA:
	crv = pk11_ConstrainAttribute(object, CKA_MODULUS,
						 RSA_MIN_MODULUS_BITS, 0, 2);
	if (crv != CKR_OK) {
--->	    return crv;
	}

crv is 19

Perhaps we need to change that constraint.  (More on this below)

The stack at this point is:

pk11_handlePublicKeyObject(0x0050deb8, 0x00548830, 0) line 1050
pk11_handleKeyObject(0x0050deb8, 0x00548830) line 1611 + 17 bytes
pk11_handleObject(0x00548830, 0x0050deb8) line 1798 + 13 bytes
NSC_CreateObject(1, 0x0012e74c, 8, 0x0012e7c8) line 3680 + 13 bytes
PK11_CreateNewObject(0x0050fa00, 0, 0x0012e74c, 8, 0, 0x0012e7c8) line 142 + 24
bytes
PK11_ImportPublicKey(0x0050fa00, 0x0053dce0, 0) line 632 + 33 bytes
PK11_PubWrapSymKey(1, 0x0053dce0, 0x0053e500, 0x0012e844) line 2429 + 15 bytes
sendRSAClientKeyExchange(0x00533070, 0x0053dce0) line 3593 + 19 bytes
[rest is same as stack trace above.]


The second issue is this:  

The above noted failure causes C_CreateObject to return 19 to
PK11_CreateNewObject.  That function maps 19 to SEC_ERROR_BAD_DATA and returns
SECFailure to PK11_ImportPublicKey, which in turn returns CK_INVALID_HANDLE
(which is zero) to PK11_PubWrapSymKey.

When PK11_PubWrapSymKey gets back CK_INVALID_HANDLE from its call to
PK11_ImportPublicKey, PK11_PubWrapSymKey doesn't check the return value for 
CK_INVALID_HANDLE, and so it calls down to C_WrapKey with an invalid handle.
This is not a new bug.  

PK11_PubWrapSymKey should detect CK_INVALID_HANDLE and not call C_WrapKey 
in that case.  

I suggest the following patch to fix this second problem.
This patch goes to function PK11_PubWrapSymKey in lib/pk11wrap/pk11skey.c

    mechanism.pParameter = NULL;
    mechanism.ulParameterLen = 0;

    id = PK11_ImportPublicKey(slot,pubKey,PR_FALSE);
+   if (id == CK_INVALID_HANDLE)
+       return SECFailure;  /* error code was set by Import */

    session = pk11_GetNewSession(slot,&owner);


Ultimately, the question is: is it proper to reject this key because 
its key length is not an even number?  
If we decide it is not proper, then we can simply remove the call to 
pk11_ConstrainAttribute shown above.  

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Taking bug.  (I thought "Accept bug" would reassign it to me )

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

IMO, this _IS_ a regression, and we should remove the test cited above.  
I will attach a real patch to this bug report shortly.

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

Although this is a regression, I don't think it's "MAJOR" or "CRITICAL".  
It only affects the very few web sites with public keys whose prime factors
are not of equal length.

But it's still P1 for NSS 3.9

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Patch v1 - allow RSA public keys with odd lengths

I don't think this is the _final_ solution to this, but it's an appropriate
expedient until we find that solution.

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 136034 [details] [diff] [review]
Patch v1 - allow RSA public keys with odd lengths

I wish we knew the right constraint for modulus, rather than eliminating the
modulus constraint test alltogether.

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

Changing bug summary to reflect actual problem.

Comment 18

[Wan-Teh Chang]

Comment on attachment 136034 [details] [diff] [review]
Patch v1 - allow RSA public keys with odd lengths

1.

>     case CKK_RSA:
>+#if 0  /* what IS an appropriate constraint test for the modulus ? XXX */
> 	crv = pk11_ConstrainAttribute(object, CKA_MODULUS,
> 						 RSA_MIN_MODULUS_BITS, 0, 2);
> 	if (crv != CKR_OK) {
> 	    return crv;
> 	}
>+#endif

Should we restore the original code (below)?

-	if ( !pk11_hasAttribute(object, CKA_MODULUS)) {
-	    return CKR_TEMPLATE_INCOMPLETE;
-	}

2.

>     id = PK11_ImportPublicKey(slot,pubKey,PR_FALSE);
>+    if (id == CK_INVALID_HANDLE)
>+	return SECFailure;   /* Error code has been set. */

It seems that this and the first error returns in this function
should call
    PK11_FreeSymKey(newKey);
?

Comment 19

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v2

In answer to your two questions:

1. I think it's better to leave the minimum modulus length constraint test in
place, and just remove the length-multiple test.

2. Yes, you're right.

I think this patch addresses those issues.

Comment 20

[Johnny Stenback (:jst)]

Yeah, this wouldn't be critical if it wasn't for the fact that it breaks
dell.com, which is a rather frequently visited site. Thanks for digging into this!

Comment 21

[Wan-Teh Chang]

Comment on attachment 136037 [details] [diff] [review]
patch v2

r=wtc.

1. Just to clarify a point in my previous comment.  I asked if we
should revert the current code:

	crv = pk11_ConstrainAttribute(object, CKA_MODULUS,
						 RSA_MIN_MODULUS_BITS, 0, 2);
	if (crv != CKR_OK) {
	    return crv;
	}

to the previous code:

	if ( !pk11_hasAttribute(object, CKA_MODULUS)) {
	    return CKR_TEMPLATE_INCOMPLETE;
	}

rather than removing the current code.

2. In lib/pk11wrap/pk11skey.c, function PK11_PubWrapSymKey,
it seems that the first error return

    if ((symKey == NULL) || (symKey->slot == NULL)) {
	PORT_SetError( SEC_ERROR_NO_MODULE );
	return SECFailure;
    }

should also free 'newKey' if 'newKey' is not NULL.

Comment 22

[Robert Relyea]

Comment on attachment 136034 [details] [diff] [review]
Patch v1 - allow RSA public keys with odd lengths

We should at least verify the modulus to be non-zero.

We probably should verify the modulus we generate are full length. a 1023
modulus is half as strong as a 1024 bit. (not for this patch).

I'm approving the patch because it does fix the regression.

(though a better patch would be to change the '2' at the end to a '0')

Comment 23

[Wan-Teh Chang]

Bob, you reviewed the obsolete patch.  Could you review the
new patch (v2)?  I believe it does exactly what you suggested
(a better patch would be to change the '2' at the end to a '0').

If you think patch v2 is good, please mark it review+ and request
approval.  Thanks.

Comment 24

[Robert Relyea]

Comment on attachment 136037 [details] [diff] [review]
patch v2

r=relyea...

looks good.

Comment 25

[Ian McGreer]

I agree with the changes in patch v2.

Comment 26

[Nelson Bolyard (seldom reads bugmail)]

Patch checked in on trunk.  Marking fixed.

Comment 27

[Brendan Eich [:brendan]]

Comment on attachment 136037 [details] [diff] [review]
patch v2

a=[e-mail] and drivers, for 1.6b.

/be

Comment 28

[Wan-Teh Chang]

Patch checked into the NSS_CLIENT_TAG for Mozilla 1.6b.

Comment 29

[Wan-Teh Chang]

Nelson, if I only apply your pk11wrap/pk11skey.c change, the error
code I get is -12219 (SSL_ERROR_CLIENT_KEY_EXCHANGE_FAILURE).  Is
that the right error code?

Comment 30

[Wan-Teh Chang]

Attachment: Free the sym key 'newKey' on another error path

Bob, Nelson, should we free 'newKey' on this error path, too?

Comment 31

[Wan-Teh Chang]

Bob, Nelson, could you take this opportunity to review the
other constraints (for RSA public exponents, DSA keys, and
DH keys) and make sure that none of them is overreaching?

Comment 32

[Wan-Teh Chang]

Comment on attachment 136037 [details] [diff] [review]
patch v2

I carried this patch back to the NSS_3_4_BRANCH,
NSS_3_7_BRANCH, and NSS_3_8_BRANCH so that it
will be included if we ever need to respin any
of these branches.