201521 - 2DES encrypt/decrypt references missing third 3DES key

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Terry Hayes]

The 2DES encryption algorithm is similar to 3DES. Both perform and encryption 
step, then a decryption step, followed by another encryption step (EDE).  
However 2DES uses the same key for both of the encryption (E) steps.

NSS does not handle 2DES keys correctly.  Rather than reusing the first 
subkey, it references key material past the end of the key data buffer.  As a 
result the incorrect result is calculated.

Workaround: if keys are imported with PK11_ImportSymKey it is possible to 
import the 2DES key as a 3DES key by expanding the key into a full 3DES key 
(copying the first subkey into the third position).

Workaround: the subkeys can also be imported into normal DES keys and the 2DES 
encryption (EDE steps) performed by the application.

I don't think there is any workaround for 2DES keys generated within the 
token. (not imported)

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

This appears to be a reference to the softoken implementations of
CKM_DES2_KEY_GEN, CKM_PBE_SHA1_DES2_EDE_CBC, and the validation of CKK_DES2 keys.

freebl only supports DES and 3-key triple-DES.  2-key triple-DES can be 
implemented by using the same key material for the first and third keys
in 3-key triple-DES.   Therefore, all support for 2-key triple-DES must be 
done in softoken.  

Terry, how does a mozilla or NSS user encounter this problem?  
Does CMS/PKCS7 use this form of encryption?  or ?

I'm marking this P2, but if it's accessing memory past the end of an allocated
block, it should be P1.  I'll also take this bug.

Comment 2

[Terry Hayes]

There are not any current applications (such as Mozilla) that use 2-key triple 
DES (2DES).  However a future application might need to use it.

Currently NSS will access memory beyond the allocated block in the following 
case.
1) Generate a 2DES key on the token (using the PKCS11 key gen mech)
2) Use it to perform encryption or decryption

A possible solution is to expand all 2DES keys into full 3-key buffers.  
However, other functions such as wrap would need to be reviewed to make sure 
the correct data (the smaller 2DES key) is used to produce the output.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

This raises an interesting question.  
Are the contents of wrapped keys defined anywhere?  
If not, do we have any reason to expect interoperability between PKCS11 devices
based on wrapped keys?

Based on Terry's statement above, I think this is a candidate for P1, but 
I don't what what is the correct fix, because I don't have any definition of
a wrapped DES2 key.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v1

This patch performs 2 main changes:
1. It checks the length of DES keys in handleObject, and
2. It transforms DES2 keys into DES3 keys in CryptInit.
I have not yet tested this patch (working from home).

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 134003 [details] [diff] [review]
patch v1

Please read bug comments before reviewing patch.

Comment 6

[Robert Relyea]

Comment on attachment 134003 [details] [diff] [review]
patch v1

This is definately the correct fix, especially as, Nelson noticed, NSS already
did key transformations before the decrypt for CDMF.

Some caveats: The stricter check in validateSecretKeys is correct, we need to
make sure that we or our apps don't depend on the more relaxed behavior (I
don't anticipate any problems, since the stricter checks only apply to DES and
bad things will happen if they are wrong anyway).

I'm not sure what the intent of the patch to NSC_Unwrap is. For DES keys
key_length should always be '0' since DES has known fixed length keys. The
current code automatically truncates the key to the size of the incoming data.
This is probably an error condition (truncating the incoming key to the value
in the template, however, is a reasonable thing to  do).

If this code is meant to handle the case where we get a DES2 key expanded to
the full EDE size, then it will fail to do so because keysize will be 0 (not
16) and the new check in validateSecretKey will reject the key with
CKR_KEY_SIZE_RANGE. 

The only iffy thing is the key_length check in Unwrap. Technically we probably
should be verifying that the keylength matches the mechanism type and failing
if it doesn't. Since the code doesn't do that today, we

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

/cvsroot/mozilla/security/nss/lib/softoken/softoken.h,v  <--  softoken.h
new revision: 1.4; previous revision: 1.3

/cvsroot/mozilla/security/nss/lib/softoken/pkcs11c.c,v  <--  pkcs11c.c
new revision: 1.53; previous revision: 1.52

/cvsroot/mozilla/security/nss/lib/softoken/pkcs11.c,v  <--  pkcs11.c
new revision: 1.83; previous revision: 1.82

Terry, please verify this fix with Monday's trunk NSS build.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Terry reports that there is an additional bug in pk11wrap which also must be
fixed.  Function PK11_GetKeyType takes a key length argument, which is only
used for 3DES mechanisms.  The function expects the length to be in bits, but
the values passed to it are in bytes (at least sometimes).  

Suggested patch:

Index: pk11wrap/pk11slot.c
===================================================================
RCS file: /cvsroot/mozilla/security/nss/lib/pk11wrap/pk11slot.c,v
retrieving revision 1.71
diff -u -r1.71 pk11slot.c
--- pk11wrap/pk11slot.c 12 Sep 2003 20:01:56 -0000      1.71
+++ pk11wrap/pk11slot.c 29 Oct 2003 22:04:03 -0000
@@ -2901,7 +2901,7 @@
     case CKM_DES3_MAC:
     case CKM_DES3_MAC_GENERAL:
     case CKM_DES3_CBC_PAD:
-       return (len == 128) ? CKK_DES2 : CKK_DES3;
+       return (len == 16) ? CKK_DES2 : CKK_DES3;
     case CKM_DES2_KEY_GEN:
     case CKM_PBE_SHA1_DES2_EDE_CBC:
        return CKK_DES2;

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Terry, please review my patch in comment 8 above and add your review comments
here,  (e.g. r=  :-)

Comment 10

[Robert Relyea]

r=relyea.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Thanks Bob.  Marking fixed again.

/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11slot.c,v  <--  pk11slot.c
new revision: 1.72; previous revision: 1.71

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

But wait! There's more!

In order to be able to generate DES2 keys, PK11_GetKeyGen must return
CKM_DES2_KEY_GEN.  This patch enables that.  Bob or Terry, please review.


diff -u -r1.72 pk11slot.c
--- lib/pk11wrap/pk11slot.c     30 Oct 2003 22:31:09 -0000      1.72
+++ lib/pk11wrap/pk11slot.c     31 Oct 2003 00:48:01 -0000
@@ -3077,6 +3077,8 @@
     case CKM_DES3_CBC_PAD:
     case CKM_DES3_KEY_GEN:
        return CKM_DES3_KEY_GEN;
+    case CKM_DES2_KEY_GEN:
+       return CKM_DES2_KEY_GEN;
     case CKM_CDMF_ECB:
     case CKM_CDMF_CBC:
     case CKM_CDMF_MAC:

Comment 13

[Terry Hayes]

r=thayes

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11slot.c,v  <--  pk11slot.c
new revision: 1.73; previous revision: 1.72