Closed Bug 1473468 Opened Last year Closed 9 months ago

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

Categories

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

enhancement

Tracking

()

RESOLVED WONTFIX

People

(Reporter: arai, Assigned: arai)

References

(Blocks 1 open bug, )

Details

(Whiteboard: [domsecurity-backlog1])

Attachments

(2 files, 7 obsolete files)

(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
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.
Attached file demo for the new API (obsolete) —
here's demo which uses the new API.
Whiteboard: [domsecurity-intermittent]
Whiteboard: [domsecurity-intermittent] → [domsecurity-backlog1]
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?
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.
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.
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?
Flags: needinfo?(franziskuskiefer)
No longer blocks: 1478190
Taking the discussion offline.
Flags: needinfo?(franziskuskiefer)
Moving major about:debugging ng work into milestone 1, leaving m0 for prior bugfix work.
Implemented based on https://github.com/franziskuskiefer/webcrypto-ec
(still WIP, but at least works)
Attachment #8989903 - Attachment is obsolete: true
Attached file demo for the new API (obsolete) —
Attachment #8989904 - Attachment is obsolete: true
Summary: Implement private API to import/export EC public key with mask → Implement private API to generate and mask EC public/private keys
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.
forgot to refresh the patch
Attachment #9010812 - Attachment is obsolete: true
filed bug 1494152 for NSS part.
Depends on: 1494152
Attached file demo for the new API (obsolete) —
Attachment #9010813 - Attachment is obsolete: true
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.
Attachment #9010816 - Attachment is obsolete: true
Attachment #9012044 - Flags: feedback?(franziskuskiefer)
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?
Attachment #9012044 - Flags: feedback?(dkeeler)
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 )
Attachment #9012044 - Flags: feedback?(dkeeler) → feedback+
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.
Attachment #9012044 - Flags: feedback?(franziskuskiefer) → feedback+
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
(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.
QA Contact: ckerschb
Attached file demo for the new API
fixed style
Attachment #9012043 - Attachment is obsolete: true
moved NSS part to bug 1494152.
Attachment #9012044 - Attachment is obsolete: true
QA Contact: ckerschb
Depends on: 1498430
given we're moving to another protocol, closing.
Status: ASSIGNED → RESOLVED
Closed: 9 months ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.