Closed Bug 1009794 Opened 6 years ago Closed 6 years ago

Enable OAEP support in pk11wrap

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

(2 files, 2 obsolete files)

Once Bug 1009785 is resolved, softoken will expose freebl's OAEP support.

However, callers will not be able to access that interface via the SECKEY* interfaces, because there is no functionality within the pk11wrap layer to support using C_Encrypt/C_Decrypt with a custom CKM.

Similar to the PK11_Encrypt/PK11_Decrypt functions added in Bug 854063, we should add a PK11_PubEncrypt/PK11_PrivDecrypt generic functions, which will complement the PK11_PubDecryptRaw/PK11_PubEncryptRaw and PK11_PrivDecryptPKCS1 / PK11_PubEncryptPKCS1

We could further provide OAEP-specific wrappers around this, but having functions that take a CK_MECHANISM_TYPE would be sufficient to allow NSS-using applications access to the C_Encrypt/C_Decrypt functionality.
Attached patch patch1.diff (obsolete) — Splinter Review
This changes the internal implementation of PK11_Encrypt/PK11_Decrypt to use a helper function, pk11_SingleShotEncrypt/Decrypt. The internal function takes a PK11SlotInfo and a CK_OBJECT_ID, which the public functions PK11_Encrypt/PK11_Decrypt and PK11_PubEncrypt/PK11_PubDecrypt call by accessing the PK11SymKey* / SECKEYPublicKey* / SECKEYPrivateKey* internals.
Attachment #8421994 - Flags: review?(wtc)
wtc: Updated version, after testing on the Chrome side. I missed the bits of the pk11wrap where the public key doesn't have an associated slot, and where the private key may have to authenticate due to CKA_ALWAYS_AUTHENTICATE.

Updated version, which mostly moves the pk11_PubEncryptRaw RSA-specific checks into the RSA-specific algorithms.
Attachment #8421994 - Attachment is obsolete: true
Attachment #8421994 - Flags: review?(wtc)
Attachment #8423641 - Flags: review?(wtc)
Comment on attachment 8423641 [details] [diff] [review]
Expose PK11_PubEncrypt and PK11_PubDecrypt

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

::: lib/pk11wrap/pk11obj.c
@@ +833,5 @@
>               unsigned char *out, unsigned int *outLen,
>               unsigned int maxLen,
>               const unsigned char *enc, unsigned encLen)
>  {
> +    PK11SlotInfo* slot = symKey->slot;

Will fix these two before committing. Force of key habit.

@@ +996,4 @@
>  {
>      PK11SlotInfo *slot;
>      CK_OBJECT_HANDLE id;
> +    CK_ULONG len = maxLen;

The reason for moving the type and size checks is to that this can function if softoken adds support for something like RSA-KEM - whose output size would be larger than the modulus size.

::: lib/pk11wrap/pk11pub.h
@@ +539,5 @@
> +                          unsigned int maxLen,
> +                          const unsigned char *enc, unsigned int encLen);
> +SECStatus PK11_PubEncrypt(SECKEYPublicKey *key,
> +                          CK_MECHANISM_TYPE mechanism, SECItem *param,
> +                          void* wincx,

debate about where wincx goes (last, first, in between? The NSS way!)
Comment on attachment 8423641 [details] [diff] [review]
Expose PK11_PubEncrypt and PK11_PubDecrypt

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

r=wtc.

::: lib/pk11wrap/pk11obj.c
@@ +1068,5 @@
> +}
> +
> +SECStatus
> +PK11_PubDecrypt(SECKEYPrivateKey *key,
> +                CK_MECHANISM_TYPE mechanism, SECItem *param,

Another design is to have these two functions just take a CK_MECHANISM_PTR input parameter. Does any existing NSS PK11_ function use this approach?

::: lib/pk11wrap/pk11pub.h
@@ +530,5 @@
>  SECStatus PK11_PrivDecryptPKCS1(SECKEYPrivateKey *key, unsigned char *data,
>     unsigned *outLen, unsigned int maxLen, unsigned char *enc, unsigned encLen);
>  /* The encrypt function that complements the above decrypt function. */
>  SECStatus PK11_PubEncryptPKCS1(SECKEYPublicKey *key, unsigned char *enc,
>                  unsigned char *data, unsigned dataLen, void *wincx);

Could you take the opportunity to add const to the input buffer pointers? Thanks.

@@ +532,5 @@
>  /* The encrypt function that complements the above decrypt function. */
>  SECStatus PK11_PubEncryptPKCS1(SECKEYPublicKey *key, unsigned char *enc,
>                  unsigned char *data, unsigned dataLen, void *wincx);
>  
> +SECStatus PK11_PubDecrypt(SECKEYPrivateKey *key,

This function should be named PK11_PrivDecrypt.

@@ +533,5 @@
>  SECStatus PK11_PubEncryptPKCS1(SECKEYPublicKey *key, unsigned char *enc,
>                  unsigned char *data, unsigned dataLen, void *wincx);
>  
> +SECStatus PK11_PubDecrypt(SECKEYPrivateKey *key,
> +                          CK_MECHANISM_TYPE mechanism, SECItem *param,

Nit: |param| can be a const pointer.

@@ +539,5 @@
> +                          unsigned int maxLen,
> +                          const unsigned char *enc, unsigned int encLen);
> +SECStatus PK11_PubEncrypt(SECKEYPublicKey *key,
> +                          CK_MECHANISM_TYPE mechanism, SECItem *param,
> +                          void* wincx,

wincx is usually the last parameter.

I know this wincx parameter comes from the existing PK11_PubEncryptPKCS1 function, but I am surprised that wincx is in the encryption but not the decryption function. wincx is intended to support a password dialog for a crypto token. Usually an operation using a private key needs a password.
Attachment #8423641 - Flags: review?(wtc) → review+
Comment on attachment 8423641 [details] [diff] [review]
Expose PK11_PubEncrypt and PK11_PubDecrypt

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

::: lib/pk11wrap/pk11obj.c
@@ +1068,5 @@
> +}
> +
> +SECStatus
> +PK11_PubDecrypt(SECKEYPrivateKey *key,
> +                CK_MECHANISM_TYPE mechanism, SECItem *param,

Only http://mxr.mozilla.org/nss/source/lib/pk11wrap/pk11pub.h#262

::: lib/pk11wrap/pk11pub.h
@@ +533,5 @@
>  SECStatus PK11_PubEncryptPKCS1(SECKEYPublicKey *key, unsigned char *enc,
>                  unsigned char *data, unsigned dataLen, void *wincx);
>  
> +SECStatus PK11_PubDecrypt(SECKEYPrivateKey *key,
> +                          CK_MECHANISM_TYPE mechanism, SECItem *param,

PKCS#11 allows the modification of the parameters - it's just that no (currently supplied) parameters allow it.

Example is CK_SSL3_KEY_MAT_OUT.

Declaring it non-const seems safer, as far as guarantees go.

@@ +539,5 @@
> +                          unsigned int maxLen,
> +                          const unsigned char *enc, unsigned int encLen);
> +SECStatus PK11_PubEncrypt(SECKEYPublicKey *key,
> +                          CK_MECHANISM_TYPE mechanism, SECItem *param,
> +                          void* wincx,

The private key has the wincx attached to it.

It's only the public key (which by default is not in any slot) that needs an explicit wincx passed with it, because of potentially 'hostile' tokens (those that require unlock even for public ops)
Carrying over r=wtc, just typographic fixes
Attachment #8423641 - Attachment is obsolete: true
Attachment #8424254 - Flags: review+
Blocks: 158747
Comment on attachment 8424254 [details] [diff] [review]
Updated for review comments, checked in

Checked in as https://hg.mozilla.org/projects/nss/rev/52bb12b4277c
Attachment #8424254 - Flags: checked-in+
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
OS: Windows 7 → All
Priority: -- → P1
Hardware: x86_64 → All
This is a compilation error on Windows:

cl -FoWIN954.0_OPT.OBJ/pk11obj.obj -c -O2 -W3 -nologo -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_WARNINGS -MD -we4002 -we4003 -we4004 -we4006 -we4009 -we4013 -we4015 -we4028 -we4033 -we4035 -we4045 -we4047 -we4053 -we4054 -we4063 -we4064 -we4078 -we4087 -we4090 -we4098 -we4390 -we4551 -we4553 -we4715 -DXP_PC -DSHLIB_SUFFIX=\"dll\" -DSHLIB_PREFIX=\"\" -DSHLIB_VERSION=\"3\" -DSOFTOKEN_SHLIB_VERSION=\"3\" -UDEBUG -U_DEBUG -DNDEBUG -DWIN32 -D_X86_ -D_WINDOWS -DWIN95 -DUSE_UTIL_DIRECTLY -DNO_NSPR_10_SUPPORT -DSSL_DISABLE_DEPRECATED_CIPHER_SUITE_NAMES -I../../../dist/WIN954.0_OPT.OBJ/include -I../../../dist/public/nss -I../../../dist/private/nss  "/e/slavedir/2-win-x32-OPT/hg/nss/lib/pk11wrap/pk11obj.c"
pk11obj.c
e:/slavedir/2-win-x32-OPT/hg/nss/lib/pk11wrap/pk11obj.c(961) : error C4090: 'function' : different 'const' qualifiers
e:/slavedir/2-win-x32-OPT/hg/nss/lib/pk11wrap/pk11obj.c(1028) : error C4090: 'function' : different 'const' qualifiers

On other platforms it's just a warning:

gcc -arch x86_64 -o Darwin10.8.0_64_DBG.OBJ/pk11obj.o -c -g -fPIC  -Wall -fno-common -pipe -DDARWIN -DHAVE_STRERROR -DHAVE_BSD_FLOCK  -DXP_UNIX -DSHLIB_SUFFIX=\"dylib\" -DSHLIB_PREFIX=\"lib\" -DSHLIB_VERSION=\"3\" -DSOFTOKEN_SHLIB_VERSION=\"3\" -DDEBUG -UNDEBUG -DDEBUG_buildbot -DUSE_UTIL_DIRECTLY -DNO_NSPR_10_SUPPORT -DSSL_DISABLE_DEPRECATED_CIPHER_SUITE_NAMES -I../../../dist/Darwin10.8.0_64_DBG.OBJ/include -I../../../dist/public/nss -I../../../dist/private/nss  pk11obj.c
pk11obj.c: In function 'pk11_PrivDecryptRaw':
pk11obj.c:961: warning: passing argument 2 of '((struct CK_FUNCTION_LIST *)slot->functionList)->C_Decrypt' discards qualifiers from pointer target type
pk11obj.c: In function 'pk11_PubEncryptRaw':
pk11obj.c:1028: warning: passing argument 2 of '((struct CK_FUNCTION_LIST *)slot->functionList)->C_Encrypt' discards qualifiers from pointer target type

Patch checked in: https://hg.mozilla.org/projects/nss/rev/b96d9c28e90d
Attachment #8425472 - Flags: review?(ryan.sleevi)
Attachment #8425472 - Flags: checked-in+
Attachment #8425472 - Flags: review?(ryan.sleevi) → review+
Richard: this is the bug report that added the PK11_PubEncrypt and
PK11_PrivDecrypt functions.

Patches pushed to mozilla-inbound as part of the NSS_3_16_2_BETA2 tag:
https://hg.mozilla.org/integration/mozilla-inbound/rev/980e7d78b358
You need to log in before you can comment on or make changes to this bug.