769856 - Implement native BrowserID assertion verification

Status: RESOLVED WONTFIX

(Core Graveyard :: Identity, defect)

Description

[Ben Adida [:benadida]]

will be useful for WebRTC.

Comment 1

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

Please file the WebRTC + BrowserID bug and mark this bug as blocking it.

Comment 2

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

Attachment: WIP

EKR suggested that we factor out a nsIIdentityPublicKey interface from nsIIdentityKeyPair, which would have the hex* attributes and the verify() function. Presumably nsIIdentityKeyPair would become nsIIdentityPrivateKey and would get a new publicKey attribute.

While I think that makes perfect sense, I think it is simpler to just add the following method to nsIIdentityCryptoService:

   void verifyDSASignature(in ACString & p,
                           in ACString & q,
                           in ACString & g,
                           in ACString & y,
                           in ACString & data,
                           in ACString & signature,
                           in nsIIdentityVerifyCallback callback);

Reasoning: If we create an nsIIdentityPublicKey interface then we'd have to add the following method to nsIIdentityCryptoService:

   nsIIdentityPublicKey createDSAPublicKey(
                           in ACString & p,
                           in ACString & q,
                           in ACString & g,
                           in ACString & y);

to be called like this:

    ics.verifyDSASignature(p, q, g, y, 

Then the JS code would look like this:

    publicKey = ics.createDSAPublicKey(p, q, g, y);
    publicKey.verify(data, signature, function() { ... });

But, since that is what we'll always be doing (AFAICT), we might as well just combine the two steps together into the verifyDSASignature function I proposed above.

Here's a patch I started yesterday but it isn't complete. Will finish it off soon.

Comment 3

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

Attachment: Patch

See if this works for you.

Comment 4

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

Attachment: Implement BrowserID assertion signature verification

Note that I changed the tests so that, after we generate a signature, we verify the signature. I don't know if there's a bug on file for addressing that TODO.

Comment 5

[Eric Rescorla (:ekr)]

Comment on attachment 643514 [details] [diff] [review]
Implement BrowserID assertion signature verification

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

lgtm, but youmay want to have someone who knows NSS better review it.

::: toolkit/identity/nsIIdentityCryptoService.idl
@@ +113,5 @@
>  interface nsIIdentitySignCallback : nsISupports
>  {
>    /** On success, base64urlSignature is the base-64-URL-encoded signature
>     *
> +   * For RS256 signatures, the signature is in PKCS#1 format.

Which PKCS#1? 1.5?

Comment 6

[Eric Rescorla (:ekr)]

Comment on attachment 643514 [details] [diff] [review]
Implement BrowserID assertion signature verification

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

lgtm, but youmay want to have someone who knows NSS better review it.

::: toolkit/identity/nsIIdentityCryptoService.idl
@@ +113,5 @@
>  interface nsIIdentitySignCallback : nsISupports
>  {
>    /** On success, base64urlSignature is the base-64-URL-encoded signature
>     *
> +   * For RS256 signatures, the signature is in PKCS#1 format.

Which PKCS#1? 1.5?

::: toolkit/identity/tests/unit/test_crypto_service.js
@@ +40,5 @@
> +        keyPair.hexDSAPrime,
> +        keyPair.hexDSASubPrime,
> +        keyPair.hexDSAGenerator,
> +        keyPair.hexDSAPublicValue,
> +        function dsaVerifyCallback(rv) {

Would it make sense to add unit tests that tested that damaged signatures didn't verify

Comment 7

[Ben Adida [:benadida]]

Comment on attachment 643514 [details] [diff] [review]
Implement BrowserID assertion signature verification

high-level, this looks good. I leave C++ details to ekr.

Comment 8

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

Comment on attachment 643514 [details] [diff] [review]
Implement BrowserID assertion signature verification

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

Justin, could you please review this patch?

Bob, could you please review the crypto-related parts of IdentityCryptoService.cpp (basically, the second half of changes to that file).

I agree it seems like it would be nice to have a negative test (a test where the signature does not verify). I am not sure how the BrowserID team has been doing such tests. Any pointers?

Comment 9

[Ben Adida [:benadida]]

ping for Justin, Bob: any chance we could get this reviewed ASAP? It's important for WebRTC.

Comment 10

[Justin Dolske [:Dolske]]

Comment on attachment 643514 [details] [diff] [review]
Implement BrowserID assertion signature verification

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

Generally looks fine with minor comments.

But r- since you need to update types for our glorious new post- bug 579517 world.

::: toolkit/identity/IdentityCryptoService.cpp
@@ +28,5 @@
> +PRUint8*
> +const_byte_cast(const char * p)
> +{
> +  return const_cast<PRUint8*>(reinterpret_cast<const PRUint8*>(p));
> +}

Man, I love working in JS. :P

@@ +62,5 @@
> +    case 'B': case 'b': *digit = 0xBu; return NS_OK;
> +    case 'C': case 'c': *digit = 0xCu; return NS_OK;
> +    case 'D': case 'd': *digit = 0xDu; return NS_OK;
> +    case 'E': case 'e': *digit = 0xEu; return NS_OK;
> +    case 'F': case 'f': *digit = 0xFu; return NS_OK;

Well, that's certainly simple. :D

@@ +72,5 @@
> +nsresult
> +HexDecodeKeyPart(PLArenaPool * arena, const nsACString & hexPart,
> +                 SECItem * part)
> +{
> +  NS_ENSURE_ARG(hexPart.Length() % 2 == 0);

Not sure if NS_ENSURE_ARG is typically used quite this way, but meh.

@@ +74,5 @@
> +                 SECItem * part)
> +{
> +  NS_ENSURE_ARG(hexPart.Length() % 2 == 0);
> +  NS_ENSURE_ARG(hexPart.Length() > 0);
> +  NS_ENSURE_ARG(hexPart.Length() < 16384 / 8/*bits*/ * 2/*hex-digits-per-byte*/);

I'd put the inline block comments on their own separate line, expecially since the operators here are / and *!

@@ +80,5 @@
> +  if (!SECITEM_AllocItem(arena, part, hexPart.Length() / 2)) {
> +    NS_RUNTIMEABORT("out of memory");
> +  }
> +
> +  const char * hexDigits = hexPart.BeginReading();

.EndReading()?

@@ +120,5 @@
>  
> +nsresult
> +Base64UrlDecodeImpl(const nsACString & base64UrlEncoded, nsACString & result)
> +{
> +  nsCString base64Encoded(base64UrlEncoded);

Are these strings short? Might want to use nsAutoCString here...

@@ +122,5 @@
> +Base64UrlDecodeImpl(const nsACString & base64UrlEncoded, nsACString & result)
> +{
> +  nsCString base64Encoded(base64UrlEncoded);
> +
> +  nsACString::char_type * out = base64Encoded.BeginWriting();

Should there be a .EndWriting() call too?

@@ +129,5 @@
> +    if (out[i] == '-') {
> +      out[i] = '+';
> +    } else if (out[i] == '_') {
> +      out[i] = '/';
> +    }

Since you're iterating over the whole string anyway, I wonder if would be faster to avoid the initial duplication, and simply copy each character with conversion... OTOH, if they're short strings it won't matter, and if they're big strings it's probably better to have fast bulk memcpy (or whatever nsString's init does) + fixup. So fine!

@@ +222,5 @@
>  public:
> +  nsresult Dispatch()
> +  {
> +    nsCOMPtr<nsIThread> thread;
> +    return NS_NewThread(getter_AddRefs(thread), this);

Should't |thread| be a member of the class? Otherwise what's keeping the thread alive (refcounted) upon return?

@@ +228,3 @@
>  
> +protected:
> +  Worker() : mRv(NS_ERROR_NOT_INITIALIZED) { }

A bit of bikeshed: I wonder if "Worker" is such a good name, given that it already means other things (eg Web Workers).

@@ +412,5 @@
>    } else {
>      return NS_ERROR_UNEXPECTED;
>    }
>  
> +  nsCOMPtr<Worker> r = new KeyGenRunnable(keyType, callback);

Maybe replace the "Runnable" in the name (here and others) with "Worker" (or whatever)? It's a little weird to call it a runnable, because it's not really used like a normal runnable now... It's a background thread thing that just happens to use a runnable to post back to the main thread.

@@ +758,5 @@
>  }
>  
> +namespace {
> +
> +} // unnamed namespace

O_o

What's this do?

@@ +769,4 @@
>  
> +  SECItem sig = { siBuffer, NULL, 0 };
> +  int sigLength = PK11_SignatureLen(mPrivateKey);
> +  nsresult rv;

Move |rv| decl down closer to where it's first used?

@@ +779,3 @@
>    }
>  
> +  PRUint8 hash[32]; // big enough for SHA-1 or SHA-256

PRUint8? Hey, didn't we... Oh dear. This patch predates the stdint conversions done in bug 579517. You're going to have some fixin' to do!

::: toolkit/identity/nsIIdentityCryptoService.idl
@@ +55,5 @@
> +                          in nsIIdentityVerifyCallback callback);
> +
> +  // Verifies an RSA signature of the form generated by nsIIdentityKeyPair.sign,
> +  // where the DSA public key is of the form returned by
> +  // nsIIdentityKeyPair.hexDSA*. 

Trailing space. r-.

<3 Bugzilla's whitespace hilighting...

@@ +113,5 @@
>  interface nsIIdentitySignCallback : nsISupports
>  {
>    /** On success, base64urlSignature is the base-64-URL-encoded signature
>     *
> +   * For RS256 signatures, the signature is in PKCS#1 format.

Either keep the "XXX bug 769858" note, or mark that one fixed/duped if it's now supported.

@@ +130,5 @@
> +interface nsIIdentityVerifyCallback : nsISupports
> +{
> +  /** Cu.isSuccessCode(rv) on success
> +   */
> +  void verifyFinished(in nsresult rv);

I wonder a bit if this is a footgun... EG someone doesn't read the docs, and assumes a call to verifyFinished means everything is hunky-dory.

But throwing would be inconsistent for this API. And having, say, verifyFinished and verifyFailed methods would be ugly for callers. So not sure if there's a great footgun remedy here.

I'm tempted to suggest "verifyFinished(in nsresult rv, in boolean signatureOk)". But now you've got API weirdness -- does a caller need to deal with rv==fail,sigOK==true? What would that even mean? rv==success,sigOK==false could be seen as confusing too. :/

::: toolkit/identity/tests/unit/test_crypto_service.js
@@ +40,5 @@
> +        keyPair.hexDSAPrime,
> +        keyPair.hexDSASubPrime,
> +        keyPair.hexDSAGenerator,
> +        keyPair.hexDSAPublicValue,
> +        function dsaVerifyCallback(rv) {

Yeah (re:ekr), we should definitely have some verify-failed coverage. Might want to throw some random edgecasy stuff at it too (null, empty-string, etc) since this coming from sites.

Also to take the signature that did verify, mangle it a bit, and ensure it fails.

Comment 12

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

Based on https://tbpl.mozilla.org/?tree=Try&rev=830009a4ac7b I am assuming that this isn't necessary for the initial WebRTC landing. Is that correct?

Comment 13

[Eric Rescorla (:ekr)]

Right. Much lower priority than bug 790517

Comment 14

[Eric Rescorla (:ekr)]

Can we revisit finishing this?

Comment 15

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

Attachment: WIP: Implement native BrowserID assertion verification

Not quite sure what the state of this is w.r.t. addressing review comments.

Comment 16

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

Attachment: Implement native BrowserID assertion verification [vWhatever]

Try run:
https://tbpl.mozilla.org/?tree=Try&rev=7a6504f6d799

Comment 17

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

Attachment: Implement native BrowserID assertion verification [vWhatever]

Comment 18

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

Comment on attachment 692685 [details] [diff] [review]
Implement native BrowserID assertion verification [vWhatever]

I discussed with benadida the dearth of tests for this, and we agreed that somebody on the identity team could implement the tests that are missing. So, after this code review is done, I will let Ben assign it to somebody else, so that it doesn't remain blocked on me.

Comment 19

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

(In reply to Justin Dolske [:Dolske] from comment #10)
> > +  const char * hexDigits = hexPart.BeginReading();
> 
> .EndReading()?

> Should there be a .EndWriting() call too?

I admit I am not sure if EndReading or EndWRiting is necessary or why. Is it? And, why?

> @@ +222,5 @@
> >  public:
> > +  nsresult Dispatch()
> > +  {
> > +    nsCOMPtr<nsIThread> thread;
> > +    return NS_NewThread(getter_AddRefs(thread), this);
> 
> Should't |thread| be a member of the class? Otherwise what's keeping the
> thread alive (refcounted) upon return?

This code got moved to security/manager/ssl/src/CryptoTask, so it isn't in the current patch any more. But, it is still important to answer:

I had assumed that the thread manager will maintain a reference to a thread as long as it's running? If not, there's a serious bug in the CryptoTask code.

> A bit of bikeshed: I wonder if "Worker" is such a good name, given that it
> already means other things (eg Web Workers).

This was renamed to CryptoTask.

> I wonder a bit if this is a footgun... EG someone doesn't read the docs, and
> assumes a call to verifyFinished means everything is hunky-dory.

Now that I've written some actual JS code, I see that JS code tends to use the onSuccess/onFailure pattern but Necko (e.g. NetUtils.asyncFetch) uses the pattern I use. If people want me to change this to onSuccess/onFailure I can, but I think it is overkill because we don't expect anybody to be using this except identity.jsm and related code.

Comment 20

[Justin Dolske [:Dolske]]

(In reply to Brian Smith (:bsmith) from comment #19)

> Now that I've written some actual JS code, I see that JS code tends to use
> the onSuccess/onFailure pattern but Necko (e.g. NetUtils.asyncFetch) uses
> the pattern I use. If people want me to change this to onSuccess/onFailure I
> can, but I think it is overkill because we don't expect anybody to be using
> this except identity.jsm and related code.

I think you're probably fine, per my earlier comment I don't think there's any great solution here.

I don't know the answer to the .EndWriting() / nsIThread bits. Just called them out as something to look at, probably by someone who works more regularly in the C++ side of Gecko.

Comment 21

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Hi, all.  What's the current status of this patch?  Bug 878941 is an RTC IdP proxy that wants a native verifier.

Comment 22

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

The patch is still waiting for review.

Also, I'm not expecting to have time to work on this in the immediate future.

Comment 23

[Ryan Kelly [:rfkelly]]

I'm WONTFIXing persona-related bugs now that we've committed to decommissioning in the persona.or service.  While we still use BrowserID format assertions in Firefox Accounts, I don't see any non-persona-related use-case for this particular functionality.