Closed Bug 1009785 Opened 6 years ago Closed 6 years ago

Enable OAEP within softoken

Categories

(NSS :: Libraries, defect, P1)

3.16.1
defect

Tracking

(Not tracked)

RESOLVED FIXED
3.16.2

People

(Reporter: ryan.sleevi, Assigned: ryan.sleevi)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
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 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.
Attachment #8421956 - Flags: review?(wtc) → review+
(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.
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.
Attachment #8421956 - Attachment is obsolete: true
Attachment #8423638 - Flags: review?(wtc)
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.
Attachment #8423638 - Flags: review?(wtc) → review+
Blocks: 158747
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
Attachment #8423638 - Flags: checked-in+
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Priority: -- → P1
You need to log in before you can comment on or make changes to this bug.