836019 - Move RSA-PKCS#1, RSA-PSS, and RSA-OAEP into freebl

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P1)

Description

[Ryan Sleevi]

Currently, the implementation of RSASSA, RSAES, RSA-PSS, and RSA-OAEP all live in softoken, specifically lib/softoken/rsawrapr.c

We should move such crypto into freebl/, and integrate these as part of bltests, allowing softoken to just be concerned about plumbing. This will also allow us to integrate tests for these modes as part of bltests.

Comment 1

[Kai Engert [:KaiE:]]

needs a new target milestone

Comment 2

[Ryan Sleevi]

Attachment: 1_move_code_1.diff

Bob,

This is the first part of the patch, which simply shifts the PKCS#1 code from softokn (rsawrapr) into freebl (rsapkcs).

The motivation is to keep, as best as possible, the crypto-side of things in freebl, and the PKCS#11-glue side of things in softokn. The crypto can then have KATs easily added to bltests.

One change I'm not sure of is the removal of RSA_BlockType from softoknt.h - given that the functions using it are private, and it never was used outside of softoken, I've gone and removed it.

brian, FYI with respect to Bug 577498, in case you had any concerns. I don't think there should be any with shuffling this code out.

Comment 3

[Ryan Sleevi]

Attachment: 1_move_code_2.diff

Er, now with rsapkcs

I hate hg.

Comment 4

[Ryan Sleevi]

Attachment: Ignoring whitespace changes & properly tracking as a mercurial move

This is a small cleanup to the above patch, in that it properly tracks the lib/softoken/rsawrapr.c -> lib/freebl/rsapkcs.c move.

Note that I supplied -w here. The actual fix uses entirely internally consistent whitespace, whereas the original was a mix of tabs and spaces depending on function and (even within a function) depending on line. Because these changes are significant enough to this file, I think this is a correct time to fix the whitespace. The "-w" was just to make it easier for review.

Comment 5

[Robert Relyea]

OK, I only have one issue to discuss: in RSA_DecryptOAEP() We have the following code:

    rv = RSA_PrivateKeyOpDoubleChecked(key, oaepEncoded, input);	
    /* rv for the private key op is ignored because this operation MUST be
     * constant time. */
    rv = eme_oaep_decode(output, outputLen, maxOutputLen, oaepEncoded,
                         modulusLen, hashAlg, maskHashAlg, label,
                         labelLen);


This has some problems. We are calling RSA_PrivateKeyOpDoubleChecked to verify that we did in fact properly decrypted the data that is encoding oaepEncoded will yield the original input. We then throw away the results, and the check function has no side effects. In addition, the function should never fail except on a malfunctioning processor (there isn't a way to corrupt input, for instance, that would cause this function to fail). This should be fixed in the one of the following ways:

1) completely remove the RSA_PrivateKeyOpDoubleChecked(); This is what we are doing in the decrypt case. Presumably the result of this call will be a key that the user doesn't see, so returning

2) restore the test. Since this function only fails in the exceptional case (that can't be induced by a remote attacker), and in FIPS mode it can only fail once, timing shouldn't be an issue. 

I think 2 is preferable, wtc added the check initially for a reason. It is probably that OEAP is covered under FIPS, so we need to lock the token if we find our math hardware is malfunctioning (leaking out an improperly decrypted value can compromise your key). 

bob

Comment 6

[Ryan Sleevi]

I'm not convinced by the argument for 1 - namely, that OAEP will only be used for key wrapping - and would prefer the API be generally robust.

That said, since it should only be with memory corruption, it's a local attacker scenario, and so far, NSS has not tried to defend against that (eg: the lack of memory cleansing)

Comment 7

[Ryan Sleevi]

Attachment: 1) Check the rv during RSA_EncryptOAEP

Comment 8

[Ryan Sleevi]

Attachment: 2) Refactor bltest

Wan-Teh: In the interest of not overloading Bob, I'm sending the test portion of this CL your way, while he reviews the file shuffling.

This is a larger patch that unifies how bltest handles asymmetric ciphers in a more uniform manner. This is to make it easier for OAEP and PSS tests to be landed (which is part of step 3).

It adds support for rsa_oaep and rsa_pss - but does not add test data - and removes a lot of duplication for the DSA/ECDSA case.

Comment 9

[Ryan Sleevi]

Attachment: 3) Add PSS and OAEP test data

wtc: One more. This is the test data for PSS and OAEP. It is from the RSA test vectors for OAEP and PSS. Only the powers-of-8 keysizes are included, because that is all NSS supports (1024, 1536, and 2048).

This diff contains binary data, and thus doesn't show up in the UI appropriately.

Comment 10

[Robert Relyea]

> I'm not convinced by the argument for 1 - namely, that OAEP will only be used for key wrapping -
> and would prefer the API be generally robust.

yes, I prefer 2 as well. I'll go back and complete the view now. (that was my only issue with the previous patch.

bob

Comment 11

[Wan-Teh Chang]

Comment on attachment 804184 [details] [diff] [review]
2) Refactor bltest

Review of attachment 804184 [details] [diff] [review]:
-----------------------------------------------------------------

r=wtc. Excellent cleanup, especially the removal of dsaOp() and ecdsaOp().

I read every line this patch and was able to follow most changes, until
I got near the end of the file (when I ran out of steam).

::: cmd/bltest/blapitest.c
@@ +646,5 @@
>      bltestSEED_ECB,       /* SEED algorithm	   */
>      bltestSEED_CBC,       /* SEED algorithm	   */
>      bltestRSA,		  /* Public Key Ciphers	   */
> +    bltestRSA_OAEP,       /* . (Public Key Enc)    */
> +    bltestRSA_PSS,        /* . (Public Key Sig)    */

1. Nit: add a "." after "Enc" and "Sig"? See line 652.

2. It'd be nice to note that the signature algorithms must
be consecutive for is_sigCipher to work. Alternatively, use
a switch statement in is_sigCipher.

@@ +779,5 @@
>      bltestIO		key;
>      bltestSymmKeyParams sk;
>      bltestAuthSymmKeyParams ask;
>      bltestRC5Params	rc5;
> +    bltestAsymParams    asym;

Nit: Perhaps this type should be named bltestAsymKeyParams? See line 780.

@@ +1244,5 @@
> +dsa_signDigest(void *cx, SECItem *output, const SECItem *input)
> +{
> +    bltestAsymParams *params = (bltestAsymParams *)cx;
> +    if (params->cipherParams.dsa.sigseed.buf.len > 0) {
> +        return DSA_SignDigestWithSeed((DSAPrivateKey *)params->privKey,

Is this new? The original code always call DSA_SignDigest.

(Same question for ecdsa_signDigest.)

@@ +1263,5 @@
>  #ifdef NSS_ENABLE_ECC
>  SECStatus
>  ecdsa_signDigest(void *key, SECItem *output, const SECItem *input)
>  {
> +    bltestAsymParams *params = (bltestAsymParams *)cx; 

Nit: delete the space at the end of the line.

@@ +1584,2 @@
>      /* RSA key gen was done during parameter setup */
> +    cipherInfo->cx = (void *)asymp;

Delete the (void *) cast. (I know this cast comes from the original code.)
Please search for all occurrences in the file.

The (void *) cast is only necessary to cast away the 'const'.

@@ +1584,3 @@
>      /* RSA key gen was done during parameter setup */
> +    cipherInfo->cx = (void *)asymp;
> +    RSAPrivateKey *privKey = (RSAPrivateKey *)asymp->privKey;

This variable needs to be declared at the beginning of this function.
Regrettably MSVC still doesn't support this C99 feature.

Please fix all occurrences of this problem by compiling the code on Windows.
There is at least one other occurrence (the |mode| variable in mode_str_to_hash_alg).

@@ +1607,3 @@
>  	/* Have to convert private key to public key.  Memory
>  	 * is freed with private key's arena  */
>  	RSAPublicKey *pubkey;

Nit: rename "pubkey" to "pubKey" to match the spelling of "privKey".

@@ -1558,2 @@
>      PQGVerify *ignore = NULL;
> -    /* DSA key gen was done during parameter setup */

Should we preserve this comment? You kept the equivalent comment for RSA.

@@ -1597,5 @@
>  	pubkey->params.subPrime.data = key->params.subPrime.data;
>  	pubkey->params.base.len = key->params.base.len;
>  	pubkey->params.base.data = key->params.base.data;
>  	pubkey->publicValue.len = key->publicValue.len;
>  	pubkey->publicValue.data = key->publicValue.data;

The original code seems to have a bug: it sets up |pubkey| but doesn't use |pubkey|.
Do you know why this doesn't break the DSA signature verification tests?

(Same issue with ECDSA signature verification in the original code.)

@@ -1611,5 @@
>      int i;
>      ECPrivateKey **dummyKey;
>      PRIntervalTime time1, time2;
> -    bltestECDSAParams *ecdsap = &cipherInfo->params.ecdsa;
> -    /* ECDSA key gen was done during parameter setup */

Should we preserve this comment? You kept the equivalent comment for RSA.

@@ +2083,2 @@
>      bltestRSAParams *rsap;
> +    RSAPrivateKey **rsaKey = NULL;

Nit: the use of these ** pointers is a little hard to understand.
Perhaps this can be avoided by changing asymp->privKey to be a
union of pointers of various concrete private key types.

@@ +2224,5 @@
>  	break;
>      case bltestRSA:
> +    case bltestRSA_OAEP:
> +    case bltestRSA_PSS:
> +	if (encrypt || cipherInfo->mode != bltestRSA_PSS) {

Can you add a comment to explain this conditional test?
I guess it describes the operations that produce output data.

@@ +2229,1 @@
>  	SECITEM_AllocItem(cipherInfo->arena, &cipherInfo->output.buf,

Is this line indented correctly?

Same question for the SECITEM_AllocItem calls in the DSA and ECDSA cases below.

@@ +2657,5 @@
>            } else {
> +              bltestAsymParams *asymp = &info->params.asym;
> +              fprintf(stdout, "%8d", asymp->cipherParams.rsa.keysizeInBits);
> +              print_exponent(
> +                  &((RSAPrivateKey *)asymp->privKey)->publicExponent);

Nit: if asymp->privKey were a union of concrete pointers, it would help here.

@@ +2765,5 @@
> +mode_str_to_hash_alg(const SECItem *modeStr)
> +{
> +    if (!modeStr || modeStr->len == 0)
> +        return HASH_AlgNULL;
> +    bltestCipherMode mode = get_mode((const char*)modeStr->data);

Does this mean modeStr->data must be null-terminated?

@@ +2847,5 @@
>  #endif
> +    case bltestRSA_PSS:
> +	sprintf(filename, "%s/tests/%s/%s%d", testdir, modestr, "ciphertext", j);
> +	load_file_data(arena, &params->asym.sig, filename, bltestBase64Encoded);
> +	/* intentional fall through */

Nit: omit "intentional". The "fall through" comments are common.

@@ +2854,5 @@
> +	load_file_data(arena, &params->asym.cipherParams.rsa.seed,
> +	               filename, bltestBase64Encoded);
> +	sprintf(filename, "%s/tests/%s/%s%d", testdir, modestr, "label", j);
> +	load_file_data(arena, &params->asym.cipherParams.rsa.label, filename,
> +	                bltestBase64Encoded);

Since "label" is OAEP only, what happens if the "label" input file is missing for RSA-PSS?

@@ +3055,5 @@
> +			cipherInfo.output.buf.len = cipherInfo.output.pBuf.len;
> +			bltestCopyIO(cipherInfo.arena,
> +			             &cipherInfo.params.asym.sig,
> +			             &cipherInfo.output);
> +		}

1. Can you explain this change? This is not in the original code.

2. The body of the if statement should be indented by four spaces.

@@ +3067,5 @@
>  	    ** then perform operation and compare to plaintext
>  	    */
> +	    if (is_sigCipher(mode)) {
> +		bltestCopyIO(arena, &cipherInfo.input, &pt);
> +		bltestCopyIO(arena, &cipherInfo.output, &ct);

Can you explain this line? In the original code, we also call memset on
&cipherInfo.output.buf in this case.

@@ +3830,5 @@
>              file = PR_Open(bltest.options[opt_SigFile].arg, PR_RDONLY, 00660);
> +            if (is_sigCipher(cipherInfo->mode)) {
> +                 memset(&params->asym.sig, 0, sizeof(bltestIO));
> +                 params->asym.sig.mode = ioMode;
> +                 setupIO(cipherInfo->arena, &params->asym.sig,

Nit: these three lines seem to be indented with five spaces.

@@ +3835,1 @@
>                          file, NULL, 0);

Nit: this may fit on the previous line.

::: cmd/bltest/tests/README
@@ +51,5 @@
> +- "seedN": The seed or salt to use when encrypting/signing
> +- "labelN": The label used (as a base64 string)
> +- "hashN" / "maskhashN" - The base digest algorithm and the digest algorithm
> +   to use with MGF1, respectively. This should be an ASCII string specifying
> +   one of the hash modes recognized by bltest (eg: "sha1", "sha256")

Nit: change "modes" to "algorithms" or "functions"?

Comment 12

[Wan-Teh Chang]

Comment on attachment 804185 [details] [diff] [review]
3) Add PSS and OAEP test data

Review of attachment 804185 [details] [diff] [review]:
-----------------------------------------------------------------

r=wtc. I don't see any labelN test data files.

Comment 13

[Wan-Teh Chang]

Comment on attachment 804184 [details] [diff] [review]
2) Refactor bltest

Review of attachment 804184 [details] [diff] [review]:
-----------------------------------------------------------------

::: cmd/bltest/blapitest.c
@@ +2845,5 @@
>  	}
>  	break;
>  #endif
> +    case bltestRSA_PSS:
> +	sprintf(filename, "%s/tests/%s/%s%d", testdir, modestr, "ciphertext", j);

In your test data patch, you also added cmd/bltest/tests/rsa_oaep/ciphertextN files.
So why doesn't the bltestRSA_OAEP case need this?

Comment 14

[Wan-Teh Chang]

Comment on attachment 804184 [details] [diff] [review]
2) Refactor bltest

Review of attachment 804184 [details] [diff] [review]:
-----------------------------------------------------------------

::: cmd/bltest/blapitest.c
@@ +2229,2 @@
>  	SECITEM_AllocItem(cipherInfo->arena, &cipherInfo->output.buf,
> +		                  RSA_MAX_MODULUS_BITS / 8);

Does this change mean cipherInfo->input.pBuf.len is not the output buffer
size for some new tests?

I assume the size of cipherInfo->output.buf doesn't need to be exact, so
it's fine to use RSA_MAX_MODULUS_BITS / 8?

Comment 15

[Wan-Teh Chang]

Comment on attachment 804185 [details] [diff] [review]
3) Add PSS and OAEP test data

Review of attachment 804185 [details] [diff] [review]:
-----------------------------------------------------------------

Please add README files in cmd/bltest/tests/rsa_oaep and
cmd/bltest/tests/rsa_pss to note where the test vectors come from.

Comment 16

[Ryan Sleevi]

Comment on attachment 804184 [details] [diff] [review]
2) Refactor bltest

Review of attachment 804184 [details] [diff] [review]:
-----------------------------------------------------------------

::: cmd/bltest/blapitest.c
@@ +1244,5 @@
> +dsa_signDigest(void *cx, SECItem *output, const SECItem *input)
> +{
> +    bltestAsymParams *params = (bltestAsymParams *)cx;
> +    if (params->cipherParams.dsa.sigseed.buf.len > 0) {
> +        return DSA_SignDigestWithSeed((DSAPrivateKey *)params->privKey,

No, this was in the original code (which was hard to notice because of all the duplication). See line 2197/2208.

@@ -1597,5 @@
>  	pubkey->params.subPrime.data = key->params.subPrime.data;
>  	pubkey->params.base.len = key->params.base.len;
>  	pubkey->params.base.data = key->params.base.data;
>  	pubkey->publicValue.len = key->publicValue.len;
>  	pubkey->publicValue.data = key->publicValue.data;

Yes, sorry, this was a bit of a quiet cleanup.

It worked because the order of fields in the public key structures for DSA and ECDSA are the exact same as their private key counterparts, so it was silently casting the private key into a public key (ignoring the allocated structure).

This change makes it actually use the allocated public key.

@@ +2083,2 @@
>      bltestRSAParams *rsap;
> +    RSAPrivateKey **rsaKey = NULL;

Mostly was done for conciseness. Wanted to avoid "&(asymp->privKey.k.rsa)". Same reason for adding the local params, to prevent having to recurse too far into the structure.

@@ +2229,2 @@
>  	SECITEM_AllocItem(cipherInfo->arena, &cipherInfo->output.buf,
> +		                  RSA_MAX_MODULUS_BITS / 8);

Correct.

It works for the existing RSA cases because the existing RSA cases do not test the PKCS#1 v1.5 padding function, only the underlying primitive (that is, they take a fully padded input and yield a fully padded output)

The PSS and OAEP tests take as input M (or more aptly, a specific hash of the message), in order to also cover the encoding/decoding functions. As such, the input length is much shorter than the output length.

The output length of ALL the tests is a function of the modulus size of the key; this just brings the tests into alignment by allocating enough to support any key size as part of the tests.

@@ +2765,5 @@
> +mode_str_to_hash_alg(const SECItem *modeStr)
> +{
> +    if (!modeStr || modeStr->len == 0)
> +        return HASH_AlgNULL;
> +    bltestCipherMode mode = get_mode((const char*)modeStr->data);

Yes

@@ +2845,5 @@
>  	}
>  	break;
>  #endif
> +    case bltestRSA_PSS:
> +	sprintf(filename, "%s/tests/%s/%s%d", testdir, modestr, "ciphertext", j);

It's only used for the signature algorithms; the existing DSA and ECDSA code was doing this (see line 2894 of the new code).

However, you've identified a bug, which is that the original code did this to support the sigfile mode of operation in verify. That is, when sigfile wasn't specified, .sig would be initialized with the ciphertext.

This new change breaks sigfile support entirely, because it always uses |ct| to verify, rather than .sig.

@@ +2854,5 @@
> +	load_file_data(arena, &params->asym.cipherParams.rsa.seed,
> +	               filename, bltestBase64Encoded);
> +	sprintf(filename, "%s/tests/%s/%s%d", testdir, modestr, "label", j);
> +	load_file_data(arena, &params->asym.cipherParams.rsa.label, filename,
> +	                bltestBase64Encoded);

No failure. Just an empty structure.

@@ +3055,5 @@
> +			cipherInfo.output.buf.len = cipherInfo.output.pBuf.len;
> +			bltestCopyIO(cipherInfo.arena,
> +			             &cipherInfo.params.asym.sig,
> +			             &cipherInfo.output);
> +		}

It's part of the BUG highlighted above.

The DSA and ECDSA code entirely ignored |ct| (aka cipherInfo.output) and instead directly looked at params.sig. This was to support SigFile, which placed the expected sig into params.sig.

This was meant to preserve their behaviour, but as described above, the new code entirely ignores |sig|, instead favoring |ct| (line 3071).

@@ +3067,5 @@
>  	    ** then perform operation and compare to plaintext
>  	    */
> +	    if (is_sigCipher(mode)) {
> +		bltestCopyIO(arena, &cipherInfo.input, &pt);
> +		bltestCopyIO(arena, &cipherInfo.output, &ct);

For signature verification, |pt| contains the message, |ct| contains the computed signature, and the result of verify is whether or not the computed signature is valid.

If we made |input| the signature, we'd need to have a way to smuggle in the original message (for which signature should validate).

In the legacy-RSA case, this wasn't necessary, because Verify only verified the RSA op was reversibly valid.

In the DSA/ECDSA case, they were bypassing the memset entirely.

Comment 17

[Ryan Sleevi]

(In reply to Wan-Teh Chang from comment #12)
> Comment on attachment 804185 [details] [diff] [review]
> 3) Add PSS and OAEP test data
> 
> Review of attachment 804185 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=wtc. I don't see any labelN test data files.

That's because a NULL label = "use the zero string". That is, it's defined behaviour.

Comment 18

[Wan-Teh Chang]

Comment on attachment 804179 [details] [diff] [review]
1) Check the rv during RSA_EncryptOAEP

Review of attachment 804179 [details] [diff] [review]:
-----------------------------------------------------------------

r=wtc. I checked every line of the patch, so there are a lot of review comments.

1. Note the one comment marked with "BUG?".

2. Since the RSA functions you moved to freebl/rsapkcs.c don't set error code
on most error paths, it is risky to check PORT_GetError() == SEC_ERROR_LIBRARY_FAILURE
when they fail.

::: lib/freebl/blapi.h
@@ +114,5 @@
> +/********************************************************************
> +** Raw signing/encryption/decryption operations.
> +**
> +** No padding or formatting will be applied.
> +** inputLen MUST be a equivalent to the modulus size (in bytes).

Nit: remove "a".

@@ +118,5 @@
> +** inputLen MUST be a equivalent to the modulus size (in bytes).
> +*/
> +extern
> +SECStatus RSA_SignRaw(RSAPrivateKey       * key,
> +                      unsigned char       * output,

1. NSS usually wraps the function prototype as follows:

extern SECStatus
RSA_SignRaw(RSAPrivateKey * key,


2. I recommend against aligning the parameter names in new code because
it is not easy to maintain

@@ +158,5 @@
> +
> +/********************************************************************
> +** RSAES-OAEP encryption/decryption, as defined in RFC 3447, Section 7.1.
> +**
> +** Note: Only MGF-1 is supported as the mask generation function. It will be

Nit: MGF-1 => MGF1

There are two occurrences in this file.

@@ +164,5 @@
> +**
> +** Unless performing Known Answer Tests, "seed" should be NULL, indicating that
> +** freebl should generate a random value. Otherwise, it should be an octet
> +** string of seedLen bytes, which should be the same size as the output of
> +** maskHashAlg.

I think this should be hashAlg rather than maskHashAlg.
See RFC 3447, Section 7.1.1, Step 2d.

@@ +173,5 @@
> +                          HASH_HashType         maskHashAlg,
> +                          const unsigned char * label,
> +                          unsigned int          labelLen,
> +                          const unsigned char * seed,
> +                          unsigned int          seedLen,

I am glad that you aren't using the CK_RSA_PKCS_OAEP_PARAMS type
in these freebl functions.

@@ +225,5 @@
> +SECStatus RSA_SignPSS(RSAPrivateKey       * key,
> +                      HASH_HashType         hashAlg,
> +                      HASH_HashType         maskHashAlg,
> +                      const unsigned char * salt,
> +                      unsigned int          saltLength,

Nit: saltLength => saltLen

There is another occurrence in RSA_CheckSignPSS.

@@ +245,5 @@
> +
> +/********************************************************************
> +** RSASSA-PKCS1-v1_5 signing/verifying, as defined in RFC 3447, Section 8.2.
> +**
> +** The Sign variants expect as input the value of T, the DER-encoded

The three functions in this group are all the Sign variants, so we
can change "The Sign variants" to "These function".

@@ +250,5 @@
> +** DigestInfo structure defined in Section 9.2, Step 2.
> +**
> +** The HashSign variants expect as input the value of H, the computed hash
> +** from Section 9.2, Step 1, and will compute the DER-encoded DigestInfo
> +** structure internally prior to signing/verifying.

This paragraph should be deleted or moved to lib/softoken/softoken.h
because the HashSign variants are in lib/softoken/softoken.h.

@@ +265,5 @@
> +SECStatus RSA_CheckSign(RSAPublicKey        * key,
> +                        const unsigned char * sig,
> +                        unsigned int          sigLen,
> +                        const unsigned char * data,
> +                        unsigned int          dataLen);

It is a little confusing that the input arguments for RSA_Sign and RSA_CheckSign
have different names (|input| and |data|). They are both DER-encoded DigestInfo,
right?

@@ +271,5 @@
> +extern
> +SECStatus RSA_CheckSignRecover(RSAPublicKey        * key,
> +                               unsigned char       * output,
> +                               unsigned int        * outputLen,
> +                               unsigned int          maxOutputLen,

I assume |output| will receive the DER-encoded DigestInfo?

::: lib/freebl/loader.c
@@ +1946,5 @@
> +                         unsigned int *outputLen,
> +                         unsigned int maxOutputLen,
> +                         const unsigned char *input,
> +                         unsigned int inputLen) {
> +   if (!vector && PR_SUCCESS != freebl_RunLoaderOnce())

Nit: indentation is off by one.

@@ +1971,5 @@
> +                          HASH_HashType maskHashAlg,
> +                          const unsigned char *label,
> +                          unsigned int labelLen,
> +                          const unsigned char *salt,
> +                          unsigned int saltLen,

These argument should be "seed", not "salt" for OAEP.

@@ +2029,5 @@
> +SECStatus RSA_SignPSS(RSAPrivateKey *key,
> +                      HASH_HashType hashAlg,
> +                      HASH_HashType maskHashAlg,
> +                      const unsigned char *salt,
> +                      unsigned int saltLength,

Nit: saltLength => saltLen (four occurrences).

::: lib/freebl/loader.h
@@ +595,5 @@
>       unsigned int bodyLen,
>       unsigned int bodyTotalLen);
>  
>    /* Version 3.015 came to here */
> + SECStatus (* p_RSA_SignRaw)(RSAPrivateKey *key,

Nit: add a blank line before this line.

@@ +662,5 @@
> + SECStatus (* p_RSA_SignPSS)(RSAPrivateKey *key,
> +                             HASH_HashType hashAlg,
> +                             HASH_HashType maskHashAlg,
> +                             const unsigned char *salt,
> +                             unsigned int saltLength,

Nit: saltLength => saltLen (two occurrences).

@@ +694,5 @@
> +                                      unsigned int maxOutputLen,
> +                                      const unsigned char *sig,
> +                                      unsigned int sigLen);
> +
> +  /* Version 3.016 came to here */

At the beginning of this file, find FREEBL_VERSION, which should
have the value 0x030F.

Change the value to 0x0310.

::: lib/freebl/manifest.mn
@@ +112,5 @@
>  	ec.c \
>  	pqg.c \
>  	dsa.c \
>  	rsa.c \
> +	rsapkcs.c \

Nit: how about rsapkcs1.c?

Note: we should rename the file later to avoid confusing the code review tool.

::: lib/freebl/rsapkcs.c
@@ +10,5 @@
> +#include "stubs.h"
> +#endif
> +
> +#include "secerr.h"
> +

Nit: this blank line can be deleted.

@@ +18,4 @@
>  
>  #define RSA_BLOCK_MIN_PAD_LEN		8
>  #define RSA_BLOCK_FIRST_OCTET		0x00
>  #define RSA_BLOCK_PRIVATE0_PAD_OCTET	0x00

Can RSA_BLOCK_PRIVATE0_PAD_OCTET be removed, since we removed RSA_BlockPrivate0?

@@ +77,5 @@
>  
> +static unsigned int
> +rsa_modulusLen(SECItem * modulus)
> +{
> +    unsigned char byteZero = modulus->data[0];

This assumes modulus->len != 0. I think that's a valid assumption.

@@ +88,5 @@
>   * the rules defined in PKCS #1.
>   */
>  static unsigned char *
> +rsa_FormatOneBlock(unsigned        modulusLen,
> +                   RSA_BlockType   blockType,

I recommend not aligning function arguments and local variable declarations
in new code because it is not easy to maintain.

@@ +269,3 @@
>      const SECHashObject *hash;
>      void *hashContext;
>      unsigned char C[4];

These local variable names should be either aligned or unaligned, nut not a mixture.

Several other functions have the same problem.

I suspect this is an artifact of the code review tool.

@@ +345,5 @@
> +                 unsigned int          hashLen)
> +{
> +    SECStatus       rv;
> +    unsigned int modulusLen  = rsa_modulusLen(&key->modulus);
> +    unsigned char * buffer;

Nit: the variable names are partially aligned.

@@ +363,5 @@
> +
> +    /*
> +     * make sure we get the same results
> +     */
> +    /* XXX(rsleevi): Constant time */

Do you want to just use NSS_SecureMemcmp()?

@@ +456,5 @@
> +{
> +    SECStatus     rv;
> +    unsigned int  modulusLen  = rsa_modulusLen(&key->modulus);
> +
> +    if (modulusLen > maxOutputLen)

The original code also has:
    if (modulus_len <= 0)
        goto failure;

Just wanted to make sure you deleted that deliberately. I think that test
isn't useful.

@@ +726,5 @@
>      /* Step 2.d - Generate seed */
>      rv = RNG_GenerateGlobalRandomBytes(em + 1, hash->length);
>      if (rv != SECSuccess) {
>          return rv;
>      }

These lines (726-730) don't seem to be indented in the code review tool.

@@ +783,5 @@
> +        return SECFailure;
> +    }
> +
> +    if ((labelLen == 0 && label != NULL) ||
> +        (labelLen > 0 && label == NULL)) {

I wonder if we should preserve this comment from the original code:

  the label is an optional byte string ... If [labelLen] is		
  zero, then [label] MUST be NULL - otherwise, it must be non-NULL.

I think this requirement comes from PKCS #11, so perhaps it should be
checked in softoken/pkcs11c.c.

In general, it seems fine for a non-null buffer pointer to be used with
a zero buffer length, i.e., the first condition (labelLen == 0 && label != NULL)
seems OK.

Also, the error code SEC_ERROR_INVALID_ALGORITHM seems wrong, but that
comes from the original code.

@@ +849,5 @@
> +
> +    rv = RSA_PrivateKeyOpDoubleChecked(key, oaepEncoded, input);
> +    if (rv != SECSuccess) {
> +        goto done;
> +    }

BUG?: this does NOT match the original code, which is (with the sftk_fatalError
code omitted):
    if (rv != SECSuccess && PORT_GetError() == SEC_ERROR_LIBRARY_FAILURE) {
        goto done;
    }

I wonder if we should fall through for a non-SEC_ERROR_LIBRARY_FAILURE error
so that we have constant-time error paths.

@@ +969,5 @@
> +                const unsigned char * mHash,
> +                HASH_HashType         hashAlg,
> +                HASH_HashType         maskHashAlg,
> +                const unsigned char * salt,
> +                unsigned int          saltLen)

Note: the original variable name |sLen| is what's used in the spec.
I think |saltLen| is fine though.

@@ +994,3 @@
>      if (rv != SECSuccess) {
>  	return rv;
>      }

These lines (994-996) don't seem to be indented in the code review tool.

@@ +1187,5 @@
> +SECStatus
> +RSA_CheckSignPSS(RSAPublicKey        * key,
> +                 HASH_HashType         hashAlg,
> +                 HASH_HashType         maskHashAlg,
> +                 unsigned int          saltLength,

Nit: saltLength => saltLen (multiple occurrences)

@@ +1255,5 @@
> +
> +    rv = RSA_PrivateKeyOpDoubleChecked(key, output, formatted.data);
> +    *outputLen = modulusLen;
> +
> +    goto done;

Delete this line. (This line comes from the orignal code.)

@@ +1354,5 @@
> +    /*
> +     * check the padding that was used
> +     */
> +    if (buffer[0] != RSA_BLOCK_FIRST_OCTET ||
> +        buffer[1] != (unsigned char)RSA_BlockPrivate) {

Is this typecast required in C? Is it because RSA_BlockPrivate
is considered signed?

I'm just wondering why the other RSA_BLOCK_xxx_OCTET macros don't need a typecast.

@@ +1371,5 @@
> +    if (*outputLen > maxOutputLen)
> +        goto loser;
> +
> +    /*
> +     * make sure we get the same results

Delete or correct this comment. (This comment comes from the original code
and is apparently inadvertently copied from RSA_CheckSign.)

::: lib/softoken/pkcs11c.c
@@ +531,5 @@
> +        return SECFailure;
> +    }
> +
> +    return RSA_EncryptBlock(&key->u.rsa, output, outputLen, maxLen, input,
> +                            inputLen);

We need to check for SEC_ERROR_LIBRARY_FAILURE and set sftk_fatalError to PR_TRUE
because RSA_EncryptBlock calls (via rsa_FormatBlock) rsa_FormatOneBlock, which used
to set sftk_fatalError to PR_TRUE.

Similarly for RSA_EncryptRaw.

@@ +549,5 @@
> +    }
> +
> +    rv = RSA_DecryptBlock(&key->u.rsa, output, outputLen, maxLen, input,
> +                          inputLen);
> +    if (rv != SECSuccess && PORT_GetError() == SEC_ERROR_LIBRARY_FAILURE) {

Since RSA_DecryptBlock doesn't set the error code on some error paths,
it is a little risky to call PORT_GetError() here.

We may need to call PORT_SetError(0) before calling RSA_DecryptBlock(),
or change RSA_DecryptBlock() to set error code.

This comment also applies to some other functions in this file.

@@ +576,5 @@
> +
> +    if ((hashAlg == HASH_AlgNULL) || (maskHashAlg == HASH_AlgNULL)) {
> +        PORT_SetError(SEC_ERROR_INVALID_ALGORITHM);
> +        return SECFailure;
> +    }

Delete this check, because RSA_EncryptOAEP will check hashAlg and maskHashAlg.

Similarly for sftk_RSADecryptOAEP.

@@ +2127,5 @@
>      return rv;
>  }
>  
>  static SECStatus
> +sftk_RSASignPKCS1(NSSLOWKEYPrivateKey *key, unsigned char *output,

This function uses the "PKCS1" suffix, but the other PKCS #1 v1.5 functions
use the "PKCS" suffix.

I actually think using "PKCS" or "PKCS1" is confusing because OAEP and PSS
are also specified in PKCS #1. Perhaps we can use "V15" or "V1_5", or just
use no suffix, which is also nice because sftk_RSAFoo maps to RSA_Foo except
for RSA_EncryptBlock and RSA_DecryptBlock.

@@ +2166,5 @@
> +    if (rv != SECSuccess && PORT_GetError() == SEC_ERROR_LIBRARY_FAILURE) {
> +        sftk_fatalError = PR_TRUE;
> +    }
> +    return rv;
> +    

Delete this line.

Comment 19

[Wan-Teh Chang]

Comment on attachment 804179 [details] [diff] [review]
1) Check the rv during RSA_EncryptOAEP

Review of attachment 804179 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/freebl/blapi.h
@@ +245,5 @@
> +
> +/********************************************************************
> +** RSASSA-PKCS1-v1_5 signing/verifying, as defined in RFC 3447, Section 8.2.
> +**
> +** The Sign variants expect as input the value of T, the DER-encoded

I think these are also used for the RSA signatures in SSL 3.0
and TLS 1.0-1.1, which don't use DigestInfo. If so, this
comment should be reworded.

@@ +265,5 @@
> +SECStatus RSA_CheckSign(RSAPublicKey        * key,
> +                        const unsigned char * sig,
> +                        unsigned int          sigLen,
> +                        const unsigned char * data,
> +                        unsigned int          dataLen);

This problem seems to come from the original code, so it is OK
to not address it. (The original code uses |hash| instead of |data|
in RSA_CheckSign.)

::: lib/freebl/loader.h
@@ +694,5 @@
> +                                      unsigned int maxOutputLen,
> +                                      const unsigned char *sig,
> +                                      unsigned int sigLen);
> +
> +  /* Version 3.016 came to here */

Can you add a reminder comment here, something like the following?

 /* Add new function pointers at the end of this struct and
  * bump FREEBL_VERSION at the beginning of this file. */

::: lib/softoken/pkcs11c.c
@@ +549,5 @@
> +    }
> +
> +    rv = RSA_DecryptBlock(&key->u.rsa, output, outputLen, maxLen, input,
> +                          inputLen);
> +    if (rv != SECSuccess && PORT_GetError() == SEC_ERROR_LIBRARY_FAILURE) {

On second thought, I think the risk is negligible.
SEC_ERROR_LIBRARY_FAILURE is a serious error. It means
NSS has detected itself is malfunctioning. So if a
previous operation failed with SEC_ERROR_LIBRARY_FAILURE,
we should not get here.

Also, it seems fine to err on being overly strict and set
sftk_fatalError to PR_TRUE on a residual SEC_ERROR_LIBRARY_FAILURE
error. In any case, the best fix is to change RSA_DecryptBlock()
etc. to set error code.

Comment 20

[Wan-Teh Chang]

Comment on attachment 804184 [details] [diff] [review]
2) Refactor bltest

Review of attachment 804184 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for answering my questions. Please note the comment
marked with "IMPORTANT" below.

::: cmd/bltest/blapitest.c
@@ +1244,5 @@
> +dsa_signDigest(void *cx, SECItem *output, const SECItem *input)
> +{
> +    bltestAsymParams *params = (bltestAsymParams *)cx;
> +    if (params->cipherParams.dsa.sigseed.buf.len > 0) {
> +        return DSA_SignDigestWithSeed((DSAPrivateKey *)params->privKey,

I see. I was comparing this with the same-named function
(dsa_signDigest) on line 1170 in the original code.

@@ +2083,2 @@
>      bltestRSAParams *rsap;
> +    RSAPrivateKey **rsaKey = NULL;

The main purpose of the union is to avoid the typecasts, e.g.,
on line 2099 and line 2661. You can still use local variables
for conciseness here. Compare
    rsaKey = (RSAPrivateKey **)&asymp->privKey;
with
    rsaKey = &(asymp->privKey.k.rsa);

In any case, I just wanted to make sure you considered the
alternative.

@@ +2854,5 @@
> +	load_file_data(arena, &params->asym.cipherParams.rsa.seed,
> +	               filename, bltestBase64Encoded);
> +	sprintf(filename, "%s/tests/%s/%s%d", testdir, modestr, "label", j);
> +	load_file_data(arena, &params->asym.cipherParams.rsa.label, filename,
> +	                bltestBase64Encoded);

IMPORTANT: Please see Julien's patch in bug 918697, which will
make load_file_data() fail if the PR_Open() fails.

Solution 1: Allow PR_Open() to fail with PR_FILE_NOT_FOUND_ERROR.
(Julien wants the test to fail when PR_Open fails because the
process runs out of file descriptors.)

Solution 2: Call load_file_data() only when we know the test data
file should be present for the test. I am not sure if it's easy
to do that for the OAEP tests.

@@ +3067,5 @@
>  	    ** then perform operation and compare to plaintext
>  	    */
> +	    if (is_sigCipher(mode)) {
> +		bltestCopyIO(arena, &cipherInfo.input, &pt);
> +		bltestCopyIO(arena, &cipherInfo.output, &ct);

You wrote:
> In the DSA/ECDSA case, they were bypassing the memset entirely.

Can you explain this? On line 3142 of the original code, that memset
seems to apply to the DSA/ECDSA case as well.

Were you referring to the dsaOp() and ecdsaOp() functions that
you deleted? dsaOp() and ecdsaOp() don't use cipherInfo.output
when verifying signatures.

::: cmd/bltest/tests/README
@@ +48,5 @@
>  
> +RSA-OAEP/RSA-PSS:
> +RSA-OAEP and RSA-PSS have a number of additional parameters to feed in.
> +- "seedN": The seed or salt to use when encrypting/signing
> +- "labelN": The label used (as a base64 string)

I looked for the labelN test data files in your test data patch
because they are documented here. You may want to clarify that
they are optional (and perhaps also what a missing labelN file
means).

Comment 21

[Wan-Teh Chang]

Attachment: Add a (unsigned char *) cast to the PORT_Alloc call in MGF1

This cast is not required by C, but is commonly recommended.
(I discovered this when I copied this code to a C++ file.)

Ryan, please make this change to your rsapkcs.c file.

Comment 22

[Kai Engert [:KaiE:]]

changing target milestone to 3.15.4

Comment 23

[Ryan Sleevi]

Attachment: 1) Move RSA from softoken to freebl

Updated with review feedback from wtc, including the MGF1 fix.

Comment 24

[Ryan Sleevi]

Attachment: 2) Refactor bltest to support PSS and OAEP

Updated with review comments.

Comment 25

[Ryan Sleevi]

Attachment: 3) Add PSS and OAEP test data

Added explicit (empty) label files

Comment 26

[Ryan Sleevi]

wtc: The only review feedback I did not address (to my knowledge) were the comments about lib/freebl/blapi.h

I intentionally kept the parameters aligned there, as that's a "public" header of blapi. As such, it cannot change (once these types are exported in freebl, as per the loader), and it serves to document the behaviour of the function.

However, I adjusted the alignment in all other places to avoid the additional alignment, since although the function signature cannot change, the parameters in use may change in the future.

Comment 27

[Wan-Teh Chang]

Comment on attachment 828270 [details] [diff] [review]
1) Move RSA from softoken to freebl

Review of attachment 828270 [details] [diff] [review]:
-----------------------------------------------------------------

r=wtc. I reviewed the previously version of this patch carefully.
I trust you have addressed my comments. I only checked the header
files and ldvector.c this time.  They all look good.

Sorry about the long delay.

Comment 28

[Wan-Teh Chang]

Comment on attachment 815207 [details] [diff] [review]
Add a (unsigned char *) cast to the PORT_Alloc call in MGF1

Marked the patch obsolete. Ryan has incorporate this change
into his patch (attachment 828270 [details] [diff] [review]).

Comment 29

[Wan-Teh Chang]

Comment on attachment 828272 [details] [diff] [review]
2) Refactor bltest to support PSS and OAEP

Review of attachment 828272 [details] [diff] [review]:
-----------------------------------------------------------------

r=wtc. Note that there is a merging error that must be
fixed. I only skimmed through the patch this time.

::: cmd/bltest/blapitest.c
@@ +622,5 @@
>  typedef SECStatus (* bltestHashCipherFn)(unsigned char *dest,
>  					 const unsigned char *src,
>  					 PRUint32 src_length);
>  
> +/* Note: Algorithms are grouped in order for is_symmkeyCipher /

Nit: for => of ?

@@ +766,5 @@
> +    union {
> +      bltestRSAParams rsa;
> +      bltestDSAParams dsa;
> +#ifdef NSS_ENABLE_ECC
> +      bltestECDSAParams ecdsa;

Nit: indent by four.

@@ +783,5 @@
>      bltestIO		key;
>      bltestSymmKeyParams sk;
>      bltestAuthSymmKeyParams ask;
>      bltestRC5Params	rc5;
> +    bltestAsymKeyParams    asymk;

Nit: align with the other members.

@@ +915,2 @@
>  	    return SECFailure;
> +	}

IMPORTANT: This is a merging error. The PR_Close call
and the curly braces were recently removed in
https://hg.mozilla.org/projects/nss/rev/e47fcade843b

@@ +2236,5 @@
>      case bltestRSA:
> +    case bltestRSA_OAEP:
> +    case bltestRSA_PSS:
> +	if (encrypt || cipherInfo->mode != bltestRSA_PSS) {
> +		/* Don't allocate a buffer for PSS in verify mode, as no actual output is

This line looks longer than 80 chars. Probably need to fold this line.

Comment 30

[Wan-Teh Chang]

Comment on attachment 828275 [details] [diff] [review]
3) Add PSS and OAEP test data

Review of attachment 828275 [details] [diff] [review]:
-----------------------------------------------------------------

r=wtc.

::: cmd/bltest/tests/README
@@ +49,5 @@
>  RSA-OAEP/RSA-PSS:
>  RSA-OAEP and RSA-PSS have a number of additional parameters to feed in.
>  - "seedN": The seed or salt to use when encrypting/signing
> +- "labelN": The label used (as a base64 string). An empty file will be treated
> +   as the empty string (""), per RFC 3447.

Hmm... I found that RFC 3447 says:

   Both the encryption and the decryption operations of RSAES-OAEP take
   the value of a label L as input.  In this version of PKCS #1, L is
   the empty string; ...

So we can also get rid of these empty "labelN" files and
just have blapitest.c pass an empty label L to the RSA-OAEP
functions.

::: tests/cipher/cipher.txt
@@ +41,5 @@
>  	0	rsa_-D		RSA_Decrypt
> +	0	rsa_oaep_-E		RSA_EncryptOAEP
> +	0	rsa_oaep_-D		RSA_DecryptOAEP
> +	0	rsa_pss_-S		RSA_SignPSS
> +	0	rsa_pss_-V		RSA_CheckSignPSS

These don't look aligned in the code review tool. Perhaps
it's caused by tab characters.

Comment 31

[Ryan Sleevi]

Comment on attachment 828270 [details] [diff] [review]
1) Move RSA from softoken to freebl

Checked in as https://hg.mozilla.org/projects/nss/rev/913f250474e5

Comment 32

[Ryan Sleevi]

Comment on attachment 828272 [details] [diff] [review]
2) Refactor bltest to support PSS and OAEP

Checked in as https://hg.mozilla.org/projects/nss/rev/175c18dde62d

Comment 33

[Ryan Sleevi]

Comment on attachment 828275 [details] [diff] [review]
3) Add PSS and OAEP test data

Checked in as https://hg.mozilla.org/projects/nss/rev/d5d532821ff6

Comment 34

[Ryan Sleevi]

The core RSA functionality has been moved into softoken. There is still unnecessary re-implementation in http://mxr.mozilla.org/nss/source/lib/cryptohi/secvfy.c#26 , but that will be fixed by supporting PSS in the VFY_ interface