183612 - Add support for EncryptedData w/ subjKeyID to CMS libraries.

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P1)

Description

[Javier Delgadillo]

I'd like to add some functionality to the smime libraries to add support for
creating, encoding, and decoding NSSCMSEncryptedData messages that use the
subjKeyID method for identifying a key as opposed to the traditional
issuer+serialNummber method.

The patch consists of the following:
1) Add functions that allows an application to create an NSSCMSRecipientInfo
that uses the subjKeyID instead of the traditional cert issuer + serialNumber.
2) Export a utility function that allows apps to wrap a bulk key for a content
info. (This made it possible to add the functionality without modifying data
structures allowing us to keep binary compatibility.)
3) Logic to the CMS encoder that respected the bulk key when it was already set
by the application.
4) Adding functions to keyhi.h that let the application register mappings
between a subjectKeyIdentifier and a private key.  This allows the CMS decoder
to find the private key needed to unwrap the bulk key.
5) Add functionality to the CMS decoder to decrypt/decode NSSCMSEnvelopedData
objects that have a recipient info that maps to the an entry in the
subjKeyID/PrivateKey Table if we don't have a cert that matches the cert issuer
+ serialNumber in a different RecipientInfo

Patch coming shortly.

Comment 1

[Javier Delgadillo]

taking bug

Comment 2

[Javier Delgadillo]

Attachment: patch to implement functionality.

Comment 3

[Wan-Teh Chang]

Comment on attachment 108292 [details] [diff] [review]
patch to implement functionality.

Javi, I'll ask Bob and Nelson to review your patch.
Here I will suggest some coding style changes.

1. In nss.def and smime.def, the exported symbols
should be sorted alphabetically.

2. The new smime/cms functions should have two
underscores in their names.  Some of them have three
or four underscores in their names.  The extra
underscores should be deleted.

I see no changes to any public struct or union.  Did
you figure out a way to avoid that?

Comment 4

[Javier Delgadillo]

Changes to public structs were avoided by allowing the application to set the
bulkkey as well as wrap the bulk key instead of making the encoder wrap the bulk
key for all cases.

On the decode side, I avoided structure changes by adding functions to keyhi.h
that allowed the app to set the mappings from subjKeyID to a private key.

With these 2 changes, no changes to structures were necessary.

Comment 5

[Javier Delgadillo]

Attachment: patch v2

Implemented suggestions from last week's meeting.

Comment 6

[Wan-Teh Chang]

Attachment: patch v3

Latest patch from Javi.

I've checked in this patch on the trunk and
NSS_3_7_BRANCH.

Comment 7

[Aleksey Nogin]

This checkin have added a warning on brad TBox:

  cmsrecinfo.c:409: warning: `extra' might be used uninitialized in this function

Indeed, if cert is non-null on line 415 and certalttag is
SEC_OID_PKCS1_RSA_ENCRYPTION on line 438, then on line 441 the "extra" will be
used uninitialized:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/smime/cmsrecinfo.c&rev=1.8&mark=409,415,438,441#397

P.S. Per bug 179819 these "uninitialized" warnings ought to be considered
compilation errors.

Comment 8

[Wan-Teh Chang]

Attachment: Patch to fix uninitialized variable in cmsrecinfo.c

Aleksey, you are right.  'extra' may indeed be used
uninitialized.

Here is my proposed patch.  I don't really understand
the code.  I just made the if-else-if statement exactly
the same as the one above.  Javi, Bob, could you please
review this patch?

Comment 9

[Aleksey Nogin]

Comment on attachment 109578 [details] [diff] [review]
Patch to fix uninitialized variable in cmsrecinfo.c

This might fix the problem, but I doubt it would shut up the compiler. It would
be nice to get rid of the warning as well.

Comment 10

[Javier Delgadillo]

Comment on attachment 109578 [details] [diff] [review]
Patch to fix uninitialized variable in cmsrecinfo.c

r=javi on the latest patch.

Comment 11

[Wan-Teh Chang]

Comment on attachment 109578 [details] [diff] [review]
Patch to fix uninitialized variable in cmsrecinfo.c

This patch has been checked into the NSS trunk
and NSS_3_7_BRANCH.

Comment 12

[Aleksey Nogin]

The warning is still there.

Comment 13

[Wan-Teh Chang]

Attachment: Patch to rename functions and declare internal functions in private headers

This proposed patch does two things.

1. It renames some functions for consistency with
existing functions.

CERT_VerifySignedDataWithPubKeyInfo is renamed
CERT_VerifySignedDataWithPublicKeyInfo because
the existing CERT_xxx functions use "PublicKey",
not "PubKey".  (PK11_xxx functions use "PubKey".)

CERT_xxxSubjKeyIDxxx is renamed
CERT_xxxSubjectKeyIDxxx because the existing
libnss3 functions use "SubjectKey", not "SubjKey".
NSS_CMSRecipientInfo_CreateWithSubjKeyID is left
unchanged because libsmime3 uses "SubjKey" more
often.	Unfortunately I can only go for consistency
within a shared library for this one.

2. The internal functions CERT_xxxSubjectKeyxxx
and SECMOD_xxxCallOncexxx are moved from the public
headers certdb.h and secmod.h to the private headers
certi.h and secmodi.h.	I didn't bother to change
their prefixes to lowercase though.

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 109687 [details] [diff] [review]
Patch to rename functions and declare internal functions in private headers

I'd like to see the new internal CERT_ functions that deal with the Hash table
and mapping of subject key IDs be named lower case cert_* to make it more
apparent that we do NOT want these functions in the public API.  But other than
that, this patch looks fine to me.  We could make that change after releasing
3.7, since they are internal functions only.

Comment 15

[Wan-Teh Chang]

Attachment: Patch to rename functions and declare internal functions in private headers, v2

Thanks for the review, Nelson.	I changed the CERT_ functions
that deal with the hash table and mapping of subject key IDs
to be named lower case cert_* as you suggested.

I've checked in this patch on the trunk and the NSS_3_7_BRANCH.

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

Wan-Teh, your most recent patch above addresses most of the concerns I had
about libNSS in "patch v3".  I have one additional review comment about libNSS.  
It's minor, and doesn't have to be fixed before 3.7 is released.  

The functions SECMOD_InitCallOnce() and SECMOD_CleanupCallOnce() should be 
declared and defined with an argument list of "(void)" instead of "()".

I'm reviewing the changes to libSMIME next.

Comment 17

[Wan-Teh Chang]

Marked the bug fixed.

Comment 18

[Wan-Teh Chang]

Attachment: Add some comments

Comment 19

[Julien Pierre]

I wish I had been copied on this bug before. The memory management model you
selected in some of the new APIs is incorrect.

Eg. CERT_FindSubjectKeyIDExtension allocates memory on the heap through the
ASN.1 decoder SEC_ASN1DecodeItem, without using an arena . This means the result of
the decoding is allocated by the decoder on the heap, and the caller is
responsible for freeing the returned retItem using PORT_Free afterwards.

Besides the fact that this is not good design for a new API, this is a
deprecated usage of the decoder, which can cause a known memory leak may occur.
Please take a look at bug 175163 for more details.

I discovered the problem while trying to convert the code to use QuickDER.
Unfortunately the code was already released in NSS 3.7, and I can't safely
convert it to QuickDER without changing the semantics of the function, and
preferably the prototype. The correct way would be to add an arena argument.

Unfortunately this seems to be a problem common to most of the cert extension
lookup functions.

Comment 20

[Wan-Teh Chang]

Yes, we overlooked this issue when we reviewed these
patches.  But I think it is not as bad as it seemed.

The fact that CERT_FindSubjectKeyIDExtension needs to
use an ASN.1 decoder is an implementation detail that
the caller should not need to know about.  Moreover,
the subject key ID extension is returned in a SECItem,
a simple data structure, so freeing a partially
allocated data structure on decoding error is not an
issue here.

What we need to make sure is that CERT_FindSubjectKeyIDExtension
either does not touch 'retItem' or sets 'retItem->data'
to NULL if it fails.  So it may need to do something
like this:

+   retItem->data = NULL;
    rv = SEC_ASN1DecodeItem (NULL, retItem, SEC_OctetStringTemplate,
                             &encodedValue);
+   if (rv == SECFailure) {
+       PORT_Free(retItem->data);
+   }

I am not sure if this is necessary though.