158747 - Support RSAOAEP in softoken

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P2)

Description

[Wan-Teh Chang]

We should implement RSAOAEP (PKCS#1 v2).

Comment 1

[Wan-Teh Chang]

Assigned the bug to Nelson.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Terry, 

The file nss/lib/softoken/rsawrapr.c already contains something that
looks a lot like an OAEP implementation.  It uses only SHA1.

Q: Is this sufficient?

Q: Are there also versions of OAEP that use SHA256, SHA512, etc?

Comment 3

[Wan-Teh Chang]

Moved to target milestone 3.8 because the original
NSS 3.7 release has been renamed 3.8.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Remove target milestone of 3.8, since these bugs didn't get into that release.

Comment 5

[Julien Pierre]

Nelson,

I checked the OAEP code in rsawrapr.c . It doesn't appear that there is any way
to get to it from accross the PKCS#11 boundary. There is no way to get to the
RSA_BlockOAEP switch blocks. Ie., the OAEP is not exposed in softoken currently .

We have an internal request for this feature, so I'm retargetting this to 3.10 .

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

retargetting to  3.12

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Unsetting target milestone in unresolved bugs whose targets have passed.

Comment 8

[Aleksey Sanin]

I've had some people asking about this feature for xmlsec-nss recently.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

What's the application for it, Aleksey?  How is it used?

Comment 10

[Aleksey Sanin]

AFAIK, it's some kind of document workflow system where all documents are encrypted with an AES session key which in its turn is encrypted with one or more RSA keys for people to whom this document needs to go. That's all I know.

Comment 11

[Dave Allan]

(In reply to Aleksey Sanin from comment #10)
> AFAIK, it's some kind of document workflow system where all documents are
> encrypted with an AES session key which in its turn is encrypted with one or
> more RSA keys for people to whom this document needs to go. That's all I
> know.

FWIW, this is the scenario that I was trying to implement when I got interested in this RFE, but I ended up using openssl and then stopped working on that application in around 2009.

Comment 12

[Ryan Sleevi]

Attachment: Initial implementation

Bob: This was the implementation I was mentioning last week.

The cmdline tool 'oaeptest' is questionable as to whether it should be checked in, as we discussed. Given that the RSA tests ( ftp://ftp.rsasecurity.com/pub/pkcs/pkcs-1/pkcs-1v2-1-vec.zip ) include test for private key moduli sizes that NSS doesn't support (eg: 1025-bit, 1026-bit), there's not as many tests that we can run.

That said, the softoken changes should be ready to go for review.

Comment 13

[Wan-Teh Chang]

Comment on attachment 702475 [details] [diff] [review]
Initial implementation

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

r=wtc.

1. I did not review oaeptest.c, but I reviewed everything else carefully.
(Bob, this means you can just do a cursory review of the softoken code.)

You may want to move oaeptest.c to a separate patch so that the reviewed
parts of this patch can be checked in first.

2. You may want to have someone else check the constant time code for
finding the padding in eme_oaep_decode.

::: mozilla/security/nss/cmd/oaeptest/Makefile
@@ +9,5 @@
> +#######################################################################
> +
> +include manifest.mn
> +#MKPROG = purify -cache-dir=/u/mcgreer/pcache -best-effort \
> +#	-always-use-cache-dir $(CC)

Don't copy the commented-out code.

::: mozilla/security/nss/lib/softoken/pkcs11c.c
@@ +457,5 @@
>      return -1;
>  }
>  
> +static SECStatus
> +sftk_EncryptOAEP(SFTKHashVerifyInfo *info, unsigned char *output,

It is confusing to reuse SFTKHashVerifyInfo and SFTKHashSignInfo
for OAEP. It would be better to define new 'info' structures for
OAEP.

::: mozilla/security/nss/lib/softoken/rsawrapr.c
@@ +717,4 @@
>      return SECSuccess;
>  }
>  
> +static unsigned char constantTimeEQ8(unsigned char a, unsigned char b) {

Please document this function.

It seems to output exactly 1 rather than any nonzero value
when a is equal to b. The reason this function must return
exactly 1 for true should be explained (constantTimeCondition
needs this property).

@@ +724,5 @@
> +    c &= c >> 1;
> +    return c;
> +}
> +
> +static unsigned char constantTimeCompare(unsigned char* a, unsigned char* b,

Declare a and b as const.

@@ +745,5 @@
> +    return (~(c - 1) & a) | ((c - 1) & b);
> +}
> +
> +static SECStatus
> +eme_oaep_decode(unsigned char* output, unsigned int* outputLen,

Document this function as you did eme_oaep_encode.

@@ +756,5 @@
> +    void *hashContext;
> +    SECStatus rv = SECFailure;
> +    unsigned char labelHash[HASH_LENGTH_MAX];
> +    unsigned int i, tmpMaskLen, paddingOffset;
> +    unsigned char *tmpMask, *tmpOutput;

Initialize both of these to NULL. Otherwise, the "goto done"
statement on line 786 will cause us to use an initialized
tmpMask at the |done| label.

@@ +763,5 @@
> +    hash = HASH_GetRawHashObject(hashAlg);
> +
> +    /* 1.c */
> +    if (inputLen < (hash->length * 2) + 2) {
> +        PORT_SetError(SEC_ERROR_OUTPUT_LEN);

SEC_ERROR_OUTPUT_LEN => SEC_ERROR_INPUT_LEN

@@ +767,5 @@
> +        PORT_SetError(SEC_ERROR_OUTPUT_LEN);
> +        return SECFailure;
> +    }
> +
> +    /* Step 2.a - Generate lHash */

2.a => 3.a

@@ +776,5 @@
> +    }
> +    (*hash->begin)(hashContext);
> +    if (labelLen > 0)
> +        (*hash->update)(hashContext, label, labelLen);
> +    (*hash->end)(hashContext, labelHash, &i, hash->length);

The last argument should be sizeof(labelHash).

@@ +833,5 @@
> +        isGood = constantTimeCondition(~foundPadding & ~isZero, 0, isGood);
> +    }
> +
> +    if (!(isGood & foundPadding))
> +        goto done;

We need to set an error code for "decryption error".

@@ +837,5 @@
> +        goto done;
> +
> +    /* End timing dependent code */
> +
> +    ++paddingOffset; /* Skip the leading 0x01 */

The "leading 0x01" seems wrong. Did you mean the "trailing 0x01"
because the 0x01 octet follows PS?

@@ +840,5 @@
> +
> +    ++paddingOffset; /* Skip the leading 0x01 */
> +
> +    *outputLen = inputLen - paddingOffset;
> +    if (*outputLen == 0 || *outputLen > maxOutputLen) {

It seems that it is OK for |M| to be zero length.

@@ +879,5 @@
> +
> +    /* Step 1.b */
> +    reservedLen = (2 * hash->length) + 2;
> +    if (emLen < reservedLen || inputLen > (emLen - reservedLen)) {
> +        PORT_SetError(SEC_ERROR_OUTPUT_LEN);

SEC_ERROR_OUTPUT_LEN => SEC_ERROR_INPUT_LEN

@@ +902,5 @@
> +     *        +--+----------+----------------------------+
> +     *  EM =  |00|maskedSeed|          maskedDB          |
> +     *        +--+----------+----------------------------+
> +     *
> +     * We use tmpBuf to hold the result of the MGF functions, and all other

Typo: tmpBuf => tmp

Perhaps we should use a meaningful name such as |mask| instead of |tmp|.

@@ +1261,5 @@
> +RSA_EncryptOAEP(CK_RSA_PKCS_OAEP_PARAMS *oaepParams,
> +        NSSLOWKEYPublicKey *key,
> +        unsigned char *output, unsigned int *outputLen,
> +        unsigned int maxOutputLen,
> +        const unsigned char *input, unsigned int inputLen)

The function arguments should be aligned on the left with the
first argument. Make the same change to RSA_DecryptOAEP.

@@ +1293,5 @@
> +        PORT_SetError(SEC_ERROR_INVALID_ALGORITHM);
> +        return SECFailure;
> +    }
> +    sourceData = (unsigned char*)oaepParams->pSourceData;
> +    sourceDataLen = oaepParams->ulSourceDataLen;

We should also perform the check of oaepParams->ulSourceDataLen described in
RFC 3447, Sec. 7.1.1, Step 1.a:

      a. If the length of L is greater than the input limitation for the
         hash function (2^61 - 1 octets for SHA-1), output "label too
         long" and stop.

2^61 - 1 octets is theoretically possible if unsigned long is 64
bits. I think we can use a much smaller maximum value.

@@ +1343,5 @@
> +        return SECFailure;
> +    }
> +
> +    if (inputLen < modulusLen) {
> +        PORT_SetError(SEC_ERROR_OUTPUT_LEN);

Typo: SEC_ERROR_OUTPUT_LEN => SEC_ERROR_INPUT_LEN

This probably should be an inequality test:
  if (inputLen != modulusLen) {

Compare this with the check at the beginning of RSA_CheckSignPSS:

    if (sign_len != modulus_len) {
        PORT_SetError(SEC_ERROR_BAD_SIGNATURE);
        return SECFailure;
    }

@@ +1374,5 @@
> +    rv = RSA_PrivateKeyOpDoubleChecked(&key->u.rsa, oaepDecoded, input);
> +    if (rv != SECSuccess && PORT_GetError() == SEC_ERROR_LIBRARY_FAILURE) {
> +	sftk_fatalError = PR_TRUE;
> +        goto done;
> +    }

Please add a comment to explain why we're falling through
on non-SEC_ERROR_LIBRARY_FAILURE errors.

@@ +1376,5 @@
> +	sftk_fatalError = PR_TRUE;
> +        goto done;
> +    }
> +
> +    rv = eme_oaep_decode(output, outputLen, maxOutputLen, oaepDecoded,

oaepDecoded should be renamed oaepEncoded because
it is the input to a decode function.

Comment 14

[Wan-Teh Chang]

Comment on attachment 702475 [details] [diff] [review]
Initial implementation

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

Two more comments.

::: mozilla/security/nss/lib/softoken/rsawrapr.c
@@ -268,5 @@
> -      /*
> -       * Blocks intended for public-key operation, using
> -       * Optimal Asymmetric Encryption Padding (OAEP).
> -       */
> -      case RSA_BlockOAEP:

Please remove the RSA_BlockOAEP enumeration constant in softoknt.h.

@@ +1288,5 @@
> +    if ((hashAlg == HASH_AlgNULL) || (maskHashAlg == HASH_AlgNULL)) {
> +        PORT_SetError(SEC_ERROR_INVALID_ALGORITHM);
> +        return SECFailure;
> +    }
> +    if (oaepParams->source != CKZ_DATA_SPECIFIED) {

Please add a comment to point out that what PKCS #11 calls the
encoding parameter source is the label in the current version of
PKCS #1.

Comment 15

[Wan-Teh Chang]

Attachment: Declare 'input' as const in RSA_EncryptBlock and RSA_DecryptBlock

I noticed this problem while reviewing your patch.

Comment 16

[Wan-Teh Chang]

Attachment: [Correct patch] Declare 'input' as const in RSA_EncryptBlock and RSA_DecryptBlock

Comment 17

[Wan-Teh Chang]

(In reply to Ryan Sleevi from comment #12)
> ... Given that the RSA tests (
> ftp://ftp.rsasecurity.com/pub/pkcs/pkcs-1/pkcs-1v2-1-vec.zip ) include test
> for private key moduli sizes that NSS doesn't support (eg: 1025-bit,
> 1026-bit), there's not as many tests that we can run.

Does NSS really not support 1025-bit and 1026-bit RSA moduli?

Comment 18

[Ryan Sleevi]

Based on http://mxr.mozilla.org/security/source/security/nss/lib/freebl/rsa.c?mark=245-249#224 , yes.

Comment 19

[Robert Relyea]

Hmm... so that means NSS can't generate a 1025-bit or 1026-bit key. That doesn't necessarily mean NSS can't use those keys sizes if presented with such keys. I'm pretty sure those keys would work if you used them (I seem to remember some RA's had some non-standard RSA keys sizes that we accepted.

bob

Comment 20

[Wan-Teh Chang]

Comment on attachment 702475 [details] [diff] [review]
Initial implementation

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

::: mozilla/security/nss/lib/softoken/rsawrapr.c
@@ +828,5 @@
> +    for (i = 1 + (hash->length * 2); i < inputLen; ++i) {
> +        unsigned char isZero = constantTimeEQ8(0x00, tmpOutput[i]);
> +        unsigned char isOne = constantTimeEQ8(0x01, tmpOutput[i]);
> +        paddingOffset = constantTimeCondition(isOne & ~foundPadding, i, paddingOffset);
> +        foundPadding = constantTimeCondition(isOne, 1, foundPadding);

It would be nice to point out that foundPadding may have false positives.
foundOne seems to be a more accurate name for this variable. In any case,
comments that elaborate on the last three lines of this for loop would
be helpful.

Comment 21

[Ryan Sleevi]

Comment on attachment 702475 [details] [diff] [review]
Initial implementation

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

::: mozilla/security/nss/lib/softoken/pkcs11c.c
@@ +457,5 @@
>      return -1;
>  }
>  
> +static SECStatus
> +sftk_EncryptOAEP(SFTKHashVerifyInfo *info, unsigned char *output,

Bob: What are your feelings on this?

One thought was simply typedef asymmetric-encrypt structures for this, which are exactly the same structures, but I wasn't sure if that was overkill.

I'm just trying to understand how much of this plays into the "public" API of softoken, and whether it's better to add more types (with identical functions for freeing) for a stabler API or to simply re-use the types and free functions as I have here.

::: mozilla/security/nss/lib/softoken/rsawrapr.c
@@ +1288,5 @@
> +    if ((hashAlg == HASH_AlgNULL) || (maskHashAlg == HASH_AlgNULL)) {
> +        PORT_SetError(SEC_ERROR_INVALID_ALGORITHM);
> +        return SECFailure;
> +    }
> +    if (oaepParams->source != CKZ_DATA_SPECIFIED) {

I'm not sure this comment is necessary. It's "clear" from both the perspective of RFC 3447 (Appendix A.2.1) and from the PKCS#11 definition. It seems a bit redundant to replicate it here?

From A.2.1:
 pSourceAlgorithm identifies the source (and possibly the value)
      of the label L.  It shall be an algorithm ID with an OID in the
      set PKCS1PSourceAlgorithms, which for this version shall consist
      of id-pSpecified, indicating that the label is specified
      explicitly.  The parameters field associated with id-pSpecified
      shall have a value of type OCTET STRING, containing the
      label.  In previous versions of this specification, the term
      "encoding parameters" was used rather than "label", hence the
      name of the type below.

@@ +1293,5 @@
> +        PORT_SetError(SEC_ERROR_INVALID_ALGORITHM);
> +        return SECFailure;
> +    }
> +    sourceData = (unsigned char*)oaepParams->pSourceData;
> +    sourceDataLen = oaepParams->ulSourceDataLen;

I debated this, but the maximum length is contingent upon the algorithm used, and the freebl interface doesn't currently do any maximum length checking (AFAICT).

It does seem a pathological edge case, since it means that the caller must have mapped in 2^61 bytes of data. Surely this is far beyond the edge of insanity? SHA-2 and SHA-3 are 2^64 or greater, and md5 is not defined for use with OAEP/PSS, so is this a real concern?

Comment 22

[Ryan Sleevi]

Attachment: [Patch 2] Updated review comments

Comment 23

[Ryan Sleevi]

Attachment: Diff between Patch 1 and Patch 2

Added as an FYI - the set of changes in Patch 2 not in Patch 1

Comment 24

[Wan-Teh Chang]

Comment on attachment 702475 [details] [diff] [review]
Initial implementation

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

::: mozilla/security/nss/lib/softoken/pkcs11c.c
@@ +457,5 @@
>      return -1;
>  }
>  
> +static SECStatus
> +sftk_EncryptOAEP(SFTKHashVerifyInfo *info, unsigned char *output,

If we define a SFTKOAEPEncryptInfo structure, it can avoid the use of void* for
|params| and the unused hashOid member can be removed:

struct SFTKOAEPEncryptInfoStr {
    CK_RSA_PKCS_OAEP_PARAMS *params;
    NSSLOWKEYPublicKey *key;
};

Similarly:

struct SFTKOAEPDecryptInfoStr {
    CK_RSA_PKCS_OAEP_PARAMS *params;
    NSSLOWKEYPrivateKey *key;
};

On lines 465 and 474, we are currently relying on C's implicit conversion
of void* to any object pointer. It is better if info->params has the right
type.

::: mozilla/security/nss/lib/softoken/rsawrapr.c
@@ +1288,5 @@
> +    if ((hashAlg == HASH_AlgNULL) || (maskHashAlg == HASH_AlgNULL)) {
> +        PORT_SetError(SEC_ERROR_INVALID_ALGORITHM);
> +        return SECFailure;
> +    }
> +    if (oaepParams->source != CKZ_DATA_SPECIFIED) {

This comment is definitely not necessary. Anyone who has done his
homework on PKCS #1 OAEP should be able to figure this out. I just
thought a short comment here to confirm that obscure fact (buried
in an appendix of a 74-page RFC) would be reassuring to the reader
of this code.

@@ +1293,5 @@
> +        PORT_SetError(SEC_ERROR_INVALID_ALGORITHM);
> +        return SECFailure;
> +    }
> +    sourceData = (unsigned char*)oaepParams->pSourceData;
> +    sourceDataLen = oaepParams->ulSourceDataLen;

Please ignore this suggestion of mine. We can assume that
the hash->update() call inside eme_oaep_encode() will take care
of this check:

    if (labelLen > 0)
        (*hash->update)(hashContext, label, labelLen);

When I suggested this check, I didn't fully understand what the
check is for. I was just comparing your code with the steps in
RFC 3447, Sec. 7.1.1 and noticed one step was missing.

Comment 25

[Ryan Sleevi]

(In reply to Robert Relyea from comment #19)
> Hmm... so that means NSS can't generate a 1025-bit or 1026-bit key. That
> doesn't necessarily mean NSS can't use those keys sizes if presented with
> such keys. I'm pretty sure those keys would work if you used them (I seem to
> remember some RA's had some non-standard RSA keys sizes that we accepted.
> 
> bob

Not for private keys.

See http://mxr.mozilla.org/security/source/security/nss/lib/freebl/rsa.c?mark=101-105,124-128#101

the parameter keySizeInBits comes from http://mxr.mozilla.org/security/source/security/nss/lib/freebl/rsa.c?mark=655-656,717,726,736,758-759#655 (either prime1, prime2, or the modulus). Note the BITS_PER_BYTE multiplier.

As a result, for a 1025-bit key, the expected keySizeInBits is set to 1032

Comment 26

[Wan-Teh Chang]

Comment on attachment 705695 [details] [diff] [review]
[Patch 2] Updated review comments

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

::: mozilla/security/nss/lib/softoken/rsawrapr.c
@@ +30,5 @@
> +    unsigned char c = ~(a ^ b);
> +    c &= c >> 4;
> +    c &= c >> 2;
> +    c &= c >> 1;
> +    return c;

I found a strange formula in Hacker's Delight, p.22:

static unsigned char constantTimeEQ8(unsigned char a, unsigned char b) {
    unsigned int c = ~(a - b | b - a);
    c >>= 31;
    return c;
}

Note the use of unsigned int. This also works:

static unsigned char constantTimeEQ8(unsigned char a, unsigned char b) {
    unsigned char c = ~(a - b | b - a);
    c >>= 7;
    return c;
}

But because of the integer promotion thing in C,
the unsigned int version might be more portable. (I don't fully understand how integer
promotion works.)

Comment 27

[Ryan Sleevi]

Thanks for the comments, Wan-Teh.

I'm going to go ahead and land this, but I think I've convinced myself that the integer to octets conversion of MPI is still acting as an oracle regarding if there is a leading zero.

This is described better in http://www.cdc.informatik.tu-darmstadt.de/reports/reports/mangers_attack_revisited.pdf , but also applies to Bug 577498, so I do not think this is a particularly new attack for NSS.

It will be good to revisit after Bug 836019 centralizes the crypto-y bits.

Comment 28

[Ryan Sleevi]

Comment on attachment 705161 [details] [diff] [review]
[Correct patch] Declare 'input' as const in RSA_EncryptBlock and RSA_DecryptBlock

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

r- from me.

RSA_EncryptBlock / RSA_EncryptRaw are matching the SFTKCipher function definition, which declares these non-const.

This is a pervasive issue in softoken/freebl, and best seems left as a separate issue, as it touches a large number of functions and files to properly viralize the const-ness.

Comment 29

[Wan-Teh Chang]

Comment on attachment 705161 [details] [diff] [review]
[Correct patch] Declare 'input' as const in RSA_EncryptBlock and RSA_DecryptBlock

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

Are you going to change sftk_EncryptOAEP and sftk_DecryptOAEP
to match the SFTKCipher function definition and declare their
|input| arguments as non-const?

Comment 30

[Wan-Teh Chang]

Comment on attachment 705161 [details] [diff] [review]
[Correct patch] Declare 'input' as const in RSA_EncryptBlock and RSA_DecryptBlock

I looked into the SFTKCipher issue a bit.

1. This MXR query on the NSS source tree shows a roughly 50-50
split on whether a function used as a SFTKCipher declares its input
argument as const:
http://mxr.mozilla.org/security/search?string=%28SFTKCipher%29

The split seems to be between symmetric encryption and everything
else: the symmetric encryption functions declare |input| as const,
whereas the other functions (RSA, HMAC, and TLS PRF) don't.

2. None of the SFTKCipher functions match the SFTKCipher typedef
exactly -- all of them declare data buffers as unsigned char *
instead of void *. This is why they all need the SFTKCipher cast
when being assigned to context->update. We should change the
SFTKCipher typedef to use unsigned char * instead.

3. It is a benign mismatch to use const unsigned char * when the
typedef only specifies a non-const void * because the 'const'
exceeds the typedef's requirement. It seems that we should take
this opportunity to change all the RSA encryption SFTKCipher
functions to use const |input| arguments. But that means I'd
also need to fix RSA_EncryptRaw and RSA_DecryptRaw.

Comment 31

[Ryan Sleevi]

wtc: I think, following NSS's instance on consistency of locality, perhaps it may be 'more' correct within NSS code to use non-const. I note that bug #183364 exists to track the cleanup for RSA (which as I mentioned Friday, I have a partially complete patch that does). We can continue the const followup there.

Comment 32

[Ryan Sleevi]

Attachment: Updated patch to be landed

Because I do not want to enable code in softoken that does not have unit tests, I have commented out the PKCS#11 bindings for now. However, the code within rsawrapr can land, as it also includes utility functions for constant timing.

I will relocate this code to freebl, as described in Bug #836019, so that unit tests can be integrated as part of bltests.

Comment 33

[Ryan Sleevi]

Attachment: Removed incorrect manifest.mm change

Comment 34

[Ryan Sleevi]

Comment on attachment 709972 [details] [diff] [review]
Removed incorrect manifest.mm change

Checking in mozilla/security/nss/lib/softoken/pkcs11c.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11c.c,v  <--  pkcs11c.c
new revision: 1.134; previous revision: 1.133
done
Checking in mozilla/security/nss/lib/softoken/pkcs11i.h;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11i.h,v  <--  pkcs11i.h
new revision: 1.63; previous revision: 1.62
done
Checking in mozilla/security/nss/lib/softoken/rsawrapr.c;
/cvsroot/mozilla/security/nss/lib/softoken/rsawrapr.c,v  <--  rsawrapr.c
new revision: 1.22; previous revision: 1.21
done
Checking in mozilla/security/nss/lib/softoken/softoken.h;
/cvsroot/mozilla/security/nss/lib/softoken/softoken.h,v  <--  softoken.h
new revision: 1.29; previous revision: 1.28
done
Checking in mozilla/security/nss/lib/softoken/softoknt.h;
/cvsroot/mozilla/security/nss/lib/softoken/softoknt.h,v  <--  softoknt.h
new revision: 1.8; previous revision: 1.7
done

Comment 35

[Wan-Teh Chang]

Comment on attachment 709972 [details] [diff] [review]
Removed incorrect manifest.mm change

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

::: mozilla/security/nss/lib/softoken/pkcs11c.c
@@ +564,5 @@
> +	if (pMechanism->ulParameterLen != sizeof(CK_RSA_PKCS_OAEP_PARAMS)) {
> +	    crv = CKR_MECHANISM_PARAM_INVALID;
> +	    break;
> +	}
> +	/\* XXX: Need Parameter validation here *\/

In the future you can use #if 0 to comment out a block of code.
Then you don't need to deal with embedded comments.

Comment 36

[Wan-Teh Chang]

Comment on attachment 705695 [details] [diff] [review]
[Patch 2] Updated review comments

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

Ryan: I reviewed the oaeptest.c file in this patch because
I assume the lack of a test was why you disabled the RSA OAEP
code that you checked in.

oaeptest.c seems to be work in progress.  Also, I may have
misunderstood how you intend to report the success/failure
of this test, so some of my comments on the exit status of
this program may be wrong.

::: mozilla/security/nss/cmd/oaeptest/Makefile
@@ +1,2 @@
> +#! gmake
> +# 

Nit: remove the space.

::: mozilla/security/nss/cmd/oaeptest/manifest.mn
@@ +1,1 @@
> +# 

Nit: remove the space.

::: mozilla/security/nss/cmd/oaeptest/oaeptest.c
@@ +12,5 @@
> +#include "secerr.h"
> +#include "secder.h"
> +#include "secdig.h"
> +#include "secoid.h"
> +#include "ec.h"

Remove "ec.h" and probably "secdig.h".

"secoid.h" and the SECOID_Init() call may be unnecessary.
I am not sure though.

@@ +49,5 @@
> + * If buffer is not large enough to hold the data, a new buffer will be
> + * reallocated, using the optional "arena".
> + */
> +PRBool read_hex_data(FILE *input, char *label, size_t labelLen,
> +                     SECItem *buffer, PLArenaPool *arena)

|label| should be 'const'.

If the PRBool return value indicates success/failure, it may be better
to return SECStatus (SECSuccess and SECFailure) instead.

@@ +112,5 @@
> +/**
> + * Test harness to run through OAEP test vectors
> + * testfn is the filename containing the test vectors
> + */
> +void oaep_test(char *testfn)

|testfn| should be 'const'.

@@ +124,5 @@
> +    SECItem encryption = {0};
> +    SECItem tmp = {0};
> +
> +    RSAPrivateKey *rsaBlapiPrivKey = NULL;
> +    RSAPublicKey *rsaBlapiPublicKey = NULL;

oaep_test() doesn't need rsaBlapiPublicKey because it doesn't do OAEP encryption.

@@ +126,5 @@
> +
> +    RSAPrivateKey *rsaBlapiPrivKey = NULL;
> +    RSAPublicKey *rsaBlapiPublicKey = NULL;
> +
> +    rsareq = fopen(testfn, "r");

Check for fopen() failure:
    if (rsareq == NULL)
        goto loser;

@@ +141,5 @@
> +                PORT_FreeArena(rsaBlapiPrivKey->arena, PR_FALSE);
> +                rsaBlapiPrivKey = NULL;
> +                rsaBlapiPublicKey = NULL;
> +            }
> +            PRArenaPool *arena = PORT_NewArena(1024);

The |arena| variable needs to be declare at the beginning of
the block.  Visual C++ 2012 still doesn't allow variable declarations
in the middle of a block in C code, contrary to what Herb Sutter
said in http://herbsutter.com/2012/05/03/reader-qa-what-about-vc-and-c99/

Some other variables are also declared in the middle of a block.

@@ +147,5 @@
> +                goto loser;
> +
> +            rsaBlapiPrivKey = PORT_ArenaZNew(arena, RSAPrivateKey);
> +            if (rsaBlapiPrivKey == NULL) {
> +                fprintf(rsaresp, "Error unable to create RSA key\n");

Error messages in this function probably should be printed to stderr instead of rsaresp.

@@ +187,5 @@
> +                fprintf(rsaresp, "Error unable to initialize key\n");
> +                goto loser;
> +            }
> +            rsaBlapiPublicKey = (RSAPublicKey *)PORT_ArenaAlloc(
> +                rsaBlapiPrivKey->arena, sizeof(RSAPublicKey));

Use the PORT_ArenaNew macro:
    rsaBlapiPublicKey = PORT_ArenaNew(rsaBlapiPrivKey->arena, RSAPublicKey);

@@ +242,5 @@
> +            continue;
> +        }
> +    }
> +
> +loser:

This function returns void. It needs a way to report failure.
For example, you can do:
    ...
    return;

loser:
    ...
    exit(1);

Or you can have this function return SECStatus and have the caller
check the return value.

@@ +255,5 @@
> +    SECITEM_FreeItem(&encryption, PR_FALSE);
> +    SECITEM_FreeItem(&tmp, PR_FALSE);
> +}
> +
> +int sign_and_decrypt_test() {

1. This function returns int, but doesn't have any return statement.
In addition, the caller doesn't check the return value.

2. sign_and_decrypt_test => encrypt_and_decrypt_test

@@ +272,5 @@
> +    RSAPrivateKey *rsaBlapiPrivKey = NULL;
> +    RSAPublicKey *rsaBlapiPublicKey = NULL;
> +
> +    rsaBlapiPrivKey = RSA_NewKey(2048, &pubExp);
> +    rsaBlapiPublicKey = (RSAPublicKey *)PORT_ArenaAlloc(rsaBlapiPrivKey->arena, sizeof(RSAPublicKey));

Use the PORT_ArenaNew macro:
    rsaBlapiPublicKey = PORT_ArenaNew(rsaBlapiPrivKey->arena, RSAPublicKey);

http://mxr.mozilla.org/security/ident?i=PORT_ArenaNew

@@ +276,5 @@
> +    rsaBlapiPublicKey = (RSAPublicKey *)PORT_ArenaAlloc(rsaBlapiPrivKey->arena, sizeof(RSAPublicKey));
> +    rsaBlapiPublicKey->modulus = rsaBlapiPrivKey->modulus;
> +    rsaBlapiPublicKey->publicExponent = rsaBlapiPrivKey->publicExponent;
> +
> +    NSSLOWKEYPrivateKey * rsa_private_key;

oaeptest.c [This comment also applies to the oaep_test() function.]

This variable doesn't seem necessary. We can just use &low_RSA_private_key.

Similarly for rsa_public_key and &low_RSA_public_key.

@@ +291,5 @@
> +    char decrypted[2048];
> +
> +    unsigned int outputLen = 0;
> +    if (SECSuccess != RSA_EncryptOAEP(&params, rsa_public_key, &encrypted[0], &outputLen,
> +                                      sizeof(encrypted), &message[0], sizeof(message))) {

Nit: on line 268, you took care to exclude the terminating null in |label|.
For consistency you should also pass sizeof(message) - 1 here to exclude
the terminating null in |message|.

This would also require testing outputLen != sizeof(message) - 1 on line 306.

@@ +294,5 @@
> +    if (SECSuccess != RSA_EncryptOAEP(&params, rsa_public_key, &encrypted[0], &outputLen,
> +                                      sizeof(encrypted), &message[0], sizeof(message))) {
> +        fprintf(stderr, "Unable to encrypt\n");
> +        goto loser;
> +    }

You should test
    if (outputLen != sizeof(encrypted)) {
        fprintf(stderr, "Message mismatch\n");
        goto loser;
    }

because you are passing sizeof(encrypted) to RSA_DecryptOAEP().

NOTE: sizeof(encrypted) is 2048 bytes, but the OAEP encryption
output should be 2048 bits.  So this seems wrong.

Alternatively, you can pass outputLen instead to RSA_DecryptOAEP().

@@ +317,5 @@
> +}
> +
> +int main(int argc, char** argv) {
> +    if (argc < 2)
> +        exit(-1);

1. Since you only use argv[1], this check can be more strict:
    if (argc != 2)

2. exit(-1) => exit(1)

(I know this is copied from an existing test program.)

The MSYS shell we're using on Windows has a bug and does not
handle a negative argument to exit() correctly.  So we should
pass a value between 0 and 255 (inclusive), which is the
documented range of the exit status.

@@ +326,5 @@
> +
> +    sign_and_decrypt_test();
> +    oaep_test(argv[1]);
> +
> +    return 1;

return 1 => return 0

A nonzero exit status is considered a failure. 0 means success.

Comment 37

[Ryan Sleevi]

Softoken PKCS#11 support added in https://hg.mozilla.org/projects/nss/rev/913f250474e5

Comment 38

[sanderson7]

I noticed that the CKM_RSA_PKCS_OAEP mechanism is still disabled which raises a CKR_MECHANISM_INVALID RV when I try to use it on 3.15.4.  Are there any plans to enable this mechanism?