1340103 - Introduction of SECKEYECPublicKey.encoding in NSS 3.28 broke ABI.

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Kai Engert (:KaiE:)]

Another ABI compatibility issue might have been introduced in NSS 3.28.

SECKEYECPublicKey was extended, and attribute ECPointEncoding encoding
was added at the end of the structure.

SECKEYECPublicKey is embedded as part of a union in SECKEYPublicKey.

According to Bob, some applications create SECKEYPublicKey objects,
and pass it to NSS.

When NSS processes SECKEYPublicKey, how does it know if the
  SECKEYECPublicKey.encoding
attribute has been set, or contains garbage (because an old application uses it,
that didn't know about that new attribute) ?

The risk is that NSS might behave incorrectly, when it consumes externally
produced public key data.

Bob suggested that NSS must check, whether or not its safe to access
SECKEYECPublicKey.encoding, prior to using it.

How can NSS find out, if that's safe or not?

The new attribute "encoding" is an enum, with two values,
ECPoint_Uncompressed and ECPoint_XOnly.

I've looked at all the places where NSS uses these values.

Both seckey_SetPointEncoding and ssl_ImportECDHKeyShare set the encoding
based on other information, which seems safe.

The only remaining place is pk11_ECPubKeySize, which reads the encoding
attribute to draw conclusions.

That function is used from one context, only, by pk11_PubDeriveECKeyWithKDF.

Is it possible that pk11_PubDeriveECKeyWithKDF may operate on externally
imported public key data?

If yes, how do we ensure that accessing the encoding attribute is safe?

Bob made the general suggestion (prior to knowing about this place of use),
that before looking at this attribute, we should look at the first attribute
of SECKEYECPublicKey, which is SECKEYECParams DEREncodedParams.

IIUC, Bob said, SECKEYECParams DEREncodedParams contains the OID of the curve,
is this correct?

He suggested, only if that parameter shows it's curve 25519 or newer,
only then look at the new "encoding" attribute. Does everyone agree?

How can we do that? What strategy can we use, to decide that the given curve
OID points to one, that is either curve 25519, or was added to NSS afterwards?

Once we know how to do that, all places that access SECKEYECPublicKey.encoding,
must go through that decision, and only if the curve is "25519 or added later",
only then it's safe to access the "encoding" attribute, and
if it's not, then the code must assume that encoding is the classic value
ECPoint_Uncompressed.

Is this summary correct?

Comment 1

[Kai Engert (:KaiE:)]

Attachment: 1340103-v1.patch

It seems that attribute
  ECPointEncoding encoding
is redundanct, because the information can be derived from the initial member of SECKEYECPublicKey.

This patch removes all mention of the new attribute, and changes the only place where that attribute was ever read.

Comment 2

[Kai Engert (:KaiE:)]

My earlier patch had a small cleanup glitch.

In addition, with trunk, the following test failed. Thank you for having created that test.

// Importing a public key in SPKI format must fail when id-ecPublicKey is
// given (so we know it's an EC key) but the namedCurve parameter is missing.
TEST_F(Pkcs11EcdsaSha256Test, ImportSpkiNoAlgorithmParams) {
  EXPECT_FALSE(ImportPublicKey(kP256SpkiNoAlgorithmParams,
                               sizeof(kP256SpkiNoAlgorithmParams)));
}

Comment 3

[Kai Engert (:KaiE:)]

Attachment: 1340103-v3-trunk.patch

Comment 4

[Kai Engert (:KaiE:)]

Comment on attachment 8838093 [details] [diff] [review]
1340103-v3-trunk.patch

Franziskus, what do you think?

The patch passes tests:
  https://treeherder.mozilla.org/#/jobs?repo=nss-try&revision=33a8e9d74674f54ec97446e56fb82e65b6f1d847

If Bob's statement is correct, that applications could try to pass SECKEYECPublicKey* into NSS, then it would be tricky to ensure, that it has been set, prior to reading it.

Because the information stored in the "encoding" attribute seems redundant, I'm trying to remove it, and whenever we read it, derive it from the curve OID.

I don't know if any application is already reading the "encoding" attribute. Do we think it's necessary to set it anyway, even if NSS itself never reads it? (For applications based on NSS 3.28 or later, that already read the attribute.) Maybe it would be safer, but a hassle to maintain the attribute going forward (if we never read it, and only write it).

If any application already attempts to set the attribute, we can simply ignore that, since we can derive it from the curve OID. I've replaced the attribute with an anonymous enum, to ensure we at least reserve the space for that attribute, to ensure that space never gets reused for something else.

Comment 5

[Kai Engert (:KaiE:)]

(In reply to Kai Engert (:kaie) from comment #4)
> 
> I don't know if any application is already reading the "encoding" attribute.
> Do we think it's necessary to set it anyway, even if NSS itself never reads
> it? (For applications based on NSS 3.28 or later, that already read the
> attribute.)

Alternatively, we could keep the old enum values, but never use them.

We could add a third enum value, ECPoint_undefined, and always set the encoding attribute to undefined. That way, applications that already attempt to read it, can notice that they aren't getting the expected values any more.

Comment 6

[Kai Engert (:KaiE:)]

Attachment: 1340103-v4-trunk.patch

Comment 7

[Robert Relyea]

Comment on attachment 8838122 [details] [diff] [review]
1340103-v4-trunk.patch

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

::: lib/pk11wrap/pk11skey.c
@@ +2048,2 @@
>  
> +    /* Is it a curve that is known to use a special encoding? */

This test should be in it's own function. The issue is there will be more curves with this encoding. The function should return the encoding based on the curve type.

The function wout look like the old seckey_SetPointEncoding function you removed in this patch, but instead return the encoding type.

Comment 8

[Robert Relyea]

The rest looks fine. I think this change was made in response to a review I did on franziskus's code where I wanted to isolate the number of places that new about curve 25519 explicitly, knowing we may be adding other berstein curves in the future.

bob

Comment 9

[Kai Engert (:KaiE:)]

Attachment: delta-from-v4-to-v5.patch

just FYI, this is the delta change that should implement Bob's request.

I'll attach the new full patch next.

Comment 10

[Kai Engert (:KaiE:)]

Attachment: 1340103-v5-trunk.patch

Comment 11

[Kai Engert (:KaiE:)]

I realize, in addition to splitting, you've also asked me to reuse the old implementation strategy for the split function. I'll have that in a few minutes.

Comment 12

[Kai Engert (:KaiE:)]

Attachment: 1340103-v6-trunk.patch

Comment 13

[Kai Engert (:KaiE:)]

try run with patch v6:
https://treeherder.mozilla.org/#/jobs?repo=nss-try&revision=673256e13f770fd42d65979c333a5f8656d8778b

Comment 14

[Martin Thomson [:mt:]]

Comment on attachment 8838204 [details] [diff] [review]
1340103-v6-trunk.patch

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

::: lib/cryptohi/seckey.c
@@ +1180,5 @@
>                                    &pubk->u.ec.DEREncodedParams);
>              if (rv != SECSuccess) {
>                  break;
>              }
> +            copyk->u.ec.encoding = ECPoint_Undefined;

You should probably call seckey_HasCurveOID() here as well.  It will at least ensure that copied public keys are correct.

@@ +1262,5 @@
>                                      CKA_EC_POINT, arena, &pubk->u.ec.publicValue);
>              if (rv != SECSuccess || pubk->u.ec.publicValue.len == 0) {
>                  break;
>              }
> +            pubk->u.ec.encoding = ECPoint_Undefined;

This one is safe, on the assumption that the OID is correctly set on the private key.

::: lib/pk11wrap/pk11skey.c
@@ +2045,5 @@
> +    SECItem oid;
> +    SECStatus rv;
> +    ECPointEncoding encoding = ECPoint_Undefined;
> +
> +    PLArenaPool *arena = PORT_NewArena(2048);

You can use a cheap arena here and save a heap allocation: 
https://searchfox.org/nss/rev/ffa508ee08f7b04bdfb9e73408fe5d5902198c11/lib/util/secport.h#102-104

@@ +2060,5 @@
> +            case SEC_OID_CURVE25519:
> +                encoding = ECPoint_XOnly;
> +                break;
> +            case SEC_OID_SECG_EC_SECP256R1:
> +            /* fall through */

The fall through comments aren't necessary if you don't have any statements after the label.

::: lib/util/eccutil.h
@@ +4,5 @@
>  
>  #ifndef _FREEBL_H_
>  #define _FREEBL_H_
>  
>  /* point encoding type */

Did you want to mark this as deprecated here too?

Comment 15

[Robert Relyea]

Comment on attachment 8838204 [details] [diff] [review]
1340103-v6-trunk.patch

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

I'm also ok with you incorporating any of martin's suggested fixes, including the one I commented on.

::: lib/cryptohi/seckey.c
@@ +1180,5 @@
>                                    &pubk->u.ec.DEREncodedParams);
>              if (rv != SECSuccess) {
>                  break;
>              }
> +            copyk->u.ec.encoding = ECPoint_Undefined;

Martin's suggestion is good sanity, though it's hard to see how copy could fail without an error code. 

It would prevent copying a deficient key (one in which the params were not encoded correctly. 

One thing about using the HasCurveOID is it allocates extra memory in the arena that we never use, so it's probably a good idea not to overuse it. It's not a leak since it only happens once per key and it's tied to the key's arena. 

Probably best to keep it as is.

Comment 16

[Kai Engert (:KaiE:)]

Bob, Martin, very helpful feedback, thanks a lot.

I think we can address Bob's worry about allocation overhead by using a local PORTCheapArenaPool inside seckey_HasCurveOID.

Comment 17

[Kai Engert (:KaiE:)]

Attachment: delta-v6-to-v7.patch

Delta patch that addresses the change requests.

Comment 18

[Kai Engert (:KaiE:)]

Attachment: 1340103-v7-trunk.patch

Updated patch v7, carrying forward r=rrelyea

Comment 19

[Kai Engert (:KaiE:)]

Attachment: 1340103-v8-trunk.patch

minor adjustment after self review, in SECKEY_CopyPublicKey, I've moved seckey_HasCurveOID to happen earlier. If the source is broken, we avoid copying.

Comment 20

[Kai Engert (:KaiE:)]

fixed for 3.30
https://hg.mozilla.org/projects/nss/rev/608b71f014fa

Comment 21

[Kai Engert (:KaiE:)]

Backported to 3.29 branch for 3.29.1:
https://hg.mozilla.org/projects/nss/rev/98031786f9cc

Backported to 3.28 branch for 3.28.3:
https://hg.mozilla.org/projects/nss/rev/0050234a859c

Only difference was the change to SECKEY_ConvertToPublicKey, which was omitted, because that function doesn't handle EC keys on the older branches.