1299676 - introduce simple js DER decoding module

Status: RESOLVED FIXED

(Core :: Security: PSM, defect, P1)

Description

[Dana Keeler (she/her) (use needinfo) [:keeler]]

To decode certificates in js (see bug 1294897), we'll need a DER decoder. The intent of this bug is to introduce a DER decoding module written in js with the minimum functionality required to get the job done.

Comment 1

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Attachment: bug 1299676 - introduce der.jsm as a minimal js ASN.1 DER decoder

Review commit: https://reviewboard.mozilla.org/r/75842/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/75842/

Comment 2

[J.C. Jones [:jcj] (he/they)]

Hey Keeler,

I'm afraid I won't be able to review this until the weekend; sorry!

Comment 3

[:Cykesiopka]

Comment on attachment 8786998 [details]
bug 1299676 - introduce der.jsm as a minimal js ASN.1 DER decoder

https://reviewboard.mozilla.org/r/75842/#review74808

Apologies for the delay.
This looks mostly good, but the comments I added below seem to warrant another look.

::: security/manager/ssl/der.jsm:62
(Diff revision 1)
> +  constructor(unusedBits, contents) {
> +    this._unusedBits = unusedBits;
> +    this._contents = contents;
> +  }
> +
> +  get unusedBits() {

We should probably add JSDocs to at least all public functions to document what the expected types and return values are (I for one am definitely going to forgot most of this a month from now - curse my poor memory).

::: security/manager/ssl/der.jsm:87
(Diff revision 1)
> +
> +  atEnd() {
> +    return this._cursor == this._bytes.length;
> +  }
> +
> +  readByte() {

This doesn't appear to have a direct test case - maybe one should be added since this is a public function?

::: security/manager/ssl/der.jsm:130
(Diff revision 1)
> +      return length;
> +    }
> +    throw new Error(ERROR_UNSUPPORTED_LENGTH);
> +  }
> +
> +  readBytes(length) {

Same as readByte() - consider adding a direct test case.

::: security/manager/ssl/der.jsm:132
(Diff revision 1)
> +    throw new Error(ERROR_UNSUPPORTED_LENGTH);
> +  }
> +
> +  readBytes(length) {
> +    if (length < 0) {
> +      throw new Error(ERROR_UNSUPPORTED_LENGTH);

Maybe ERROR_INVALID_LENGTH would be more appropriate?

::: security/manager/ssl/tests/unit/test_der.js:8
(Diff revision 1)
> +
> +"use strict";
> +
> +// Tests der.jsm functionality.
> +
> +var { DER } = Cu.import("resource://gre/modules/psm/der.jsm", {});

Naming nit: I *think* most JSMs that export a single symbol name export that symbol with the same case as the JSM file name.
So maybe it's better for the file to be named DER.jsm instead.

::: security/manager/ssl/tests/unit/test_der.js:74
(Diff revision 1)
> +  // A BIT STRING can't be just 1 byte. If it were 0 bytes, the length tag would
> +  // be 0x00 and there would be nothing following. The unused bits field is
> +  // mandatory, so there must always be at least two bytes.

Hmm, I'm not really following (maybe I'm reading the wrong reference for encoding bit strings, or I'm reading it wrong) - isn't the minimum length for the contents of a bit string 1, for the case of an empty bit string?

Comment 4

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8786998 [details]
bug 1299676 - introduce der.jsm as a minimal js ASN.1 DER decoder

https://reviewboard.mozilla.org/r/75842/#review74808

Thanks!
No worries about time - if I ask for a review near the weekend I'm not really expecting a response until Monday or Tuesday (particularly if Monday is a holiday in some parts). If any particular review is time-sensitive I'll let you know.

> We should probably add JSDocs to at least all public functions to document what the expected types and return values are (I for one am definitely going to forgot most of this a month from now - curse my poor memory).

Yep - good idea.

> This doesn't appear to have a direct test case - maybe one should be added since this is a public function?

Good catch.

> Same as readByte() - consider adding a direct test case.

Yep.

> Maybe ERROR_INVALID_LENGTH would be more appropriate?

Sounds good.

> Naming nit: I *think* most JSMs that export a single symbol name export that symbol with the same case as the JSM file name.
> So maybe it's better for the file to be named DER.jsm instead.

Ok - sounds good. Should test_der.js still be test_der or should it be test_DER?

> Hmm, I'm not really following (maybe I'm reading the wrong reference for encoding bit strings, or I'm reading it wrong) - isn't the minimum length for the contents of a bit string 1, for the case of an empty bit string?

I think I was just wrong here. Reading http://luca.ntop.org/Teaching/Appunti/asn1.html again, I think the minimal BIT STRING is 03 01 00 (zero padding bits, zero bytes of value).

Comment 5

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8786998 [details]
bug 1299676 - introduce der.jsm as a minimal js ASN.1 DER decoder

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/75842/diff/1-2/

Comment 6

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Hmmm - looks like reviewboard interdiff doesn't handle renames that well :/

Comment 7

[:Cykesiopka]

Comment on attachment 8786998 [details]
bug 1299676 - introduce der.jsm as a minimal js ASN.1 DER decoder

https://reviewboard.mozilla.org/r/75842/#review74808

> Ok - sounds good. Should test_der.js still be test_der or should it be test_DER?

I think leaving it as test_der.js is fine.

> I think I was just wrong here. Reading http://luca.ntop.org/Teaching/Appunti/asn1.html again, I think the minimal BIT STRING is 03 01 00 (zero padding bits, zero bytes of value).

OK, good to know.

Comment 8

[:Cykesiopka]

Comment on attachment 8786998 [details]
bug 1299676 - introduce der.jsm as a minimal js ASN.1 DER decoder

https://reviewboard.mozilla.org/r/75842/#review76216

Looks good!

::: security/manager/ssl/tests/unit/test_der.js:15
(Diff revisions 1 - 2)
>  function run_simple_tests() {
> +  throws(() => new DER.DER("this is not an array"), /invalid input/,
> +         "should throw given non-array input");
> +  throws(() => new DER.DER([0, "invalid input", 1]), /invalid input/,
> +         "should throw given non-byte data (string case)");
> +  throws(() => new DER.DER([31, 1, new Object()]), /invalid input/,

Nit: Use {} notation.

::: security/manager/ssl/tests/unit/test_der.js:147
(Diff revisions 1 - 2)
> +       "minimal BIT STRING should have 0 unused bits");
> +  equal(minimalBitstring.contents.length, 0,
> +        "minimal BIT STRING should have empty contents");
> +
> +  // However, a BIT STRING with zero bytes of content can't have any padding,
> +  // beacuse that makes no sense.

Typo: beacuse.

Comment 9

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8786998 [details]
bug 1299676 - introduce der.jsm as a minimal js ASN.1 DER decoder

https://reviewboard.mozilla.org/r/75842/#review76510

Looks good as a decoder. It wouldn't be too hard to richen the error reporting mechanisms in this when that's necessary, so r+.

::: security/manager/ssl/DER.jsm:99
(Diff revision 2)
> +    }
> +    // Reject inputs containing non-integer values or values too small or large.
> +    if (bytes.some(b => !Number.isInteger(b) || b < 0 || b > 255)) {
> +      throw new Error(ERROR_INVALID_INPUT);
> +    }
> +    this._bytes = bytes;

Since this only supports input lengths up to 65539, should we check `bytes`' length here?

Comment 10

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8786998 [details]
bug 1299676 - introduce der.jsm as a minimal js ASN.1 DER decoder

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/75842/diff/2-3/

Comment 11

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Thanks!
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a3a621417f7

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6abd2090658b
introduce der.jsm as a minimal js ASN.1 DER decoder r=Cykesiopka,jcj

Comment 13

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Shoot - I just realized the commit message says der.jsm as opposed to DER.jsm. Oh well. If it bounces I'll fix that.

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6abd2090658b