Closed Bug 1666891 Opened 2 years ago Closed 2 years ago

Support key wrap/unwrap with RSA-OAEP

Categories

(NSS :: Libraries, enhancement, P1)

enhancement

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: alexander.m.scheel, Assigned: alexander.m.scheel)

Details

Attachments

(1 file, 3 obsolete files)

User Agent: Mozilla/5.0 (X11; Fedora; Linux x86_64; rv:80.0) Gecko/20100101 Firefox/80.0

Steps to reproduce:

Attempted to use PK11_PubWrapSymKey with type = CKM_RSA_PKCS_OAEP.
Also failed to use PK11_PubUnwrapSymKey with OAEP as this lacks a way to indicate mechanism type.

Actual results:

When attempting to use PK11_PubWrapSymKey with type = CKM_RSA_PKCS_OAEP, PK11_PubWrapSymKey succeeds but silently (!) switches to the CKM_RSA_PKCS mechanism. Partly this is because PK11_PubWrapSymKey lacks a way to specify mechanism parameters. PK11_PubUnwrapSymKey behaves similarly, but it also lacks a way to specify the exact mechanism (again, defaulting to CKM_RSA_PKCS).

Expected results:

In consultation with Bob Relyea, I'm proposing three patches:

  1. Causes PK11_PubWrapSymKey to return an error when type != CKM_RSA_PKCS and a RSA key is passed in.
  2. Introduces PK11_PubWrapSymKeyWithMechanism, which also accepts a mechanism parameter value to pass down into C_Wrap.
  3. Introduces PK11_PubUnwrapSymKeyWithMechanism, which also accepts a type and a mechanism parameter value to pass down into C_Unwrap.

IMO, silently switching mechanisms on the caller is a bug.


The motivation for this work is in conjunction with an HSM partner which has added OAEP support ahead of NIST's deprecation of PKCS#1 v1.5 in ~2023. We currently use PKCS key wrapping via JSS and would like to introduce OAEP support for people interested in using it instead. This bug blocks adoption in JSS; see also https://github.com/dogtagpki/jss/pull/624.

In consultation with Bob Relyea, I'm proposing three patches:

Causes PK11_PubWrapSymKey to return an error when type != CKM_RSA_PKCS and a RSA key is passed in.
Introduces PK11_PubWrapSymKeyWithMechanism, which also accepts a mechanism parameter value to pass down into C_Wrap.
Introduces PK11_PubUnwrapSymKeyWithMechanism, which also accepts a type and a mechanism parameter value to pass down into C_Unwrap.

IMO, silently switching mechanisms on the caller is a bug.

In consultation with Bob Relyea, I'm proposing three patches:

  1. Causes PK11_PubWrapSymKey to return an error when type != CKM_RSA_PKCS and a RSA key is passed in.
  2. Introduces PK11_PubWrapSymKeyWithMechanism, which also accepts a mechanism parameter value to pass down into C_Wrap.
  3. Introduces PK11_PubUnwrapSymKeyWithMechanism, which also accepts a type and a mechanism parameter value to pass down into C_Unwrap.

IMO, silently switching mechanisms on the caller is a bug.

Attachment #9177470 - Attachment description: Support key wrap/unwrap with RSA-OAEP → Support key wrap/unwrap with RSA-OAEP (1/3)

In consultation with Bob Relyea, I'm proposing three patches:

  1. Causes PK11_PubWrapSymKey to return an error when type != CKM_RSA_PKCS and a RSA key is passed in.
  2. Introduces PK11_PubWrapSymKeyWithMechanism, which also accepts a mechanism parameter value to pass down into C_Wrap.
  3. Introduces PK11_PubUnwrapSymKeyWithMechanism, which also accepts a type and a mechanism parameter value to pass down into C_Unwrap.

IMO, silently switching mechanisms on the caller is a bug.

Assignee: nobody → alexander.m.scheel
Status: UNCONFIRMED → NEW
Type: defect → enhancement
Ever confirmed: true
Priority: -- → P1

I think that Bob is arguing for the passed-in mechanism to be ignored in favour of the mechanism implied by the key. I think that I agree with Alexander that switching mechanisms is a bug and we should not do that. If we're told to use ROT13 we should use ROT13 (assuming that the key supports that). To that end, the patch looks mostly fine, with one question.

What I don't understand is how these patches do that. We have two mechanisms: CKM_RSA_PKCS_OAEP and CKM_RSA_PKCS, both of which are (potentially) supported by an RSA key. pk11_mapWrapKeyType(KeyType keyType) only takes a KeyType, which means that it can't distinguish these two types. So I don't know how you could provide a single key that could work for both mechanisms.

If you could demonstrate that this is OK, that would be good. I would consider test cases sufficient evidence. r- until then.

p.s., I don't know how these patches were uploaded to phabricator, but something clearly went wrong in the process.

Sorry Martin :-) I uh... Don't get along with hg and moz-phab always seems to make completely new revisions for me whenever I make minor changes in my NSS checkout. I've also never gotten it to work with git from https://github.com/nss-dev/nss, despite support being advertised on the user's page... https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html

This time I ended up clicking the "new diff" button on the website and manually uploading three .diffs and linking the revisions manually. I've kept the tree here: https://github.com/cipherboy/nss/pull/5 Once we get final comments in, I can resubmit via moz-phab if there's some magic missing from submitting via the web UI... :)

Figured I'd rather take more time getting community feedback and revising the patches and less time fighting with tools.

(If Web UI submission is broken, should it be disabled for rNSS...?)


Back to the matter at hand...

The trick is that pk11_mapWrapKeyType will only get called from the old PK11_PubWrapSymKey -- we do not call it in the new PK11_PubWrapSymKeyWithMechanism. In the second patch we make this explicit -- PK11_PubWrapSymKey is implemented via PK11_PubWrapSymKeyWithMechanism, but we explicitly check the inferred mech type before calling down.

We could discuss letting the caller pass in type=CKM_INVALID_MECHANISM and having it not err. I'd be fine with that.

But the point is, regardless of key type, PK11_PubWrapSymKey will never work with OAEP. Why? PKCS#11 v2.40 (as I read it) requires OEAP to have a non-empty parameter set and NSS enforces that! So without a way of providing mechanism parameters in PK11_PubWrapSymKey, even if pk11_mapWrapKeyType could claim to detect some magical difference between PKCS#1 v1.5 and OAEP at the key level (it can't), it doesn't matter -- it'll just fail if it tries to do OAEP!

So, PK11_PubWrapSymKeyWithMechanism gives the caller the tools to construct the PKCS#11 mechanism object as they see fit (passing mech type and params) and doesn't second-guess them with a call to pk11_mapWrapKeyType.

(Perhaps, you missed that there are three separate revisions here; I couldn't figure out how to do three layered diffs on top of a single Phabricator "Revision"... The first revision merely adds the check to the existing code (fixing the "bug") -- the second implements WrapWithMechanism(...) and the third implements UnwrapWithMechanism(...). It is in the second that the "magic" to enable OAEP happens).

I agree that PK11_PubWrapSymKey will never work with OAEP. The mechanism parameter is effectively a noop, and NSS ABI guarantees means we need to keep it as a noop. Ideally PK11_PubWrapSymKey would not have the mechanism parameter, but that's not where we are.

We do have flexibility of with PK11_PubWrapSymKeyWithMechanism(), so we need to make sure it has reasonable semantics.

RE martin's question about OAEP versus PKCS. They use the same keys, so you can't use keytype to decide the mechanism type.

Have you tried moz-phab? (pip3 install --user MozPhab) I've only used it with a mercurial checkout, so I don't know if this works.

I looked at all three patches, but without context and spread across three pages, it's nigh impossible for me to internalize everything. I don't have Bob's familiarity with this code. For something of this size, a single patch would have been better.

I think that I now understand Bob's objection to the change in behaviour with the existing API. It's unfortunate, but we do have to let people provide the same garbage inputs they might have been providing before.

I look forward to seeing some tests that demonstrate this working.

Attachment #9177473 - Attachment is obsolete: true
Attachment #9177472 - Attachment is obsolete: true
Attachment #9177470 - Attachment is obsolete: true

This is useful for RSA-OAEP support.

The CKM_RSA_PKCS_OAEP mechanism requires a CK_RSA_PKCS_OAEP_PARAMS
be present for PKCS#11 calls. This provides required context for OAEP.
However, PK11_PubWrapSymKey lacks a way of providing this context and
historically silently converted CKM_RSA_PKCS_OAEP to CKM_RSA_PKCS when
a RSA key is provided. Introducing a new call will let us indicate
parameters and potentially support other mechanisms in the future.
This call mirrors the earlier calls introduced for RSA-PSS:
PK11_SignWithMechanism and PK11_VerifyWithMechanism.

The CKM_RSA_PKCS_OAEP mechanism requires a CK_RSA_PKCS_OAEP_PARAMS
be present for PKCS#11 calls. This provides required context for OAEP.
However, PK11_PubUnwrapSymKey lacks a way of providing this context,
and additionally lacked a way of indicating which mechanism type to use
for the unwrap operation (instead detecting it by key type). Introducing
a new call will let us indicate parameters and potentially support other
mechanisms in the future.

Signed-off-by: Alexander Scheel <ascheel@redhat.com>

I tried Moz-Phab last time, like my last comment said. Lots was broken with git support; the new version seems to work better.

I've abandoned the last revisions and submitted a new one.

  • A
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.59
You need to log in before you can comment on or make changes to this bug.