Last Comment Bug 376417 - PK11_GenerateKeyPair needs to get the key usage from the caller.
: PK11_GenerateKeyPair needs to get the key usage from the caller.
Status: RESOLVED FIXED
See comment 8
:
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: trunk
: All All
: P2 normal (vote)
: 3.12
Assigned To: Robert Relyea
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-04-03 15:57 PDT by Robert Relyea
Modified: 2008-07-01 15:00 PDT (History)
0 users
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Implement a version of PK11_GenerateKeyPair that allows us to pass key usage (7.64 KB, patch)
2007-04-03 16:33 PDT, Robert Relyea
nelson: review+
Details | Diff | Review

Description Robert Relyea 2007-04-03 15:57:00 PDT
PK11_GenerateKeyPair cannot automatically determine the key usage based on the key type. In some cases tokens only support a single key usage for a given key. In order to support those tokens, PK11_GenerateKeyPair needs to be able to determine that key usage.

This problem is most accute for ECC keys, though it is, in theory, possible for this problem to manifest itself in RSA keys as well.
Comment 1 Robert Relyea 2007-04-03 16:22:03 PDT
In addition to getting the key usage, PK11_GenerateKeyPair also examines the capabilities of the token to deside key usage. For RSA, it's a single mechanism (even though it can be used for Signing, Encryption, etc.). For ECC there are separate mechnisms for signing and encryption. This needs to be taken into account as well.
Comment 2 Robert Relyea 2007-04-03 16:33:06 PDT
Created attachment 260525 [details] [diff] [review]
Implement a version of PK11_GenerateKeyPair that allows us to pass key usage

This patch fixes both issues identified above.
It also fixes a bug where we are currently only setting the Derive key usage. This works with softoken today because softoken doesn't store the key usages and simply defaults them, so softoken always has both derive and sign for private ECC keys no matter how the key was generated.

The most important part of this patch is the API for the new function. This proposed API allows the application to force some key usage values while defaulting on others. This is done by a mask, since PKCS #11 already specifies
a Flag to attribute mapping (otherwise I would have used the same scheme we used for the attribute flags (sensitive, token, etc) where we had one bit that meant
on and one bit that meant off.

Another tact would be to add an actual keyusage parameter (instead of PKCS #11 flags) and do the mapping inside the function. In this case we would probably make the current function a private API (since we still need to 'default' the key usage) and call it from the new public API which takes a key usage. We could also include helper function which takes a key usage and kicks out PKCS #11 flags (and optionally a mask).
Comment 3 Robert Relyea 2007-04-03 16:41:17 PDT
Comment on attachment 260525 [details] [diff] [review]
Implement a version of PK11_GenerateKeyPair that allows us to pass key usage

setting the patch flag
Comment 4 Nelson Bolyard (seldom reads bugmail) 2007-04-20 17:48:40 PDT
Comment on attachment 260525 [details] [diff] [review]
Implement a version of PK11_GenerateKeyPair that allows us to pass key usage

r=nelson.  I have some suggested changes.

>+     * opFlags give the actual setting while opFlagMask specifies if
>+     * opFlags specifies the value, or wether we should take the
>+     * default value. */

I read that sentence many times, and still don't understand what it said.
After reading the code, I decided "default value" meant the value set by
the PKCS#11 module.  (also, whether is misspelled.)

When I looked at the code, I decided that:
  opFlags specifies the bits that are to be set to 1.  
  opFlagsMask specifies the bits that are to be reset to 0.  
  When a bit is 1 in both opFlags and opFlagsMask, it is set to 1.  
I suggest you just say that.

>+    /* if we are trying to turn on a flag, it better be in the mask */
>+    PORT_Assert ((opFlags & ~opFlagsMask) == 0);

I see no reason for this.  Why require this?  How does it help?
If the bit is set in opFlags, then it will be set in the result, 
whether it is set in the mask or not.  

>+     * EC keys are used in multiple different types of mechanism, if we
>+     * are using dual use keys, we need to query the the second mechanism
                                                   ^^^ ^^^ double the
Paris in the the spring.
Comment 5 Robert Relyea 2007-05-09 16:09:49 PDT
 How about:

 * opFlagMask is set to one if the flag is specified in opFlags and zero if it
 *   is to take on a default value calculated by PK11_GenerateKeyPair.
 * opFlags specifies the actual value of the flag 1 or 0. Bits not corresponding
 *   to opFlagMask should be zero.

>>+    /* if we are trying to turn on a flag, it better be in the mask */
>>+    PORT_Assert ((opFlags & ~opFlagsMask) == 0);
>
> I see no reason for this. 
>

This saves a step later. The actual code without it would be:
  flags = (default_flags & ~opFlagMask) | (opFlags & opFlagMask);

These are not set/reset flags as you surmised.
Comment 6 Robert Relyea 2007-05-09 16:21:53 PDT
Checking in pk11akey.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11akey.c,v  <--  pk11akey.c
new revision: 1.17; previous revision: 1.16
done
Checking in pk11pub.h;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11pub.h,v  <--  pk11pub.h
new revision: 1.16; previous revision: 1.15
done
Comment 7 Robert Relyea 2007-08-29 16:02:22 PDT
code is checked in, marking fixed
Comment 8 Nelson Bolyard (seldom reads bugmail) 2008-07-01 12:43:51 PDT
Comment on attachment 260525 [details] [diff] [review]
Implement a version of PK11_GenerateKeyPair that allows us to pass key usage

The chief effect that this patch has had is NOT the purpose for which 
it was created.  This patch was ostensibly created to allow the caller to 
explicitly specify the key usage flags for the pair to be generated, but
NO CODE in NSS ever passes any value other than zero for those flags. 
So, the chief value of this patch was NOT that it allowed the caller to 
pass usage flags, because no NSS code does that.  

This patch also did another change that is NOT part of this bug's description
and is not stated as an objective of this bug.  It changes the way that the 
default key usage flags are computed/derived for EC keys.  You can see that
part of the patch at this URL:
https://bugzilla.mozilla.org/attachment.cgi?id=260525&action=diff#mozilla/security/nss/lib/pk11wrap/pk11akey.c_sec5

That patch has had a good positive effect on the usability of NSS with third
party ECC modules, but it didn't get checked in to NSS 3.11.x, because the 
ostensible purpose of this patch was to add a new API.  

So, we need to create a new patch for NSS 3.11.x that carries back into it
the part of this patch that changes the default behavior for EC key pair
usage flags.  Without that patch, NSS 3.11.x is not able to generate useful
ECC keys with some third party PKCS#11 modules.

I will file a separate bug to do that.
Comment 9 Robert Relyea 2008-07-01 15:00:53 PDT
So both issues were identified in the initial description.

PK11_GenerateKeyPair is primarily used by applications. Before the new API, applications had no way of specifying the key usage. The idea is now PSM can specify the usage explicitly if the user has specified it in the keygen request. (though there needs to be a separate PSM bug for this).

All other uses in NSS proper are simply wrappers for PK11_GenerateKeyPair 

Except for 
   1) pk11kea, which is used to move keys from one FIPS token to another).
   2) SSL indirectly calls these functions (through the SECKEY_CreateXXXKey functions) to generate step down and ephemeral keys.

Both of these generate ephermeral/session keys.

We probably should change these 2 to the new PK11_GenerateKeyPairWithOpFlags() function, but most keys are generated externally from the application itself.

bob

Note You need to log in before you can comment on or make changes to this bug.