Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Design and implement crypto API for Mozilla ID

RESOLVED DUPLICATE of bug 753238

Status

()

Core
Security: PSM
RESOLVED DUPLICATE of bug 753238
6 years ago
5 years ago

People

(Reporter: briansmith, Assigned: briansmith)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-kilimanjaro:+, blocking-basecamp:+)

Details

Attachments

(1 attachment, 45 obsolete attachments)

84.68 KB, patch
benadida
: feedback-
Details | Diff | Splinter Review
+++ 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

6 years ago
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

Updated

6 years ago
Blocks: 669927

Comment 2

6 years ago
(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

6 years ago
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?

Updated

6 years ago
Assignee: nobody → ddahl
Keywords: dev-doc-needed

Comment 4

6 years ago
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

6 years ago
Created attachment 561918 [details] [diff] [review]
v 0.1 WIP untested

saving work

Comment 6

6 years ago
(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

6 years ago
Created attachment 562177 [details] [diff] [review]
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.
Attachment #561918 - Attachment is obsolete: true

Comment 8

6 years ago
(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

6 years ago
Created attachment 563223 [details] [diff] [review]
v 0.3 WIP adding a new class and interface to abstract the keys

Untested, saving work
Attachment #562177 - Attachment is obsolete: true

Comment 10

6 years ago
Created attachment 563895 [details] [diff] [review]
v 0.4 WIP - coming together

Created nsBrowserIDService to hand out nsIBrowserIDKeypair instances when needed, started getting nsIBrowserIDKeypair class implemented
Attachment #563223 - Attachment is obsolete: true

Comment 11

6 years ago
Created attachment 564737 [details] [diff] [review]
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.
Attachment #563895 - Attachment is obsolete: true
Attachment #564737 - Flags: feedback?(bsmith)

Updated

6 years ago
Blocks: 692500

Comment 12

6 years ago
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

6 years ago
Created attachment 565648 [details] [diff] [review]
v 0.6 Saving my work
Attachment #564737 - Attachment is obsolete: true
Attachment #564737 - Flags: feedback?(bsmith)

Comment 14

6 years ago
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 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

6 years ago
Created attachment 568803 [details] [diff] [review]
v 7 saving work, renamed everything, moving stuff into services/identity

Still needs more clean up before it will build cleanly
Attachment #565648 - Attachment is obsolete: true

Comment 17

6 years ago
Created attachment 569848 [details] [diff] [review]
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.
Attachment #568803 - Attachment is obsolete: true
Attachment #569848 - Flags: feedback?(bsmith)

Comment 18

6 years ago
Created attachment 569851 [details]
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

6 years ago
Created attachment 570390 [details] [diff] [review]
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.
Attachment #569848 - Attachment is obsolete: true
Attachment #569851 - Attachment is obsolete: true
Attachment #569848 - Flags: feedback?(bsmith)
Attachment #570390 - Flags: review?(bsmith)

Comment 20

6 years ago
Created attachment 570394 [details]
debugging Sign() output from gdb

Comment 21

6 years ago
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

6 years ago
Created attachment 570887 [details] [diff] [review]
v 10 in review with bsmith
Attachment #570390 - Attachment is obsolete: true
Attachment #570394 - Attachment is obsolete: true
Attachment #570390 - Flags: review?(bsmith)
Attachment #570887 - Flags: review?(bsmith)

Comment 23

6 years ago
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

6 years ago
Created attachment 572120 [details] [diff] [review]
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
Attachment #570887 - Attachment is obsolete: true
Attachment #570887 - Flags: review?(bsmith)
Attachment #572120 - Flags: review?(bsmith)

Comment 25

6 years ago
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/ddahl@mozilla.com-e79df42cf106

Comment 26

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

Comment 27

6 years ago
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

6 years ago
Created attachment 586228 [details]
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.
Attachment #586228 - Flags: feedback?(bsmith)

Updated

6 years ago
Attachment #586228 - Flags: feedback?(jseward)

Comment 29

6 years ago
Created attachment 586247 [details]
Valgrind output when test_id_service_keypair.js is run

Updated the output with VALGRIND_OPTS=--smc-check=all
Attachment #586228 - Attachment is obsolete: true
Attachment #586228 - Flags: feedback?(jseward)
Attachment #586228 - Flags: feedback?(bsmith)

Comment 30

6 years ago
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

Updated

6 years ago
Attachment #586247 - Flags: feedback?(bsmith)

Comment 31

6 years ago
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 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

6 years ago
Created attachment 586334 [details] [diff] [review]
v 12 no more crashy crashy

Ok, khuey figured out that I was missing an NS_ADDREF. yay.
Attachment #572120 - Attachment is obsolete: true
Attachment #586247 - Attachment is obsolete: true
Attachment #572120 - Flags: review?(bsmith)
Attachment #586247 - Flags: feedback?(bsmith)
Attachment #586334 - Flags: review?(bsmith)
(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

6 years ago
Created attachment 586432 [details]
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
Attachment #586432 - Flags: feedback?(jseward)

Comment 36

6 years ago
filed bug 715917, bug 715908, bug 715912
(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.
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

6 years ago
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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

6 years ago
Created attachment 589330 [details] [diff] [review]
v 13 Adding DSA keyType

Saving work, still need tests for DSA keys and signing
Attachment #586334 - Attachment is obsolete: true
Attachment #586334 - Flags: review?(bsmith)

Comment 41

6 years ago
Created attachment 589682 [details] [diff] [review]
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
Attachment #589330 - Attachment is obsolete: true
Attachment #589682 - Flags: feedback?(bsmith)
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

6 years ago
(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

6 years ago
Created attachment 589958 [details] [diff] [review]
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);
Attachment #589682 - Attachment is obsolete: true
Attachment #589682 - Flags: feedback?(bsmith)
Attachment #589958 - Flags: feedback?(bsmith)

Comment 45

6 years ago
Created attachment 590366 [details]
backtrace

DSA key gen fails
Attachment #590366 - Flags: feedback?(bsmith)

Comment 46

6 years ago
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

6 years ago
Created attachment 590387 [details]
backtrace

updated backtrace, attaching latest patch as well
Attachment #590366 - Attachment is obsolete: true
Attachment #590366 - Flags: feedback?(bsmith)
Attachment #590387 - Flags: feedback?(bsmith)

Comment 48

6 years ago
Created attachment 590388 [details] [diff] [review]
v 16 latest with keygen failure
Attachment #589958 - Attachment is obsolete: true
Attachment #589958 - Flags: feedback?(bsmith)
Attachment #590388 - Flags: feedback?(bsmith)
(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

6 years ago
Created attachment 590926 [details] [diff] [review]
v 16.2 Latest

pqg probs
Attachment #590388 - Attachment is obsolete: true
Attachment #590388 - Flags: feedback?(bsmith)
Attachment #590926 - Flags: feedback?(bsmith)
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.
Created attachment 590963 [details] [diff] [review]
correct dsa test data

Comment 53

6 years ago
Created attachment 590983 [details] [diff] [review]
v 17 RSA, DSA keypair and signature support with tests
Attachment #590387 - Attachment is obsolete: true
Attachment #590926 - Attachment is obsolete: true
Attachment #590963 - Attachment is obsolete: true
Attachment #590387 - Flags: feedback?(bsmith)
Attachment #590926 - Flags: feedback?(bsmith)

Comment 54

6 years ago
Created attachment 592293 [details] [diff] [review]
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.
Attachment #586432 - Attachment is obsolete: true
Attachment #590983 - Attachment is obsolete: true
Attachment #586432 - Flags: feedback?(jseward)

Comment 55

6 years ago
Created attachment 595958 [details] [diff] [review]
v 19 navigator.id works, .get returns properties of the pub key
Attachment #592293 - Attachment is obsolete: true

Updated

6 years ago
Blocks: 664541

Comment 56

6 years ago
Created attachment 600465 [details] [diff] [review]
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
Attachment #595958 - Attachment is obsolete: true

Comment 57

5 years ago
Created attachment 610358 [details] [diff] [review]
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?
Attachment #600465 - Attachment is obsolete: true
Attachment #610358 - Flags: feedback?(khuey)

Comment 58

5 years ago
Created attachment 610741 [details] [diff] [review]
Created separate source files, added tests for entire interface
Attachment #610358 - Attachment is obsolete: true
Attachment #610358 - Flags: feedback?(khuey)
Attachment #610741 - Flags: review?(bsmith)

Comment 59

5 years ago
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.

Updated

5 years ago
Blocks: 742380

Comment 60

5 years ago
Created attachment 612414 [details] [diff] [review]
Added a keygen runnable

The idls are interdependent. I must be designing this wrong.
Attachment #612414 - Flags: feedback?(bsmith)
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

5 years ago
(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

5 years ago
Created attachment 612649 [details] [diff] [review]
generated header problem
Attachment #612414 - Attachment is obsolete: true
Attachment #612414 - Flags: feedback?(bsmith)
Attachment #612649 - Flags: feedback?(bsmith)
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.
Attachment #612649 - Flags: feedback?(bsmith)

Comment 65

5 years ago
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

5 years ago
Created attachment 613394 [details] [diff] [review]
vtable linking error

/home/ddahl/code/moz/mozilla-valgrind/src/security/manager/ssl/src/nsIdentityServiceKeyPairGenRunnable.cpp:22: undefined reference to `vtable for nsIdentityServiceKeyPairGenRunnable'
Attachment #612649 - Attachment is obsolete: true
You need to implement:

    nsIdentityServiceKeyPairGenRunnable::virtualDestroyNSSReference(void)"

and 

    nsIdentityServiceKeyPairGenRunnable::~nsIdentityServiceKeyPairGenRunnable(void)" 

and the linker errors should go away.

Comment 68

5 years ago
(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

5 years ago
Created attachment 613645 [details] [diff] [review]
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);
    }
Attachment #610741 - Attachment is obsolete: true
Attachment #613394 - Attachment is obsolete: true
Attachment #610741 - Flags: review?(bsmith)
Attachment #613645 - Flags: feedback?(bsmith)

Comment 70

5 years ago
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

5 years ago
Created attachment 615445 [details] [diff] [review]
Latest patch

Comment 72

5 years ago
Created attachment 615543 [details] [diff] [review]
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!
Attachment #613645 - Attachment is obsolete: true
Attachment #615445 - Attachment is obsolete: true
Attachment #613645 - Flags: feedback?(bsmith)
Attachment #615543 - Flags: review?(bsmith)
Blocks: 753238

Comment 73

5 years ago
Created attachment 622614 [details] [diff] [review]
Patch tweaked for frontend hacking

a few tweaks
Attachment #615543 - Attachment is obsolete: true
Attachment #615543 - Flags: review?(bsmith)
Attachment #622614 - Flags: review?(bsmith)
Created attachment 626592 [details] [diff] [review]
Follow-up patch for infallible strings

Comment 75

5 years ago
(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

5 years ago
Created attachment 628158 [details] [diff] [review]
Lastest WIP, off main thread signature creation

WIP, tests will be working soon
Attachment #622614 - Attachment is obsolete: true
Attachment #626592 - Attachment is obsolete: true
Attachment #622614 - Flags: review?(bsmith)

Comment 77

5 years ago
Created attachment 628478 [details] [diff] [review]
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
Attachment #628158 - Attachment is obsolete: true
Attachment #628478 - Flags: feedback?(khuey)

Updated

5 years ago
Attachment #628478 - Flags: feedback?(khuey)
Attachment #628478 - Flags: feedback?(bsmith)
Attachment #628478 - Flags: feedback?(bobbyholley+bmo)

Comment 78

5 years ago
Created attachment 628529 [details] [diff] [review]
Latest Patch, off main thread signatures - all tests pass
Attachment #628478 - Attachment is obsolete: true
Attachment #628478 - Flags: feedback?(bsmith)
Attachment #628478 - Flags: feedback?(bobbyholley+bmo)
Attachment #628529 - Flags: review?(bsmith)
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?
Attachment #628529 - Flags: review?(bsmith)
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

5 years ago
(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

5 years ago
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?
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

5 years ago
Created attachment 630275 [details] [diff] [review]
Latest WIP, non-building

saving work
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.
Assignee: ddahl → bsmith
Created attachment 630493 [details] [diff] [review]
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

5 years ago
(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.
Created attachment 630688 [details] [diff] [review]
Latest WIP

Ben, this is working, for some value of "working". We need to try to test the interoperability with the existing JS implementation.
Attachment #628529 - Attachment is obsolete: true
Attachment #630275 - Attachment is obsolete: true
Attachment #630493 - Attachment is obsolete: true
Attachment #630688 - Flags: feedback?(benadida)
Created attachment 630851 [details] [diff] [review]
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.
Attachment #630688 - Attachment is obsolete: true
Attachment #630688 - Flags: feedback?(benadida)
Attachment #630851 - Flags: feedback?(benadida)
(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.

Updated

5 years ago
blocking-basecamp: --- → +
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.
Attachment #630851 - Flags: feedback?(benadida) → feedback-
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.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 753238
blocking-kilimanjaro: --- → +
Blocks: 769519
No longer blocks: 769519
No longer blocks: 753238
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.