207033 - Add func to export encrypted private key info using SECKEYPrivateKey

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P2)

Description

[Tej Arora]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.0.1) Gecko/20020823 Netscape/7.0
Build Identifier: 

Format for DSA/RSA public/private Key components (arbitrary-length 
integers) in XML is base64-encoded big-endian octet strings.
(described at http://www.w3.org/TR/xmldsig-core/#sec-CryptoBinary)


Background:

To support NSS as a crypto-provider for xmlsec library, the following
functionality is needed for RSA & DSA keys. There are PUBLIC functions 
available for all except 6 & 7. The types used for the keys will
be SECKEYPublicKey and SECKEYPrivateKey.

(Note: private keys in XML is not required by XML-DSIG spec but
supported in xmlsec for testing purposes. The private keys are
in the clear in XML - since there is no methodology
to specify a encryption/decryption mechanism for the private key).

1. Generate key pair
2. Dup Key Handle
3. Sign & Encrypt
4. Free
5. Get Key Strength

6. Build key from components in an XML file
   (More info at http://www.w3.org/2000/09/xmldsig#DSAKeyValue and
                 http://www.w3.org/2000/09/xmldsig#RSAKeyValue
   <!ELEMENT DSAKeyValue ((P, Q)?, G?, X?, Y, J?, (Seed, PgenCounter)?) >
   <!ELEMENT RSAKeyValue (Modulus, exponent, private exponent?) > 
7. Decompose key into components, so that the components
   can be written to an XML file


Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Two comments:

1. Sending private keys in the clear is a very bad practice.  We don't want to 
encourage it, not even for "test" purposes.  I can't think of any legit reason 
why any software, including test software, should be sending private keys in 
the clear at all.  If a test program thinks it needs to transmit the private 
key, then that test program is generating the key pair in the wrong place.
It should be generating the key pair where the private key is needed, and
transmitting the public key to the other party.

We CERTAINLY do not want to make it easy for people who don't know better to
start sending private keys in the clear in real applications!  If we allow
it, they will do it, guaranteed.

2. It seems to me that the XMLSig/XMLSec library will need some "glue" code
to make it work with NSS.  It's not at all clear to me that we want to put
all such glue code into NSS itself.  The serialiaziation code requested here
seems like a prime example of such glue.

Comment 2

[Wan-Teh Chang]

Assigned the bug to Nelson.

Comment 3

[Tej Arora]

1. I should first clarify that the private keys are not really
transported, but kept in the clear in an XML file as components.
But I think, the crux of your objection is private keys in the
clear. Am I right?.

Anyway, NSS already supports importing clear private keys in DER
format. So, allowing the import of clear private key components isn't
really encouraging any NEW bad behavior.
As for export of clear private keys, I don't think NSS allows it
(even in non-FIPS mode). So, if I'm right, then supporting the
export of private key components in the clear would add a new
behavior that is undesirable.

The xmlsec test harness places generated and pre-generated 
private keys in the clear in an xml file. It then uses that
xml file for running various tests using that key for signing,
and key wrapping. Without the support in NSS, the test harness
would need some work if we want to run it for NSS.


2. IMHO, the ability to source PKI keys from its components is not
glue code and belongs in the crypto library.

In any case, if the consensus is that it is "glue", I'd be happy
to write the glue - I just need help from someone in pointing me to
** PUBLIC APIs** that I can use for this purpose. I had no luck
finding such APIs.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

For Public keys:

Public function PK11_ImportPublicKey exists, and will import any of the
supported public key types from their components.  It takes a SECKEYPublicKey
struct which has the components in it and imports it into the selected slot
creating a PKCS11 key object.  It puts the info about the created key object
into that same SECKEYPublicKey struct (the struct serves as both input and output). 

For exporting public keys, once the program has obtained the relevant
SECKEYPublicKey structure, that structure already contains the components, and
no additional function calls are needed to obtain the components.


For Private Keys:  

NSS's API design is committed to promoting good secure key hygiene, and not
promoting bad key hygiene.  

Private keys should, in general, be generated in a secure token, and should
preferably never leave that token.  They should be used in the same token in
which they are generated.  They should not require transport.  When transport is
required, each private key should be encrypted, and should not leave the token
unencrypted.  Similarly, the private key should only be imported in encrypted
form.  When a private key is imported or exported, and is encrypted (or
"wrapped") with a symmetric key, that symmetric key should preferably be derived
via a key exchange or key agreement protocol between trusted tokens.  Private
keys should exist in unencrypted form only within trusted tokens, and should
exist only in encrypted form, and only for limited periods of time outside of
trusted tokens.  To store or transport a private key unencrypted is egregious
key hygiene.  Use of inadequately protected private keys yields false security.
 Software that practices good key hygiene has no need of APIs to import or
export private keys from raw unencrypted components, except inside the
boundaries of a highly trusted "token".  This is what our FIPS 140 evaluations
are all about.

Functions PK11_ImportPrivateKey and PK11_ImportAndReturnPrivateKey exist,  but
are deliberately private NSS functions, using a deliberately private type,
SECKEYRawPrivateKey.  This type and all the functions that use it are private
for the reasons explained above. 

NSS interfaces intentionally make good key hygiene easier than bad key hygiene.
 Allowing the import of DER-encoded-but-unencrypted private keys allows users to
make the same mistakes as if NSS allowed the private keys to be imported from
components.  They're both bad key hygiene.  But requiring the NSS user to DER
encode the key for import makes doing this bad thing much more work for the
progammer.  It's a matter of degree.  The extra work discourages the bad
practice.  Removing that work encourages the bad practice.  Anything we do to
make it easier to use private keys in the clear, in  component form, encourages
bad key hygiene among programs that use NSS.

NSS imports unencrypted-but-DER-encoded private keys.  This was done to
facilitate the migration of keys used in Java programs that use another
company's crypto API (which does NOT encourage good key hygiene, IMO) to JSS. 
It is a one way migration.  Unencrypted keys come in, they don't go back out. 
Providing a migration path for lots of Java code to use JSS and NSS was deemed
sufficient justification to weaken NSS's public API in this way. 

So far, all I've read as justification for allowing the import of private keys
from raw unencrypted components is that some TEST PROGRAM for libXMLSec wants to
store private keys way.  That does NOT seem like a good justification for
further weakening NSS's defenses against bad key hygiene.  And it's not setting
a good example for programmers who use test programs as sample code, IMO.

Test programs may have legitimate need to do things that should never be done in
applications that really strive for security.  That is why some of NSS's test
programs link with NSS's static libraries - so they can access private
NSS-internal interfaces.   However, I do not recommend this, for the reason
that progammers who use libXMLSec will tend to use the test programs as sample
code to followin in their implementations.  So, it's extra important that those
test programs practice good key hygiene.

My recommendation is to change the test programs to store encrypted private keys
in the form of an "encryptedPrivateKeyInfo" rather than in plain text component
form. I believe that encryptedPrivateKeyInfo format is supported by the other
crypto library presently used with libXMLsec, since that crypto llbrary also 
supports PKCS12 files.  

PK11_ExportEncryptedPrivateKeyInfo exports private keys wrapped appropriately.

Having said that, I see one possible issue.  The function to export encrypted
private key infos takes a cert argument to identify the private key.  That is,
the private key to be exported is identified by passing a cert that contains the
corresponding public key.  I could see adding another function that uses a
SECKEYPrivateKey pointer to identify the private key, instead.  

Shall I do that?

Comment 5

[Tej Arora]

Importing/exporting Public Keys:
   What you're recommending implies directly accessing fields of the
   SECKEYPublicKey structure (since there are no accessor functions for
   key components). I just want to make sure this is OK. So far, I was
   trying to avoid directly accessing fields of any structure 
   (e.g. using SECKEY_GetPublicKeyType instead of pubkey->keyType)

Importing/exporting private keys:
   Your point is well taken. I'll investigate the use of encrypted
   private keys in the xmlsec test harness.

>Having said that, I see one possible issue.  The function to export encrypted
>private key infos takes a cert argument to identify the private key.  That is,
>the private key to be exported is identified by passing a cert that contains the
>corresponding public key.  I could see adding another function that uses a
>SECKEYPrivateKey pointer to identify the private key, instead. 
>
>Shall I do that? 
   That would be great. Thanks.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Tej, AFAIK, directly accessing the members of SECKEYPublicKey's union "u", 
and even constructing your own SECKEYPublicKey (with a NULL slot pointer) 
to pass into PK11_ImportPublicKey, are permitted and supported operations. 
Indeed, that's the only way to import a public key from components, AFAIK.

It's probably a good idea to the SECKEYPublicKey's arena pointer to be non-null
and for the contents of the public key to actually come from that arena.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

I'm not sure there's any remaining work here to be done in NSS.

I think Tej has already changed the test code to use PKCS12 files for 
the private keys, so I think that part of this request is no longer 
needed.

NSS provides direct access to the components of a public key.
serialization and deserialization of those components can be done in 
in the XML SEC glue according to whatever format XML wants to use.
Putting those things in XML format certainly seems like a job for the
XML code, not for NSS.

So, what part of this RFE, if any, still needs to be done in NSS?

Comment 8

[Tej Arora]

I beleive there is no work needed now related to this bug.
I think it can be closed as invalid or wontfix.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Upon re-reading comment 4 and comment 5 above, I see that I offered to 
produce a new function equivalent to PK11_ExportEncryptedPrivateKeyInfo
except that it takes a key argument rather than a cert argument to specify
the private key to be exported.  I'm not sure how useful that really is,
but it's easy to do (I think) so I'll do that for NSS 3.9.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v1

Implement new function PK11_ExportEncryptedPrivKeyInfo which is just like 
PK11_ExportEncryptedPrivateKeyInfo except that it takes a SECKEYPrivateKey
pointer as an argument, rather than a cert pointer.  

ReImplement PK11_ExportEncryptedPrivateKeyInfo to get the private key for
the cert and then call PK11_ExportEncryptedPrivKeyInfo to do the work.

Comment 11

[Wan-Teh Chang]

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

Can we choose a function name that's more different
from the existing PK11_ExportEncryptedPrivateKeyInfo
than PK11_ExportEncryptedPrivKeyInfo is?

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

I'm open to suggestions.  

This change was intended to ensure that the two names were unique within 
the first 30 characters.  If I had added an additional suffix, the two names 
would have been the same for the first 30 characters, which seems somewhat
undesirable.  Other names could also meet that objective.

Comment 13

[Wan-Teh Chang]

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

In the new function PK11_ExportEncryptedPrivKeyInfo,
we should move the "if(pk == NULL)" check to the beginning
of the function and set the SEC_ERROR_INVALID_ARGS error
code before the "goto loser" statement.

I see why you chose this function name.  I don't have a
better suggestion.  Bob?

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v2

This patch incorporates Wan-Teh's suggestsions from comment 13.
It also plugs an apparent leak of an algid.

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

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

I believe that the old code leaked the algid that is returned from this call:
algid = SEC_PKCS5CreateAlgorithmID(algTag, NULL, iteration);
This code plugs that leak.  Please confirm that it was a leak.

This patch catches several places where NULL pointers could have been returned
and were not previously caught.

I suspect that after creating that algid from the oidtag "algtag", the
expression 
SECOID_FindOIDTag(&algid->algorithm)
is equivalent to the variable algTag.  
Bob, can you confirm or refust that?

Comment 16

[Wan-Teh Chang]

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

r=wtc.

As far as I can tell, the old code leaks the algid created
by the SEC_PKCS5CreateAlgorithmID call.  I'd like Bob to confirm
this.

Don't forget the nss.def change to export the new function in
NSS 3.9.

Comment 17

[Robert Relyea]

Yup, it leaked the algid. And yes the call to look up the algtag is redundant
and wasteful since we used it to create the algid.

Good patch.

Comment 18

[Robert Relyea]

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

r=relyea

Comment 19

[Nelson Bolyard (seldom reads bugmail)]

/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11func.h,v  <--  pk11func.h
new revision: 1.43; previous revision: 1.42

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

/cvsroot/mozilla/security/nss/lib/nss/nss.def,v  <--  nss.def
new revision: 1.113; previous revision: 1.112

I have one more patch for this bug, coming up shortly.

Comment 20

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Supplemental patch, v1

This patch removes one (of many) redundant SECOID table lookups, and adds a
missing error check (that should never happen, but better safe than sorry).

Comment 21

[Robert Relyea]

Comment on attachment 132773 [details] [diff] [review]
Supplemental patch, v1

r=relyea

Comment 22

[Nelson Bolyard (seldom reads bugmail)]

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