972582 - jwcrypto: base64UrlEncoding/Decoding can alter assertion audience

Status: RESOLVED FIXED

(Core Graveyard :: Identity, defect)

Description

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

If you generateAssertion with an audience of "i-like-pie.com", the resulting assertion audience is "i+like+pie.com".

Test code to demonstrate the error:

  Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
  Components.utils.import("resource://gre/modules/identity/jwcrypto.jsm");

  XPCOMUtils.defineLazyServiceGetter(this,
                                     "CryptoService",
                                     "@mozilla.org/identity/crypto-service;1",
                                     "nsIIdentityCryptoService");
                                    
  let audience = "i-like-pie.com";
                                   
  jwcrypto.generateKeyPair("DS160", (err, kp) => {
    jwcrypto.generateAssertion("cert", kp, audience, (err, assertion) => {
      let [_, signedObject] = assertion.split('~');
      let parts = signedObject.split('.');
      let payload = JSON.parse(CryptoService.base64UrlDecode(parts[1]));
      
      // uh oh, we got "i+like+pie.com" back
      assert(payload.aud == audience);
    });
  });

Comment 1

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

I landed the broken patch 18 months ago (Bug 778817), so Nemesis demands that I take this bug and fix it.

Comment 2

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

Attachment: Fix base64UrlDecode in toolkit/identity

The culprit appears to be that the Base64UrlDecode function is doing the base64 decode *before* it swaps the '-' and '_' characters back.

I think :emk is correct in Bug 778817 Comment 13 that we should really just be using btoa and atob here.  These are the standard base64 encoders and decoders.

In this patch, I back out the base64UrlDecoder and simply substitute the use of atob in tests.  We hardly need to decode assertions on the platform anyway, apart from in tests.

I'd like to suggest that we remove the base64 encoder from IdentityCryptoService and that we just make a common js util for base64Url{encode,decode}.  (If you search mxr for base64url, you'll find that this functionality is duplicated all over the place.)

Comment 3

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

Attachment: Fix base64UrlDecode in toolkit/identity

Exported previous patch too soon; hg qrefresh was needed

Comment 4

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

Attachment: Fix base64UrlDecode in toolkit/identity

This patch essentially backs out Bug 778817.  That bug introduced a (broken) base64UrlDecode function into IdentityCryptoService.

Note that the decode function is only needed on the client for testing.  As :emk points out in Bug 778817 Comment 13, this is what atob() is for.

So the present patch:
- removes the broken base64UrlDecode
- restores the former uuid for the former idl contract
- uses atob for testing encode/decode
- adds test vectors for base64 encode/decode

I think a follow-up bug could propose to:
- remove base64UrlEncode from IdentityCryptoService
- create a toolkit jsm that does base64 encode/decode with optional padding
  - because this wheel is reinvented several times throughout the firefox code

MattN, what do you think of the approach for the current bug, and for the idea of a follow-up?

Thanks!
j

Comment 5

[Matthew N. [:MattN]]

Comment on attachment 8376564 [details] [diff] [review]
Fix base64UrlDecode in toolkit/identity

Works for me

Comment 6

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

Comment on attachment 8376564 [details] [diff] [review]
Fix base64UrlDecode in toolkit/identity

Thanks for your feedback, MattN.  Can I get your r=you for this, then?

Comment 7

[Matthew N. [:MattN]]

Comment on attachment 8376564 [details] [diff] [review]
Fix base64UrlDecode in toolkit/identity

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

::: toolkit/identity/nsIIdentityCryptoService.idl
@@ +37,5 @@
>   * http://tools.ietf.org/html/draft-ietf-jose-json-web-algorithms-02
>   */
>  
>  // "@mozilla.org/identity/crypto-service;1"
> +[scriptable, builtinclass, uuid(f087e6bc-dd33-4f6c-a106-dd786e052ee9)]

I don't know if it's a good idea to re-use the old uuid. It seems like it would be okay but I'm not sure. It's probably safer to just change the UUID IMO since I doubt there is much of a compatibility impact. Flagging Mossop for sr?

::: toolkit/identity/tests/unit/head_identity.js
@@ +74,5 @@
> +function base64UrlDecode(s) {
> +  s = s.replace(/-/g, '+');
> +  s = s.replace(/_/g, '/');
> +
> +  switch (s.length % 4) { // Pad with trailing '='s

Could you have someone more familiar with the RFCs review this function (if it came from existing reviewed Mozilla code, I'm not sure where it came from)

::: toolkit/identity/tests/unit/test_crypto_service.js
@@ +97,5 @@
> +}
> +
> +function test_base64UrlDecode() {
> +  let utf8Converter = Cc["@mozilla.org/intl/scriptableunicodeconverter"]
> +                         .createInstance(Ci.nsIScriptableUnicodeConverter);

Nit: it looks like this line is indented with 3 spaces instead of 2.

Comment 8

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

(In reply to Matthew N. [:MattN] from comment #7)
> Comment on attachment 8376564 [details] [diff] [review]
> Fix base64UrlDecode in toolkit/identity

Thanks as always for your review, Matthew

> Review of attachment 8376564 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/identity/nsIIdentityCryptoService.idl
> @@ +37,5 @@
> >   * http://tools.ietf.org/html/draft-ietf-jose-json-web-algorithms-02
> >   */
> >  
> >  // "@mozilla.org/identity/crypto-service;1"
> > +[scriptable, builtinclass, uuid(f087e6bc-dd33-4f6c-a106-dd786e052ee9)]
> 
> I don't know if it's a good idea to re-use the old uuid. It seems like it
> would be okay but I'm not sure. It's probably safer to just change the UUID
> IMO since I doubt there is much of a compatibility impact. Flagging Mossop
> for sr?

I wasn't certain.  I thought that since this restores the previous interface, which used that uuid, that it would be best, since it would still be binary compatible.  Curious to hear what the verdict is on this.

> ::: toolkit/identity/tests/unit/head_identity.js
> @@ +74,5 @@
> > +function base64UrlDecode(s) {
> > +  s = s.replace(/-/g, '+');
> > +  s = s.replace(/_/g, '/');
> > +
> > +  switch (s.length % 4) { // Pad with trailing '='s
> 
> Could you have someone more familiar with the RFCs review this function (if
> it came from existing reviewed Mozilla code, I'm not sure where it came from)

I'll look for a reviewer.

This code is taken straight from the javascript implementation of jwcrypto by Ben Adida and Lloyd Hilaiel.

> ::: toolkit/identity/tests/unit/test_crypto_service.js
> @@ +97,5 @@
> > +}
> > +
> > +function test_base64UrlDecode() {
> > +  let utf8Converter = Cc["@mozilla.org/intl/scriptableunicodeconverter"]
> > +                         .createInstance(Ci.nsIScriptableUnicodeConverter);
> 
> Nit: it looks like this line is indented with 3 spaces instead of 2.

Oops. Sorry.

Thanks for your help, Matthew.

Comment 9

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

Attachment: Fix base64UrlDecode in toolkit/identity

r=MattN

Brian, per Comment 7, does the implementation of base64UrlDecode in toolkit/identity/tests/unit/head_identity.js here look ok to you?  (It's copied from the all-JS jwcrypto).

Dave, thanks for your guidance re reverting to the former uuids (Comment 7)

Comment 10

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

Attachment: Fix base64UrlDecode in toolkit/identity

r=MattN; sr=mossop

Comment 11

[Brian Warner [:warner :bwarner]]

Comment on attachment 8378478 [details] [diff] [review]
Fix base64UrlDecode in toolkit/identity

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

r=warner with a comment or function-name change

::: toolkit/identity/tests/unit/head_identity.js
@@ +70,5 @@
>  function uuid() {
>    return uuidGenerator.generateUUID().toString();
>  }
>  
> +function base64UrlDecode(s) {

this function does indeed correctly transform padding-stripped URL-safed base64-encoded strings into their original binary form. http://tools.ietf.org/html/rfc4648#section-4 defines the padding rules.

It might be best to split it up into multiple functions, though, since the "Pad with trailing =" comment is confusing. Base64 encoding defines the padding process (binary -> ascii -> ascii+padding), so what this function is really doing is replacing some padding that was stripped out earlier (binary -> ascii -> ascii+padding -> ascii -> urlsafe_ascii).

Or, just change the comment ("replace padding that was stripped out by the sender"), or change the function name (unpaddedBase64UrlDecode), those might be simpler ways to reduce the confusion.

Comment 12

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

(In reply to Brian Warner [:warner :bwarner] from comment #11)
> Comment on attachment 8378478 [details] [diff] [review]
> Fix base64UrlDecode in toolkit/identity
> 
> Review of attachment 8378478 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=warner with a comment or function-name change

Thank you for your review, Brian, and for the thorough clarification.

I've updated the comments in the function.

I proposed in Comment 4 that we file a follow-up bug to consolidate the various base64 decoders that are scattered about the codebase.  I think that would be a good place to refactor further.

Cheers!
j

Comment 13

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

Attachment: Fix base64UrlDecode in toolkit/identity

r=MattN,warner; sr=mossop

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/953fda48369a

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/953fda48369a

Comment 16

[:Gavin Sharp [email: [e-mail]]]

Sounds like we need this uplifted to 29?

Comment 17

[Benjamin Kerensa [:bkerensa]]

Gavin, Could you request uplift?

Comment 18

[:Gavin Sharp [email: [e-mail]]]

That's Jed's job!

(ping if you want advice)

Comment 19

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

Comment on attachment 8378570 [details] [diff] [review]
Fix base64UrlDecode in toolkit/identity

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 778817
User impact if declined: 
  No user-facing impact,
  but may result in failing tests/builds if not accepted
Testing completed (on m-c, etc.): m-i and m-c
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none

Comment 20

[Matthew N. [:MattN]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/64631885216a