1009785 - Enable OAEP within softoken

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Ryan Sleevi]

Bug 836019 laid the framework for RSA-OAEP support within freebl in order to support unittesting.

Currently, softoken does not expose the CKM_RSA_PKCS_OAEP mechanism in the Decrypt/Encrypt functions.

Comment 1

[Ryan Sleevi]

Attachment: Expose CKM_RSA_PKCS_OAEP as a single-shot RSA encrypt/decrypt function

Comment 2

[Ryan Sleevi]

Note that the above patch also adds more robust parameter validation to the PSS case, so that they can return a more meaningful error (CKR_MECHANISM_PARAM_INVALID), rather than the default CKR_DEVICE_ERROR (because invalid params are mapped to SEC_ERROR_INVALID_ALGORITHM, which then maps to CKR_DEVICE_ERROR)

Comment 3

[Wan-Teh Chang]

Comment on attachment 8421956 [details] [diff] [review]
Expose CKM_RSA_PKCS_OAEP as a single-shot RSA encrypt/decrypt function

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

r=wtc.

Please add an entry for CKM_RSA_PKCS_OAEP to the |mechanisms|
table in nss/lib/softoken/pkcs11.c"
http://mxr.mozilla.org/nss/source/lib/softoken/pkcs11.c#265

::: lib/softoken/pkcs11c.c
@@ +305,5 @@
> +/*
> + * Returns true if "params" contains a valid set of PSS parameters
> + */
> +static PRBool
> +sftk_ValidatePssParams(const CK_RSA_PKCS_PSS_PARAMS *params)

Nit: it may be a good idea to add a sftk_ValidateOaepParams
function even though it will be called only once.

@@ +728,5 @@
>  	    crv = CKR_MECHANISM_PARAM_INVALID;
>  	    break;
>  	}
> +	/* Validate parameters */
> +	oaep_param = (CK_RSA_PKCS_OAEP_PARAMS*)pMechanism->pParameter;

Nit: it seems that we should also check if oaep_param is a
null pointer, to match the null pointer check in
sftk_ValidatePssParams (line 311).

@@ +738,5 @@
> +	    (GetHashTypeFromMechanism(oaep_param->mgf) == HASH_AlgNULL)) {
> +		crv = CKR_MECHANISM_PARAM_INVALID;
> +		break;
> +	}
> +	if ((oaep_param->ulSourceDataLen == 0 && oaep_param->pSourceData != NULL) ||

Nit: In general this is not a problem, so it would be nice to
document why we disallow it. It comes from PKCS #11:

  If the parameter is empty, pSourceData must be NULL and 
  ulSourceDataLen must be zero.

@@ +741,5 @@
> +	}
> +	if ((oaep_param->ulSourceDataLen == 0 && oaep_param->pSourceData != NULL) ||
> +	    (oaep_param->ulSourceDataLen != 0 && oaep_param->pSourceData == NULL)) {
> +	    crv = CKR_MECHANISM_PARAM_INVALID;
> +		break;

The indentation of lines 735, 739-740, and 745 seems wrong.

@@ +2411,4 @@
>  	    crv = CKR_MECHANISM_PARAM_INVALID;
>  	    break;
>  	}
>  	info = PORT_New(SFTKHashSignInfo);

Nit: it seems that you should also add a blank line before
this line to match your change on line 3052.

Comment 4

[Wan-Teh Chang]

(In reply to Ryan Sleevi from comment #2)
> Note that the above patch also adds more robust parameter validation to the
> PSS case, so that they can return a more meaningful error
> (CKR_MECHANISM_PARAM_INVALID), rather than the default CKR_DEVICE_ERROR
> (because invalid params are mapped to SEC_ERROR_INVALID_ALGORITHM, which
> then maps to CKR_DEVICE_ERROR)

NSS maps CKR_MECHANISM_INVALID to SEC_ERROR_INVALID_ALGORITHM:
http://mxr.mozilla.org/nss/source/lib/pk11wrap/pk11err.c#70

So we should map SEC_ERROR_INVALID_ALGORITHM to CKR_MECHANISM_INVALID.
Can you tell me where NSS maps SEC_ERROR_INVALID_ALGORITHM to
CKR_DEVICE_ERROR? Is it sftk_MapCryptError?

http://mxr.mozilla.org/nss/ident?i=sftk_MapCryptError

We should fix that.

Comment 5

[Ryan Sleevi]

Attachment: Expose CKM_RSA_PKCS_OAEP as a single-shot RSA encrypt/decrypt function

Updated patch that incorporates review feedback, as well as the most important part - making sure it appears in C_GetMechanismList as a mechanism that supports wrap, unwrap, encrypt, and decrypt.

Comment 6

[Wan-Teh Chang]

Comment on attachment 8423638 [details] [diff] [review]
Expose CKM_RSA_PKCS_OAEP as a single-shot RSA encrypt/decrypt function

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

r=wtc.

Comment 7

[Ryan Sleevi]

Comment on attachment 8423638 [details] [diff] [review]
Expose CKM_RSA_PKCS_OAEP as a single-shot RSA encrypt/decrypt function

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