665057 - Design and implement crypto API for Mozilla ID

Status: RESOLVED DUPLICATE of bug 753238

(Core :: Security: PSM, defect)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

+++ This bug was initially created as a clone of Bug #662674 +++

I split this off of bug 662674 so that we can focus on Mozilla ID first, to avoid having the discussions of the DOMCrypt interfering/delaying Mozilla ID. The API will be specific to Mozilla ID and must be callable from ChromeWorkers--i.e. off the main thread. More details will be provided soon.

Comment 1

[David Dahl :ddahl]

I have added this bug to the VEP plan page for Firefox front-end: https://wiki.mozilla.org/Identity/EngPlan/fVEC

I have written up an IDL:

https://wiki.mozilla.org/Identity/EngPlan/fVEC#Crypto_API

Comment 2

[David Dahl :ddahl]

(In reply to comment #1)
> I have added this bug to the VEP plan page for Firefox front-end:
> https://wiki.mozilla.org/Identity/EngPlan/fVEC
> 
> I have written up an IDL:
> 
> https://wiki.mozilla.org/Identity/EngPlan/fVEC#Crypto_API

There should be a bit of a design session on this nature of this platform API - as I think the IDL I wrote up probably does not accept and return the proper types.

Comment 3

[David Dahl :ddahl]

I can start poking at this bug. It seems like the function "cryptojs_generateOneKeyPair" here: http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsCrypto.cpp#684 is a good starting model, what do you think?

Comment 4

[David Dahl :ddahl]

A couple of questions:

I was looking for the "PORT_Error" or "PORT_ERROR" function to capture any errors while generating - mxr does not find anything: http://mxr.mozilla.org/mozilla-central/search?string=Port_Error&find=&findi=&filter=%5BPp%5DORT_Error&hitlimit=&tree=mozilla-central

For now, I am folding this new function into into nsCrypto, however, the IDL is in dom/interfaces/base - Is it Ok to do that as (unreviewed) bug 673432 removes the GetCrypto getter from nsGlobalWindow.cpp - making the methods of nsIDOMCrypto not accessible unless they are added to a new nsIDOMGlobalPropertyInitializer that provides a JS wrapper object for window.crypto?

Perhaps an entirely new interface is required - I just thought these new functions would be "at home" in nsCrypto.cpp.

Comment 5

[David Dahl :ddahl]

Attachment: v 0.1 WIP untested

saving work

Comment 6

[Kaspar]

(In reply to David Dahl :ddahl from comment #4)
> I was looking for the "PORT_Error" or "PORT_ERROR" function to capture any
> errors while generating

You're probably looking for PR_GetError(). For a list of error codes, see http://www.mozilla.org/projects/security/pki/nss/ref/ssl/sslerr.html#1039257.

Comment 7

[David Dahl :ddahl]

Attachment: v 0.2 Key Generation is working

Tested and verified KeyGeneration in gdb, need to figure out the correct signature here as this is really an "internal API" to this internal API. A chrome-privileged API will be exposed as a JSM for extensions and browser front-end code to consume.

The question is: I was thinking this key generation function should return an ArrayBufferView so we can easily use this interface in JS by itself (if need be). The dumb part of the question is should there be two ArrayBuffer args, one for each key? Secondly, how do we cast the SECKeyPrivate/PublicKey to then push into the ArrayBufferView?

Btw: this API is synchronous as we will be using it inside of a ChromeWorker thread via an nsIDOMGlobalPropertyInitializer. That patch is forthcoming.

Comment 8

[David Dahl :ddahl]

(In reply to Kaspar Brand from comment #6)
> You're probably looking for PR_GetError(). For a list of error codes, see
> http://www.mozilla.org/projects/security/pki/nss/ref/ssl/sslerr.html#1039257.

Thanks. I must have mis-heard what function to use.

Comment 9

[David Dahl :ddahl]

Attachment: v 0.3 WIP adding a new class and interface to abstract the keys

Untested, saving work

Comment 10

[David Dahl :ddahl]

Attachment: v 0.4 WIP - coming together

Created nsBrowserIDService to hand out nsIBrowserIDKeypair instances when needed, started getting nsIBrowserIDKeypair class implemented

Comment 11

[David Dahl :ddahl]

Attachment: v 0.5 nsIBrowserIDKeypair implementation rough draft, untested

Still need to get this building correctly and tests written. Looking for some feedback on the approach.

Comment 12

[David Dahl :ddahl]

Notes from 10/06/2011 meeting:

Add expiration to the keypair
Generate the identifier internally as a uuid or something 
Change the name of the interface from nsIBrowserID* to nsIKeyStore* (BrowserID and VerifiedEmailProtocol are not good names for this interface as per ben adida)

also see bug 692500 about adding DSA support and algorithm argument to the keygen method

Comment 13

[David Dahl :ddahl]

Attachment: v 0.6 Saving my work

Comment 14

[David Dahl :ddahl]

Brian:

Did you make any headway on the feedback for this patch? I started working on the testing for this via xpcshell, (just to get a headstart).

Comment 15

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 565648 [details] [diff] [review]
v 0.6 Saving my work

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

See also how other PSM interface implementations are declared in nsNSSModule.cpp, using NS_NSS_GENERIC_FACTORY_CONSTRUCTOR and other XPCOM-isms.

::: dom/interfaces/base/nsIDOMCrypto.idl
@@ +52,5 @@
>    DOMString                 popChallengeResponse(in DOMString challenge);
>    DOMString                 random(in long numBytes);
>    [implicit_jscontext]
>    void                      getRandomValues(in jsval aData);
> +  void                      generateBrowserIDKeypair(in jsval aKeyPair);

nsIBrowserIDKeyPair generateBrowserIDKeyPair();

::: security/manager/ssl/public/nsIBrowserIDKeypair.idl
@@ +44,5 @@
> +
> +[scriptable, uuid(73962dc7-8ee7-4346-a12b-b039e1d9b54d)]
> +interface nsIBrowserIDKeypair : nsISupports
> +{
> +  void getSerializedRSAPublicKey(in ACString aOutString);

remove this

@@ +46,5 @@
> +interface nsIBrowserIDKeypair : nsISupports
> +{
> +  void getSerializedRSAPublicKey(in ACString aOutString);
> +  
> +  void getRSAPublicKeyExponent(inout octet aOutval);

// returns a Base64 URL encoded string representation of the RSA public key exponent.
readonly attribute AUTF8String encodedRSAPublicKeyExponent;

@@ +48,5 @@
> +  void getSerializedRSAPublicKey(in ACString aOutString);
> +  
> +  void getRSAPublicKeyExponent(inout octet aOutval);
> +  
> +  void getRSAPublicKeyModulus(inout AString aOutString);

// returns a Base64 URL encoded string representation of the RSA public key modulus.
readonly attribute AUTF8String encodedRSAPublicKeyModulus;

@@ +50,5 @@
> +  void getRSAPublicKeyExponent(inout octet aOutval);
> +  
> +  void getRSAPublicKeyModulus(inout AString aOutString);
> +  
> +  void getOrigin(inout nsIURI aOutOrigin);

Remove this. Origin and identity should be handled outside of PSM in the BrowserID component.

@@ +53,5 @@
> +  
> +  void getOrigin(inout nsIURI aOutOrigin);
> +
> +  // Typically, this is an email address
> +  void getIdentifier(inout AString aOutIdentifier); 

Remove this. Origin and identity should be handled outside of PSM in the BrowserID component.

@@ +55,5 @@
> +
> +  // Typically, this is an email address
> +  void getIdentifier(inout AString aOutIdentifier); 
> +  
> +  void sign(in AString aText, inout AString aOutSignature);

// Returns a base64 URL encoded PKCS#1 signature of aText.
AUTF8String sign(in AUTF8String aText);

::: security/manager/ssl/public/nsIBrowserIDService.idl
@@ +44,5 @@
> + * until shutdown.
> + */
> +
> +[scriptable, uuid(7335490a-ead7-4b4f-a22c-80a5cb3b2aa0)]
> +interface nsIBrowserIDService : nsISupports

This service should live outside of PSM, in the BrowserID component.

::: security/manager/ssl/src/nsCrypto.cpp
@@ +2552,5 @@
> +    // The service will keep a reference to each keypair by Origin [nsIURI] 
> +    // and Identity [email address]
> +    nsCOMPtr<nsIBrowserIDService> bidSvc(do_GetService(NS_BROWSERIDSERVICE_CONTRACTID));
> +    if (bidSvc) {
> +      bidSvc->registerKeypair(browserIDKeypair);

This component shouldn't have any dependencies on the BrowserID service. Instead, the BrowserID service should depend on this one.

::: security/manager/ssl/src/nsNSSComponent.cpp
@@ +3194,5 @@
> +
> +  hash->len = hashedData.Length();
> +  hash->data = (unsigned char*) hashedData.BeginReading();
> +
> +  SECStatus status = PK11_Sign(mPrivateKey, &signature, &hash);

You should be able to use SEC_SignData instead of using PK11_Sign directly. This should make the implementation simpler.

::: security/manager/ssl/src/nsNSSComponent.h
@@ +237,5 @@
>    virtual void virtualDestroyNSSReference();
>    void destructorSafeDestroyNSSReference();
>  };
>  
> +class nsBrowserIDKeypair : public nsIBrowserIDKeypair

You also need to extend nsNSSShutDownObject. See its wonderful documentation. Also, every method must use the nsNSSShutdownPreventionLock and must check that it hasn't been shut down already, before doing anything with NSS. See other implementations of nsNSSShutDownObject. Also, the destructor must do funny stuff (again, see other implementations).

@@ +249,5 @@
> +  // void GetSerializedRSAPublicKey(const nsAString &aOutString);
> +  // void GetRSAPublicKeyExponent(int &aOutval);
> +  // void GetRSAPublicKeyModulus(nsAString &aOutString);
> +  // void GetIdentifier(nsAString &aOutString);
> +  // void Sign(nsACString &aText, nsAString &aOutSignature);

Remove these comments (this is the same as NS_DECL_NSIBROWSERIDKEYPAIR).

@@ +251,5 @@
> +  // void GetRSAPublicKeyModulus(nsAString &aOutString);
> +  // void GetIdentifier(nsAString &aOutString);
> +  // void Sign(nsACString &aText, nsAString &aOutSignature);
> +
> +  void SetRSAKeypair(SECKEYPublicKey* aPublicKey, SECKEYPrivateKey* aPrivateKey);

Merge this with the constructor.

Comment 16

[David Dahl :ddahl]

Attachment: v 7 saving work, renamed everything, moving stuff into services/identity

Still needs more clean up before it will build cleanly

Comment 17

[David Dahl :ddahl]

Attachment: v 8 It Compiles!

This patch compiles, the new method is exposed to nsIDOMCrypto, however, I am not sure how to make sure we return the nsIIdentityServiceKeyPair object

I will attach a scratchpad test.

I will be adding the xpcshell tests for this tomorrow.

Comment 18

[David Dahl :ddahl]

Attachment: scratchpad test

To use this test, you need to add a boolean pref: devtools.chrome.enabled

then shift-f4 to open the scratchpad. Open this file and hit ctrl-r

Comment 19

[David Dahl :ddahl]

Attachment: v 9 we have a working PSM API, services/identity API with tests

To run the xpcshell tests do (from your obj dir):

make -C services/identity/tests xpcshell-tests

This crashes xpcshell pretty hard, but basically the methods work, I will attach my gdb output so you can see what happens after the Sign() function is invoked.

Comment 20

[David Dahl :ddahl]

Attachment: debugging Sign() output from gdb

Comment 21

[David Dahl :ddahl]

The weird thing here is that the Sign function should have returned NS_OK, but I see additional execution:

3210	    _retval = tmpStr;
(gdb) 
3214	  return NS_OK; // <---- The end of the method
(gdb) print _retval
$14 = (nsACString_internal &) @0x2ade25140070: {
  mData = 0x2ade2043cc78 "NSF9KZLNTknoiOSAs0KDlWZkdGkeih_x-aHhYGnii54kf6F_IT1BSbUEmWh4jJ5XcT3IQVU7roGeoN3fi_BCwHfe2_PiKrwNPQbadv3b4qnpbeSttaPcCDKenuh4Bimf4R3O-_4vzvWydfmYTLGYOxlA0lLLRc3ArmxjtUc7uppoMjxwDdLi9Nb3zPMz45cTb-7DLT4a"..., mLength = 344, mFlags = 5}
(gdb) n
3183	    do_CreateInstance(NS_CRYPTO_HASH_CONTRACTID, &rv); // <-- I would not expect this to happen 
(gdb) 
3171	  nsNSSShutDownPreventionLock locker;
(gdb) 
3215	}
(gdb) 
0x00002adc00000018 in ?? ()
(gdb) n
Cannot find bounds of current function
(gdb) n
Cannot find bounds of current function
(gdb) c
Continuing.

Comment 22

[David Dahl :ddahl]

Attachment: v 10 in review with bsmith

Comment 23

[David Dahl :ddahl]

brain: 

with a test for nsIdentityKeyPair only, i still see the crash, after all checks pass:

TEST-INFO | /home/ddahl/code/moz/obj-x86_64-unknown-linux-gnu-debug/_tests/xpcshell/security/manager/ssl/tests/unit/test_id_service_keypair.js | running test ...
TEST-UNEXPECTED-FAIL | /home/ddahl/code/moz/obj-x86_64-unknown-linux-gnu-debug/_tests/xpcshell/security/manager/ssl/tests/unit/test_id_service_keypair.js | test failed (with xpcshell return code: 1), see following log:
>>>>>>>
### XPCOM_MEM_LEAK_LOG defined -- logging leaks to /tmp/tmpYYJ_ef/runxpcshelltests_leaks.log

TEST-INFO | (xpcshell/head.js) | test 1 pending
IDServKeyPair test: KP: [xpconnect wrapped nsIIdentityServiceKeyPair @ 0x2b5986d85040 (native @ 0x2b5986df58b0)]

TEST-PASS | /home/ddahl/code/moz/obj-x86_64-unknown-linux-gnu-debug/_tests/xpcshell/security/manager/ssl/tests/unit/test_id_service_keypair.js | [run_test : 24] true == true
IDServKeyPair test: sig: iVVYtXBYDCXe5FNFwss8AxjEg7M5LGK2cIBnbiXsikRwQoO6KScVCAciD30c-jcg9tQu8kiH4dIFC-sEKQ41jAbedMwlvZVz74XEpf2GPBHBYihMiQi_IWoNi5jFNJU0pNW4ce8l8_sUFDMsY5fC_DKQXt6FUYqz693mRkd8zZRAn4aq7PpsIlQiD-X0xc0W3CIE6XZe5ipdCGa9CU0jwwx1QslE6zi6MBDqnDj3XsEEhoTkE6qPs-trfvzFcJE3q663b1CwBMm0OoYVSxNIB5_sqcIAg9cvYqWd_N-1cvj8nWLab1GzLgs1Z0wi4Fwu1OQkrX0H9ChLz5IH5IZBaw==

TEST-PASS | /home/ddahl/code/moz/obj-x86_64-unknown-linux-gnu-debug/_tests/xpcshell/security/manager/ssl/tests/unit/test_id_service_keypair.js | [run_test : 28] true == true

TEST-INFO | (xpcshell/head.js) | test 1 finished

TEST-INFO | (xpcshell/head.js) | exiting test

TEST-PASS | (xpcshell/head.js) | 2 (+ 0) check(s) passed

TEST-INFO | (xpcshell/head.js) | 0 check(s) todo
<<<<<<<
PROCESS-CRASH | /home/ddahl/code/moz/obj-x86_64-unknown-linux-gnu-debug/_tests/xpcshell/security/manager/ssl/tests/unit/test_id_service_keypair.js | application crashed (minidump found)
Crash dump filename: /home/ddahl/code/moz/obj-x86_64-unknown-linux-gnu-debug/_tests/xpcshell/security/manager/ssl/tests/unit/5b6c1716-acd6-f900-70998014-518fe354.dmp
Neither MINIDUMP_STACKWALK nor MINIDUMP_STACKWALK_CGI is set, can't process dump.
INFO | Result summary:
INFO | Passed: 3
INFO | Failed: 1
INFO | Todo: 0
make: *** [xpcshell-tests] Error 1

Will update the patch later...

Comment 24

[David Dahl :ddahl]

Attachment: v 11 still crashes, but all tests pass

I will see if I can get this into my Visual Studio debugger, as gdb is not so helpful with this crash. All of the code does what I expect except for a crash after the sign method executes

Comment 25

[Mozilla RelEng Bot]

Try run for e79df42cf106 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=e79df42cf106
Results (out of 2 total builds):
    exception: 2
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-e79df42cf106

Comment 26

[David Dahl :ddahl]

Just talked to brian last night - he is in China but he will be looking at this crash soon

Comment 27

[David Dahl :ddahl]

I setup my windows dev env again and ran the unit test through the debugger:

SOLO_FILE=unit/test_id_service_keypair.js make -C security/manager/ssl/tests/ check-interactive

This test runs fine with the debugger observing - I could not determine what is causing the crash when the tests are run outside of the debugger.

Comment 28

[David Dahl :ddahl]

Attachment: Valgrind output when test_id_service_keypiar.js is run

Just got valgrind up and running, ran the xpcshell test with valgrind - in case anyone knows how to interpret the output - please have a look.

Comment 29

[David Dahl :ddahl]

Attachment: Valgrind output when test_id_service_keypair.js is run

Updated the output with VALGRIND_OPTS=--smc-check=all

Comment 30

[David Dahl :ddahl]

Discussion on irc with jseward:

<sewardj> ddahl: so .. not sure what I can tell you, that you don't already know (also, not sure what you're really asking)
<sewardj> ddahl: there's clearly some confusion about when object lifetimes end
<ddahl> sewardj: np, thanks for the help. Just trying figure out this very weird crash
<sewardj> ddahl: well, access-after-free is potentially fatal
<sewardj> ddahl: especially as it moves on to writing freed memory further down the log
<sewardj> ddahl: if i had to guess, there's some refcounting error to do with nsIdentityServiceKeyPair
<ddahl> sewardj: that sound very plausible
<sewardj> ddahl: essentially you are trying to drive them below zero
<sewardj> (that much is kinda inferrable from the output)
<philor> but fuck it, it's just inexplicable near-permaorange on the stuff we're about to ship, drive on
<ddahl> sewardj: so I am trying to free an object that is already freed? 
<sewardj> ddahl: yes
<sewardj> ddahl: look at this snippet: http://pastebin.mozilla.org/1435743

==25757== Invalid read of size 4
==25757==    at 0x5CD6038: nsAutoRefCnt::operator unsigned int() const (nsISupportsImpl.h:266)
==25757==    by 0x6F2CF6A: nsIdentityServiceKeyPair::Release() (in /home/ddahl/code/moz/mozilla-valgrind/obj-x86_64-unknown-linux-gnu-debug/toolkit/library/libxul.so)
==25757==    by 0x6CCCA58: void DoDeferredRelease<nsISupports*>(nsTArray<nsISupports*, nsTArrayDefaultAllocator>&) (XPCJSRuntime.cpp:616)
==25757==    by 0x6CC6AB4: XPCJSRuntime::GCCallback(JSContext*, JSGCStatus) (XPCJSRuntime.cpp:913)
==25757==    by 0x7977FD2: js_GC(JSContext*, JSCompartment*, JSGCInvocationKind, js::gcstats::Reason) (jsgc.cpp:3130)
==25757==    by 0x78EDDD9: JS_CompartmentGC (jsapi.cpp:2853)
==25757==    by 0x78EDDF8: JS_GC (jsapi.cpp:2859)
==25757==    by 0x409F3F: main (xpcshell.cpp:2009)

==25757==  Address 0x11c173f4 is 20 bytes inside a block of size 48 free'd
==25757==    at 0x4C28654: free (vg_replace_malloc.c:427)
==25757==    by 0x5252FA1: moz_free (mozalloc.cpp:97)
==25757==    by 0x6F2D7DD: nsIdentityServiceKeyPair::~nsIdentityServiceKeyPair() (mozalloc.h:253)
==25757==    by 0x6F2D0DD: nsIdentityServiceKeyPair::Release() (in /home/ddahl/code/moz/mozilla-valgrind/obj-x86_64-unknown-linux-gnu-debug/toolkit/library/libxul.so)
==25757==    by 0x6CCCA58: void DoDeferredRelease<nsISupports*>(nsTArray<nsISupports*, nsTArrayDefaultAllocator>&) (XPCJSRuntime.cpp:616)
==25757==    by 0x6CC6AB4: XPCJSRuntime::GCCallback(JSContext*, JSGCStatus) (XPCJSRuntime.cpp:913)
==25757==    by 0x7977FD2: js_GC(JSContext*, JSCompartment*, JSGCInvocationKind, js::gcstats::Reason) (jsgc.cpp:3130)
==25757==    by 0x78EDDD9: JS_CompartmentGC (jsapi.cpp:2853)
==25757==    by 0x78EDDF8: JS_GC (jsapi.cpp:2859)
==25757==    by 0x409F3F: main (xpcshell.cpp:2009)

<sewardj> ddahl: in the second stack trace (which happens first in execution)
<sewardj> ddahl: we wind up in nsIdentityServiceKeyPair::Release() and then nsIdentityServiceKeyPair::~nsIdentityServiceKeyPair()
<sewardj> ddahl: so the refcount must have gone to zero at that point
<sewardj> ddahl: but then later on (first stack trace) we are again calling nsIdentityServiceKeyPair::Release() on what is very likely the same object
<sewardj> ddahl: #include <disclaimer.h> .. this is my best guess
<ddahl> sewardj: that is very informative
<ddahl> sewardj: i wonder if this is some NSS wonkiness
<sewardj> ddahl: one common way to get into such situations is if you have multiple threads being confused about ownership
<ddahl> where we use their "check for nss shutdown before doing anything"
<sewardj> ddahl: one possible way to simplify the problem is to see if you can get it to happen when there's only one thread on the go
<sewardj> ddahl: what the snippet doesn't tell you is whether it's the same thread in both calls to Release, or different ones
<ddahl> sewardj: I thought all of the NSSComponent code was main thread only, but I am very new to this code
<sewardj> ddahl: i have no idea .. just suggesting the general-case possibilities

Comment 31

[David Dahl :ddahl]

according to khuey we need to AddRef the outparam. I tried getter_Copies() to no avail. I am sure this is a simple thing to fix.

Comment 32

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 572120 [details] [diff] [review]
v 11 still crashes, but all tests pass

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

::: security/manager/ssl/src/nsCrypto.cpp
@@ +2549,5 @@
> +                               NULL /*&pwdata*/);
> +
> +  if (privk != NULL && pubk != NULL) {    
> +    // nsRefPtr<nsIdentityServiceKeyPair> identitySvcKeyPair = new nsIdentityServiceKeyPair(pubk, privk);
> +    *_retval = new nsIdentityServiceKeyPair(pubk, privk);

NS_ADDREF(*_retval = new nsIdentityServiceKeyPair(pubk, privk));

Comment 33

[David Dahl :ddahl]

Attachment: v 12 no more crashy crashy

Ok, khuey figured out that I was missing an NS_ADDREF. yay.

Comment 34

[Julian Seward [:jseward]]

(In reply to David Dahl :ddahl from comment #33)
> v 12 no more crashy crashy

No more crashy crashy is obviously good good, but you really also need to
verify that it now runs valgrind-clean.  (maybe you did already).

Comment 35

[David Dahl :ddahl]

Attachment: Valgrind output when test_id_service_keypair.js is run with NO crash

New Valgrind output - I imagine we should file bugs on some of these memory problems

Comment 36

[David Dahl :ddahl]

filed bug 715917, bug 715908, bug 715912

Comment 37

[Julian Seward [:jseward]]

(In reply to David Dahl :ddahl from comment #35)
> Created attachment 586432 [details]
> Valgrind output when test_id_service_keypair.js is run with NO crash

Looks like you've successfully fixed the refcounting problem.  Whether the
leaks you're observing are expected I don't know (you'd have to ask the
module owners); but in general try to fix them.  Small leaks can turn
into big leaks given the right workload and it's safest to have none at all.

Comment 38

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I see lots of little Telemetry leaks (file a bug on that please) and some XPCOM service manager leaks that are by design.  There's also some NSS leaks, but those look like they're from Init and not from your patch.

Comment 39

[David Dahl :ddahl]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #38)
> I see lots of little Telemetry leaks (file a bug on that please) and some
> XPCOM service manager leaks that are by design.  There's also some NSS
> leaks, but those look like they're from Init and not from your patch.

I only filed bugs on leaks that were "definite".  I can file bugs on the rest that were "probablies" - I was under the impression they might be false positives.

Comment 40

[David Dahl :ddahl]

Attachment: v 13 Adding DSA keyType

Saving work, still need tests for DSA keys and signing

Comment 41

[David Dahl :ddahl]

Attachment: v 14 DSA Key failure

Seeing a failure in PK11_PQG_ParamGen(keySizeInBits, &pqgParams, &pqgVfy)
Brian: I am still debugging, but thought i'd see if you know what is wrong offhand

Comment 42

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

1. You don't need to call PK11_PQG_ParamGen because the DSA PQG parameters in BrowserID are hard-coded into the protocol. Check the JS implementation to find out what they are.

2. NSS's DSA implementation only works with 1024-bit keys. See bug 475578.

Comment 43

[David Dahl :ddahl]

(In reply to Brian Smith (:bsmith) from comment #42)
> 1. You don't need to call PK11_PQG_ParamGen because the DSA PQG parameters
> in BrowserID are hard-coded into the protocol. Check the JS implementation
> to find out what they are.

So I should just specify a struct for this instead of all of the verification steps - like: http://mxr.mozilla.org/mozilla-central/source/security/nss/lib/softoken/fipstest.c#1903

also, here are the hard-coded values: https://github.com/mozilla/jwcrypto/blob/master/algs/ds.js#L74

Comment 44

[David Dahl :ddahl]

Attachment: v 15 DSA Pubkey is generated, Privkey fails

I added all of the FIPS p q g values from the above mentioned snippet and get further: 

In my debugger I see a failure inside of:

PK11_GenerateKeyPairWithOpFlags (slot=0x1bbb550, type=16, param=0x7fff81c81270, pubKey=0x7fff81c812c0, attrFlags=70, opFlags=0, opFlagsMask=0, wincx=0x0) at pk11akey.c:1059
 
Right here: 

1432	    crv = PK11_GETTAB(slot)->C_GenerateKeyPair(session_handle, &mechanism,
1433		pubTemplate,pubCount,privTemplate,privCount,&pubID,&privID);

Comment 45

[David Dahl :ddahl]

Attachment: backtrace

DSA key gen fails

Comment 46

[David Dahl :ddahl]

Latest debugging: looks like we are crashing inside of sftk_AddAttributeType()

gdb output: 
----------
NSC_GenerateKeyPair (hSession=16777217, pMechanism=0x7fff8cd455e0, pPublicKeyTemplate=0x7fff8cd45190, ulPublicKeyAttributeCount=9, pPrivateKeyTemplate=0x7fff8cd45280, 
    ulPrivateKeyAttributeCount=7, phPublicKey=0x7fff8cd456a8, phPrivateKey=0x7fff8cd456a0) at pkcs11c.c:3767
3767	    int 		public_modulus_bits = 0;
(gdb) s
3777	    int 		private_value_bits = 0;
(gdb) 
3787	    CHECK_FORK();
(gdb) 
3789	    if (!slot) {
(gdb) 
3795	    publicKey = sftk_NewObject(slot); /* fill in the handle later */
(gdb) n
3796	    if (publicKey == NULL) {
(gdb) 
3803	    for (i=0; i < (int) ulPublicKeyAttributeCount; i++) {
(gdb) 
3804		if (pPublicKeyTemplate[i].type == CKA_MODULUS_BITS) {
(gdb) 
3810					    sftk_attr_expand(&pPublicKeyTemplate[i]));
(gdb) 
3809		crv = sftk_AddAttributeType(publicKey,
(gdb) 
3810					    sftk_attr_expand(&pPublicKeyTemplate[i]));
(gdb) 
3809		crv = sftk_AddAttributeType(publicKey,
(gdb) 
3810					    sftk_attr_expand(&pPublicKeyTemplate[i]));
(gdb) 
3809		crv = sftk_AddAttributeType(publicKey,
(gdb) 

Program received signal SIGSEGV, Segmentation fault.
__memcpy_ssse3_back () at ../sysdeps/x86_64/multiarch/memcpy-ssse3-back.S:1579
1579	../sysdeps/x86_64/multiarch/memcpy-ssse3-back.S: No such file or directory.
	in ../sysdeps/x86_64/multiarch/memcpy-ssse3-back.S

And what does the above "no such file..." error mean?

Comment 47

[David Dahl :ddahl]

Attachment: backtrace

updated backtrace, attaching latest patch as well

Comment 48

[David Dahl :ddahl]

Attachment: v 16 latest with keygen failure

Comment 49

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to David Dahl :ddahl from comment #46)
> Program received signal SIGSEGV, Segmentation fault.
> __memcpy_ssse3_back () at
> ../sysdeps/x86_64/multiarch/memcpy-ssse3-back.S:1579
> 1579	../sysdeps/x86_64/multiarch/memcpy-ssse3-back.S: No such file or
> directory.
> 	in ../sysdeps/x86_64/multiarch/memcpy-ssse3-back.S
> 
> And what does the above "no such file..." error mean?

It means you don't have the source code for libc installed.

> #0  __memcpy_ssse3_back () at ../sysdeps/x86_64/multiarch/memcpy-ssse3-back.S:1579
> #1  0x00002b06aa05da20 in sftk_NewAttribute (object=0x1a37660, type=304,
> value=0x7fff8cd45910, len=27222848) at pkcs11u.c:98
> #2  0x00002b06aa05ef22 in sftk_AddAttributeType (object=0x1a37660, type=304, 
> valPtr=0x7fff8cd45910, length=27222848) at pkcs11u.c:823

len/length should not be 27,222,848 (27MB).

Comment 50

[David Dahl :ddahl]

Attachment: v 16.2 Latest

pqg probs

Comment 51

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

David, I ran your tests, both using the hard-coded values, and asking NSS to generate the PQG parameters. In the latter case, PK11_PQG_ParamGenSeedLen returns SEC_ERROR_INVALID_ARGS. I did not investigate that further.

In the hard-coded case, I uncommented the lines that deal with the hard-coded values, and I commented these lines:

      //SECStatus srv = PK11_PQG_ParamGenSeedLen(keySizeIndex, seedBytes, 
      //                                    &pqgParams, &pqgVfy);
      // SECStatus srv = PK11_PQG_ParamGen(keySizeInBits, &pqgParams, &pqgVfy);

      //if (srv != SECSuccess) {
      //  return NS_ERROR_FAILURE;
      //}

      //PK11_PQG_DestroyVerify(pqgVfy);

I ran the code in the debugger and I found that sftk_PairwiseConsistencyCheck fails. That means that NSS is not able to verify a signature (using a public key) that was created using the private key. That means there is almost definitely some problem with the public or private key. Make sure you copy/pasted the key data correctly.

Comment 52

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: correct dsa test data

Comment 53

[David Dahl :ddahl]

Attachment: v 17 RSA, DSA keypair and signature support with tests

Comment 54

[David Dahl :ddahl]

Attachment: v 18 playing with a prototype navigator.id object

Need to work out exactly how assertions are generated so the navigator.id property can mint them from DSA keypairs/ sign() function. This patch will need to be refactored to align with bsmith's PSM code requirements: no xpcom, etc...

The JS/DOM parts will be broken off and added to another bug as well.

Comment 55

[David Dahl :ddahl]

Attachment: v 19 navigator.id works, .get returns properties of the pub key

Comment 56

[David Dahl :ddahl]

Attachment: v 20 added more navigator.id functionality

Will be re-factoring and splitting this patch up into crypto/ xpcom / front end next after feedback from bug 664614

Comment 57

[David Dahl :ddahl]

Attachment: broke the class out of nsNSSComponent

bsmith or khuey:

Do I add the XPCOM Module bits to the one in layout/build - or do I create a new Module.cpp as this is PSM code?

Comment 58

[David Dahl :ddahl]

Attachment: Created separate source files, added tests for entire interface

Comment 59

[David Dahl :ddahl]

Brian:

One missing thing here is the keygen operation happens on main thread - I imagine I will have to use an nsIKeyGenThread to do that correctly. I am unclear by looking at the uses in nsCrypto how this works - or if this is how we want it to work these days.

Comment 60

[David Dahl :ddahl]

Attachment: Added a keygen runnable

The idls are interdependent. I must be designing this wrong.

Comment 61

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 612414 [details] [diff] [review]
Added a keygen runnable

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

::: security/manager/ssl/public/nsIIdentityServiceKeyGenCallback.idl
@@ +10,5 @@
> + * This interface provides a JavaScript callback object used to collect the 
> + * nsIIdentityServeKeyPair when the keygen operation is complete 
> + */
> +[scriptable, uuid(f38100a5-94f1-415c-ac05-7e8351345dec)]
> +interface nsIIdentityServiceKeyGenCallback : nsISupports

1. Put all the interfaces implemented in PSM for this bug in one IDL file.

2. If you need to have interfaces which reference each other in a circular manner, in separate files, then you can do this:

-- nsIBar.idl ---------------------

interface nsIFoo; // instead of #include "nsIFoo.idl"

interface nsIBar
{
    void bar(in nsIFoo foo);   
};

---------------------------------------

-- nsIFoo.idl -------------------------

interface nsIBar; // instead of #include "nsIBar.idl"

interface nsIFoo {
   void foo(in nsIBar bar);
}

Comment 62

[David Dahl :ddahl]

(In reply to Brian Smith (:bsmith) from comment #61)
> Comment on attachment 612414 [details] [diff] [review]
> Added a keygen runnable
> 
> Review of attachment 612414 [details] [diff] [review]:

> 1. Put all the interfaces implemented in PSM for this bug in one IDL file.
> 
I tried using a single idl but had the same problem. I will try the 'forward declaration' style

Comment 63

[David Dahl :ddahl]

Attachment: generated header problem

Comment 64

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 612649 [details] [diff] [review]
generated header problem

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

0. The problem you were running into is caused by having two definitions of nsIIdentityServiceKeyGenCallback: one in each file. The correct one, in nsIIdentityServiceKeyPair.idl, has a method named keyPairGenFinished, not keyPairGenerated like you wrote it the caller.

1. Please put the changes to dom/ in a separate patch from the changes to security/. The changes and tests for security/ should build without error independently of the changes to dom/, and there will be separate sets of reviewers for each.

2. Instead of naming the implementations nsIdentity*, name them mozilla::psm::Identity*. i.e. wrap them in:

    namespace mozilla { namespace psm {
 
    } } // namespace mozilla::psm

and rename the source files from nsIdentity* to nsIdentity*. (But, don't do this for the nsIIdentity* *interfaces*). This will make it easier to read; right now it is hard to differentiate nsIIdentity* from nsIdentity*.

3. nsIdentityServiceKeyPair.h, nsIdentityServiceKeyPairGenRunnable.h, nsIdentityServiceKeyPair.cpp, and nsIdentityServiceKeyPairGenRunnable.cpp can all be combined into a single IdentityService.cpp file. (A class only needs to be defined in a header file if some other file actually includes it.)

4. Rename nsIIdentityServiceKeyPair.idl to nsIIdentityService.idl

5. Drop nsIIdentityServiceKeyGenCallback.idl.

Comment 65

[David Dahl :ddahl]

In just trying to get this to build, I am now getting an error during linking:

../../security/manager/ssl/src/nsIdentityServiceKeyPairGenRunnable.o: In function `nsIdentityServiceKeyPairGenRunnable':
/home/ddahl/code/moz/mozilla-valgrind/src/security/manager/ssl/src/nsIdentityServiceKeyPairGenRunnable.cpp:22: undefined reference to `vtable for nsIdentityServiceKeyPairGenRunnable'
/usr/bin/ld.bfd.real: ../../security/manager/ssl/src/nsIdentityServiceKeyPairGenRunnable.o: relocation R_X86_64_PC32 against undefined hidden symbol `vtable for nsIdentityServiceKeyPairGenRunnable' can not be used when making a shared object

Comment 66

[David Dahl :ddahl]

Attachment: vtable linking error

/home/ddahl/code/moz/mozilla-valgrind/src/security/manager/ssl/src/nsIdentityServiceKeyPairGenRunnable.cpp:22: undefined reference to `vtable for nsIdentityServiceKeyPairGenRunnable'

Comment 67

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

You need to implement:

    nsIdentityServiceKeyPairGenRunnable::virtualDestroyNSSReference(void)"

and 

    nsIdentityServiceKeyPairGenRunnable::~nsIdentityServiceKeyPairGenRunnable(void)" 

and the linker errors should go away.

Comment 68

[David Dahl :ddahl]

(In reply to Brian Smith (:bsmith) from comment #67)
> You need to implement:
> 
>     nsIdentityServiceKeyPairGenRunnable::virtualDestroyNSSReference(void)"
> ...
> 
> and the linker errors should go away.

Yes! awesome it works- well I am working on the tests now.

Comment 69

[David Dahl :ddahl]

Attachment: test error

I am getting this error: 
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'JavaScript component does not have a method named: "keyPairGenFinished"' when calling method: [nsIIdentityServiceKeyGenCallback::keyPairGenFinished]"  nsresult: "0x80570030 (NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED)"  location: "native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0"  data: no]
************************************************************

I have implemented the method in the callback:

    keyPairGenFinished: function rsa_keyPairGenFinished(aKeyPair)
    {
      do_check_true(typeof aKeyPair.sign == "function");
      let sig = aKeyPair.sign("I am a string that mentions bacon. Yum.");
      do_check_true(sig.length > 1);
      do_check_true(aKeyPair.keyType == "1");
      do_check_true(aKeyPair.encodedPublicKey.length > 1);
      do_check_true(aKeyPair.encodedRSAPublicKeyModulus.length > 1);
      do_check_true(aKeyPair.encodedRSAPublicKeyExponent.length > 1);
    }

Comment 70

[David Dahl :ddahl]

In gdb, I am seeing this error:

nsCOMPtr<nsIIdentityServiceKeyGenCallback>::operator-> (this=0x1af8f28) at ../../../../dist/include/nsCOMPtr.h:809
809	          NS_ABORT_IF_FALSE(mRawPtr != 0, "You can't dereference a NULL nsCOMPtr with operator->().");

I removed the (void) from     mCallback->KeyPairGenFinished(mKeyPair);

Now, that error is gone, but I still see:

[Exception... "'JavaScript component does not have a method named: "keyPairGenFinished"' when calling method: [nsIIdentityServiceKeyGenCallback::keyPairGenFinished]"  nsresult: "0x80570030 (NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED)"  location: "<unknown>"  data: no]

Comment 71

[David Dahl :ddahl]

Attachment: Latest patch

Comment 72

[David Dahl :ddahl]

Attachment: Patch: off main thread keygen working, tests working

bsmith:
I'll probably clean this up a bit more before you get to reviewing it. Everything is working now, thanks again for the debugging time!

Comment 73

[David Dahl :ddahl]

Attachment: Patch tweaked for frontend hacking

a few tweaks

Comment 74

[Matthew N. [:MattN]]

Attachment: Follow-up patch for infallible strings

Comment 75

[David Dahl :ddahl]

(In reply to Matthew N. [:MattN] from comment #74)
> Created attachment 626592 [details] [diff] [review]
> Follow-up patch for infallible strings

Thanks Matt! I am folding this into the official patch now.

Comment 76

[David Dahl :ddahl]

Attachment: Lastest WIP, off main thread signature creation

WIP, tests will be working soon

Comment 77

[David Dahl :ddahl]

Attachment: Latest Patch - thread never returns to main thread

For some reason, the off-main-thread signature does work, but the nsIRunnable never returns to the main thread - we just hang during the test, which can be run like so: make SOLO_FILE="test_id_service_keypair.js" -C security/manager/ssl/tests/ check-one

Comment 78

[David Dahl :ddahl]

Attachment: Latest Patch, off main thread signatures - all tests pass

Comment 79

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 628529 [details] [diff] [review]
Latest Patch, off main thread signatures - all tests pass

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

David, today I will write a ScopedHeapSECItem class that will help avoid the memory leaks that I noted below, so don't worry too much about the comments regarding memory leaks.

The code pattern that is required to be used here is:
   1. Create a new (invalid key pair)
   2. Call GenerateKeyPair on it.
   3. Wait for the callback to be called.

It would be cleaner if the pattern was:
   1. Request a new keypair to be generated
   2. Wait for the callback to be called with the generated keypair passed as a parameter.

Because, with the latter pattern, the JS code never has a reference to an invalid keypair. However, that structural change is something we should do after all the other comments here have been addressed.

Please address Ben Adida's comment about making the "keyType" attribute a string that specifies the keytype according to the JWT names.

I am going to review the patch again after you've addressed these cleanup items.

I will also try to get nsNSSShutdownObject exposed outside of PSM so that we can move this code to identity/. But, that can be done after this lands if necessary.

What is the higher-layer BrowserID code going to use the parameters (RSA modulus, RSA exponent, DSA PQG) for? Where is the code that uses this component?

::: security/manager/ssl/public/nsIIdentityServiceKeyPair.idl
@@ +16,5 @@
> +interface nsIIdentityServiceKeyPair : nsISupports
> +{
> +  void generateKeyPair(in unsigned long aAlgorithm, in nsIIdentityServiceKeyGenCallback aCallback);
> +
> +  readonly attribute AUTF8String keyType; 

Again, is this the name used in the JWT spec or something else?

@@ +18,5 @@
> +  void generateKeyPair(in unsigned long aAlgorithm, in nsIIdentityServiceKeyGenCallback aCallback);
> +
> +  readonly attribute AUTF8String keyType; 
> +
> +  readonly attribute AUTF8String encodedPublicKey;

Please document how the various components of the public key are encoded in this string, in addition to the normal "encoding" (to be) documented above. Again, reference the JWT spec if that defines the encoding.

@@ +61,5 @@
> + * This interface provides a JavaScript callback object used to collect the 
> + * AUTF8String signature 
> + */
> +[scriptable, uuid(52992987-2725-4f71-94f4-ec40a141388b)]
> +interface nsIIdentityServiceKeyPairSignCallback : nsISupports

Nit: Move this to be under nsIIdentityServiceKeyPair.

::: security/manager/ssl/src/nsIdentityServiceKeyPair.cpp
@@ +31,5 @@
> +using namespace mozilla;
> +
> +// Stolen from /toolkit/components/places/Helpers.cpp
> +// TODO: use the new base64 encoder khuey wrote
> +static

Instead of static, use 

namespace {

static things go here

} // unnamed namespace

Move your NSSCleanupAutoPtrClass inside the namespace { }.

@@ +35,5 @@
> +static
> +nsresult
> +Base64urlEncode(const PRUint8* aBytes,
> +                PRUint32 aNumBytes,
> +                nsCString& _result)

Change the signature to:

nsresult
Base64UrlEncode(const SECItem * item, nsACString & result);

This will allow you to eliminate most of the boilerplate you have below; see the example "EXAMPLE 1" below.

@@ +40,5 @@
> +{
> +  // SetLength does not set aside space for NULL termination.  PL_Base64Encode
> +  // will not NULL terminate, however, nsCStrings must be NULL terminated.  As a
> +  // result, we set the capacity to be one greater than what we need, and the
> +  // length to our desired length.

I think you copy/pasted this from somewhere. Where?

@@ +54,5 @@
> +  _result.ReplaceChar('/', '_');
> +  return NS_OK;
> +}
> +
> +// NS_IMPL_ISUPPORTS1(nsIdentityServiceKeyPair, nsIIdentityServiceKeyPair)

remove all commented-out code.

@@ +65,5 @@
> +  mPublicKey = NULL;
> +}
> +
> +nsresult
> +nsIdentityServiceKeyPair::SetKeyPair(SECKEYPrivateKey * aPrivateKey, 

Rename this method to Init() to match the convention of other gecko code that does two-phase construction.

@@ +82,5 @@
> +}
> +
> +// NS_IMETHODIMP
> +SECKEYPublicKey*
> +nsIdentityServiceKeyPair::GetPublicKey()

Just remove this method.

@@ +96,5 @@
> +}
> +
> +// NS_IMETHODIMP
> +SECKEYPrivateKey*
> +nsIdentityServiceKeyPair::GetPrivateKey()

Remoe this method.

@@ +112,5 @@
> +NS_IMETHODIMP
> +nsIdentityServiceKeyPair::GetEncodedPublicKey(nsACString &aOutString)
> +{
> +  nsNSSShutDownPreventionLock locker;
> +

Nit: Remove this blank line here and elsewhere. We normally do not put a blank line between the locker and isAlreadyShutDown().

@@ +115,5 @@
> +  nsNSSShutDownPreventionLock locker;
> +
> +  if (isAlreadyShutDown())
> +    return NS_OK;
> +

*** BEGIN EXAMPLE 1 ***

@@ +135,5 @@
> +  if (NS_FAILED(rv)) {
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  aOutString = tmpStr;

*** END EXAMPLE 1 ***

With the suggested signature change to Base64UrlEncode, All of this code will be reduced to:

ScopedHeapSECItem key(PK11_DEREncodePublicKey(mPublicKey));
NS_ENSURE_TRUE(key, NS_ERROR_UNEXPECTED); // BUG: missing in the original code
return Base64UrlEncode(key, aOutString);

This pattern can be repeated everywhere down below.

I will write the ScopedHeapSECItem class for you on Monday. For now, you can use this code:

SECItem * key(PK11_DEREncodePublicKey(mPublicKey));
NS_ENSURE_TRUE(key, NS_ERROR_UNEXPECTED); // BUG: missing in the original code
nsresult rv = Base64UrlEncode(key, aOutString);
SECItem_FreeItem(key, true); // BUG: missing in the original code
return rv;

Please use this pattern for all the similar methods below.

@@ +153,5 @@
> +
> +  bool isRSAKey = mPublicKey->keyType == rsaKey;
> +
> +  NS_ENSURE_TRUE(isRSAKey, NS_ERROR_UNEXPECTED);
> +  NS_ENSURE_TRUE(mPublicKey->u.rsa.publicExponent.len, NS_ERROR_UNEXPECTED);

You do not need this second check. We can assume that the keypair is fully constructed if Init() returns successfully, and we can assume that the application will not call any methods on the keypair if the Init() method returns an error.

@@ +217,5 @@
> +  bool isDSAKey = (mPublicKey->keyType == dsaKey);
> +  NS_ENSURE_TRUE(isDSAKey, NS_ERROR_UNEXPECTED);
> +
> +  NS_ENSURE_TRUE(mPublicKey->u.dsa.params.base.len, NS_ERROR_UNEXPECTED);
> +  // get DSA public key 'base'

ax this comment.

@@ +427,5 @@
> +  if (isAlreadyShutDown())
> +    return;
> +
> +  SECKEY_DestroyPrivateKey(mPrivateKey);
> +  SECKEY_DestroyPublicKey(mPublicKey);

mPrivateKey = NULL;
mPrivateKey = NULL;

::: security/manager/ssl/src/nsIdentityServiceKeyPair.h
@@ +27,5 @@
> +  NS_DECL_ISUPPORTS
> +  NS_DECL_NSIIDENTITYSERVICEKEYPAIR
> +  
> +  nsIdentityServiceKeyPair();
> +  nsresult SetKeyPair(SECKEYPrivateKey* aPrivateKey, SECKEYPublicKey*  aPublicKey);

MOZ_WARN_UNUSED_RESULT nsresult Init(SECKEYPrivateKey* aPrivateKey, SECKEYPublicKey* aPublicKey, KeyType keyType);

@@ +29,5 @@
> +  
> +  nsIdentityServiceKeyPair();
> +  nsresult SetKeyPair(SECKEYPrivateKey* aPrivateKey, SECKEYPublicKey*  aPublicKey);
> +  SECKEYPublicKey* GetPublicKey();
> +  SECKEYPrivateKey* GetPrivateKey();

Kill these two.

@@ +38,5 @@
> +  SECKEYPublicKey*  mPublicKey;
> +  KeyType mKeyType;
> +
> +  virtual void virtualDestroyNSSReference();
> +  void destructorSafeDestroyNSSReference();

nsIdentityServiceKeyPair(const nsIdentityServiceKeyPair&) MOZ_DELETE;
void operator=(const nsIdentityServiceKeyPair&) MOZ_DELETE;

::: security/manager/ssl/src/nsIdentityServiceKeyPairGenRunnable.cpp
@@ +18,5 @@
> +#include "secerr.h"
> +#include "keyhi.h"
> +#include "cryptohi.h"
> +
> +nsIdentityServiceKeyPairGenRunnable::nsIdentityServiceKeyPairGenRunnable(PRUint32 aAlgorithm, nsIIdentityServiceKeyGenCallback * aCallback, nsIdentityServiceKeyPair * aKeyPair)

: mAlgorithm(aAlgorithm)
  , mCallback(aCallback)
{
}

@@ +28,5 @@
> +
> +NS_IMETHODIMP 
> +nsIdentityServiceKeyPairGenRunnable::Run()
> +{
> +  nsNSSShutDownPreventionLock locker;

Need to check if we're already shut down. And, this check should be moved to be within (!NS_IsMainThread()), right?

@@ +37,5 @@
> +    PK11SlotInfo *slot = PK11_GetInternalSlot();
> +    nsresult rv;
> +
> +    switch (mAlgorithm) {
> +    case rsaKey:

Please put the code for each case in separate functions: GenerateRSAKeyPair and GenerateDSAKeyPair. This will make this method easier to read. Right now, the if (!NS_IsMainThread()) branch is too big.

@@ +91,5 @@
> +          "\xc4\x2e\x9f\x6f\x46\x4b\x08\x8c"
> +          "\xc5\x72\xaf\x53\xe6\xd7\x88\x02";
> +
> +        PQGParams pqgParams  = { NULL,
> +                                 { FIPS_DSA_TYPE, 

Nit: one line per entry, if it fits in 80 columns:

{ FIPS_DSA_TYPE, (unsigned char *)dsa_P, FIPS_DSA_PRIME_LENGTH},
...

@@ +107,5 @@
> +                                     &pqgParams, 
> +                                     &pubk, 
> +                                     PR_FALSE /*isPerm*/, 
> +                                     PR_TRUE /*isSensitive*/, 
> +                                     NULL /*&pwdata*/);

TODO: I need to figure out whether we need to pass in the password data, or how that works.

@@ +115,5 @@
> +      return NS_ERROR_INVALID_ARG;
> +    }
> +
> +    if (privk != NULL && pubk != NULL) {    
> +

Let's use fewer blank lines.

@@ +117,5 @@
> +
> +    if (privk != NULL && pubk != NULL) {    
> +
> +      mKeyPair->SetKeyPair(privk, pubk);
> +

""

@@ +121,5 @@
> +
> +      NS_DispatchToMainThread(this);
> +      
> +    } else {
> +      int error;

PRErrorCode error; (#include "prerror.h")

::: security/manager/ssl/src/nsIdentityServiceKeyPairSignRunnable.cpp
@@ +21,5 @@
> +#include "cryptohi.h"
> +
> +static
> +nsresult
> +Base64urlEncode(const PRUint8* aBytes,

Delete this duplicate definition. Create a new header that declares the function and #include it in both of the .cpp files that use it.

@@ +43,5 @@
> +  return NS_OK;
> +}
> +
> +
> +nsIdentityServiceKeyPairSignRunnable::nsIdentityServiceKeyPairSignRunnable(const nsACString & aText, nsIIdentityServiceKeyPairSignCallback * aCallback, nsIdentityServiceKeyPair * aKeyPair)

Use constructor initialization syntax, as I demonstrated in my comments above. Also, observe the 80 column rule.

@@ +97,5 @@
> +
> +      rv = Base64urlEncode(data, len, mSignature); // XXX: use revtval directly
> +      if (!NS_SUCCEEDED(rv)) {
> +        rv = NS_ERROR_UNEXPECTED;
> +        return rv;

Be mindful of returning early before you've done SECItem_FreeItem() or you will leak memory.

@@ +100,5 @@
> +        rv = NS_ERROR_UNEXPECTED;
> +        return rv;
> +      }
> +    } else {
> +      rv = NS_ERROR_UNEXPECTED;

Ditto.

::: security/manager/ssl/tests/unit/test_id_service_keypair.js
@@ +91,5 @@
> +    }
> +  };
> +  log("DSA_KEYPAIR: " + DSA_KEYPAIR);
> +  do_check_true(DSA_KEYPAIR != null);
> +  DSA_KEYPAIR.sign("Some text to sign", signCallback);

It is hard to test this without being able to create static keypairs for known-answer tests or without implementing the Verify() method so that we can do a (Verify(Sign(x)) consistency check. :( Do you think you could implement a Verify() function in a reasonable amount of time to do the consistency check?

Comment 80

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

By the way, if you do implement Verify(), you do not need to make it async because it is test-only code. But, if you have time to make it async, that would be nice.

Comment 81

[David Dahl :ddahl]

(In reply to Brian Smith (:bsmith) from comment #79)
> Comment on attachment 628529 [details] [diff] [review]

> What is the higher-layer BrowserID code going to use the parameters (RSA
> modulus, RSA exponent, DSA PQG) for? Where is the code that uses this
> component?

It is the code in toolkit/Identity.jsm in bug 753238

Comment 82

[David Dahl :ddahl]

Since we are not returning any base64encoded strings, but hex-encoded strings, I think we need to do something like:

nsresult HexEncode(const char* aBytes, nsCString& _result)
{
  PRUint32 size = strlen(aBytes);
  for (PRUint32i = 0; i < size;) {
    _result.AppendPrintf("%0x", aBytes[i]);
  }
  return NS_OK;
} 

Does that look right?

Comment 83

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

The signature should be (const SECItem*, nsACString &) and we should avoid using AppendPrintf because it is very inefficient for this kinds of task. I would just do the hex conversion manually if there's really no XPCOM function to do it.

Comment 84

[David Dahl :ddahl]

Attachment: Latest WIP, non-building

saving work

Comment 85

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

David, Ben and I decided to change lots of small things today and I'm going to just try to crank out those changes, based on your latest WIP patch, to get a complete implementation by the morning, due to some constraints on my availability later in the week that might make it difficult to keep reviewing patches.

Comment 86

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: WIP

This is a patch that applies on top of ddahl's latest WIP patch.

I noticed that the DSA domain parameters are wrong--they are only 512 bits, instead of 1024 bits. But, I could not find the official BrowserID domain parameters. What are they?

Although this looks like a pretty big change, it is primarily rearranging David's code and renaming things:

1. I moved everything to identity/
2. I combined all the *.h and *.cpp files into one file
3. I made many names shorter
4. I fixed some of the error handling.
5. I added some documentation to the IDL.
6. I made the hex encoding more efficient.
7. I changed the encoding of the DSA signature output from DER to PKCS#11 output format (two concatenated values).

There is still a little more work to do--in particular, I didn't do the work to get the tests David wrote up and running.

Comment 87

[David Dahl :ddahl]

(In reply to Brian Smith (:bsmith) from comment #85)
> David, Ben and I decided to change lots of small things today and I'm going
> to just try to crank out those changes, based on your latest WIP patch, to
> get a complete implementation by the morning, due to some constraints on my
> availability later in the week that might make it difficult to keep
> reviewing patches.

Sounds good. Thank you for doing this.

Comment 88

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Latest WIP

Ben, this is working, for some value of "working". We need to try to test the interoperability with the existing JS implementation.

Comment 89

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: WIP on Pine branch, tests do not pass

Ben, I got this building on Pine, and I even started the work on modifying Identity.jsm to use the native component for key generation and signing. BUT:

1. I got to the point where there were test failures in toolkit/identity/tests that I could not decipher, because I don't know all the code that is on top of the crypto component I created. I didn't want to leave pine burning so I am just posting the patch here.

2. I spent WAY too much time trying to get the identity/ module I created merged into toolkit/identity, but I couldn't figure out how to trick the build system into doing what I wanted, for some strange reason. So, the native crypto component is still in identity/ and we'll have to find somebody that knows the build system better to merge it into your existing module.

Comment 90

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Brian Smith (:bsmith) from comment #89)
> 1. I got to the point where there were test failures in
> toolkit/identity/tests that I could not decipher, because I don't know all
> the code that is on top of the crypto component I created. I didn't want to
> leave pine burning so I am just posting the patch here.

Tests in toolkit/identity/tests/unit now pass.

> 2. I spent WAY too much time trying to get the identity/ module I created
> merged into toolkit/identity, but I couldn't figure out how to trick the
> build system into doing what I wanted, for some strange reason. So, the
> native crypto component is still in identity/ and we'll have to find
> somebody that knows the build system better to merge it into your existing
> module.

Kept trying random things till it worked, basically.

I pushed these changes to pine.

Comment 91

[Ben Adida [:benadida]]

Comment on attachment 630851 [details] [diff] [review]
WIP on Pine branch, tests do not pass

need base10 serialization for the key components, otherwise looks good.

Comment 92

[Ben Adida [:benadida]]

never mind, no need for base10!

Also, given discussion with bsmith, this has been moved to toolkit/identity and is now covered by bug 753238.

Closing and pointing to Identity.jsm bug.