865789 - (web-crypto) Implement W3C Web Crypto API

Status: NEW

(Core :: DOM: Web Crypto, enhancement)

Description

[David Dahl :ddahl]

Implement Web Crypto API: https://dvcs.w3.org/hg/webcrypto-api/raw-file/tip/spec/Overview.html

The spec is still evolving (April 2013), the plan is for LC in October, 2013

Comment 1

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Hi, ddahl:
Is there any bug for KeyStorage, KeyManager, ..etc to manager those keys/certificates?
Like SubtleCrypto in WebCrypto.

Also do we plan to support importing PKCS 12 on Firefox OS?
The KeyFormat defined in the spec doesn't contain pkcs12. 

Thanks

Comment 2

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #1)
> Hi, ddahl:
> Is there any bug for KeyStorage, KeyManager, ..etc to manager those
> keys/certificates? 
Also like Bug 746476.

Comment 3

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #1)
> 
> Also do we plan to support importing PKCS 12 on Firefox OS?
> The KeyFormat defined in the spec doesn't contain pkcs12. 
I was wrong in comment 1,  the "spki" in KeyFormat can fit for pkcs12.

Comment 4

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Hi, ddahl
Do you think I can start working on some API first?
On B2G we'll need a feature to import PKCS12 (for Wifi EAP), 
I've checked WebCrypto and KeyDiscovery API, I think I can start implementing  Crypto.importKey and CryptoKeys.getKeysByName.(will file another bug for Key Discovery if needed).

Thanks

Comment 5

[David Dahl :ddahl]

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #4)
> Hi, ddahl
> Do you think I can start working on some API first?

One issue is that the entire API will be changing to a "Promises" based API. The spec is not updated yet. 

> On B2G we'll need a feature to import PKCS12 (for Wifi EAP), 
> I've checked WebCrypto and KeyDiscovery API, I think I can start
> implementing  Crypto.importKey and CryptoKeys.getKeysByName.(will file
> another bug for Key Discovery if needed).

Are you familiar with NSS? If you begin working on implementation, and you want to keep conformance with the W3C spec, your API will change later. Mianly, it will not implement EventTarget and will implement "Promises" (referred to as "Futures" here: http://dom.spec.whatwg.org/#futures )

Comment 6

[David Dahl :ddahl]

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #4)
> Hi, ddahl
> Do you think I can start working on some API first?

Also, this bug is more of a Meta Bug, you should file a bug for both methods you want to work on and make them block this one.

Yoshi:

Are these APIs a major blocker for FxOS feature work? If so, what features are dependent on this?

bsmith & mhamrick:

Perhaps we should begin figuring out what all of the internal IDL/key storage looks like for these APIs.

Comment 7

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to David Dahl :ddahl from comment #5) 
> One issue is that the entire API will be changing to a "Promises" based API.
> The spec is not updated yet. 
> 
> > On B2G we'll need a feature to import PKCS12 (for Wifi EAP), 
> > I've checked WebCrypto and KeyDiscovery API, I think I can start
> > implementing  Crypto.importKey and CryptoKeys.getKeysByName.(will file
> > another bug for Key Discovery if needed).
> 
> Are you familiar with NSS? If you begin working on implementation, and you
> want to keep conformance with the W3C spec, your API will change later.
> Mianly, it will not implement EventTarget and will implement "Promises"
> (referred to as "Futures" here: http://dom.spec.whatwg.org/#futures )

Thanks for this information.
I am trying to understand NSS recently.
I know the W3C process is going to take a while and will change very often.
But do you think we could start some implementations of WebCrypto first in Gecko?
(As I know, Blink has filed the bug for WebCrypto recently)
If we should start implement WebCrypto, 
I'll file bugs and still keep this as a meta bug.

Or there's another Bug 877535, which is talking about KeyChain API for FirefoxOS,
(but that bug might also involve credential storage like password)
do you think I'd start some WebAPI on that bug(Bug 877535) first and so we don't have to wait for WebCrypt is ready?

Currently the use case on FirefoxOS is for Enterprise Wifi (and later for VPN), which needs a PKCS12 certificate and pass the name of the PKCS12 to WifiManager(DOM) . So the requirement here I think is Crypto.importKey and CryptoKeys.getKeysByName. It doesn't need those encrypt/decrypt ...etc functionalities. And these features are planed in FirefoxOS 1.2, which might be around September 2013, (might not be correct but this is the time I've been told)

Thanks

Comment 8

[David Dahl :ddahl]

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #7)

> Thanks for this information.
> I am trying to understand NSS recently.
> I know the W3C process is going to take a while and will change very often.
> But do you think we could start some implementations of WebCrypto first in
> Gecko?
I don't think you should wait for a spec to begin adding features you need for FxOS. If you want to be in conformance when the actual spec is final, you will have to update your API.

> (As I know, Blink has filed the bug for WebCrypto recently)
> If we should start implement WebCrypto, 
> I'll file bugs and still keep this as a meta bug.
> 
> Or there's another Bug 877535, which is talking about KeyChain API for
> FirefoxOS,
> (but that bug might also involve credential storage like password)
> do you think I'd start some WebAPI on that bug(Bug 877535) first and so we
> don't have to wait for WebCrypt is ready?
> 
Please don't wait for a spec:) You should file bugs for what you actually need, with the idea that perhaps it might converge with Web Crypto later.

> Currently the use case on FirefoxOS is for Enterprise Wifi (and later for
> VPN), which needs a PKCS12 certificate and pass the name of the PKCS12 to
> WifiManager(DOM) . So the requirement here I think is Crypto.importKey and
> CryptoKeys.getKeysByName. It doesn't need those encrypt/decrypt ...etc
> functionalities. And these features are planed in FirefoxOS 1.2, which might
> be around September 2013, (might not be correct but this is the time I've
> been told)
> 
Sure, only work on the things you need. I've been told by sicking that we can land experimental crypto APIs right into window.crypto without prefixing since all of this stuff is new - especially on mobile release targets.

Comment 9

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Great, thanks for the tips, David.

I'll file some bugs for the implementation of importKey and getKeysByName, and make them depend on this bug(web-crypto)

Also, since WebCrypto and KeyDiscovery already deals with the keys, I'll goto Bug 877535 to ask pauljt about the if the 'KeyChain API' is still needed.

Finally, currently nsIDOMCrypt is still in XPIDL, I'll try to see if I can convert it to WebIDL and extend it with functions we need on FirefoxOS.

Comment 10

[David Dahl :ddahl]

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #9)

> Finally, currently nsIDOMCrypt is still in XPIDL, I'll try to see if I can
> convert it to WebIDL and extend it with functions we need on FirefoxOS.

Please don't work from any older designs/IDL that might be on the mozilla wiki. If you want to work from the spec in progress, use the w3c spec https://dvcs.w3.org/hg/webcrypto-api/raw-file/tip/spec/Overview.html

Comment 11

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Add dependency on Bug 875289(rename Future to Promise), as I found Bug 856410 is still using the older naming 'Future'.

Comment 12

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to David Dahl :ddahl from comment #10)
> (In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #9)
> 
> > Finally, currently nsIDOMCrypt is still in XPIDL, I'll try to see if I can
> > convert it to WebIDL and extend it with functions we need on FirefoxOS.
> 
> Please don't work from any older designs/IDL that might be on the mozilla
> wiki. If you want to work from the spec in progress, use the w3c spec
> https://dvcs.w3.org/hg/webcrypto-api/raw-file/tip/spec/Overview.html

Filed Bug 883741 to move crypto to WebIDL.

Comment 13

[Mountie Lee]

question1) where the generated key will be stored?
requirement1) secure deletion for generated keys when needed (7 times overwrite ...)

Comment 14

[Richard Barnes [:rbarnes]]

Attachment: webcrypto-moz-wip-20140311.patch

This is an initial WIP WebCrypto patch for feedback.  It implements the following aspects of the API:
* digest() implementing SHA1 and SHA2 algorithms
* importKey() and exportKey() with the "raw" format
* Keys are clonable (so you can use them with IndexedDB and postMessage), but do not store anything but key material (so no extractable, algorithm, usages)
* encrypt() implementing AES-CBC

The idea of this WIP is to capture most of the essential aspects of the DOM interaction, as a prelude to implementing the rest of the API.

Comment 15

[Richard Barnes [:rbarnes]]

Attachment: webcrypto-moz-wip-20140320.patch

Updated WIP patch.  As of this version, the following features should basically work:

* Encrypt  : AES-CBC, AES-CTR, AES-GCM, RSAES-PKCS1-v1_5
* Decrypt  : AES-CBC, AES-CTR, AES-GCM, RSAES-PKCS1-v1_5
* Sign     : HMAC-SHA*, RSASSA-PKCS1-v1_5
* Verify   : HMAC-SHA*, RSASSA-PKCS1-v1_5
* Digest   : SHA-1, SHA-224, SHA-256, SHA-384, SHA-512
* Import   : raw (AES/HMAC), spki (for RSA)
* Export   : raw (AES/HMAC), spki (for RSA)
* Generate : symmetric (AES/HMAC), RSA

Unfortunately, there's one regression here: Keys are no longer structured clonable, so they can't be stored in IndexedDB or passed via PostMessage.  This should be fixed soon.

WARNING:
* I have not validated output against external test vectors, just done simple encrypt/decrypt or sign/verify round-trip testing, and checking of output sizes.
* This is still very rough code.  Crashes are not uncommon, largely due to really poor memory management

Comment 16

[Anders Rundgren]

I believe I have the trunk code (very n00b firefox programmer...) but got this when applied the patch:

patching file dom/bindings/Codegen.py
Reversed (or previously applied) patch detected!  Assume -R? [n]
Apply anyway? [n] yes
Hunk #1 FAILED at 10647.
1 out of 1 hunk FAILED -- saving rejects to file dom/bindings/Codegen.py.rej

I restored the original and will continue testing with this setup.  Wrong solution?

Comment 17

[Anders Rundgren]

13:21.16 nsDOMWindowUtils.cpp
13:27.95 Unified_cpp_dom_base1.cpp
13:27.95 c:\mozilla-source\mozilla-central\dom\base\CryptoTasks.h(170) : error C2059: syntax error : '{'
13:27.95 c:\mozilla-source\mozilla-central\dom\base\CryptoTasks.h(170) : error C2143: syntax error : missing ';' before '{'
13:27.95 c:\mozilla-source\mozilla-central\dom\base\CryptoTasks.h(170) : error C2143: syntax error : missing ';' before '}'
13:27.95 Native command 'mozbuild.action.cl main' returned value '2'

Yes, I'm on windows...

Comment 18

[Anders Rundgren]

I have now made a successful run on Ubuntu instead.  Yay!

Bug found in https://mobilepki.org/jcs/webcrypto makes multiple "Sign Sample Data" go wrong

Missing parameter in: http://webpki.org/demo/mozbug1.html causes Firefox to crash.  In addition, extraneous parameter data doesn't get rejected.

Anyway, it is getting better :-)

Comment 19

[Richard Barnes [:rbarnes]]

Attachment: webcrypto-moz-wip-20140407.patch

Updated WIP patch.  Same capabilities as last patch, plus:
* PKCS#8 import/export (for RSA private keys)
* JWK import/export (for symmetric keys and RSA public+private keys)
* Keys are once again structured clonable
* Initial test suite using externally-generated test vectors
* Refactored to be more readable
* Slightly better memory management

WARNING:
* This is still very rough code.  Crashes are not uncommon, largely due to really poor memory management
* For example, have not fixed the bugs pointed out in Comment 16 

bz, keeler: Feedback appreciated, especially regarding DOM/WebIDL usage and NSS usage (respectively).

Comment 20

[Richard Barnes [:rbarnes]]

Attachment: webcrypto-moz-wip-20140408.patch

Quick fix to a bug that caused HMAC to break.  Should now pass all tests.

Comment 21

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

Comment on attachment 8403250 [details] [diff] [review]
webcrypto-moz-wip-20140408.patch

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

Wow - you've implemented quite a lot here. In fact, the patch is rather large - is there a way to break it up into smaller sub-parts? Also, in general, watch out for trailing whitespace, and I think a few of the files added by this patch need some sort of license boilerplate: http://www.mozilla.org/MPL/headers/ (although I'm not sure it's mandatory for things like test-vectors.js, etc.)
Another concern I have is how does this interact with e10s/sandboxing? Ideally, no NSS calls would happen in content processes. Rather than implement this once without e10s and then again with e10s, I think it would be best to implement it in a way that's compatible with e10s from the beginning. I think this means implementing the bindings so they remote to the parent process if they're in a content process.
Your use of NSS functions otherwise seems reasonable. There's a possibility we may have to deal with NSS shutdown, but it doesn't look like you're calling functions that would require that (maybe some of the PK11 ones need it). Be sure to use scoped types wrapped around NSS types whenever resources are allocated (which I think you mostly do).

::: dom/crypto/WebCryptoCommon.h
@@ +42,5 @@
> +namespace mozilla {
> +namespace dom {
> +
> +// Polyfill around NSS's lack of a generic PK11SymKey copy 
> +PK11SymKey* PK11_CopySymKey(PK11SymKey *aKey, CK_MECHANISM_TYPE aMechanism, 

I wouldn't call this "PK11_CopySymKey" unless you already have an NSS bug on file to add it. Maybe just CopySymKey? (To differentiate it from PK11_ functions that actually are in NSS.)

@@ +48,5 @@
> +{
> +  nsresult rv = MapSECStatus(PK11_ExtractKeyValue(aKey));
> +  if (NS_FAILED(rv)) { return nullptr; }
> +
> +  SECItem* keyItem = PK11_GetKeyData(aKey);

use a ScopedSECItem here

::: dom/crypto/WebCryptoTask.cpp
@@ +184,5 @@
> +  }
> +};
> +
> +
> +class AesCtrTask : public ReturnArrayBufferViewTask

A lot of these tasks seem fairly similar in their implementations - can you refactor the common code out?

::: dom/webidl/SubtleCrypto.webidl
@@ +19,5 @@
> +};
> +
> +[NoInterfaceObject]
> +interface AesKeyAlgorithm : KeyAlgorithm {
> +  readonly attribute unsigned short length; 

nit: trailing space

@@ +122,5 @@
> +  Promise decrypt(AlgorithmIdentifier algorithm, Key key, CryptoOperationData data);
> +  Promise sign(AlgorithmIdentifier algorithm, Key key, CryptoOperationData data);
> +  Promise verify(AlgorithmIdentifier algorithm, Key key, CryptoOperationData signature, CryptoOperationData data);
> +  Promise digest(AlgorithmIdentifier algorithm, CryptoOperationData data);
> +  

nit: trailing spaces

@@ +126,5 @@
> +  
> +  Promise importKey(KeyFormat format, KeyData keyData, AlgorithmIdentifier algorithm, boolean extractable, sequence<KeyUsage> keyUsages );
> +  Promise exportKey(KeyFormat format, Key key);
> +  Promise generateKey(AlgorithmIdentifier algorithm, boolean extractable, sequence<KeyUsage> keyUsages );
> +  

nit: trailing spaces

::: xpcom/base/ErrorList.h
@@ +487,5 @@
>    ERROR(NS_ERROR_DOM_ENCODING_NOT_UTF_ERR,         FAILURE(29)),
>    ERROR(NS_ERROR_DOM_ENCODING_DECODE_ERR,          FAILURE(30)),
>    ERROR(NS_ERROR_DOM_INVALID_POINTER_ERR,          FAILURE(31)),
> +  /* WebCrypto API errors from http://www.w3.org/TR/WebCryptoAPI/ */
> +  ERROR(NS_ERROR_DOM_UNKNOWN_ERR,                  FAILURE(31)),

Looks like you've got two FAILURE(31) errors here, which I don't think we want.

Comment 22

[Anders Rundgren]

On a plain-vanilla quick build using Ubuntu the patch generated compilation errors due to path issues.  Nightly without the patch works.

Comment 23

[Richard Barnes [:rbarnes]]

In the interest of getting things reviewed and checked in, this bug will now be used as a tracking bug, and specific parts of the implementation will be filed as separate bugs (starting with Bug 995385).

Comment 24

[Florian Bender]

Since Firefox Accounts seem to use Curve25519 (or rather Ed25519, I think I read that on PlanetMoz but can't seem to find it right now) it'd probably be beneficial to implement those as well. There's a bug where Moz could chime in to indicate vendor support and get this into the spec: https://www.w3.org/Bugs/Public/show_bug.cgi?id=25839

(Chrome also has an implementation of these curves for some component so a two-implementations state seems reachable.)

Comment 25

[:Harald Kirschner :digitarald]

Blocking 2.1 as several app partners depend on this.

Comment 26

[Sandip Kamat]

Feature-b2g = 2.1 for Tako

Comment 27

[Kai-Chih Hu [:khu]]

Looks like no progress in this bug. Are we still targeting to land this in 2.1? Can we move it to 2.2? 
None of the dependencies have feature-b2g tagged. Do we know what is planned to land in 2.1?

Comment 28

[Dietrich Ayala (:dietrich)]

ni? rbarnes for comment #27.

Comment 29

[Richard Barnes [:rbarnes]]

Tracking the state of this bug is a bad way to track the state of WebCrypto.  WebCrypto is a big API with lots of independent parts, and we've been landing them incrementally (in the bugs listed under dependencies).  If you want to know which parts are in landed which versions, you can look at the individual bugs above.  I've also been trying to summarize in this spreadsheet:

https://docs.google.com/spreadsheet/ccc?key=0AiAcidBZRLxndE9LWEs2R1oxZ0xidUVoU3FQbFFobkE&usp=sharing

With regard to B2G: You might also take a look at Bug 1010743.

Comment 30

[bhavana bajaj [:bajaj]]

NI, :dveditz for sec-review here.

Comment 31

[Kai-Chih Hu [:khu]]

(In reply to Richard Barnes [:rbarnes] from comment #29)
> Tracking the state of this bug is a bad way to track the state of WebCrypto.
> WebCrypto is a big API with lots of independent parts, and we've been
> landing them incrementally (in the bugs listed under dependencies).  If you
> want to know which parts are in landed which versions, you can look at the
> individual bugs above.  I've also been trying to summarize in this
> spreadsheet:
> 
> https://docs.google.com/spreadsheet/
> ccc?key=0AiAcidBZRLxndE9LWEs2R1oxZ0xidUVoU3FQbFFobkE&usp=sharing
> 
> With regard to B2G: You might also take a look at Bug 1010743.

Thanks for the information. So, based on this comment, we are expecting to land this in multiple versions of Firefox OS. Remove feature-b2g:2.1, because we're not targeting to 2.1 only.

Comment 32

[Daniel Veditz [:dveditz]]

Comment 25 says "several app partners depend on this" for 2.1 -- which "this"? The WebCrypto spec isn't even done yet, but maybe the partners only need some of the features that have already landed. We would need more specific requirements to be able to pin something to a particular release.

Comment 33

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to Daniel Veditz [:dveditz] from comment #32)
> Comment 25 says "several app partners depend on this" for 2.1 -- which
> "this"? The WebCrypto spec isn't even done yet, but maybe the partners only
> need some of the features that have already landed. We would need more
> specific requirements to be able to pin something to a particular release.

I believe we've had communications with the Tako partner about which pieces of the spec that we have implemented, and gotten confirmation that this covers their use cases.

Candice can confirm though I believe.

Comment 34

[Candice Serran (:cserran)]

(In reply to Jonas Sicking (:sicking) from comment #33)
> (In reply to Daniel Veditz [:dveditz] from comment #32)
> > Comment 25 says "several app partners depend on this" for 2.1 -- which
> > "this"? The WebCrypto spec isn't even done yet, but maybe the partners only
> > need some of the features that have already landed. We would need more
> > specific requirements to be able to pin something to a particular release.
> 
> I believe we've had communications with the Tako partner about which pieces
> of the spec that we have implemented, and gotten confirmation that this
> covers their use cases.
> 
> Candice can confirm though I believe.

Correct, Tako partner confirmed this was satisfactory for their use cases.

Comment 35

[Jason Smith [:jsmith]]

Feature request, which should not use the blocking flag, so removing nomination.

Comment 36

[Anne (:annevk)]

This was raised on dev.platform but the answer was not entirely clear. Is our non-TLS version of crypto sandboxed from the crypto library used for authenticated connections?

Comment 37

[adria]

Firefox returns ArrayBufferView everywhere the spec calls for an ArrayBuffer.
It's cumbersome to have this workaround in every callback:

result = new Uint32Array( buffer.buffer || buffer );

Comment 38

[Tim Taubert [:ttaubert] (inactive)]

(In reply to adria from comment #37)
> Firefox returns ArrayBufferView everywhere the spec calls for an ArrayBuffer.
> It's cumbersome to have this workaround in every callback:
> 
> result = new Uint32Array( buffer.buffer || buffer );

Bug 1044071 was fixed for Firefox 34. Which version are you testing on? What API methods return an ABV?

Comment 39

[adria]

(In reply to Tim Taubert [:ttaubert] from comment #38)

> Bug 1044071 was fixed for Firefox 34. Which version are you testing on? What
> API methods return an ABV?

Thanks, I was using 33