1496124 - NSS does not create a corresponding public key for an imported private key.

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P1)

Description

[Robert Relyea]

When using  PK11_ImportEncryptedPrivateKeyInfo() or PK11_ImportEncryptedPrivateKeyInfoAndReturn() to import token (persistent) private keys, We also need to create the corresponding persistent public key, or NSS won't be able to match up an imported certificate with that private key when the token isn't logged in. This lack was masked because nssdbm can't store public keys specifically, so it would automatically create a public key for each private key under the covers. This was non-standard PKCS #11 semantics.

The solution is to simply create the public key if we successfully import the private key. We don't need to fail the import if the public key isn't created, because NSS can handle the case where the public key doesn't exist (as long as the token isn't marked 'publicly readable certs').

Comment 1

[Robert Relyea]

Attachment: Create a token public key when we import a token private key

I won't get to this patch for a couple of weeks, so there is no hurry on the review. I just wanted to make sure it doesn't get lost.

Comment 2

[Robert Relyea]

Attachment: Test cased to verify that we create a token public key when we import a token private key

This is the test case for the change. Same comment on timing as the previous patch.

Comment 3

[Robert Relyea]

Martin, if you want to wait for scratch test builds before you review, just let me know.

bob

Comment 4

[Daiki Ueno [:ueno]]

Comment on attachment 9014964 [details] [diff] [review]
Create a token public key when we import a token private key

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

::: lib/pk11wrap/pk11akey.c
@@ +1875,5 @@
> +        pubKey.pkcs11ID = CK_INVALID_HANDLE;
> +        rv2 = SECFailure;
> +        pubKey.arena = arena = PORT_NewArena(DER_DEFAULT_CHUNKSIZE);
> +        if (arena != NULL) {
> +            switch (keyType) {

Can't we simply use SECKEY_ConvertToPublicKey() here?

Comment 5

[Martin Thomson [:mt:]]

Comment on attachment 9014965 [details] [diff] [review]
Test cased to verify that we create a token public key when we import a token private key

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

Bob, I think that we really need to start testing this sort of thing using gtests.  They are far more flexible and easier to work with.  This could be added to our existing pk11_gtest suite, maybe using https://searchfox.org/nss/source/gtests/pk11_gtest/pk11_export_unittest.cc as a template.  I can help with adding parameters to the test function so that the same code can test DH, DSA, RSA, and ECDSA/ECDH, if you want to get just one key type working first.

::: cmd/pk11import/pk11import.c
@@ +187,5 @@
> +    "pk11import - test PK11_PrivateKeyImport()"
> +    "Options:",
> +    " -d certdir            directory containing cert database",
> +    " -k keysize            size of the rsa, dh, and dsa key to test (default 1024)",
> +    " -C ecc_curve          ecc curve (default )",

what is the default?

Comment 6

[Martin Thomson [:mt:]]

Comment on attachment 9014964 [details] [diff] [review]
Create a token public key when we import a token private key

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

Thanks for writing this one up.  It's somewhat annoying that we lose the public value for private keys.  It's something we've had to work around.

::: lib/pk11wrap/pk11akey.c
@@ +1856,5 @@
>      /* key import really did fail */
>      rv = SECFailure;
>  
>  done:
> +    if ((rv == SECSuccess) && isPerm) {

It looks like all of this needs to be extracted into a new function.  There's a lot going on here and it's hard to know what from the calling context needs to be relied on.

@@ +1873,5 @@
> +        pubKey.keyType = keyType;
> +        pubKey.pkcs11Slot = NULL;
> +        pubKey.pkcs11ID = CK_INVALID_HANDLE;
> +        rv2 = SECFailure;
> +        pubKey.arena = arena = PORT_NewArena(DER_DEFAULT_CHUNKSIZE);

You can use a PORTCheapArenaPool here to save an allocation.  I don't think that you should proceed if the arena can't be initialized though.  That way, you can just release the arena and you avoid having to destroy the public key at the end of this.  That saves relying on side-effects of SECKEY_DestroyPublicKey().

@@ +1875,5 @@
> +        pubKey.pkcs11ID = CK_INVALID_HANDLE;
> +        rv2 = SECFailure;
> +        pubKey.arena = arena = PORT_NewArena(DER_DEFAULT_CHUNKSIZE);
> +        if (arena != NULL) {
> +            switch (keyType) {

SECKEY_ConvertToPublicKey() doesn't have a public value to work from, so it won't work.  Right now, it does nothing for dhKey and dsaKey.  That said, we should probably make this a new function like SECKEY_SetPublicValue(SECKEYPrivateKey*, SECItem*).  I wouldn't mind exposing a function like that.

Comment 7

[Robert Relyea]

> It looks like all of this needs to be extracted into a new function.  There's a lot going on here and it's hard to know what from the 
> calling context needs to be relied on.

That sounds reasonable. It will actually make error handling easier.

> You can use a PORTCheapArenaPool here to save an allocation.  I don't think that you should proceed if the arena can't be initialized 
> though.  That way, you can just release the arena and you avoid having to destroy the public key at the end of this.  That saves relying 
> on side-effects of SECKEY_DestroyPublicKey().

I agree we can probably ditch if we can't get the arena. Is PORTCheapArenaPool compatible with PLArenaPool? If not we can't use it. The destroy cleans up more than just the arena. It also cleans up any slot references we need as well. I don't want to replicated the logic that SECKEY_DestroyPublicKey() uses here to get the slot destruction right.

> That said, we should probably make this a new function like SECKEY_SetPublicValue(SECKEYPrivateKey*, SECItem*).  
> I wouldn't mind exposing a function like that.

That's doable.

Comment 8

[Martin Thomson [:mt:]]

(In reply to Robert Relyea from comment #7)
> I agree we can probably ditch if we can't get the arena. Is
> PORTCheapArenaPool compatible with PLArenaPool? 

Completely.  The only real change is where the allocation is made for the object itself.  It otherwise follows all the same logic as a PLArenaPool.

Comment 9

[Robert Relyea]

Just to be clear: So PR_FreeArenaPool frees it?

Comment 10

[Robert Relyea]

OK, I've verified with the code that PORTCheapArenaPools will work with PORT_FreeArenaPool, so it's OK to allocate one and then have it freed by SECKEY_DestropyPublicKey().

Comment 11

[Robert Relyea]

Attachment: import_patch_v2.patch

https://treeherder.mozilla.org/#/jobs?repo=nss-try&revision=02e65397a4157e0188cd9f2d975c3d05b25fea09

Comment 12

[Robert Relyea]

It turns out it's NOT ok to pass PORTCheapArenePools to PORT_FreeArenaPool. Since I depend on SECKEY_DestroyPublicKey for proper clean up, I can't use the CheapArenaPools.

Comment 13

[Martin Thomson [:mt:]]

Comment on attachment 9024151 [details] [diff] [review]
import_patch_v2.patch

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

I'd really like to review this, but it's big enough that it will require something better than splinter.  I will put it up on phabricator if that's still a challenge (the login problems should be fixed by now, so that shouldn't be an impediment).

It's not going to happen this week, but I'd like to see if pk11import could be changed into a gtest.  That should be far more efficient.  Before doing that, is this something that might behave differently in FIPS mode? Could any of the options we test in dbtests affect the results similarly?

Comment 14

[Robert Relyea]

> I will put it up on phabricator if that's still a challenge (the login problems should be fixed by now, so that shouldn't be an impediment).

login is still a challenge. I got a phabricator login set up once, but it clobbered my access to bugzilla. I can't live without bugzilla access to I dropped phabricator. I can read phabricator stuff, but I can't respond or edit. I'm OK with you importing it, but I need to respond to comments in the bug.

> but I'd like to see if pk11import could be changed into a gtest. That should be far more efficient.

I really hate gtest, so I'm very much not motivated to deal with it. It's not at all efficient for me (gtest is more tan 50% of my nss build time, and I find gtest failures are much harder to run down than), so I actually prefer to keep this out of gtest.

> Before doing that, is this something that might behave differently in FIPS mode?

It's possible, but unlikely. We are dealing with private key import, so we already need to be a authenticated. The function is importing by unwrap, so there's no bare key FIPS issues.

> Could any of the options we test in dbtests affect the results similarly?

We need to test both sql and dbm. The former is where the issue shows up, and dbm to make sure we don't have regression. The test case I've included automatically tests both options (the advantage of the normal NSS test cases). If you run the tests on a non-patched nss sql will fail and dbm will succeed.

Comment 15

[Martin Thomson [:mt:]]

Attachment: pk11import

Comment 16

[Martin Thomson [:mt:]]

Bob,

I took a look at converting this to a gtest.  I missed the fact that you have a bunch of code for DH that can't be used, which is a shame, but also confusing (my patch has the same problem because I only realized late in the process).

Of more concern is that X25519 keys don't work with your changes.  It says:
Assertion failure: pubKey->u.ec.ecParams.name != ECCurve25519, at ../../lib/softoken/pkcs11.c:1818

The stack is:

#2  0x00007ffff7d9cef2 in PR_Assert (s=0x7ffff7878ef8 "pubKey->u.ec.ecParams.name != ECCurve25519",
    file=0x7ffff7878ed2 "../../lib/softoken/pkcs11.c", ln=1818) at ../../../../pr/src/io/prlog.c:553
#3  0x00007ffff7847c2f in sftk_GetPubKey (object=0x555555768850, key_type=3, crvp=0x7fffffffdc58)
    at ../../lib/softoken/pkcs11.c:1818
#4  0x00007ffff784620d in sftk_handlePublicKeyObject (session=0x555555737460, object=0x555555768850, key_type=3)
    at ../../lib/softoken/pkcs11.c:979
#5  0x00007ffff7846fa7 in sftk_handleKeyObject (session=0x555555737460, object=0x555555768850)
    at ../../lib/softoken/pkcs11.c:1403
#6  0x00007ffff78477ef in sftk_handleObject (object=0x555555768850, session=0x555555737460) at ../../lib/softoken/pkcs11.c:1668
#7  0x00007ffff784d5b2 in NSC_CreateObject (hSession=16777227, pTemplate=0x7fffffffde80, ulCount=8, phObject=0x7fffffffde50)
    at ../../lib/softoken/pkcs11.c:4313
#8  0x00007ffff7f626e2 in PK11_CreateNewObject (slot=0x55555572be60, session=0, theTemplate=0x7fffffffde80, count=8, token=1,
    objectID=0x7fffffffde50) at ../../lib/pk11wrap/pk11obj.c:404
#9  0x00007ffff7f3fdcd in PK11_ImportPublicKey (slot=0x55555572be60, pubKey=0x7fffffffdfd0, isToken=1)
    at ../../lib/pk11wrap/pk11akey.c:232
#10 0x00007ffff7f4354e in SECKEY_SetPublicValue (privKey=0x555555762c80, publicValue=0x555555739a90)
    at ../../lib/pk11wrap/pk11akey.c:1745
#11 0x00007ffff7f43a21 in PK11_ImportEncryptedPrivateKeyInfoAndReturnKey (slot=0x55555572be60, epki=0x555555768060,
    pwitem=0x5555557394c0, nickname=0x7fffffffe2b0, publicValue=0x555555739a90, isPerm=1, isPrivate=1, keyType=ecKey,
    keyUsage=0, privk=0x7fffffffe280, wincx=0x0) at ../../lib/pk11wrap/pk11akey.c:1922

I forget what process we use for populating the 'name' parameter here.

Comment 17

[Robert Relyea]

> I missed the fact that you have a bunch of code for DH that can't be used, which is a shame, 
> but also confusing (my patch has the same problem because I only realized late in the process).

I wanted to be able to handle dh once we get around to making dh unwrap work properly.


> Of more concern is that X25519 keys don't work with your changes.  It says:
> Assertion failure: pubKey->u.ec.ecParams.name != ECCurve25519, at ../../lib/softoken/pkcs11.c:1818

So the assert is tripping because ECCurve25519 has restricted public key values (the EC_POINT shouldn't be der encoded).
It looks like a bug in PK11_ImportPublicKey(). It always encoded the public key before we import it (unless NSS_USE_DECODED_CKA_EC_POINT) environment variable is set.

This can be fixed in the following ways: 
1) remove the assert. It's not needed. At this point we are dealing with an internally generated NSS key value anyway. The Encoding restriction really is any transport encoding of the key (certificate, spki data, etc.). Even if we don't remove it, we probably should just turn it into an error. Softoken shouldn't actually assert on bad data, it should return CKR_INVALID_TEMPLATE or similiar returns.
2) Fix PK11_ImportPublicKey to not encode the public key unconditionally for Curve25519.
3) Remove the encoding of the public value altogether. Softoken can handle either (except for Curve25519, but only because of the assert). I'll have to look at the PKCS #11 spec on which it should be. We should always use the case that the spec says at this point.

bob

Comment 18

[Robert Relyea]

If we want to do 2)  we could use pk11_get_EC_PointLenInBytes() to see if plain is set to true before we decode.

bob

Comment 19

[Martin Thomson [:mt:]]

Comment on attachment 9024151 [details] [diff] [review]
import_patch_v2.patch

Clearing the r? until we have a plan for 25519.  It seems like option 2 is most plausible, but I don't have the chops to do that part.

Comment 20

[Robert Relyea]

Attachment: import_patch_v3.patch

I've verified that the PKCS #11 spec says the params should be der encoded, so I've updated softoken to accept der encoded params for curve 22519 rather than change NSS to create non-der encoded params for importing into PKCS #11 modules.

Comment 21

[Robert Relyea]

I've also updated my tests to test for curve 25519.

Comment 22

[Martin Thomson [:mt:]]

Comment on attachment 9030057 [details] [diff] [review]
import_patch_v3.patch

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

Just so that I understand the change for 25519: we are encoding the key using the DER SubjectPublicKeyInfo format, and claiming that it is a compressed public key.  Is that right?

::: lib/pk11wrap/pk11akey.c
@@ +1690,5 @@
> +    pubKey.keyType = privKey->keyType;
> +    pubKey.pkcs11Slot = NULL;
> +    pubKey.pkcs11ID = CK_INVALID_HANDLE;
> +    /* can't use PORT_InitCheapArena here becase SECKEY_DestroyPublic is used
> +      * to free it, and it uses PORT_FreeArena which not only frees the 

Check formatting.

::: lib/softoken/pkcs11.c
@@ +1816,5 @@
>                  }
>  
> +                /* The PKCS #11 spec says that the Params should be DER encoded. Even though the params from the
> +                 * Certificate aren't according the the ECCurve 25519 spec. We should accept this encoding.
> +                PORT_Assert(pubKey->u.ec.ecParams.name != ECCurve25519); */

Just remove the code.  It's confusing.  The comment below is fine.

@@ +1833,5 @@
> +                        crv = CKR_ATTRIBUTE_VALUE_INVALID;
> +                        break;
> +                    }
> +                    /* we don't handle compressed points except in the case of ECCurve25519 */
> +                    if ((pubKey->u.ec.ecParams.fieldID.type != ec_field_plain) && (publicValue.data[0] != EC_POINT_FORM_UNCOMPRESSED)) {

Two lines.

Comment 23

[Martin Thomson [:mt:]]

Hi Bob,

This seems like the right sort of fix, but I did a search for the use of ec_field_plain and it seems like we use that to mean something different than what your code implies.  See https://searchfox.org/nss/rev/3b6a2dd92c56b13f15ad3d0bf2a0e451b898612f/lib/softoken/pkcs11c.c#5123

I quickly checked and generating a new key results in an object with a CKA_EC_POINT value set to the raw 32-byte value.  Importing the same key results in an object with a 32-bit DER-encoded value.  The code mentioned probably needs to be amended.  Though I don't know whether the definition of ec_field_plain/ECFieldType is such that we can't change its semantics.  I think that's OK.

I implemented a check for the value staying constant and that shows something worrying.  It's not just 25519 that changes, but all the curves do the same.  All EC points include an extra value after import, which seems to indicate the length of the public value.  For instance, 25519 is prefixed with 0x0420 and P-521 with 0x048185 (though my understanding of BER suggests that it should have been 0x048105).  Do you know what is going on here?  The public values for RSA and DSA are stable.


Separate topic: gtests.

I know that you don't like using these, but I think that this is a good case for gtests.  I'm able to run a single test here very quickly, even though the compilation step is a little slow.  Using the scripts, I've been running:

./build.sh --test
GTESTS=pk11_gtest GTESTFILTER=Pk11KeyImportTestEC/Pk11KeyImportTestEC.GenerateExportImport/3 ./tests/gtests/gtests.sh

Rebuilding takes 2 seconds this way, and running that one test takes 0.25 seconds.  The ability to target a single test case is a real asset here.  I have a script that lets me run the suite directly, which I'm happy to share if you think that would help with things like debugging, but you can just run the gtest executable directly as long as you have LD_LIBRARY_PATH set.

I've a patch that works (aside from the above CKA_EC_POINT stability test) with gtests.  I'll update the phabricator diff.

Comment 24

[Robert Relyea]

It looks like there is a lot of inconsistency with CKA_EC_POINT, not just in curve 25519. We should create another bug to clean that all up. We are inconsistent on whether or not u.ec.publicValue == CKA_EC_POINT or has a der encoded versus decoded value. 

I'm OK with adding the gtest, as long as the other test I wrote can go in as well.

bob

Comment 25

[Martin Thomson [:mt:]]

Took me a little while to get around to this. Thanks to JC for checking a few extra things.

https://hg.mozilla.org/projects/nss/rev/fdd7568b1edb5e86f740566538341497f82e56e8

Comment 26

[Martin Thomson [:mt:]]

Comment on attachment 9030057 [details] [diff] [review]
import_patch_v3.patch

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

Clearing r?