401928 - Support generalized PKCS#5 v2 PBEs

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P1)

Description

[Robert Relyea]

Currently NSS only supports PKCS #5 v1 and PKCS #12 PBE algorithms. 

PKCS #5 v2 defines a generalized PBE algorithm that can be used with any encryption or mac scheme.

NSS supports PKCS #5 at the softoken level, but not in the generalized PBE parameters.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Bob, I think you meant to write:
Currently, the only PBE algorithms supported by NSS are PKCS#5v1 and PKCS#12.
That's not the same as what you wrote. 

Comment 2

[Robert Relyea]

Right, NSS does support other algorithms which are not PBE algorithms.:).

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

I don't know what's driving this RFE, but I doubt that it's FireFox.
If it changes APIs, then its NSS312B1, otherwise it's not, I think.

Comment 4

[Robert Relyea]

Attachment: Softoken changes for pkcs5

This is the most critical patch. It affects the shared database code.
This enables pkcs5 for softoken, and corrects an implementation error in the pkcs 5 code.

Comment 5

[Robert Relyea]

Attachment: Implement pkcs 5 in the pk11 wrap layer.

this implements pkcs #5 v2 in the pk11 wrap layer. Applications can access this directly.

Comment 6

[Robert Relyea]

Attachment: Use the pkcs 5 v2 compatible interfaces in CMS/pkcs7 and pkcs12

This allows pkcs12 and others to use the pkcs5 v2 interfaces.

Comment 7

[Robert Relyea]

Attachment: pk12util patch, v1

This patch allows us to modify what pkcs 12 cipher we are using from pk12util on the fly. It also print out more diagnostic information when we list whats in a pkcs12 package.

Comment 8

[Robert Relyea]

order of importance:

attachment 288575 [details] [diff] [review]
Softoken changes for pkcs5:   beta1 blocker (critical).

attachment 288578 [details] [diff] [review]
Implement pkcs 5 in the pk11 wrap layer: would like to get, (blocks mozilla 400742). has api changes, so is probably a beta1 blocker. (requires the softoken patch above).

attachment 288579 [details] [diff] [review]
Use the pkcs 5 v2 compatible interfaces in CMS/pkcs7 and pkcs12: Not as critical can probably wait until 3.13 (requires both patches above).

Created an attachment (id=288581) [details]
Add the ability to control the cipher used in PKCS 12 : nice to have, not critical for 3.12. (requires all the patches above).

bob

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 288575 [details] [diff] [review]
Softoken changes for pkcs5

Bob, please attach a "unified diff" (output of diff -u) for this patch.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 288575 [details] [diff] [review]
Softoken changes for pkcs5

Some review comments.
>427c427
><     for (i=0,rp=result->data; i < l ; i++, rp +=hLen) {
>---
>>     for (i=1,rp=result->data; i <= l ; i++, rp +=hLen) {

Both the old loop and the new one never loop.  They both execute
exactly one pass.  The only difference here is that the old loop
executed one time with i==0, and the new loop executes one time
with i==1.  So, why are they structured like loops?  

>2633c2639,2640
>< 	pwitem.len = (unsigned int)pbkd2_params->ulPasswordLen;
>---
>> 	/* was this a typo in the PKCS #11 spec? */
>> 	pwitem.len = *pbkd2_params->ulPasswordLen;

I remember there was a bunch of discussion about this in the 
cryptoki mailing list.  I don't remember the decision, and I 
have lost my mail archive for that list.  But I'd prefer that
we be confident that this change is right, rather than just
hoping/guessing that it is (which is what this comment sounds 
like).

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 288578 [details] [diff] [review]
Implement pkcs 5 in the pk11 wrap layer.

Some preliminary review comments.  Review is still ongoing.

1) Some spelling errors (first one occurs many times)
:g/epricated/s//eprecated/g
:g/\<lui\>/s//liu/g

2) You significantly altered the declaration of function 
SEC_PKCS5CreateAlgorithmID in public header file secpkcs5.h .
At first, I thought you were changing a public function.  But then I 
realized that SEC_PKCS5CreateAlgorithmID is a private function.
IMO, the right thing to do is to move the declaration of that function  
out of that header file and into a private header file.  Maybe you 
should change the name, while you're at it, to have a lower case prefix 
to call attention to the fact that it's a private function.

Comment 12

[Robert Relyea]

>     for (i=1,rp=result->data; i <= l ; i++, rp +=hLen) {

> Both the old loop and the new one never loop.  They both execute
> exactly one pass.  The only difference here is that the old loop
> executed one time with i==0, and the new loop executes one time
> with i==1.  So, why are they structured like loops?  

That's an 'el' not a 'one' in the test part of the loop above. I've run interoperability tests where 'l' is not one, so I know the loop actually loops;).

>> 	/* was this a typo in the PKCS #11 spec? */
>> 	pwitem.len = *pbkd2_params->ulPasswordLen;
> I remember there was a bunch of discussion about this in the 
cryptoki mailing list. 

I vaguely remember something as well. I've been trying to get an answer to this issue from the cryptoki mailing list, but the list seems to be down. 

I agree that we need to get a definitive answer before we check in the final version.  I had to have something that's consistent. The two places where this hits is in this patch and the one Julian is reviewing.


> Stuff from comment #11-- all sound reasonable.

Comment 13

[Wan-Teh Chang]

Comment on attachment 288575 [details] [diff] [review]
Softoken changes for pkcs5

Bob, perhaps you can use capital L or a longer variable name.

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

One vs. ell: Interesting.  When I look at the diff with 
https://bugzilla.mozilla.org/attachment.cgi?id=288575&action=diff
The two characters are identical in every pixel.  
Maybe I need a different font. :-/
Let me suggest that you use a better name than the single ell character.

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

Attachment: screen shot, enlarged, showing one == ell

Comment 16

[Robert Relyea]

wtc and nelsonb  If i had written the original code, I would have picked a better name (iter probably, since it's an iteration count;). I don't see a problem changing it in the next version of the patch (comment 11 guarrentees that there will be one...

It's definately your font. I see a distinct difference in my browser...

Comment 17

[Robert Relyea]

Attachment: screen shot with a different font and a different story;)

Comment 18

[Robert Relyea]

Just to set the record straight... initial archeology into lowpbe.c seems to indicate that probably actually write the initial code that used the variable 'l'. Also the correct variable name should be nblocks instead of iter.

bob

Comment 19

[Wan-Teh Chang]

I have no problem seeing the l either.

Comment 20

[Robert Relyea]

Attachment: Update softoken patch according to review comments

Comment 21

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 292966 [details] [diff] [review]
Update softoken patch according to review comments

r=nelson for this patch

Comment 22

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 288578 [details] [diff] [review]
Implement pkcs 5 in the pk11 wrap layer.

Comments are mostly cosmetic issues with function and variable names.
They are sufficiently numerous that I'm giving r-

>     CK_MECHANISM_TYPE              mechanism;
>-    CK_MECHANISM                   pbeMech;
>+    CK_MECHANISM_TYPE              cryptoMechanism;
>     CK_MECHANISM                   cryptoMech;

These names are confusing.  There are two types of objects:
a MECHANISM (a struct) and a MECHANISM_TYPE (a small integer).  So the MECHANISM_TYPE is given the name mechanism, and the MECHANISM is given 
the name "mech".  This makes my head hurt. 

Let's call MECHANISMs mechanism, and MECHANISM_TYPEs mechtype, or 
something that clearly distinguishes between the struct and the int.  
Would be nice to clean that up everywhere, but for now, let's do it 
in this function, at least.

(I realize that this is codified in the PKCS#11 API definition now.
The CK_MECHANISM_TYPE field inside of a CK_MECHANISM has the 
name "mechanism", which is hopelessly ambiguous.  But we can do 
better in new code.

>@@ -61,8 +61,22 @@ struct SEC_PKCS5PBEParameterStr {
>     PRArenaPool *poolp;
>     SECItem     salt;           /* octet string */
>     SECItem     iteration;      /* integer */
>+    SECItem	keyLength;	/* PKCS5v2 only */
>+    SECAlgorithmID  prfAlgId;	/* PKCS5v2 only */

Please line up those columns.

>+struct sec_pkcs5V2ParameterStr {
>+    PRArenaPool	   *poolp;
>+    SECAlgorithmID pbeAlgId;   /* real pbe algorithms */
>+    SECAlgorithmID cipherAlgId; /* encryption/mac */

Alignment.

>+static SECOidTag
>+sec_pkcs5v2_get_pbe(SECOidTag algTag)
>+{
>+    /* if it's a valid hash oid... */
>+    if (HASH_GetHashOidTagByHMACOidTag(algTag) != SEC_OID_UNKNOWN) {
>+	/* use the MAC tag */
>+	return SEC_OID_PKCS5_PBMAC1;
>+    } else if (HASH_GetHashTypeByOidTag(algTag) != HASH_AlgNULL) {

Please don't use else here, after a return.  
The if can just start a new statement.

>+	/* eliminate Hash algorithms */
>+	return SEC_OID_UNKNOWN;
>+    } else if (PK11_AlgtagToMechanism(algTag) != CKM_INVALID_MECHANISM) {

Likewise, no else here, after a return.

>+	/* it's not a hash, if it has a PKCS #11 mechanism associated
>+	 * with it, assume it's a cipher. (NOTE this will generate
>+	 * some false positives). */
>+	return SEC_OID_PKCS5_PBES2;
>+    }
>+    return SEC_OID_UNKNOWN;
>+}


>@@ -273,7 +493,9 @@ sec_pkcs5_destroy_pbe_param(SEC_PKCS5PBE
> static SEC_PKCS5PBEParameter *
> sec_pkcs5_create_pbe_parameter(SECOidTag algorithm, 
> 			SECItem *salt, 
>-			int iteration)
>+			int iteration,
>+			int keyLength,  
what are the length units?  bits? Bytes? Furlongs? :) 


I don't understand this new comment paragraph below.  
It says algorithm is an algorithm.  (of course! :)
I think you're trying to explain a distinction between "algorithm", "pbeAlgorithm" and "cipherAlgorithm", but that distinction is not coming 
through to the reader.  What is an "overall algorithm"? 

Also: "Get do a get" ?  I think you meant to say that it is the 
value returned by some function whose name is something like 
"get algorithm tag".  Maybe you can give the actual function name.

>+	 * algorithm is the overall algorithm we find when get do a get
>+	 * algorithm tag on the OID.  For PKCS 5, that is not the same as
>+	 * the algorithm that points to the PKCS #11 mechanism we are going
>+	 * to use (that is what pbeAlgorithm is).  We use the cipherAlgorithm
>+	 * to determin what this should be (MAC1 or PBES2).

s/determin/determine/
What is "this" in the preceding sentence?  
I would have suggested some alternative text, but I really don't know
what you were trying to say.  Let me suggest that you send me some draft 
replacements for that paragraph in email.


>+ * public, Depricated, This function is only for binary compatibility with 
>+ * public, depricated. 

s/epricated/eprecated/ 
That spelling error occurs in many places in this patch.
Please fix them all..

In numerous places, you added a comment to an old function, saying 
that it is deprecated.  The new comments should tell the reader 
what function to use instead, like the following one.
>+ * public, depricated. use PK11_PBEKeyGen instead.

>+/* warning: cannot work with pkcs 5 v2 */
> CK_RV PK11_MapPBEMechanismToCryptoMechanism(CK_MECHANISM_PTR pPBEMechanism,
What should be used instead?

>+/* warning: cannot work with PKCS 5 v2 */
> PK11SymKey *
> PK11_RawPBEKeyGen(PK11SlotInfo *slot, CK_MECHANISM_TYPE type, SECItem *params,
What should be used instead?

>+ * This function uses the existing PKCS #11 module to find the
>+ * longest supported key length in the prefered token for a mechanism.

s/prefered/preferred/

>+ * This varies from the above function in that 1) it returns the key length
>+ * even for fixed key algorithms, and 2) it looks through the tokens
>+ * generally rather than for a specific token. This is used in lui of

s/lui/liu/  or 
s/in lui/instead/

>+ * a PK11_GetKeyLength function in pk11mech.c since we can actually read
>+ * supported key lengths from PKCS #11.
>+ *
>+ * For symmetric key operations the length is returned in bytes.
>+ */
>+int
>+PK11_GetBestKeyLengthGeneral(CK_MECHANISM_TYPE mechanism)

Given the explanation above, that name seems non-descriptive.
Maybe MaxKeyLength, or LongestKeyLength, not BestKeyLength.
Also what is General intended to convey?  Maybe "ForMechType" instead?


>+    /* no tokens regonize this mechanism */
s/regonize/recognize/


>--- lib/nss/nss.def	31 Oct 2007 17:25:28 -0000	1.181
>+++ lib/nss/nss.def	13 Nov 2007 23:56:09 -0000
>@@ -918,7 +918,9 @@ CERT_PKIXVerifyCert;
> PK11_CreateGenericObject;
> PK11_GenerateKeyPairWithOpFlags;
> PK11_GetAllSlotsForCert;
>+PK11_GetPBECryptoMechanism;
> PK11_WriteRawAttribute;
>+SEC_PKCS5IsAlgorithmPBEAlgTag;       <---  This line should ...
> SECKEY_ECParamsToBasePointOrderLen;
> SECKEY_ECParamsToKeySize;
> SECMOD_DeleteModuleEx;
> SEC_GetRegisteredHttpClient;         <--- ... be after this one
> VFY_CreateContextDirect;

Comment 23

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 288579 [details] [diff] [review]
Use the pkcs 5 v2 compatible interfaces in CMS/pkcs7 and pkcs12

Please resubmit this patch as a "unified" diff, e.g. the output of cvs diff -pu5

Comment 24

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 288581 [details] [diff] [review]
pk12util patch, v1

Bob, regarding the new function PKCS12U_MapCipherFromString
can you list here the different strings that are valid inputs to this function?  

This method of specifying algorithm OIDs seems awkward and exacting, and doesn't help the user to figure out what values are allowed.  

I'm wondering if perhaps the other commands have solved this in another way, such as using single-letter values or short strings to signify each algorithm name.

Comment 25

[Robert Relyea]

RE comment #24

The following is what I use:

AES-192-CBC
DES-EDE3-CBC
DES-CBC
RC2-CBC
etc.

You could also specify:
"PKCS #5 Password Based Encryption with MD2 and DES CBC"

but that would probably be awkward.

My goal here was to start moving to a way of specifying ciphers that the application did not need to know apriori.

What would be better is if the oid table had a list of 'command-line' friendly names for each oid separate from the display description (pkcs5-md2-des-cbc) for instance.

bob

Comment 26

[Nelson Bolyard (seldom reads bugmail)]

Regarding the strings shown in comment 25: Those are fine.  Thanks.

Comment 27

[Robert Relyea]

Attachment: Implement pkcs 5 in the pk11 wrap layer. - V2

new patch including review comments.

Comment 28

[Robert Relyea]

Attachment: Unified diff: Use the pkcs 5 v2 compatible interfaces in CMS/pkcs7 and pkcs12

this patch is simply the unified diff form of the previous ": Use the pkcs 5 v2 compatible interfaces in CMS/pkcs7 and pkcs12" (attachment 228579 [details] [diff] [review]).

Comment 29

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 296025 [details] [diff] [review]
Implement pkcs 5 in the pk11 wrap layer. - V2

I previously reviewed this version of the patch, but apparently my review comments never got attached to this bug, and are now lost.  I don't recall those comments now. :(

This patch appears to address my published comments for the previous version, so r=nelson

Comment 30

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 296028 [details] [diff] [review]
Unified diff: Use the pkcs 5 v2 compatible interfaces in CMS/pkcs7 and pkcs12

r=nelson

The code being patched by this patch has the same problems with 
ambiguous variable naming as the other patch did.  There's no 
distinction in variable names between mechanisms and mechanism_types.
Ideally I'd like that to all get cleaned up, as you did for the 
other patch.  But that can be done later in a separate pass, and 
need not hold up this beta.

Here are a few format issues that I'd like you to fix before committing.

1. In file lib/smime/cmsencdata.c  there were some way-too-long lines
that this patch makes even longer by adding more indentation. 
Please fold the too-long lines in function 
NSS_CMSEncryptedData_Create, including these:

> 	rv = NSS_CMSContentInfo_SetContentEncAlg(poolp, &(encd->contentInfo), algorithm, NULL, keysize);

>+	    rv = NSS_CMSContentInfo_SetContentEncAlgID(poolp, &(encd->contentInfo), pbe_algid, keysize);
>+	    SECOID_DestroyAlgorithmID (pbe_algid, PR_TRUE);
> 	}
>-	rv = NSS_CMSContentInfo_SetContentEncAlgID(poolp, &(encd->contentInfo), pbe_algid, keysize);
>-	SECOID_DestroyAlgorithmID (pbe_algid, PR_TRUE);


2. In lib/pkcs12/p12e.c
> 	    /* initialize hmac */
>-	    /* XXX NBB, why is this mech different than the one above? */
>+	    /* Get the hmac mechanism. from the hash oid */
  Please remove this extraneous dot  ^  and capitalize HMAC and OID.

Comment 31

[Robert Relyea]

Attachment: Use the pkcs 5 v2 compatible interfaces in CMS/pkcs7 and pkcs12 - V2

Implement review comments on.

Comment 32

[Robert Relyea]

Comment on attachment 299306 [details] [diff] [review]
Use the pkcs 5 v2 compatible interfaces in CMS/pkcs7 and pkcs12 - V2

This is the patch your r+ plus the requested line wrapping and spelling changes plus the variable rename (mechanism -> xxxxMechType where xxxx is crypto, pbe, or integrity as appropriate).

Interdiff should work on this patch..


bob

Comment 33

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 299306 [details] [diff] [review]
Use the pkcs 5 v2 compatible interfaces in CMS/pkcs7 and pkcs12 - V2

Bob, thanks for folding all those long lines.  
Here are a few more nits that you can fix (or not) before you commit.

In  lib/smime/cmscipher.c

>  * NSS_CMSCipherContext_StartDecrypt - create a cipher context to do decryption
>- * based on the given bulk * encryption key and algorithm identifier (which may include an iv).
>+ * based on the given bulk * encryption key and algorithm identifier (which 
>+ * may include an iv).

I don't understand the '*' between bulk and encryption.  Assuming it's
merely accidental, please remove it.  Else please explain it.

In lib/smime/cmsencdata.c

>-    encd = (NSSCMSEncryptedData *)PORT_ArenaZAlloc(poolp, sizeof(NSSCMSEncryptedData));
>+    encd = (NSSCMSEncryptedData *)PORT_ArenaZAlloc(poolp, 
>+					sizeof(NSSCMSEncryptedData));

This could be written more succinctly as 

      encd = PORT_ArenaZNew(poolp, NSSCMSEncryptedData);

Comment 34

[Robert Relyea]

Attachment: Use the pkcs 5 v2 compatible interfaces in CMS/pkcs7 and pkcs12 - V3

Patch as checked in (same as V2 except it incorporates Nelson's optional review comments).

Comment 35

[Robert Relyea]

Comment on attachment 288581 [details] [diff] [review]
pk12util patch, v1

This is the last piece, it's not necessary to make NSS 3.12, but would be a nice testing addition.

OK to put off this review or pass it off to someone else.

Thanks

Comment 36

[Slavomir Katuscak]

Bob, after patch from this bug was checked into trunk, all Tinderboxes came to orange. Please back out or fix ASAP. Thanks.

Comment 37

[Nelson Bolyard (seldom reads bugmail)]

Hmmm.  
I assumed Bob had run all.sh with his patch before submitting it for review.
Bob, If you can fix it today, please do.  Otherwise, I'll have to put on my 
sheriff hat and back it out at sundown. :)

Comment 38

[Nelson Bolyard (seldom reads bugmail)]

According to Glen, this is a JSS failure, so all.sh would not have caught it. :(
Maybe the fix belongs in JSS?  

Either way, I must act by tonight if the problem remains.
Glen, if you can see a fix for JSS this afternoon, please comment here.

Comment 39

[Nelson Bolyard (seldom reads bugmail)]

If the files being produced by NSS in the JSS tests are correct, and the 
problem is that JSS is unable to cope with them for some reason, then I 
regard this as a JSS bug, and a new P1 JSS bug should be filed in bugzilla.

IFF this is a JSS bug, I don't want it to hold up NSS 3.12.  That may mean
that the "fix" for the orange tinderbox trees is to change the JSS test 
script, rather than backing out the NSS change.  

Comment 40

[glen beasley]

This is not a JSS bug. The JAVA PKCS12KeyStore class  cannot decode the NSS pkcs12 files anymore. 

Java.io.IOException: failed to decrypt safe contents entry: javax.crypto.BadPaddingException: Given final block not properly padded


The JSS test script all.pl creates 3 pkcs12 files using pk12util which then get used by JSSE_SSLClient and JSSE_SSLServer for interoperability tests with JSS SSL test programs. The JSSE_SSL tests use the specified pkcs12 file as it's keystore. 

The JSSE_SSLClient is pure JAVA. JSSE_SSLServer can load the Mozilla-JSS provider as a command line option, but in the test case failures it is using 
the JAVA implementation. 

Java.io.IOException: failed to decrypt safe contents entry: javax.crypto.BadPaddingException: Given final block not properly padded
   at com.sun.net.ssl.internal.ssl.PKCS12KeyStore.engineLoad(PKCS12KeyStore.java:1275)
   at java.security.KeyStore.load(KeyStore.java:1150)
   at org.mozilla.jss.tests.JSSE_SSLClient.initSocketFactory(JSSE_SSLClient.java:305)
   at org.mozilla.jss.tests.JSSE_SSLClient.configureCipherSuites(JSSE_SSLClient.java:209)
   at org.mozilla.jss.tests.JSSE_SSLClient.main(JSSE_SSLClient.java:577) 

Comment 41

[Nelson Bolyard (seldom reads bugmail)]

Glen, I surmise that the test script is using NSS to create a PKCS#12 file
and that file is now being created using PKCS#5v2 PBEs rather than with 
PKCS#5v1 PBEs, as is previously did.  From your output above, I gather that
this means the some Java crypto code is unable to handle PKCS#5v2 PBEs,
although it MIGHT be the case that some higher-level JSS code is not 
invoking the lower layer crypto code properly for PKCS#5v2 PBEs.  

My first thought towards a solution is to get the JSS test scripts to go 
back to generating PKCS#12 files with PKCS5v1 PBEs again.  I don't know 
if that's possible.  

I think further investigation into this issue should progress by answering
these questions, in roughly this order:

- Why have the PKCS#12 files that are being produced by this JSS test changed?
- Does NSS now produce PKCS#12 files with PKCS5v2 PBEs by default?
- Is there any control (e.g. function parameter) by which the caller can 
choose PKCS#5 v1 vs PKCS#5v2 when creating the PKCS#12 file?
- Is there any way to tell pk12util to generate files using the older v1 PBEs?
- Is this change a consequence of the fact that the patch for pk12util that
is already attached to this bug has not yet been reviewed and committed?
- Was it premature to commit the PKCS#5v2 work without the pk12util patch?

Bob, If we cannot easily fix the JSS test script to generate PKCS#12 files 
that it understands, then we must make some NSS change for the short term.
Please advise an answer to this question:

- What is the minimum amount of code we can back out from NSS to get the JSS
tests working again?

Glen, After we've gotten the tree green again, the next step is to analyze 
JSS and the relevant Java code, to determine where exactly the problem lies.  
Somewhere there is some code that doesn't properly handle the PKCS#5v2 PBEs.  
Where is that code?  In JSS?  Elsewhere? 
If it's in JSS, we need to fix that ASAP.

Comment 42

[Robert Relyea]

> - Why have the PKCS#12 files that are being produced by this JSS test changed?

They shouldn't have.

> - Does NSS now produce PKCS#12 files with PKCS5v2 PBEs by default?

Definately not.

> - Is there any control (e.g. function parameter) by which the caller can 
choose PKCS#5 v1 vs PKCS#5v2 when creating the PKCS#12 file?

yes, pkcs #5v2 is only used if you select an encryption scheme not supported by PKCS #5 v1 or PKCS #12 PBE mechanisms.

> - Is there any way to tell pk12util to generate files using the older v1 PBEs?

A path in this bug is required to tell pkcs #12 to generate v2 PBE's, otherwise it will continue to produce pkcs #12 v1.

> - Is this change a consequence of the fact that the patch for pk12util that
is already attached to this bug has not yet been reviewed and committed?

No

My guess is there is a bug which is causing some of the answers I gave above to be correct. If I can't identify the bug and fix it, I will back out the patch.

(It might be sufficient to back out the PKCS #12 portion of the patch)..

bob

Comment 43

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 288581 [details] [diff] [review]
pk12util patch, v1

In the patch to file cmd/lib/secutil.c, I have some concerns with
these new functions 
void secu_PBEFreeParams(secuPBEParams *param)
void secu_PrintKDF2Params(FILE *out, SECItem *value, char *m, int level)
void secu_PrintPKCS5V2Params(FILE *out, SECItem *value, char *m, int level)
void secu_PrintPBEParams(FILE *out, SECItem *value, char *m, int level)

The 3 new secu_Print* functions all call SEC_ASN1DecodeItem without an
arenapool.  secu_PBEFreeParams then tries to free all the individual 
elements parsed by SEC_ASN1DecodeItem.  We know that SEC_ASN1DecodeItem 
can and often does leak memory when called without an arena pool.  
So, I'd rather see an arenapool be used in each of these 3 functions, 
and then free the arenapool rather than individually freeing the parsed
components.  Would there be any problem doing it that way?

Are PBE parameters allowed to be BER encoded? or are they always DER?  
If DER, then I suggest using SEC_QuickDERDecodeItem instead of 
SEC_ASN1DecodeItem.

Comment 44

[Robert Relyea]

Attachment: Fix tinderbox bustage...

The new PKCS 5 API was not passing appropriate key lengths around when creating PKCS #11 parameters. This was breaking the use of RC2_40, which we use to 'lightly' wrap PKCS #12 files.

When I turned enabled pkcs7 and pkcs12 to use the new interfaces, the would wrap and unwrap RC2_40 wrappers as RC2_128 (sigh).

This patch corrects that deficiency.

(NOTE: at this point its likely that pk11_GenerateNewParameter could be implmented more simply using the new pk11_ParamFromIV, getting rid of a switch statement. Also note that we may want to export the new calls that take a key length for external callers.

bob

Comment 45

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 300962 [details] [diff] [review]
Fix tinderbox bustage...

Ah, RC2 strikes again!
I'm impressed that you found and corrected all those issues so 
quickly, Bob.  r+

Comment 46

[glen beasley]

Attachment: program to demostrate to pkcs12 decode issue

The attached test program pkcs12.java does not use JSS classes at all.

If I take a pkcs12 file created using the nss311 branch the test program
pkcs12.java can load it, if I take the pkcs12 file from the trunk pkcs12.java
cannot load it.  
1639][b@Macintosh-2:~/test]$ javac pkcs12.java 
[1639][b@Macintosh-2:~/test]$ cp /Users/b/nss311/mozilla/tests_results/jss/Macintosh-2.local.1/rsa.pfx .
[1640][b@Macintosh-2:~/test]$ java -cp . pkcs12 rsa.pfx m1oZilla
The provider used SunJSSE version 1.5
Successfully loaded PKCS12 file.
[1640][b@Macintosh-2:~/test]$ cp /Users/b/tip/mozilla/tests_results/jss/Macintosh-2.local.6/rsa.pfx .
[1640][b@Macintosh-2:~/test]$ java -cp . pkcs12 rsa.pfx m1oZilla
The provider used SunJSSE version 1.5
java.io.IOException: failed to decrypt safe contents entry: javax.crypto.BadPaddingException: Given final block not properly padded
	at com.sun.net.ssl.internal.ssl.PKCS12KeyStore.engineLoad(PKCS12KeyStore.java:1275)
	at java.security.KeyStore.load(KeyStore.java:1150)
	at pkcs12.main(pkcs12.java:49)
Caused by: javax.crypto.BadPaddingException: Given final block not properly padded
	at com.sun.crypto.provider.SunJCE_h.b(DashoA12275)
	at com.sun.crypto.provider.SunJCE_h.b(DashoA12275)
	at com.sun.crypto.provider.SunJCE_ac.b(DashoA12275)
	at com.sun.crypto.provider.PKCS12PBECipherCore$PBEWithSHA1AndRC2_40.engineDoFinal(DashoA12275)
	at javax.crypto.Cipher.doFinal(DashoA12275)
	at com.sun.net.ssl.internal.ssl.PKCS12KeyStore.engineLoad(PKCS12KeyStore.java:1272)
	... 2 more
Configuration error.  Cannot load PKCS12 file.

The file rsa.pfx was created with pk12util. 
JSS did create the original certificate and key in both cases. 
This comment is mainly an FYI on my current status looking into this problem.

Comment 47

[glen beasley]

sorry, I hadn't seen comment 44. 

Thanks bob! I will test your new patch. 

Comment 48

[Nelson Bolyard (seldom reads bugmail)]

Bob, shall I interpret your "note" from comment 44 as saying that you plan
to make further refinements to this code before 3.12 ships?

Comment 49

[Nelson Bolyard (seldom reads bugmail)]

Attachment: alternative patch for pk12util (checked in)

Here's the change I proposed.  I also found that the original patch
wouldn't build on Windows, so I fixed that.  This patch also fixes
some warnings.

Bob, please review

Also, to be complete, this bug should include an enhancement to tools.sh
to test any new pk12util features.

Comment 50

[Nelson Bolyard (seldom reads bugmail)]

Bob,
The additional test commands to be added to tools.sh should generate one
or more p12 files with the PkCS5v2 PBEs, and show that they can be listed
(decoded) and imported.  

All the previously committed patches for nss/lib in this bug have been 
re-committed as a group, in a single commit.  The tree stayed green.
This shows that the problem we had before, where the tree did not go 
green after attachment 300962 [details] [diff] [review] was committed, was actually a bug in our
testing, not in the committed patch itself.  I will file a separate bug
about the testing problem.  

Comment 51

[Robert Relyea]

Comment on attachment 301060 [details] [diff] [review]
alternative patch for pk12util (checked in)

r+ rrelyea

Comment 52

[Nelson Bolyard (seldom reads bugmail)]

Bob, before I commit that latest patch, can you suggest some pk12util command
lines for testing these new pk12util features?  

Comment 53

[Robert Relyea]

pk12util -L -i sample.p12

Should now output information about how the .p12 file was encrypted

pk12util -c aes-192-cbc -o sample.p12 -n mycert -d directory

should produce a .p12 file that has AES (you can check with the -L flag).

pk12util -c DES-EDE3-CBC -k 92 -o sample.p12 -n mycert -d directory

should produce a .p12 file that has a double des key.

The AES file would not be readable by an old pk12util, the double-des should be.

bob

Comment 54

[Robert Relyea]

-c camellia-192-cbc is also another good test

Comment 55

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 301060 [details] [diff] [review]
alternative patch for pk12util (checked in)

lib/secutil.c;       new revision: 1.83; previous revision: 1.82
lib/secutil.h;       new revision: 1.25; previous revision: 1.24
pk12util/pk12util.c; new revision: 1.39; previous revision: 1.38
pk12util/pk12util.h; new revision: 1.4; previous revision: 1.3

Comment 56

[Nelson Bolyard (seldom reads bugmail)]

Well, here's bad news.  After I (thought I) tested all the above, and 
committed it.  I tried this command using a very old expired cert/key from
my personal cert DB.

  pk12util -c aes-192-cbc -o sample.p12 -n "nickname"

It crashed.  Tried it several times.  Completely repeatable.  Stack follows:

nspr4.dll!PR_Assert()                                  Line 577	C
nssutil3.dll!sec_asn1d_init_state_based_on_template()  Line 610  + 0x26 bytes	C
nssutil3.dll!sec_asn1d_next_in_sequence()              Line 2023 + 0x9 bytes	C
nssutil3.dll!SEC_ASN1DecoderUpdate_Util()              Line 2672 + 0x9 bytes	C
nssutil3.dll!SEC_ASN1Decode_Util()                     Line 2962 + 0x11 bytes	C
nssutil3.dll!SEC_ASN1DecodeItem_Util()                 Line 2978 + 0x1f bytes	C
nss3.dll!pbe_PK11AlgidToParam()                        Line 800  + 0x19 bytes	C
nss3.dll!PK11_ParamFromAlgid()                         Line 1239 + 0xd bytes	C
nss3.dll!PK11_PBEKeyGen()                              Line 1368 + 0xc bytes	C
nss3.dll!PK11_ExportEncryptedPrivKeyInfo()             Line 1439 + 0x17 bytes	C
nss3.dll!PK11_ExportEncryptedPrivateKeyInfo()          Line 1550 + 0x1d bytes	C
smime3.dll!SEC_PKCS12AddKeyForCert()                   Line 1353 + 0x1e bytes	C
smime3.dll!SEC_PKCS12AddCertAndKey()                   Line 1560 + 0x2a bytes	C
pk12util.exe!P12U_ExportPKCS12Object()                 Line 697  + 0x29 bytes	C
pk12util.exe!main()                                    Line 1103 + 0x27 bytes	C

The crash is in this assertion failure:

	    if (encode_kind & SEC_ASN1_INLINE) {
		/* check that there are no extraneous bits */
		PORT_Assert (encode_kind == SEC_ASN1_INLINE && !optional);
		state->place = afterInline;
	    } else {

Comment 57

[Nelson Bolyard (seldom reads bugmail)]

More info about this crash.  The call to SEC_ASN1DecodeItem_Util was 
in pk11pbe.c at this line:

    } else if (algorithm == SEC_OID_PKCS5_PBKDF2) {
	iv_len = 0;
        PORT_Memset(&p5_param, 0, sizeof(p5_param));
        rv = SEC_ASN1DecodeItem(arena,&p5_param,
			 SEC_PKCS5V2PBEParameterTemplate, &algid->parameters);
    } else {

SEC_PKCS5V2PBEParameterTemplate has a member template that specifies both
optional and inline:

 116     { SEC_ASN1_INLINE | SEC_ASN1_XTRN | SEC_ASN1_OPTIONAL, 
 117         offsetof(SEC_PKCS5PBEParameter, prfAlgId),
 118         SEC_ASN1_SUB(SECOID_AlgorithmIDTemplate) },

There are several templates in NSS that have this combination.  
For a list, see
<http://mxr.mozilla.org/security/search?string=inline.*optional&regexp=on>

Are they all wrong?  Will they all cause assertions?

Comment 58

[Nelson Bolyard (seldom reads bugmail)]

I continue past that particular assertion and hit it repeatedly.
When I skipped past the last of them, I hit another assertion,
the one that detects reference leaks on shutdown. 

Comment 59

[Nelson Bolyard (seldom reads bugmail)]

When I disabled all assertions, the command 
  pk12util -c aes-192-cbc -o sample.p12 -n "nickname"
ran to completion and reported that the export was successful.
It produced the file sample.p12

When I then ran the command 
  pk12util -L sample.p12

It listed the certificate, preceded by this output:

    Encryption algorithm: PKCS #5 Password Based Encryption v2
        Encryption:
            KDF: PKCS #5 Password Based Key Dervive Function v2
                Parameters:
                    Salt:
                        74:b5:c0:dd:30:b3:79:9c:3a:49:c4:eb:ce:3d:07:99
                    Iteration Count: 1 (0x1)
                    Key Length: 32 (0x20)
                    KDF algorithm: HMAC SHA-1
            Cipher: AES-192-CBC
                Args:
                    04:10:d0:5e:76:d2:91:e9:c8:8a:f9:a8:fb:50:8f:71:
                    ba:77

I *GUESS* that's correct.  Is it Bob?

Comment 60

[Wan-Teh Chang]

Bob, there is a bug in your code:

pk11pbe.c: In function 'sec_pkcs5CreateAlgorithmID':pk11pbe.c:623: warning: 'cipherAlgorithm' is used uninitialized in this function

Comment 61

[Justin Dolske [:Dolske]]

Hmm, I just filed bug 436577 for the bug mentioned in the last comment, and also just hit the assert in comment 57. This code is shipping in Firefox 3, is this bug still open because it's unfinished?

Comment 62

[Robert Relyea]

Nelson:

I looks correct to me. I wish I knew more about the reason for the ASSERT. Is there anything in the template we can adjust, or is the assert just wrong.

bob

Comment 63

[Nelson Bolyard (seldom reads bugmail)]

Bob, the assertion is correct.  It is detecting the very situation it was 
intended to detect.  The template is wrong, and so is the structure SEC_PKCS5PBEParameterStr (unless we decide to remove the SEC_ASN1_OPTIONAL flag).  I wonder if we can get away with modifying that structure definition
before 3.12.1.

Recall with me, here, how SEC_ASN1_OPTIONAL works in our decoder and encoder.

Our ASN.1 decoder signifies that an optional item was omitted (was not present) in the encoded input data by leaving a NULL pointer for the decoded item in 
the "dest" structure (the structure whose size is given in the "size" member
of the first template in a template array, and into which an offset is given
in the "offset" member of every subsequent member of the template array).  

For templates that are neither SEC_ASN1_INLINE or SEC_ASN1_POINTER types, 
where there is always a SECItem in the "dest" structure corresponding to the template, and which are generally primitive types, the decoder leaves the 
NULL pointer in the SECItem's data pointer member.  The decoder's caller 
knows that the optional item was omitted because the pointer to the decoded 
data is NULL.

For constructed types (sequences and sets) and for optional choices, 
it is generally necessary to use SEC_ASN1_INLINE or SEC_ASN1_POINTER.  

In a SEC_ASN1_POINTER template, the "offset" template member is the offset in 
the "dest" structure of a pointer to the memory allocated to be the "dest" 
structure for the sequence or set described by the subtemplate.  If the 
optional sequence, set or choice was omitted, then the value of that pointer 
will be set to NULL by the decoder.  The decoder's caller knows that the optional sequence, set or choice was omitted because the pointer to the dest structure for that subtemplate is NULL.

In a SEC_ASN1_INLINE template, the memory for the "dest" structure for the 
subtemplate is part of the dest structure for the parent template.  The 
memory for that subtemplate dest structure always exists, by definition.  
The address of that subtemplate's dest structure is found by adding the 
parent template's "offset" member value to the address of the parent 
template's dest structure.  The resultant sum cannot be null, and is not 
stored explicitly anywhere in the parent template's dest structure, so there
is NO pointer corresponding to the subtemplate that the caller can examine to 
tell if the optional item was omitted or was present.  

With INLINE templates, for the decoder (only, not the encoder), the caller 
may be able to determine if the optional sequence, set or choice was omitted 
by examining the data pointer member of any SECItems in the subtemplate's 
dest structure that correspond to non-optional members of the subtemplate.  

But there is NO WAY for the caller of the encoder to tell the encoder to omit an optional inline subtemplate because the address value that the encoder
will test for NULL will never be NULL.  So, our encoder does not (and cannot) support the combination of OPTIONAL and INLINE.  That combination is just one
of several that are not allowed by our encoder.  

The assertion in the decoder is there to help developers of ASN1 templates to detect that they've produced a template with an invalid combination.  That's
what it has done here.  

Our only reasonable choices are:
a) remove the OPTIONAL flag from the template for the algorithm ID in 
SEC_PKCS5V2PBEParameterTemplate, or
b) change the INLINE flag to a POINTER flag in that template, and then 
change the corresponding struct SEC_PKCS5PBEParameterStr, changing 
        SECAlgorithmID  prfAlgId;
to 
        SECAlgorithmID  *prfAlgId;
and changing the corresponding code that works with that structure to 
accomodate that difference.  

As for the other templates now in NSS that use the combination of INLINE 
and OPTIONAL, we probably need to file separate bugs for each of the 
source files that contains defective templates.  

I'd say that the fact that we added this template to NSS without hitting 
this assertion suggests that the code paths that use this new template
were not adequately tested. :(  

Comment 64

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch to fix assertion failure

I tested this patch with the same tests I used in comment 56 and comment 59.
With this patch, I no longer see any assertion failures in the ASN.1 decoder.
I do, however, still see the assertion failure during shutdown, which I 
previously reported in comment 58.  I want to tackle that problem separately,
not in this patch.

Comment 65

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch to fix assertion failures and other crashes

This patch is a superset of the previous one. 
It fixes the crash on shutdown due to a leaked list element.
It fixes several other bugs found while testing, which include:
- a crash freeing an uninitialized pointer in an error path
- a leak due to a return that should have been goto loser;
I bleive this patch resolves the remaining issues with this bug.

Comment 66

[Robert Relyea]

Comment on attachment 324399 [details] [diff] [review]
patch to fix assertion failures and other crashes

r+ rrelyea

Comment 67

[Nelson Bolyard (seldom reads bugmail)]

Checking in pk11wrap/pk11pbe.c;  new revision: 1.22; previous revision: 1.21
Checking in pk11wrap/pk11slot.c; new revision: 1.93; previous revision: 1.92
Checking in pkcs12/p12e.c;       new revision: 1.18; previous revision: 1.17

Comment 68

[Nelson Bolyard (seldom reads bugmail)]

Oh dear!  

Bob and I collaborated on the patches that were committed for this RFE.
In the process of writing, reviewing and committing them, we both failed
to notice that the patches added two new command line options using letters
that were already in use.  There are now *TWO* -k and *TWO* -K options 
defined for pk12util.  See 
<http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/cmd/pk12util/pk12util.c&rev=1.39&mark=944-945,956-957#939>

This tells me several things.  
a) the new -k and -K options were not tested, at all, or it would have been
found that they do not work.
b) function SECU_ParseCommandLine does not check its inputs for duplicates
of this nature (it should, and it should assert that no such duplicates exist)

I will file separate bugs about these problems.