Closed Bug 972582 Opened 7 years ago Closed 7 years ago

jwcrypto: base64UrlEncoding/Decoding can alter assertion audience

Categories

(Core Graveyard :: Identity, defect)

defect
Not set
normal

Tracking

(firefox29+ fixed, firefox30 fixed)

RESOLVED FIXED
mozilla30
Tracking Status
firefox29 + fixed
firefox30 --- fixed

People

(Reporter: jedp, Assigned: jedp)

References

Details

Attachments

(1 file, 5 obsolete files)

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);
    });
  });
I landed the broken patch 18 months ago (Bug 778817), so Nemesis demands that I take this bug and fix it.
QA Contact: jparsons
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.)
Assignee: nobody → jparsons
Status: NEW → ASSIGNED
QA Contact: jparsons
Exported previous patch too soon; hg qrefresh was needed
Attachment #8375947 - Attachment is obsolete: true
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
Attachment #8375949 - Attachment is obsolete: true
Attachment #8376564 - Flags: feedback?(MattN+bmo)
Comment on attachment 8376564 [details] [diff] [review]
Fix base64UrlDecode in toolkit/identity

Works for me
Attachment #8376564 - Flags: feedback?(MattN+bmo) → feedback+
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?
Attachment #8376564 - Flags: review?(MattN+bmo)
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.
Attachment #8376564 - Flags: superreview?(dtownsend+bugmail)
Attachment #8376564 - Flags: review?(MattN+bmo)
Attachment #8376564 - Flags: review+
(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.
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)
Attachment #8376564 - Attachment is obsolete: true
Attachment #8376564 - Flags: superreview?(dtownsend+bugmail)
Attachment #8378415 - Flags: superreview?(dtownsend+bugmail)
Attachment #8378415 - Flags: review?(warner-bugzilla)
Attachment #8378415 - Flags: superreview?(dtownsend+bugmail) → superreview+
r=MattN; sr=mossop
Attachment #8378415 - Attachment is obsolete: true
Attachment #8378415 - Flags: review?(warner-bugzilla)
Attachment #8378478 - Flags: review?(warner-bugzilla)
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.
Attachment #8378478 - Flags: review?(warner-bugzilla) → review+
(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
r=MattN,warner; sr=mossop
Attachment #8378478 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/953fda48369a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Sounds like we need this uplifted to 29?
Gavin, Could you request uplift?
Flags: needinfo?(gavin.sharp)
That's Jed's job!

(ping if you want advice)
Flags: needinfo?(gavin.sharp)
Flags: needinfo?(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
Attachment #8378570 - Flags: approval-mozilla-aurora?
Flags: needinfo?(jparsons)
Attachment #8378570 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.