1304188 - introduce minimal js x509 certificate decoder

Status: RESOLVED FIXED

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

Description

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

As part of bug 1294897, we'll need to be able to decode x509 certificates. This bug aims to provide a minimal implementation (no extension support yet).

Comment 1

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

Attachment: bug 1304188 - introduce X509.jsm

This is mostly a preliminary review request, although I think everything that
should be done in this bug is present. This intentionally does not include
support for decoding extensions or subject public keys.

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

Comment 2

[:Cykesiopka]

Comment on attachment 8793957 [details]
bug 1304188 - introduce X509.jsm

https://reviewboard.mozilla.org/r/80534/#review80026

Looks good in general, but whether the implementations of ```parseOverride()``` are supposed to ignore some degree of decoding errors needs to be clarified.

::: security/manager/ssl/X509.jsm:25
(Diff revision 1)
> +class ByteArray {
> +  /**
> +   * @param {Number[]|DER} bytesOrDER the data to represent
> +   */
> +  constructor(bytesOrDER = []) {
> +    this._bytes = bytesOrDER instanceof DER.DER

Seems like a good idea to do the same sort of input checking as the ```DER.DER``` constructor.
Maybe we can cheat and just construct a ```DER.DER``` object to reuse the existing code.

::: security/manager/ssl/X509.jsm:26
(Diff revision 1)
> +  /**
> +   * @param {Number[]|DER} bytesOrDER the data to represent
> +   */
> +  constructor(bytesOrDER = []) {
> +    this._bytes = bytesOrDER instanceof DER.DER
> +                ? bytesOrDER.readBytes(bytesOrDER.getRemainingLength())

```getRemainingLength()``` doesn't appear to be defined anywhere (I guess it was missed in Bug 1299676).
We should add a test to ensure passing a DER.DER object works here, or just remove allowing a DER object to be passed.

::: security/manager/ssl/X509.jsm:49
(Diff revision 1)
> +  }
> +}
> +
> +/**
> + * Helper function to read a NULL tag from the given DER.
> + * @param {DER} a DER object to read a NULL from

@param {DER} der ...

::: security/manager/ssl/X509.jsm:72
(Diff revision 1)
> +  }
> +}
> +
> +/**
> + * Helper function to read an OBJECT IDENTIFIER from the given DER.
> + * @param {DER} the DER to read an OBJECT IDENTIFIER from

@param {DER} der ...

::: security/manager/ssl/X509.jsm:82
(Diff revision 1)
> +}
> +
> +/** Class representing an ASN.1 OBJECT IDENTIFIER */
> +class OID {
> +  /**
> +   * @param {Number[]} bytes the encoded bytes of the OBJECT IDENTIFIER

Could just be me, but I initially assumed "encoded bytes" meant the entire TLV.
Maybe "contents" or something?

::: security/manager/ssl/X509.jsm:87
(Diff revision 1)
> +   * @param {Number[]} bytes the encoded bytes of the OBJECT IDENTIFIER
> +   */
> +  constructor(bytes) {
> +    this._values = [];
> +    // First octet has value 40 * value1 + value2
> +    // Lint TODO: range checks on the input

I assume this means checking value 1 is [0, 2], and value 2 is [0, 39]?
(Although I haven't found anything saying value 2 is also restricted to [0, 39] if value 1 is 2...)

::: security/manager/ssl/X509.jsm:203
(Diff revision 1)
> +    this._tbsCertificate.parse(contents.readTLV());
> +    this._signatureAlgorithm.parse(contents.readTLV());

Hmm, if ```parse()``` fails the only thing that happens is the exception gets stashed, right?
Is the intention here (and elsewhere this pattern appears) to keep going and try to decode as much as possible?

If so, the ```DecodedDER.parseOverride()``` JSDoc probably should reflect this.

::: security/manager/ssl/X509.jsm:207
(Diff revision 1)
> +    if (signatureValue.unusedBits != 0) {
> +      throw new Error(ERROR_UNSUPPORTED_ASN1);
> +    }

Is this check here for similar reasons pointed out in
https://hg.mozilla.org/mozilla-central/file/c55bcb7c777ea09431b4d16903ed079ae5632648/security/pkix/lib/pkixder.cpp#l302 ?

::: security/manager/ssl/X509.jsm:549
(Diff revision 1)
> +    let s1 = this._validateDigit(contents.readByte());
> +    let s2 = this._validateDigit(contents.readByte());
> +    let second = s1 * 10 + s2;
> +
> +    let z = contents.readByte();
> +    if (z != 'Z'.charCodeAt(0)) {

Nit: Let's use double quotes here and below.

::: security/manager/ssl/tests/unit/test_x509.js:52
(Diff revision 1)
> +        new Date("2014-11-27T00:00:00.000Z").getTime(),
> +        "default-ee.pem should have the correct value for notBefore");
> +  equal(certificate.tbsCertificate.validity.notAfter.time.getTime(),
> +        new Date("2017-02-04T00:00:00.000Z").getTime(),

Nit: ```Date.parse("...")```.

Comment 3

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

Comment on attachment 8793957 [details]
bug 1304188 - introduce X509.jsm

https://reviewboard.mozilla.org/r/80534/#review80876

Looks like Cykesiopka got everything. It'll be faster, and I'll not be jetlagged, when re-reviewing.

::: security/manager/ssl/X509.jsm:288
(Diff revision 1)
> +      let versionContents = readTagAndMakeDER(contents, versionTag);
> +      let versionBytes = versionContents.readTagAndGetContents(DER.INTEGER);
> +      if (versionBytes.length == 1 && versionBytes[0] == X509v3) {
> +        this._version = 3;
> +      } else {
> +        // Lint TODO: warn about non-v3 certificates

Don't forget, it could have a wonky length here.

::: security/manager/ssl/X509.jsm:534
(Diff revision 1)
> +    let m2 = this._validateDigit(contents.readByte());
> +    let month = m1 * 10 + m2;
> +
> +    let d1 = this._validateDigit(contents.readByte());
> +    let d2 = this._validateDigit(contents.readByte());
> +    let day = d1 * 10 + d2;

We should validate that `0 < day <= 31`, and similarly that `0 < month <= 12`, hour, minute and second.

Comment 4

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

Comment on attachment 8793957 [details]
bug 1304188 - introduce X509.jsm

https://reviewboard.mozilla.org/r/80534/#review80026

Thanks! My thinking on catching exceptions was that one sub-item might fail to parse correctly but that it would be beneficial in a linter if parsing could continue as much as possible. So, if we have something like this:

SEQUENCE
  INTEGER
  SEQUENCE
    SET
      SEQUENCE ...
      SEQUENCE ...
      SEQUENCE ...
    INTEGER
    OID
    OID

Say something goes wrong decoding the SEQUENCES in the SET (maybe one of the SEQUENCES has the wrong tag). In theory it should still be possible to decode the INTEGER and OIDs following the SET, even if decoding the SET failed.

> Seems like a good idea to do the same sort of input checking as the ```DER.DER``` constructor.
> Maybe we can cheat and just construct a ```DER.DER``` object to reuse the existing code.

I ended up just taking out ByteArray for now since it really only provides the functionality native JS arrays provide.

> ```getRemainingLength()``` doesn't appear to be defined anywhere (I guess it was missed in Bug 1299676).
> We should add a test to ensure passing a DER.DER object works here, or just remove allowing a DER object to be passed.

After dropping ByteArray, this wasn't necessary anymore.

> @param {DER} der ...

Whoops - fixed.

> @param {DER} der ...

Fixed too.

> Could just be me, but I initially assumed "encoded bytes" meant the entire TLV.
> Maybe "contents" or something?

Good call - I made this more clear.

> I assume this means checking value 1 is [0, 2], and value 2 is [0, 39]?
> (Although I haven't found anything saying value 2 is also restricted to [0, 39] if value 1 is 2...)

Right - I made the comment more clear.

> Hmm, if ```parse()``` fails the only thing that happens is the exception gets stashed, right?
> Is the intention here (and elsewhere this pattern appears) to keep going and try to decode as much as possible?
> 
> If so, the ```DecodedDER.parseOverride()``` JSDoc probably should reflect this.

Right - I updated the documentation. I could probably be more clear with it - let me know what you think.

> Is this check here for similar reasons pointed out in
> https://hg.mozilla.org/mozilla-central/file/c55bcb7c777ea09431b4d16903ed079ae5632648/security/pkix/lib/pkixder.cpp#l302 ?

Yeah, I thought it would be simpler to just not support BIT STRING with unused bits here, but maybe that's a task for a linter. Then again, we haven't had any compatibility problems with this, so I'm inclined to keep it this way.

> Nit: Let's use double quotes here and below.

Fixed.

> Nit: ```Date.parse("...")```.

Fixed.

Comment 5

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

Comment on attachment 8793957 [details]
bug 1304188 - introduce X509.jsm

https://reviewboard.mozilla.org/r/80534/#review80876

Thanks!

> Don't forget, it could have a wonky length here.

Right - I updated the comment.

> We should validate that `0 < day <= 31`, and similarly that `0 < month <= 12`, hour, minute and second.

Ok - sounds good.

Comment 6

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

Comment on attachment 8793957 [details]
bug 1304188 - introduce X509.jsm

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

Comment 7

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

Comment on attachment 8793957 [details]
bug 1304188 - introduce X509.jsm

https://reviewboard.mozilla.org/r/80534/#review81112

LGTM.

Comment 8

[:Cykesiopka]

Comment on attachment 8793957 [details]
bug 1304188 - introduce X509.jsm

https://reviewboard.mozilla.org/r/80534/#review81126

Looks good!

Comment 9

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

Thanks for the reviews!
https://treeherder.mozilla.org/#/jobs?repo=try&revision=317d26b94ea7

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2bf6541e14fc
introduce X509.jsm r=Cykesiopka,jcj

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2bf6541e14fc