Implement native BrowserID assertion verification

RESOLVED WONTFIX

Status

()

Core
Identity
RESOLVED WONTFIX
6 years ago
2 years ago

People

(Reporter: benadida, Unassigned)

Tracking

unspecified
mozilla16
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:-)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

6 years ago
will be useful for WebRTC.
Please file the WebRTC + BrowserID bug and mark this bug as blocking it.
Assignee: nobody → bsmith
Summary: implement native verification → Implement native BrowserID assertion verification
Target Milestone: --- → mozilla16
Created attachment 641312 [details] [diff] [review]
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.
Created attachment 643505 [details] [diff] [review]
Patch

See if this works for you.
Attachment #641312 - Attachment is obsolete: true
Attachment #643505 - Flags: feedback?(ekr)
Attachment #643505 - Flags: feedback?(benadida)
Created attachment 643514 [details] [diff] [review]
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.
Attachment #643505 - Attachment is obsolete: true
Attachment #643505 - Flags: feedback?(ekr)
Attachment #643505 - Flags: feedback?(benadida)
Attachment #643514 - Flags: feedback?(ekr)
Attachment #643514 - Flags: feedback?(benadida)

Comment 5

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

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

Comment 7

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

high-level, this looks good. I leave C++ details to ekr.
Attachment #643514 - Flags: feedback?(benadida) → feedback+
Blocks: 783105
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?
Attachment #643514 - Flags: review?(rrelyea)
Attachment #643514 - Flags: review?(dolske)
(Reporter)

Comment 9

5 years ago
ping for Justin, Bob: any chance we could get this reviewed ASAP? It's important for WebRTC.
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.
Attachment #643514 - Flags: review?(dolske) → review-
(Reporter)

Updated

5 years ago
blocking-basecamp: --- → -
(Reporter)

Updated

5 years ago
Duplicate of this bug: 783105
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?
Right. Much lower priority than bug 790517
Can we revisit finishing this?
Depends on: 804663
Created attachment 674287 [details] [diff] [review]
WIP: Implement native BrowserID assertion verification

Not quite sure what the state of this is w.r.t. addressing review comments.
Attachment #643514 - Attachment is obsolete: true
Attachment #643514 - Flags: review?(rrelyea)
Attachment #643514 - Flags: feedback?(ekr)
Created attachment 692684 [details] [diff] [review]
Implement native BrowserID assertion verification [vWhatever]

Try run:
https://tbpl.mozilla.org/?tree=Try&rev=7a6504f6d799
Attachment #674287 - Attachment is obsolete: true
Created attachment 692685 [details] [diff] [review]
Implement native BrowserID assertion verification [vWhatever]
Attachment #692684 - Attachment is obsolete: true
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.
Attachment #692685 - Flags: review?(ekr)
(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.
Flags: needinfo?(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.
Flags: needinfo?(dolske)
Hi, all.  What's the current status of this patch?  Bug 878941 is an RTC IdP proxy that wants a native verifier.
Blocks: 878941
The patch is still waiting for review.

Also, I'm not expecting to have time to work on this in the immediate future.
Assignee: brian → nobody

Updated

4 years ago
No longer blocks: 878941
Attachment #692685 - Flags: review?(ekr)
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.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.