1473468 - Implement private API to generate and mask EC public/private keys

Status: RESOLVED WONTFIX

(Core :: DOM: Security, enhancement, P3)

Description

[Tooru Fujisawa [:arai]]

(derived from bug 1470352)

in order to implement WiFi authentication with tSOKE protocol [1], we need a private WebCrypto API which performs the following two operations:

  Y* = Y + (h mod n) G'
  Y = Y* - (h mod n) G'

where Y is EC public key, h is a hash of session info, G' is a generator and n = <G'> = <G> where G is the generator of P-256.
this operation masks the public key Y which is used for ECDH key exchange.

the API I propose is the following:

interface SubtleCrypto {
   ...
  [Throws]
  Promise<any> importRawKeyWithMask(object keyData, BufferSource mask,
                                    AlgorithmIdentifier algorithm,
                                    boolean extractable,
                                    sequence<KeyUsage> keyUsages);

  [Throws]
  Promise<any> exportRawKeyWithMask(CryptoKey key, BufferSource mask);
  ...
}

exportRawKeyWithMask performs (Y* = Y + (h mod n) G') and exports Y* as raw public key.
importRawKeyWithMask performs (Y = Y* - (h mod n) G') and imports Y as raw public key.
except masking operation, they perform the same operation as importKey/exportKey with "raw" format.

[1] https://link.springer.com/article/10.1007/s10207-016-0348-7

Comment 1

[Tooru Fujisawa [:arai]]

Attachment: Implement private API to import/export EC public key with mask.

here's the WIP patch based on m-c 987ea0d6a000.

This implements the basic calculation part.
I'll post the demo HTML which uses the new API.

currently G' has the same parameter as G.
I'll look for the parameter which matches the requirement.

Comment 2

[Tooru Fujisawa [:arai]]

Attachment: demo for the new API

here's demo which uses the new API.

Comment 3

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8989903 [details] [diff] [review]
Implement private API to import/export EC public key with mask.

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

Eventually we'll need two separate patches for this (one for NSS and one for Firefox), but this is fine for toying around.
I only added a few comments but they are pretty basic and should change most of the NSS code in your patch.
You can have a look at how this was done for J-PAKE (bug 609076).

::: security/nss/lib/freebl/blapi.h
@@ +469,5 @@
>                                        SECItem *publicValue);
>  
> +
> +/* TODOarai: document */
> +extern SECStatus EC_Mask(ECParams *params,

Not sure if this is the right interface. You probably don't want to hard-code the other base point G' but pass it in as an argument as this might be different for different protocols.

::: security/nss/lib/pk11wrap/pk11pub.h
@@ +880,5 @@
> + **********************************************************************/
> +
> +/* TODOarai: document */
> +SECStatus
> +PK11_EC_Mask(PK11SlotInfo *slot,

We probably shouldn't create a new function here but use a new mechanism for PK11_GenKey or something.

::: security/nss/lib/pk11wrap/pk11util.c
@@ +3,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  /*
>   * Initialize the PCKS 11 subsystem
>   */
> +#include "blapi.h"

Unfortunately you can't include that here. Do you need it?

Comment 4

[Tooru Fujisawa [:arai]]

Thank you for your feedback :D

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #3)
> Comment on attachment 8989903 [details] [diff] [review]
> Implement private API to import/export EC public key with mask.
> 
> Review of attachment 8989903 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Eventually we'll need two separate patches for this (one for NSS and one for
> Firefox), but this is fine for toying around.

Yes, once the patch gets ready, I'll split it into two.


> I only added a few comments but they are pretty basic and should change most
> of the NSS code in your patch.
> You can have a look at how this was done for J-PAKE (bug 609076).

Okay, I'll modify this patch based on the patches there.


> ::: security/nss/lib/freebl/blapi.h
> @@ +469,5 @@
> >                                        SECItem *publicValue);
> >  
> > +
> > +/* TODOarai: document */
> > +extern SECStatus EC_Mask(ECParams *params,
> 
> Not sure if this is the right interface. You probably don't want to
> hard-code the other base point G' but pass it in as an argument as this
> might be different for different protocols.

What level of abstraction do you think we should provide for each layer?

to my understanding, in WebCrypto API level, we should hide most things behind, like the actual calculation and the parameters, except algorithm name (of course it's private API tho...),
but I'm not sure for other layers (pk11, freebl, etc).
the current abstraction in the patch is:
  WebCrypto:
    * Export public key + mask operation with given mask
    * Import public key + mask operation with given mask
  NSS (all layers):
    * Export public key (existing)
    * Import public key (existing)
    * Mask operation with given mask

The other levels I can think of are:
    * Mask operation with given base point and mask
or
    * Raw public key calculation with given private key and base point
    * Adding/subtracting 2 public keys
maybe each layer need different abstraction level, but I'm not sure how much detail is suitable for other protocols.

Can I have your opinion?


> ::: security/nss/lib/pk11wrap/pk11pub.h
> @@ +880,5 @@
> > + **********************************************************************/
> > +
> > +/* TODOarai: document */
> > +SECStatus
> > +PK11_EC_Mask(PK11SlotInfo *slot,
> 
> We probably shouldn't create a new function here but use a new mechanism for
> PK11_GenKey or something.

Yes, I'll look for some nice interface for PK11.


> ::: security/nss/lib/pk11wrap/pk11util.c
> @@ +3,5 @@
> >   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> >  /*
> >   * Initialize the PCKS 11 subsystem
> >   */
> > +#include "blapi.h"
> 
> Unfortunately you can't include that here. Do you need it?

Looks like I forgot to remove it.


Also, looks like I broke the patch while refactoring before submitting, and also forgot to perform "mod n" operation.
I'll fix those parts as well.

Comment 5

[Franziskus Kiefer [:franziskus]]

Sorry for the delay.

> What level of abstraction do you think we should provide for each layer?

This should probably be similar to the existing (EC)DH interface. We really only need a change in the `deriveKey` API. So this could either be a single step with `deriveKey(publicKey, privateKey, G', x)` where x is the mask value and G' the second generator, or we can use the existing `deriveKey` and add another API like `mask(Y, G', x)` that has to be called after `deriveKey`. In NSS I'd mirror that API through PK11 and then either have a corresponding freebl API or maybe expose EC_AddPoints and EC_MulPoints separately.

Comment 6

[Tooru Fujisawa [:arai]]

I'm having hard time figuring out how deriveKey fits into the usage.
it takes 2 algorithms, but what are they for this case, and where to pass G' and mask?
can you tell me some more about the details?

Comment 7

[Franziskus Kiefer [:franziskus]]

Taking the discussion offline.

Comment 8

[Tooru Fujisawa [:arai]]

here's the spec repository for the API
https://github.com/franziskuskiefer/webcrypto-ec

Comment 9

[:Harald Kirschner :digitarald]

Moving major about:debugging ng work into milestone 1, leaving m0 for prior bugfix work.

Comment 10

[Tooru Fujisawa [:arai]]

Attachment: WIP: Implement private Web Cryptography EC API.

Implemented based on https://github.com/franziskuskiefer/webcrypto-ec
(still WIP, but at least works)

Comment 11

[Tooru Fujisawa [:arai]]

Attachment: demo for the new API

Comment 12

[Tooru Fujisawa [:arai]]

here's the current API callstack

now SubtleCrypto.getPrivateKey uses mostly pre-existing path for NSS.
SubtleCrypto.{mul,add,sub} uses dedicated/separate APIs instead of pre-existing path, since I cannot find any other suitable path.

SubtleCrypto.getPrivateKey (WebIDL)
  SubtleCrypto::GetPrivateKey
    WebCryptoTask::CreateGetPrivateKeyTask
      GetPrivateKeyTask::DoCrypto
        PK11_GenerateKeyPair (NSS)
          PK11_GenerateKeyPairWithFlags
            PK11_GenerateKeyPairWithOpFlags
              CK_FUNCTION_LIST.C_GenerateKeyPair
                FC_GenerateKeyPair
                  NSC_GenerateKeyPair
                    EC_NewKeyFromSeed (ec)

SubtleCrypto.mul (WebIDL)
  SubtleCrypto::Mul
    WebCryptoTask::CreateMulTask
      MulTask::DoCrypto
      CryptoKey::ECMul
        PK11_EC_Mul (NSS)
          CK_FUNCTION_LIST.C_EC_Mul
            FC_EC_Mul
              NSC_EC_Mul
                EC_Mul (ec)

SubtleCrypto.{add,sub} (WebIDL)
  SubtleCrypto::{Add,Sub}
    WebCryptoTask::{CreateAddTask,CreateSubTask}
      AddSubTask::DoCrypto
        CryptoKey::ECAddSubb
          {PK11_EC_Add,PK11_EC_Sub} (NSS)
            CK_FUNCTION_LIST.{C_EC_Add,C_EC_Sub}
              {FC_EC_Add,FC_EC_Sub}
                {NSC_EC_Add,NSC_EC_Sub}
                  {EC_Add,EC_Sub} (ec)

the above patch does not perform `h mod mask` operation, but it has NSS API.
I need to put it somewhere that can read the generator's order.

Comment 13

[Tooru Fujisawa [:arai]]

Attachment: WIP: Implement private Web Cryptography EC API.

forgot to refresh the patch

Comment 14

[Tooru Fujisawa [:arai]]

updated to the latest spec
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5479f2a20f19523a42256c51c70281e91464e21f

Comment 15

[Tooru Fujisawa [:arai]]

filed bug 1494152 for NSS part.

Comment 16

[Tooru Fujisawa [:arai]]

Attachment: demo for the new API

Comment 17

[Tooru Fujisawa [:arai]]

Attachment: WIP: Implement private Web Cryptography EC API.

Implemented based on the latest spec.

Can I have your feedback on the NSS APIs?
if they're fine, I'll move NSS part to bug 1494152.

Comment 18

[Tooru Fujisawa [:arai]]

Comment on attachment 9012044 [details] [diff] [review]
WIP: Implement private Web Cryptography EC API.

:keeler, can I have feedback on the WebCrypto implementation side (especially WebCryptoTask.cpp and CryptoKey.cpp) of this?

also, I have question.
ECGetPrivateKeyTask allocates several short-lived SECItems.
which is it better to use UniquePLArenaPool and UniqueSECItem?

Comment 19

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 9012044 [details] [diff] [review]
WIP: Implement private Web Cryptography EC API.

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

(In reply to Tooru Fujisawa [:arai] from comment #18)
> Comment on attachment 9012044 [details] [diff] [review]
> :keeler, can I have feedback on the WebCrypto implementation side
> (especially WebCryptoTask.cpp and CryptoKey.cpp) of this?

The webcrypto side generally looks like the right sort of thing.

> also, I have question.
> ECGetPrivateKeyTask allocates several short-lived SECItems.
> which is it better to use UniquePLArenaPool and UniqueSECItem?

If you have to make many SECItems in a row, it will be more efficient to use a single UniquePLArenaPool, but I imagine runtimes will be dominated by the EC operations here, so it probably won't matter one way or another. I would write it the way that makes the implementation most clear.

::: dom/base/moz.build
@@ +376,5 @@
>      'StructuredCloneBlob.cpp',
>      'StructuredCloneHolder.cpp',
>      'StyleSheetList.cpp',
>      'SubtleCrypto.cpp',
> +    'SubtleCryptoEC.cpp',

These files don't seem to be included in this patch - are they supposed to be?

::: dom/crypto/CryptoKey.cpp
@@ +1275,5 @@
> +nsresult
> +CryptoKey::ECAddSub(AddSubMode aMode,
> +                    const nsString& aNamedCurve,
> +                    SECKEYPublicKey* aY,
> +                    SECKEYPublicKey* aM,

Consider making these parameters e.g. `UniqueSECKEYPublicKey& aM`, etc. rather than passing around bare pointers, so the ownership model is a bit more clear.

::: dom/webidl/moz.build
@@ +775,5 @@
>      'StreamFilterDataEvent.webidl',
>      'StyleSheet.webidl',
>      'StyleSheetList.webidl',
>      'SubtleCrypto.webidl',
> +    'SubtleCryptoEC.webidl',

Again I don't see this file.

::: security/nss/lib/freebl/blapi.h
@@ +463,1 @@
>  

Any changes to files in security/nss will have to be done separately and shouldn't be in this patch.

::: security/nss/lib/util/pkcs11f.h
@@ +812,5 @@
>  #endif
> +
> +/* C_EC_GetOrder stores the required length of the SECItem to *length if result
> + * is null.  Otherwise, stores the order of the generator into result. */
> +CK_PKCS11_FUNCTION_INFO(C_EC_GetOrder)

I think changing the PKCS#11 API is going to be more trouble than it's worth (the API is defined by a technical committee: https://www.oasis-open.org/committees/pkcs11/charter.php )

Comment 20

[Franziskus Kiefer [:franziskus]]

Comment on attachment 9012044 [details] [diff] [review]
WIP: Implement private Web Cryptography EC API.

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

Generally looks good. I think it would be best to get the freebl changes in first, that gives us some time to decide how to handle the PK11 layer.

::: dom/crypto/WebCryptoTask.cpp
@@ +3213,5 @@
> +
> +  // Step 9.
> +  size_t orderLen = 0;
> +  nsresult rv;
> +  rv = MapSECStatus(PK11_EC_GetOrder(slot.get(), curveParams, nullptr, &orderLen));

I'd probably implement all of `getPrivateKey` in NSS and not get the order out first.

::: security/nss/lib/freebl/ec.c
@@ +1162,5 @@
> +        *length = params->order.len;
> +        return SECSuccess;
> +    }
> +
> +    // FIXMEarai: Should they be an assertion?

Asserting is always a good idea for these things.

@@ +1167,5 @@
> +    if (result->len != *length || !result->data) {
> +        return SECFailure;
> +    }
> +
> +    if (!(arena = PORT_NewArena(NSS_FREEBL_DEFAULT_CHUNKSIZE)))

braces

::: security/nss/lib/util/pkcs11f.h
@@ +812,5 @@
>  #endif
> +
> +/* C_EC_GetOrder stores the required length of the SECItem to *length if result
> + * is null.  Otherwise, stores the order of the generator into result. */
> +CK_PKCS11_FUNCTION_INFO(C_EC_GetOrder)

Well, we already a lot of custom PK11 stuff and this is the only way to do this if not everything is implemented in NSS (which would be fine as well). But vendor-specific code should go into pkcs11n.h.

Comment 21

[Tooru Fujisawa [:arai]]

Thank you for feedback :D

(In reply to Dana Keeler [:keeler] (she/her) (use needinfo) from comment #19)
> ::: dom/base/moz.build
> @@ +376,5 @@
> >      'StructuredCloneBlob.cpp',
> >      'StructuredCloneHolder.cpp',
> >      'StyleSheetList.cpp',
> >      'SubtleCrypto.cpp',
> > +    'SubtleCryptoEC.cpp',
> 
> These files don't seem to be included in this patch - are they supposed to
> be?

oops, forgot to add them.
will post again after addressing comments.

(In reply to Franziskus Kiefer [:franziskus] from comment #20)
> ::: dom/crypto/WebCryptoTask.cpp
> @@ +3213,5 @@
> > +
> > +  // Step 9.
> > +  size_t orderLen = 0;
> > +  nsresult rv;
> > +  rv = MapSECStatus(PK11_EC_GetOrder(slot.get(), curveParams, nullptr, &orderLen));
> 
> I'd probably implement all of `getPrivateKey` in NSS and not get the order
> out first.

okay, I'll put order and mod operations into ec.c

Comment 22

[Tooru Fujisawa [:arai]]

(In reply to Franziskus Kiefer [:franziskus] from comment #20)
> ::: security/nss/lib/util/pkcs11f.h
> @@ +812,5 @@
> >  #endif
> > +
> > +/* C_EC_GetOrder stores the required length of the SECItem to *length if result
> > + * is null.  Otherwise, stores the order of the generator into result. */
> > +CK_PKCS11_FUNCTION_INFO(C_EC_GetOrder)
> 
> Well, we already a lot of custom PK11 stuff and this is the only way to do
> this if not everything is implemented in NSS (which would be fine as well).
> But vendor-specific code should go into pkcs11n.h.

I'm not sure how I can define them in pkcs11n.h.
pkcs11n.h seems to be used in different way than pkcs11f.h, and just moving the definition there doesn't work.

Comment 23

[Tooru Fujisawa [:arai]]

Attachment: demo for the new API

fixed style

Comment 24

[Tooru Fujisawa [:arai]]

Attachment: Implement private Web Cryptography EC API.

moved NSS part to bug 1494152.

Comment 25

[Tooru Fujisawa [:arai]]

given we're moving to another protocol, closing.