1294897 - track implementing and using js certificate decoder

Status: NEW

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

Description

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

This is a tracking bug for implementing an x509 certificate decoder in JS. Doing so will allow us to remove large sections of poorly-tested C++ and replace them with memory-safe code. Furthermore, we will be able to re-use this component to provide a more informational experience to users when certificate decoding or verification fails.

Comment 1

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

Attachment: bug 1294897 - initial lintx509 implementation and demonstration of use

This isn't complete - it's more of a proof-of-concept of where this project is
headed. I'm thinking this should be broken up into a few pieces:
1. Implement lintx509 (we can change the name), including tests
2. Reimplement the certificate viewer using lintx509, land it pref'd off
3. Slowly pref it on (I think we have the ability to do that now)
4. Fix bugs as necessary
5. Start using lintx509 elsewhere (e.g. certificate error pages could be a lot
   more informative)

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

Comment 2

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

Comment on attachment 8780751 [details]
bug 1294897 - initial lintx509 implementation and demonstration of use

This is the initial proof-of-concept of the JS certificate decoder. In this patch it replaces much of the implementation of the certificate viewer (it's not complete yet). This is just a request for feedback to see if this seems like a good approach or if there's something I can do to simplify this work and/or make it easier to review (I'm intending to break this up into a number of sub-bugs to do the actual implementation work).

Comment 3

[:Cykesiopka]

Comment on attachment 8780751 [details]
bug 1294897 - initial lintx509 implementation and demonstration of use

https://reviewboard.mozilla.org/r/71392/#review69516

Great stuff! The general approach here seems good to me.

The only suggestion I have for making this easier to review is to try and split up the implementation of lintx509.jsm into separate sub-bugs if possible (if you weren't planning to already) - I don't think I can effectively review 1.5k lines in one go.

::: security/manager/pki/resources/content/viewCertDetails.js:9
(Diff revision 1)
>  "use strict";
>  
>  const { classes: Cc, interfaces: Ci, utils: Cu, results: Cr } = Components;
>  const { Services } = Cu.import("resource://gre/modules/Services.jsm", {});
> +const { lintx509 } = Cu.import("resource://gre/modules/psm/lintx509.jsm", {});
> +const { console } = Cu.import("resource://gre/modules/Console.jsm", {});

Just as a reminder, we'll need to remove the ESLint rule we have enabled that forbids use of ```console```.

::: security/manager/pki/resources/content/viewCertDetails.js:799
(Diff revision 1)
> +            return ava.value.toString();
> +          }
> +        }
> +      }
> +    } catch (e) {
> +      console.log(e);

Nit: ```console.error(e);``` might be more appropriate.

::: security/manager/pki/resources/lintx509.jsm:47
(Diff revision 1)
> +const ERROR_UNSUPPORTED_GENERAL_NAME_TYPE = "error: unsupported GeneralName type";
> +
> +const X509v3 = 2;
> +const EC_UNCOMPRESSED_FORM = 4;
> +
> +class LintX509Error {

We should add ```extends Error``` and ```this.name = "LintX509Error";``` to take advantage of better exception handling and error output in some cases.

::: security/manager/pki/resources/lintx509.jsm:125
(Diff revision 1)
> +  readBytes(length) {
> +    if (this._cursor > this._bytes.length - length) {
> +      throw new LintX509Error(ERROR_DATA_TRUNCATED);
> +    }
> +    let contents = this._bytes.slice(this._cursor, this._cursor + length);
> +    this._cursor += length;
> +    return contents;
> +  }

I kinda feel like it would be better to just make this call readByte() in a loop so there are less places that mess with ```this._cursor``` directly, but I dunno if it's worth it.

If not, maybe it's a good idea to check that ```length``` is greater than 0, so we don't trigger the negative end index behaviour of ```slice()``` and get weird results back.

::: security/manager/pki/resources/lintx509.jsm:263
(Diff revision 1)
> +  }
> +}
> +
> +class OID {
> +  constructor(bytes) {
> +  this._values = [];

Nit: This line is indented wrong.

::: security/manager/pki/resources/lintx509.jsm:363
(Diff revision 1)
> +  parse() {
> +    try {
> +      this.parseOverride();
> +    } catch (e) {
> +      this._error = e;
> +    }
> +  }

Hmm, I'm not sure why we would want to eat exceptions here and in this manner.
It looks like nothing in this patch ever checks ```.error``` either.

If we end up keeping this exception eating behaviour, it seems less error prone to just have ```parse()``` return the success or error status directly.

::: security/manager/pki/resources/lintx509.jsm:710
(Diff revision 1)
> +      result += String.fromCharCode(byte1);
> +    } else if ((byte1 >> 5) == 6) {
> +      // If the next byte is of the form 110xxxxx, this codepoint consists of
> +      // two bytes. The other byte must be of the form 10xxxxxx.
> +      if (i >= bytes.length) {
> +        throw new LintX509Error(ERROR_INVALID_UTF8_ENCODING);

ERROR_INVALID_UTF8_ENCODING isn't defined anywhere.

::: security/manager/pki/resources/moz.build:8
(Diff revision 1)
>  # This Source Code Form is subject to the terms of the Mozilla Public
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
> +EXTRA_JS_MODULES.psm += [
> +    'lintx509.jsm'

Commence the bikeshedding I guess. I think we should avoid "lint", since this module does more than that, but I struggle to think of a decent name right now.
Maybe just "X509.jsm"?

Comment 4

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

Comment on attachment 8780751 [details]
bug 1294897 - initial lintx509 implementation and demonstration of use

https://reviewboard.mozilla.org/r/71392/#review70774

This is quite a body of work already. :)

I provided a couple comments as well, but I know what you're really looking for is architectural. This may be your plan already, but my main suggestion is to build yourself a mechanism for your parser to accumulate errors in some structure, and those errors should have enough context to be able to eventually drive the UI - reference to the object itself, byte offsets, whatever else is handy.

Then, when possible, instead of throwing exceptions, log the error and continue. The time classes are great examples; there's so much that can go wrong there.

IMO, the Lint exception you throw should just be for cases where the ASN.1 format has failed and we can't continue processing -- this isn't security-critical code, after all, and we want it to be error-tolerant so the UI can populate even when things are weird.

::: security/manager/pki/resources/lintx509.jsm:125
(Diff revision 1)
> +  readBytes(length) {
> +    if (this._cursor > this._bytes.length - length) {
> +      throw new LintX509Error(ERROR_DATA_TRUNCATED);
> +    }
> +    let contents = this._bytes.slice(this._cursor, this._cursor + length);
> +    this._cursor += length;
> +    return contents;
> +  }

Agree with Cykesiopka: definitely check `length` is non-negative.

::: security/manager/pki/resources/lintx509.jsm:180
(Diff revision 1)
> +  }
> +
> +  assertAtEnd() {
> +    if (this._cursor != this._bytes.length) {
> +      throw new LintX509Error(ERROR_EXTRA_DATA);
> +    }

Nit: Why not check `!atEnd()`?

::: security/manager/pki/resources/lintx509.jsm:1152
(Diff revision 1)
> +      }
> +    } else {
> +      this._cA = false;
> +    }
> +
> +    if (contents.peekTag(INTEGER)) {

Is it actually allowed to have a `pathLen` when the cA flag is not set?

::: security/manager/pki/resources/lintx509.jsm:1410
(Diff revision 1)
> +  parseOverride() {
> +    let contents = this._der.readSEQUENCE();
> +    this._features = [];
> +    while (!contents.atEnd()) {
> +      this._features.push(new ByteArray(contents.readINTEGER(), ":"));
> +    }

You should assertEnd here, too, as you do for other loops even when the test is `atEnd()`. (See line 1304.)

Comment 5

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

Thank you both for the feedback. I've been working on decomposing this project into reviewable pieces. The first will be available in bug 1299676 shortly.