1064670 - (CVE-2014-1569) ASN.1 DER decoding of lengths is too permissive, allowing undetected smuggling of arbitrary data

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Test demonstrating the problem (GTest)

+++ This bug was initially created as a clone of Bug #1064636 +++

In some contexts, such as when decoding the AlgorithmIdentifier within a PKCS#1 signature, it is critical that we minimize the variance of possible encodings that are accepted when we parse a DER-encoded data stream. Usually "minimize the variance" means "only accept the legal DER encoding of the data, and reject any overly-long encodings or any other variant." In some contexts, failing to restrict variance in accepted encodings completely undoes the cryptography. See bug 1064636, bug 351079, and bug 350640, for example.

See the attached test, where I demonstrate that SEC_QuickDERDecodeItem (a.k.a. QuickDER) accepts many malformed encodings of ASN.1 lengths. Note, in particular the test case where the length of the NULL item is encoded as { 0x90, <arbitrary junk>, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 }, which is wrongly interpreted by QuickDER as equivalent to { 0x00 } on both 32-bit and 64-bit platforms. I believe it is likely that this would facilitate the same types of attacks as described in the aforementioned bugs.

In quickder.c, the function definite_length_decoder is used to decode tags and lengths. It decodes the length as follows:

    unsigned int data_len;

    // [...]

    data_len = buf[used_length++];

    if (data_len&0x80)
    {
        int  len_count = data_len & 0x7f;

        data_len = 0;

        while (len_count-- > 0)
        {
            if (used_length >= length)
            {
                return NULL;
            }
            data_len = (data_len << 8) | buf[used_length++];
        }
    }

Note that there is no check that the encoding of the length is the shortest necssary encoding. And also note the integer overflow of data_len.

In the case of PKCS#1 (i.e. the aforementioned bugs), we shouldn't even be doing DER decoding in the first place. However, it isn't obvious to me yet what contexts besides PKCS#1 are also problematic, so it prudent to fix QuickDER, regardless of the fix for the PKCS#1 decoder in bug 1064636.

The attached GTest integrates into Gecko's mozilla::pkix test suite, for my own convenience. However, it doesn't use any of the mozilla::pkix or Gecko GTest utilities, so it should be easy to integrate it into other GTest-based test suites.

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

[Tracking Requested - why for this release]: If bug 1064636 is a exploitable, critical security vulnerability then this one almost definitely is too, because these bugs (this one, bug 1064636, bug 351079, and bug 350640) are all instances of the same general problem.

Comment 2

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

My understanding is Dan's looking for an explanation of what kinds of attacks this enables. I'm not familiar with the details here, so Brian, could you give a quick run-down of the consequences of this bug?

Comment 3

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

See bug 1064636 comment 11, where Antoine is actually exploiting *THIS* bug (bug 1064670), not that bug (bug 1064636). In particular, here is the encoded DigestInfo from Antoine's forged certificate's forged signature:

30 7b 30 07 06 05 2b 0e 03 02 1a 04 dc 00 00 00
03 64 1c cb 7e 65 53 c9 91 55 21 91 9b 3c 45 1e
07 1a 25 0d 2f 7a ac ce 7f 16 0d e8 61 ed c3 1c
af 7a e6 14 06 9f 31 eb 7c a2 b3 e7 c3 3a ed 78
b8 ab 00 3f 1c 5c 25 b2 c7 06 31 f1 db 2e cd 64
53 96 2c a6 53 c7 1e 1e ee f1 41 59 36 1f 85 29
7b 72 88 a0 83 00 00 00 14 6f 9f 54 4f 35 45 f8
49 77 54 9d 01 ef cf 66 4c c4 c1 b6 03

which decodes to:

tag:  30 (sequence DigestInfo)
len:  7b
data:
  { tag:  30 (sequence DigestInfo.AlgorithmIdentifier)
    len:  07 (length 7)
    data: { tag: 06 (OID DigestInfo.AlgorithmIdentifier.algorithm)
            len: 05 (length 5)
            data: 2b 0e 03 02 1a
          }
  }
  
  { tag:  04
    len:  dc
          00 00 00 03 64 1c cb 7e 65 53 c9 91 55 21 91 9b
          3c 45 1e 07 1a 25 0d 2f 7a ac ce 7f 16 0d e8 61
          ed c3 1c af 7a e6 14 06 9f 31 eb 7c a2 b3 e7 c3
          3a ed 78 b8 ab 00 3f 1c 5c 25 b2 c7 06 31 f1 db
          2e cd 64 53 96 2c a6 53 c7 1e 1e ee f1 41 59 36
          1f 85 29 7b 72 88 a0 83 00 00 00 14
          (length 20, ***OVER-LONG ENCODING***)
    data: 6f 9f 54 4f 35 45 f8 49 77 54 9d 01 ef cf 66 4c
          c4 c1 b6 03
  }

Notice that Antoine's test certificate doesn't even include parameters in the AlgorithmIdentifier. So, Antoine Delignat-Lavaud has demonstrated that this is this bug (bug 1064670) is actually exploitable in a way that will allow people to forge certificates that Firefox will then trust.

Note that even if we have removed all the 1024-bit root CAs with e=3 RSA keys, there are unknown numbers of sub-CA certificates issued by them with e=3. Also, note that any end-entity certificates may also have e=3, and so ServerKeyExchange messages in TLS are also an issue. Similarly, S/MIME. Also, the signed packaged apps CA uses e=3. Finally, note that Antoine claimed that he also created a PoC for an e=65537 public key in the other bug.

Comment 4

[Lawrence Mandel [:lmandel] (use needinfo)]

I don't see a lot of action here. We're very likely going to be releasing Firefox 32.0.2 this week. Wanted to poke this bug as it may be able to ride along in 32.0.2 if we can get a patch together in time.

Comment 5

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Lawrence Mandel [:lmandel] from comment #4)
> I don't see a lot of action here. We're very likely going to be releasing
> Firefox 32.0.2 this week. Wanted to poke this bug as it may be able to ride
> along in 32.0.2 if we can get a patch together in time.

The patches in bug 1064636 will fix this bug and will make bug 1067214 less urgent to fix.

Comment 6

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #5)
> (In reply to Lawrence Mandel [:lmandel] from comment #4)
> > I don't see a lot of action here. We're very likely going to be releasing
> > Firefox 32.0.2 this week. Wanted to poke this bug as it may be able to ride
> > along in 32.0.2 if we can get a patch together in time.
> 
> The patches in bug 1064636 will fix this bug and will make bug 1067214 less
> urgent to fix.

Oops, sorry. The patches in bug 1064636 will NOT fix this bug, but they will reduce the severity of this issue.

To fix this bug, NSS can copy the logic that mozilla::pkix's DER decoder uses:
https://mxr.mozilla.org/mozilla-central/source/security/pkix/lib/pkixder.cpp?rev=88c41feb981d#48

Note that mozilla::pkix's decoder only handles lengths up to 2^16-1, so the logic would need to be extended to handle longer lengths.

Comment 7

[Antoine Delignat-Lavaud]

That brings up an interesting question, what else is the QuickDER decoder used for in NSS? I wonder if there  may be some other more exciting (or at least novel) exploit to make.

Comment 8

[Daniel Veditz [:dveditz]]

(In reply to Antoine Delignat-Lavaud from comment #7)
> That brings up an interesting question, what else is the QuickDER decoder
> used for in NSS? I wonder if there  may be some other more exciting (or at
> least novel) exploit to make.

It's used quite a bit:
https://mxr.mozilla.org/mozilla-central/search?string=quickderdecodeitem&find=&findi=&filter=^%5B^\0%5D*%24&hitlimit=&tree=mozilla-central

Comment 9

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

This has now been publicly disclosed:
https://www.reddit.com/r/netsec/comments/2hd1m8/rsa_signature_forgery_in_nss/cksnr02

Comment 10

[Sylvestre Ledru [:Sylvestre]]

Brian, do we have an ETA for a fix for this issue? thanks

By the way, do we know if ESR is affected?

Comment 11

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Sylvestre Ledru [:sylvestre] from comment #10)
> Brian, do we have an ETA for a fix for this issue? thanks
> 
> By the way, do we know if ESR is affected?

Somebody needs to be assigned to this bug.

Comment 12

[Antoine Delignat-Lavaud]

Please note that there were other DER failures mentioned in bug 1064636 that need to be fixed in the patch for this bug, either that I noticed when building an exploit or from the test cases Adam provided. From the top of my head:

- length encoding can be overlong regardless of the overflow problem (03 == 81 03 == 82 00 03), i.e. this bug
- INTEGER can have arbitrary many null bytes of padding. This is wrong, there can only be one byte of padding to distinguish between negative and positive values
- BITSTRING: the unused bits are not checked, and the computed length in bits is not verified either.
- the method check for non-universal tags is probably too weak
- I don't think there is support for default values in optional components. This is required to implement DER as optional fields that are equal to their default values must be omitted
- many checks are missing for primitive types. For instance, BOOL must be either 00 of FF; object identifiers must satisfy certain requirements, TIME types must be validated...

Overall, the DER parser in PKIX appears a lot more robust, and it would make sense to port other parts of NSS that rely on DER for non-malleability to use that instead of QuickDER.

Comment 13

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

All the sensitive information in this bug has been publicly disclosed:
https://www.imperialviolet.org/2014/09/26/pkcs1.html
https://www.reddit.com/r/netsec/comments/2hd1m8/rsa_signature_forgery_in_nss/cksnr02
http://www.intelsecurity.com/resources/wp-berserk-analysis-part-1.pdf

I'd like to open up the bug today or sometime this weekend. Let me know if (and how) it would be problematic to do so.

Comment 14

[Daniel Veditz [:dveditz]]

(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #13)
> I'd like to open up the bug today or sometime this weekend. Let me know if
> (and how) it would be problematic to do so.

A comment in the bug isn't the right approach this, for reasons someone with "use NEEDINFO?" in their user name ought to realize. Mailing nss-dev or security-group would be a better approach.

Comment 15

[Al Billings [:abillings - ex-MoCo]]

Attachment: [e-mail],3000?,2014-09-08,,2014-10-13,true,

Comment 16

[Matt Wobensmith [:mwobensmith][:matt:]]

David, do you know who might own this on the developer side?

Comment 17

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

(In reply to Matt Wobensmith from comment #16)
> David, do you know who might own this on the developer side?

Looking at hg annotate for quickder.c, I would ask Wan-Teh.

Comment 18

[Wan-Teh Chang]

It should not be hard to implement the fix that Brian described
in comment 6.

Some of the problems that Antoine described in comment 12 don't
seem hard. Some of them may require deeper knowledge of ASN.1
than I have now.

Comment 19

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

I'm going to take a shot at fixing this bug. I'll get the length fix in place from comment 6 and get feedback from you, Wan-Teh (if that's alright), and then move on to the other parsing issues in comment 12.

If anyone wants to gin up some tests for the issues in comment 12, that'd be swell, but I'll see what I can do.

Comment 20

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

Attachment: This patch intends to handle Brian Smith's original findings; it does not address the other DER-correctness issues for INTEGER, BOOL and such.

Comment 21

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

I've found an interesting thing: after writing a routine to catch padded ASN.1 integers that aren't DER-compliant, apparently they are moderately common. There's a comment in quickder.c's DecodeItem() that says:

> /* remove leading zeroes if the caller requested
>  siUnsignedInteger
>  This is to allow RSA key operations to work */

Assuming that to be true and unavoidable, I added an else that checks for over-long (padded) integers. A simple check of
 
 if ( item length > 1 && ( byte[0] == 0x00 || byte[0] == 0xFF ))
 {
    abort
 }

aborts on Firefox startup with an error that an EV certificate wasn't available; I'm not certain what certificate is the source of the problem at this point, but clearly there are some compatibility issues.

Specifically for the INTEGER type, one way through this is to look for more than one padding byte at the front (0x00 or 0xFF) and to only fail if someone is padding more than a single byte. That would still limit the danger of a malicious certificate while not breaking the apparently common single-byte padding that I'm seeing.

That's how I'll move forward for part 2 of this patch until I hear otherwise.

Comment 22

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to J.C. Jones [:jcj] from comment #21)
> I've found an interesting thing: after writing a routine to catch padded
> ASN.1 integers that aren't DER-compliant, apparently they are moderately
> common.

Please see pkixder.cpp and pkixder.h to get an idea of what workarounds you need to do to deal with invalid DER in order to be backward compatible with existing certificates. If you implement workarounds that are different than the mozilla::pkix workarounds then please highlight them, as one of the goals of the mozilla::pkix project is to serve as a reference implementation for these types of things.

Comment 23

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

Brian,

As far as I can tell, the pkixder.h implementation of Integer right now only supports parsing positive numbers between 0 and 127 (http://dxr.mozilla.org/mozilla-central/source/security/pkix/lib/pkixder.h?from=pkixder.h#258), so that's a substantially different constraint than the quickder parser. 

I haven't run into anything putting constraints on INTEGER types elsewhere yet, but I will keep my eyes open.

Comment 24

[Wan-Teh Chang]

Comment on attachment 8510691 [details] [diff] [review]
This patch intends to handle Brian Smith's original findings; it does not address the other DER-correctness issues for INTEGER, BOOL and such.

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

CJ: thanks a lot for writing the patch quickly, and sorry about the late review.
I reviewed your patch carefully. I believe your sanity check on the long-form
length field has an off-by-one error. Most of my other review comments are
about coding style nits.

::: security/nss/lib/util/quickder.c
@@ +20,4 @@
>                                                PRBool includeTag)
>  {
>      unsigned char tag;
> +    unsigned int buf_idx = 0;

Nit: I think the original variable name 'used_length' is easier to understand in comparisons with buf_length.

@@ +27,2 @@
>  
> +    if (buf_idx >= buf_length) {

Please don't change the braces style unless you change it throughout the
file. Consistency is important.

@@ +27,3 @@
>  
> +    if (buf_idx >= buf_length) {
> +        // Tag field was not found!

Please use old-style C comment delimiters /* */ for consistency.

@@ +33,3 @@
>  
> +    // Exit out when we come to the end of the object
> +    if (tag == 0) {

Do you know what a tag value of 0 means? Is it the end-of-content marker?

@@ +36,5 @@
>          return NULL;
>      }
>  
> +    if ((tag & 0x1F) == 0x1F) {
> +        // TODO: Allow high tag number form?

Nit: should we call this "long tag number form" or "long-form tag number"?

@@ +56,3 @@
>  
> +        // Extract the field length
> +        length_field_len += (byte & 0x7F);

Use = instead of +=.

@@ +61,5 @@
> +            // This is the definite length decoder, after all.
> +            return NULL;
> +        }
> +
> +        if (length_field_len > 4) {

Nit: it may be good to replace 4 with sizeof(data_length), because
that is one reason we allow at most 4 bytes.

@@ +67,5 @@
> +            return NULL;
> +        }
> +
> +        // Iterate across the extended length field
> +        for (size_t i = 0; i < length_field_len; i++) {

Nit: 'i' can be unsigned int.

@@ +68,5 @@
> +        }
> +
> +        // Iterate across the extended length field
> +        for (size_t i = 0; i < length_field_len; i++) {
> +            if (buf_idx >= buf_length) {

You can do a single length check before the for loop:

    if (length_field_len > buf_length - buf_idx)
    {
        /* Extended length field was not found */
        return NULL;
    }

@@ +74,4 @@
>                  return NULL;
>              }
> +            byte = buf[buf_idx++];
> +            // Shift the previous byte left

Nit: this comment can be omitted.

@@ +83,5 @@
> +    // Sanity checks
> +    //
> +
> +    // Determine if the ASN.1 length is longer than needed.
> +    // Add 1 to the minimum sizes so we can do less-than

1. IMPORTANT: adding 1 to the minimum size seems wrong.

For example, if length_field_len is 1, then your check disallow a data_length of 128 (0x80). But I think 128 (0x80) should be allowed.

2. I think we should just do this:

    /* Sanity-check that the long form uses the shortest possible length. */
    if (length_field_len != 0)
    {
        if (length_field_len == 1)
            minimum_size = 1 << 7;
        else
            minimum_size = 1 << (8 * (length_field_len - 1));
        if (data_length < minimum_size)
            return NULL;
    }length_field_len

@@ +86,5 @@
> +    // Determine if the ASN.1 length is longer than needed.
> +    // Add 1 to the minimum sizes so we can do less-than
> +    // comparison and properly treat zero-lengths.
> +    unsigned int minimum_size;
> +    switch(length_field_len) {

Nit: add a space after 'switch'.

@@ +101,5 @@
> +            break;
> +    }
> +
> +    if (data_length < minimum_size) {
> +        // (Note: Zero length is OK if length_field_len was 0.)

Nit: this comment should be moved to the case 0 above.

@@ +110,2 @@
>  
> +    if (data_length > (buf_length-buf_idx)) {

Nit: add spaces around '-'.

@@ +113,5 @@
> +        return NULL;
> +    }
> +
> +    if (includeTag) {
> +        // We want to count the tag in the length

Nit: this comment is a little inaccurate -- we also include the length field in *out_data_length. I would just omit this comment. Or you can say if includeTag is true, the returned pointer starts from the tag.

@@ +118,5 @@
> +       data_length += buf_idx;
> +    }
> +
> +    // The length goes into the outpointer data_length; we
> +    // directly return an updated pointer in the buffer.

This comment can be omitted. In general avoid comments that describe the obvious.

Comment 25

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

(In reply to Wan-Teh Chang from comment #24)
> I reviewed your patch carefully. I believe your sanity check on the long-form
> length field has an off-by-one error. Most of my other review comments are
> about coding style nits.

Wan-Teh, thank you! This is why we code review. And also, I'm still getting used to there being different styles in different places. Sorry for the poor style hygiene. It will improve. Unless otherwise noted below, I took all of your changes.

> @@ +33,3 @@
> >  
> > +    // Exit out when we come to the end of the object
> > +    if (tag == 0) {
> 
> Do you know what a tag value of 0 means? Is it the end-of-content marker?

It appears to be how the rest of NSS finds delineates the end-of-content, that all ASN.1 strings end with a NULL that will be read as the next tag.

> 
> @@ +36,5 @@
> >          return NULL;
> >      }
> >  
> > +    if ((tag & 0x1F) == 0x1F) {
> > +        // TODO: Allow high tag number form?
> 
> Nit: should we call this "long tag number form" or "long-form tag number"?

It seems to appear both ways on the Internet. FWIW, Moz::pkix denies the high tags, but to-date NSS allows them. The safe way to resolve the TOODO is to remove this block. In the updated patch it will be gone (for now). Thoughts?

> 1. IMPORTANT: adding 1 to the minimum size seems wrong.
> 
> For example, if length_field_len is 1, then your check disallow a
> data_length of 128 (0x80). But I think 128 (0x80) should be allowed.

You are absolutely correct. Thank you.

Comment 26

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

Attachment: bug_1064670

Updates per wtc's review.

Comment 27

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

Attachment: More tests, and more DER-compliance checks to quickder.c

This patch intends to cover what Antoine brought up in comment 12, particularly DER-compliance for INTEGER, BIT STRING and BOOLEAN. It's a follow-on to the first patch,  8513137: bug_1064670 (not folded).

Note that per comment 21, this patch has a weakened check for INTEGER DER-compliance, allowing one extra padding byte. I chose this by experimentation (since single-byte padding seems to readily exist). I worry that allowing one byte may even not be flexible enough. It may be that we need to do some certificate analysis in the population before choosing a strategy on the INTEGER validation.

The BIT STRING and BOOLEAN validations are equivalent to mozilla::pkix's approaches.

Comment 28

[Wan-Teh Chang]

Comment on attachment 8513137 [details] [diff] [review]
bug_1064670

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

J.C.: this patch is much better. I'm sorry a suggestion of mine misled you
into avoiding +=. I suggest some other changes.

Nit: you tend to comment every step, a style common in assembly code.
In general try to be more concise, by only summarizing a longer block
of code and only explaining subtle points. Note there are exceptions
to these guidelines. For example, when documenting a function in the
style of a man page it is often required to document the obvious.

::: security/nss/lib/util/quickder.c
@@ +30,3 @@
>          return NULL;
>      }
>      tag = buf[used_length++];

I like your high tag number check. Please add it back.

Note: in your comment for the check, please say something like
"high tag number (a tag number > 30)". I found that "high tag nunber"
is the term used in the X.690 standard. It would be good to define
that term concisely in the comment.

@@ +34,2 @@
>      if (tag == 0)
> +    {   /* Exit out when we come to the end of the object */

Let's change this comment to something like "End-of-contents octets
should not be present in DER because DER doesn't use the indefinite
length form".

@@ +47,5 @@
> +        data_length = byte; /* clarity; we're returning a 32-bit int. */
> +    }
> +    else
> +    {   /* Long form, of some manner.
> +           Extract the field length */

Nit: this comment fits in one line.

@@ +48,5 @@
> +    }
> +    else
> +    {   /* Long form, of some manner.
> +           Extract the field length */
> +        length_field_len = length_field_len + (byte & 0x7F);

This should be
    length_field_len = byte & 0x7F;
because we don't need to use the (dummy) initial value of 0.

@@ +50,5 @@
> +    {   /* Long form, of some manner.
> +           Extract the field length */
> +        length_field_len = length_field_len + (byte & 0x7F);
> +        if (length_field_len == 0)
> +        {   /* We don't support the indefinite long form.

Change "indefinite long form" to "indefinite form" or "indefinite length form".

@@ +51,5 @@
> +           Extract the field length */
> +        length_field_len = length_field_len + (byte & 0x7F);
> +        if (length_field_len == 0)
> +        {   /* We don't support the indefinite long form.
> +               This is the definite length decoder, after all. */

Nit: this entire comment can just say "DER doesn't use the indefinite length form."

@@ +71,2 @@
>          {
> +            byte = buf[used_length++];

Another way to do the length sanity check is to do it here.
We check the first length byte.

    if (i == 0)
    {
        PRBool too_long = PR_FALSE;
        if (length_field_len == 1)
        {
            too_long = ((byte & 0x80) == 0); /* Short form suffices */
        }
        else
        {
            too_long = (byte == 0); /* This zero byte can be omitted */
        }
        if (too_long)
        {
            /* The length is longer than needed. */
            return NULL;
        }
    }

@@ +77,5 @@
> +    /* Sanity checks */
> +
> +    /* Determine if the ASN.1 length is longer than needed.
> +       Add 1 to the minimum sizes so we can do less-than
> +       comparison and properly treat zero-lengths. */

Delete this stale comment.

@@ +78,5 @@
> +
> +    /* Determine if the ASN.1 length is longer than needed.
> +       Add 1 to the minimum sizes so we can do less-than
> +       comparison and properly treat zero-lengths. */
> +    unsigned int minimum_size;

Move this declaration into the if statement.

NSS still needs to support C compilers that require declaring variables
at the beginning of a block. (We may remove this restriction soon.)

@@ +106,4 @@
>  
> +    if (includeTag)
> +    {
> +        data_length = data_length + used_length;

Use += here.

You misunderstood an earlier suggestion of mine. I suggested replacing += with =
in that case because we don't need to use the initial value of that variable there.
Here we need to increment the variable, so += is more concise.

Comment 29

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

(In reply to Wan-Teh Chang from comment #28)
> J.C.: this patch is much better. I'm sorry a suggestion of mine misled you
> into avoiding +=. I suggest some other changes.

Sorry! I've found enough style differences that I didn't blink an eye, I just assumed that was one of them, too.
 
> Nit: you tend to comment every step, a style common in assembly code.
> In general try to be more concise, by only summarizing a longer block
> of code and only explaining subtle points. Note there are exceptions
> to these guidelines. For example, when documenting a function in the
> style of a man page it is often required to document the obvious.

My apologies; practically all of my C, C++ (and assembly) code has been for US gov't high assurance projects, where high comment-to-code ratios are mandatory. Habits die hard, as they say. I'll be less comment heavy in the future.

Thanks for taking the time to go through this with me. I appreciate your patience!

I'll be back with another updated review probably early AM tomorrow (interrupted for the afternoon).

Comment 30

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

NSS and Gecko (including mozilla::pkix) are separate products. Please file another bug in the "Core: Security:PSM" product:component for the changes to pkixder_input_tests.cpp, which are much appreciated. Also, please move the NSS tests from security/pkix/test/gtest to security/nss/test/<something>, for the same reason.

> I've found an interesting thing: after writing a routine to catch padded
> ASN.1 integers that aren't DER-compliant, apparently they are moderately
> common. There's a comment in quickder.c's DecodeItem() that says:
> 
> > /* remove leading zeroes if the caller requested
> >  siUnsignedInteger
> >  This is to allow RSA key operations to work */
> 
> Assuming that to be true and unavoidable, I added an else that checks for
> over-long (padded) integers. A simple check of
>  
>  if ( item length > 1 && ( byte[0] == 0x00 || byte[0] == 0xFF ))
>  {
>     abort
>  }
> 
> aborts on Firefox startup with an error that an EV certificate wasn't
> available; I'm not certain what certificate is the source of the problem at
> this point, but clearly there are some compatibility issues.

1. Please identify which certificate is causing the check to fail. You should be able to do this by setting a breakpoint where you put your abort(), and then looking at the stack trace to find which certificate is being parsed.

2. If the most significant bit of the first byte of a positive integer would be set (i.e. that byte's value is 0x80 or higher) then there must be a leading 0x00 to disambiguate it from a negative number. So a leading 0x00 is sometimes necessary.

> Specifically for the INTEGER type, one way through this is to look for more
> than one padding byte at the front (0x00 or 0xFF) and to only fail if
> someone is padding more than a single byte. That would still limit the
> danger of a malicious certificate while not breaking the apparently common
> single-byte padding that I'm seeing.

I think that a stricter check will probably work: check that there's a leading zero only if the high bit of the second byte is set. See the function CertificateSerialNumber in pkixder.h:

  // Check for overly-long encodings. If the first byte is 0x00 then the high
  // bit on the second byte must be 1; otherwise the same *positive* value
  // could be encoded without the leading 0x00 byte. If the first byte is 0xFF
  // then the second byte must NOT have its high bit set; otherwise the same
  // *negative* value could be encoded without the leading 0xFF byte.
  if (value.GetLength() > 1) {
    Reader valueInput(value);
    uint8_t firstByte;
    rv = valueInput.Read(firstByte);
    if (rv != Success) {
      return rv;
    }
    uint8_t secondByte;
    rv = valueInput.Read(secondByte);
    if (rv != Success) {
      return rv;
    }
    if ((firstByte == 0x00 && (secondByte & 0x80) == 0) ||
        (firstByte == 0xff && (secondByte & 0x80) != 0)) {
      return Result::ERROR_BAD_DER;
    }
  }

(In reply to J.C. Jones [:jcj] from comment #23)
> As far as I can tell, the pkixder.h implementation of Integer right now only
> supports parsing positive numbers between 0 and 127
> (http://dxr.mozilla.org/mozilla-central/source/security/pkix/lib/pkixder.
> h?from=pkixder.h#258), so that's a substantially different constraint than
> the quickder parser. 

Right. It turns out that the Integer values we care about in certificates are all very small, so I didn't bother trying to parse bigger integers in that function. Because QuickDER needs to be more general, it needs to support a larger range of values. But, other than the expanded range, I expect that it should be made to work like the functions in pkixder.h/pkixder.cpp.

Comment 31

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

I highly recommend this document for learning the details of ASN.1 DER encoding/decoding: http://luca.ntop.org/Teaching/Appunti/asn1.html. Also, the ASN.1 reference information on Microsoft's MSDN site was very helpful to me. As a final fallback, the actual specifications for ASN.1 and DER are available for free online, but they are not as accessible as the first two. If you diverge in any way from what the Layman's Guide says, please document that divergence very clearly, as we've done in pkixder.h. I believe the main divergences that are needed are about DEFAULT values (they are not supposed to be explicitly encoded, but they often are in real-world certificates) and empty SEQUENCEs/SETs (some SEQUENCEs/SETs are defined as not being allowed to be empty, but some certificates have encoded them as empty anyway).

Comment 32

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

As for the high tag number form and indefinite encoding (which isn't even allowed in DER): I believe that it is likely that none that use QuickDER have any need for the high tag number form or indefinite encoding, so I agree that you can just hard-code simple logic to always forbid them, as we've done in mozilla::pkix.

Comment 33

[Wan-Teh Chang]

QuickDER must forbid the indefinite length form. As for high
tag numbers, I don't think QuickDER needs to support them, but
we should detect them and return an (unsupported) error.

Comment 34

[Sylvestre Ledru [:Sylvestre]]

J.C. any eta for this? Go to build of 33.1 is going to be very soon. Thanks

Comment 35

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

(In reply to Sylvestre Ledru [:sylvestre] from comment #34)
> J.C. any eta for this? Go to build of 33.1 is going to be very soon. Thanks

Sylvestre - I'm not entirely sure; I'm doing the newbie thing with moving the new unit test to NSS currently. It's my top priority, but my lack of familiarity (e.g., fighting with even the NSS/gtest build environment) is hindering making an estimate.

Comment 36

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

Attachment: bug_1064670 test and quickder changes for NSS

This patch moves the quickder code and test to properly apply to the tip of NSS as of now, and uses the ssl_gtest to validate it. I folded Brian's test into this patch to move it to NSS as well.

I rolled in all of Wan-Teh's comments from comment #28. 

I am afraid I haven't addressed the INTEGER, BIT STRING or BOOLEAN issues in this patch. I still need to go through Brian's comment 30 and handle the issues he identified in the "part 2", so that is not ready. Since Sylvestre is asking after the original bug, I hope this "part 1" will be close to ready to apply, and maybe I can open follow-up bugs for the new PKIX tests and the other DER compliance issues.

Comment 37

[Sylvestre Ledru [:Sylvestre]]

ok, thanks.
This is probably going to be too late for 33.1.

Comment 38

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

Attachment: bug_1064670_part_2 More DER-compliance checks to quickder.c

Brian, thank you for the helpful links on ASN.1. You're totally correct, I had forgotten about the need for a leading zero. I've verified the issues I saw with Firefox are all from certificates that needed a leading zero to indicate they were positive, so all is well.

I've adjusted the implementation of the INTEGER checks per your suggestion in comment 30, and this patch applies on top of the one from earlier today in NSS.

Thanks again Brian and Wah-Teh for all the help as I fumble my way around here.

Comment 39

[Wan-Teh Chang]

Comment on attachment 8514717 [details] [diff] [review]
bug_1064670 test and quickder changes for NSS

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

J.C.: you can check in your quickder.c changes first. I only
ask for some small changes to the comments.

The quickder_unittest.cc file needs a bit more work. In
particular, I need to know which coding style it should follow.
I only comment on the non-stylistic issues for now.

::: external_tests/ssl_gtest/manifest.mn
@@ +6,5 @@
>  DEPTH      = ../..
>  MODULE = nss
>  
>  CPPSRCS = \
> +      quickder_unittest.cc \

This directory is external_tests/ssl_gtest. We probably
should rename the directory so that it is not SSL specific.
Or create subdirectories for ssl, cert, util, etc.

::: external_tests/ssl_gtest/quickder_unittest.cc
@@ +1,1 @@
> +#include "gtest/gtest.h"

You need to add an MPL 2 header to this file.

@@ +3,5 @@
> +#include "prerror.h"
> +#include "secasn1.h"
> +#include "secerr.h"
> +#include "secitem.h"
> +#include "stdint.h"

stdint.h is a standard library header, so we should use <stdint.h> instead of "stdint.h".

@@ +9,5 @@
> +class quickder_tests
> +  : public ::testing::Test
> +  , public ::testing::WithParamInterface<SECItem>
> +
> +{

I seem to remember we are following Google's C++ Style Guide for
the C++ test files in NSS. Can you please find out which Style
Guide these files should follow?

@@ +38,5 @@
> +
> +// Length of zero wrongly encoded as:
> +//
> +//     { 0x90, <arbitrary junk>,
> +//       0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 }

0x90 means the long length should be 16 (decimal) bytes long.
So you may want to point out <arbitrary junk> is 8 bytes long.

@@ +48,5 @@
> +// parser would ignore.
> +static uint8_t OVERLONG_LENGTH_16_0[] = {
> +  NULL_TAG,
> +  LONG_LENGTH | 0x10,
> +  0x11, 0x22, 0x33, 0x44, 0x55, 0x65, 0x77, 0x88,

Nit: I guess you meant to type 0x66.

@@ +64,5 @@
> +{
> +  const SECItem& originalInput(GetParam());
> +
> +  // Mozilla's version of the NSS shared library on Windows doesn't export
> +  // the function needed by SEC_ASN1_GET(SEC_NullTemplate), so I've just

SEC_NullTemplate is exported. But you need to access it
using the SEC_ASN1_MKSUB and SEC_ASN1_SUB macros.

Look at lib/certhigh/ocsp.c as an example:
http://mxr.mozilla.org/nss/ident?i=SEC_NullTemplate

::: lib/util/quickder.c
@@ +29,3 @@
>  
> +    if (used_length >= buf_length)
> +    {   /* Tag field was not found! */

Nit: we don't usually put a comment at this location (after an
opening curly brace).

In general look around in the file you are modifying and try
to imitate the prevalent style in that file.

I can tell you are trying to conserve the vertical space. But
we value consistency very much. If necessary, we can run this
file through a source code pretty printer first.

@@ +39,5 @@
>          return NULL;
>      }
>  
> +    if ((tag & 0x1F) == 0x1F)
> +    {   /* high tag number (a tag number > 30) */

Nit: please add something like "is not supported" or "is not implemented"
to this comment.

@@ +52,4 @@
>  
> +    if (!(byte & 0x80))
> +    {   /* Short form: The high bit is not set, so the length is the value. */
> +        data_length = byte; /* clarity; we're returning a 32-bit int. */

Nit: this comment isn't that useful... we need to put the value
in data_length in both the short form and long form cases. So
this seems like something we have to do anyway.

Also, it is actually OK to read a byte (unsigned char) into an
unsigned int variable.

Comment 40

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

Attachment: 8514717: bug_1064670 test and quickder changes for NSS; restructure gtests

Following up Wah-Teh's review, I've restructured the tests a little bit than Kai had them in https://bugzilla.mozilla.org/show_bug.cgi?id=1057584#c37 .

Specifically, I've renamed external_tests/ssl_gtest to external_tests/gtests/  and created subdirs ssl/ and asn1/. All associated scripts are updated, and I've also edited and updated the tests' README.

> I seem to remember we are following Google's C++ Style Guide for
> the C++ test files in NSS. Can you please find out which Style
> Guide these files should follow?

That appears to be the intention, per the above bug 1057584 which I hadn't found until this morning. I've adjusted quickder_unittest.cc and made minor adjustments for compliance into ssl_loopback_unittest.cc. (Technically, clang-format wants to adjust a bunch of spacing too).

I've addressed the other nits as well. Thanks a lot for the pointer to SEC_ASN1_SUB -- I had no idea that's what that did.

At this point should I open a follow-on for the INTEGER, BIT STRING and BOOLEAN compliance checks?

Comment 41

[Wan-Teh Chang]

(In reply to J.C. Jones [:jcj] from comment #40)
> 
> Specifically, I've renamed external_tests/ssl_gtest to
> external_tests/gtests/  and created subdirs ssl/ and asn1/.

Please name the new directory "gtest" (singular) because "gtest" is
the name of the test framework.

> At this point should I open a follow-on for the INTEGER, BIT STRING and
> BOOLEAN compliance checks?

That's a good idea. Your current patch should focus on the tag and length
decoding function.

Comment 42

[Wan-Teh Chang]

Comment on attachment 8516274 [details] [diff] [review]
8514717: bug_1064670 test and quickder changes for NSS; restructure gtests

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

r=wtc. Please make the suggested changes and attach a new patch.
You can check in the new patch without waiting for my review. I
will review it after the checkin.

Please refrain from making big changes or adding new changes to
this patch. At this point we need to stabilize this patch.

::: external_tests/README
@@ +2,5 @@
>  
>  This directory contains GTest-based unit tests for NSS.
>  
> +Currently, these are broken into TLS-based tests in the tls/ subdirectory,
> +and ASN.1 tests in the asn1/ directory. To make these work:

Just say tests are broken into subdirectories, so that we don't need
to update this comment when a new subdirectory is added.

@@ +8,5 @@
>  - Set NSS_BUILD_GTESTS=1 before starting your build
>  
>  - cd tests/
>  
> +- Set NSS_TESTS=nss_gtests and NSS_CYCLES=standard

Nit: perhaps "nss_gtests" can be shortened to just "gtest"? The "nss_" part is implied.

Or is this referring to the name of the nss_gtest executable program?

Update: after reading the entire patch, I think this should be named "nss_gtest". This
requires renaming the nss_gtests.sh script to nss_gtest.sh as well.

@@ +18,4 @@
>  
>  You should be able to run the unit tests manually as:
>  
> +  nss_gtest -d ${NSSGTESTDIR}

On the other hand, it is good to keep "NSS" in this environment variable name.

@@ +22,2 @@
>  
> +Where $NSSGTESTDIR the directory created by ./all.sh or a manually

Nit: lowercase "where"

Nit: I think we're missing "is" before "the directory created by"?

@@ +29,3 @@
>  
> +  cd dist/Linux3.16_x86_glibc_PTH_DBG.OBJ/bin
> +  LD_LIBRARY_PATH=../lib/ ./nss_gtest --gtest_filter="quickder*"

Nit: it is not necessary to quote the argument to --gtest_filter=.

::: external_tests/gtests/asn1/quickder_unittest.cc
@@ +1,5 @@
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* vim: set ts=2 et sw=2 tw=80: */
> +/* 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/. */

Nit: add a blank line after this line.

@@ +14,5 @@
> +#include "secitem.h"
> +
> +namespace nss_test {
> +
> +class quickderTests : public ::testing::Test,

This class should be named QuickDerTest (singular) or QuickDERTest.

@@ +25,5 @@
> +  }
> +};
> +
> +static const uint8_t NULL_TAG = 0x05;
> +static const uint8_t LONG_LENGTH = 0x80;

Nit: in the Google Style Guide, constants should be named as kFooBar.
So these should be named kNullTag and kLongLength.

@@ +28,5 @@
> +static const uint8_t NULL_TAG = 0x05;
> +static const uint8_t LONG_LENGTH = 0x80;
> +
> +// Length of zero wrongly encoded as 0x80 instead of 0x00.
> +static uint8_t OVERLONG_LENGTH_0_0[] = {NULL_TAG, LONG_LENGTH | 0};

Nit: this constant should be named kOverlongLength_0_0.

Similarly for the other OVERLONG_LENGTH_x_y constants.

@@ +36,5 @@
> +
> +// Length of zero wrongly encoded as:
> +//
> +//     { 0x90, <arbitrary junk of 8 bytes>,
> +//       0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 }

Now that I understand what this test case is testing, I suggest
you use exactly four 0x00 bytes at the end, because that's
sufficient to make the original quickder.c code compute a
data length of 0. (The 'data_len' local variable is four bytes
long.)

@@ +45,5 @@
> +// smuggle arbitrary input into DER-encoded data in a way that an non-careful
> +// parser would ignore.
> +static uint8_t OVERLONG_LENGTH_16_0[] = {
> +    NULL_TAG, LONG_LENGTH | 0x10, 0x11, 0x22, 0x33, 0x44, 0x55, 0x66, 0x77,
> +    0x88,     0x00,               0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00};

I suggest only four 0x00 bytes at the end. You can use change the other 0x00
bytes to 0x99, 0xaa, 0xbb, etc.

@@ +47,5 @@
> +static uint8_t OVERLONG_LENGTH_16_0[] = {
> +    NULL_TAG, LONG_LENGTH | 0x10, 0x11, 0x22, 0x33, 0x44, 0x55, 0x66, 0x77,
> +    0x88,     0x00,               0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00};
> +
> +static const SECItem INVALID_DER[] = {

This constant array should be named kInvalidDer or kInvalidDER.

@@ +54,5 @@
> +    {siBuffer, OVERLONG_LENGTH_16_0, sizeof(OVERLONG_LENGTH_16_0)},
> +};
> +
> +TEST_P(quickderTests, InvalidLengths) {
> +  const SECItem& originalInput(GetParam());

This local variable should be named original_input.

Similarly for the other local variables: copy_of_input, parsed_value, etc.

::: external_tests/gtests/gtest_utils.h
@@ +6,5 @@
>  
>  #ifndef gtest_utils_h__
>  #define gtest_utils_h__
>  
> +#include "test_io.h"

Just wanted to double check that you meant to add this header #include.

In general we should include "test_io.h" only if something inside this
header file needs "test_io.h".

::: external_tests/gtests/manifest.mn
@@ +9,5 @@
>  CPPSRCS = \
> +      asn1/quickder_unittest.cc \
> +      nss_gtest.cc \
> +      tls/ssl_loopback_unittest.cc \
> +      tls/tls_parser.cc \

Our makefiles are usually hierarchical. That is, we would add
makefiles inside asn1/ and tls/ to compile the source files in
those directories. You can deal with this later.

::: external_tests/gtests/tls/ssl_loopback_unittest.cc
@@ +1,5 @@
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* vim: set ts=2 et sw=2 tw=80: */
> +/* 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/. */

Nit: add a blank line after this line.

@@ +21,2 @@
>  #include "test_io.h"
> +#include "tls/tls_parser.h"

Nit: "tls/" can be omitted because tls_parser.h is in the same directory.
But it may be good style to include "tls/".

::: external_tests/manifest.mn
@@ +6,5 @@
>  DEPTH      = ..
>  
>  DIRS = \
>  	google_test \
> +  gtests \

Nit: the indentation seems wrong.

::: lib/util/quickder.c
@@ +16,5 @@
>   */
>  
>  static unsigned char* definite_length_decoder(const unsigned char *buf,
> +                                              const unsigned int buf_length,
> +                                              unsigned int *out_data_length,

Just a side note: in general output parameters in NSS don't need to have "out"
in their names. Here I know it is useful for telling it apart from the local
variable "data_length".

@@ +45,2 @@
>      {
> +        /* high tag number (a tag number > 30) is not supported */

Nit: capitalize "High".

@@ +56,5 @@
>  
> +    if (!(byte & 0x80))
> +    {
> +        /* Short form: The high bit is not set, so the length is the value. */
> +        data_length = byte; /* clarity; we're returning a 32-bit int. */

Nit: remove this comment. "the length is the value" already describes the
code "data_length = byte".

@@ +60,5 @@
> +        data_length = byte; /* clarity; we're returning a 32-bit int. */
> +    }
> +    else
> +    {
> +        /* Long form, of some manner. Extract the field length */

Nit: remove ", of some manner".

@@ +73,2 @@
>          {
> +            /* We don't support longer forms than 4 bytes (2^32) */

Nit: change "longer forms than" to "a long-form length longer than"
or "an extended length field longer than".

@@ +74,5 @@
> +            /* We don't support longer forms than 4 bytes (2^32) */
> +            return NULL;
> +        }
> +
> +        if (length_field_len > buf_length - used_length)

Nit: let's also parenthesize "buf_length - used_length" to match
the style used on line 110.

::: tests/common/init.sh
@@ +75,5 @@
>          IOPR_OCSP_CLIENTDIR=${HOSTDIR}/client_ocsp_iopr
>  
>          CERT_EXTENSIONS_DIR=${HOSTDIR}/cert_extensions
>          STAPLINGDIR=${HOSTDIR}/stapling
> +        SSLGTESTDIR=${HOSTDIR}/nss_gtests

This shell script variable should be renamed "NSSGTESTDIR", right?

@@ +538,5 @@
>      R_EXT_SERVERDIR=../ext_server
>      R_EXT_CLIENTDIR=../ext_client
>      R_CERT_EXT=../cert_extensions
>      R_STAPLINGDIR=../stapling
> +    R_SSLGTESTDIR=../nss_gtests

This variable should be renamed "R_NSSGTESTDIR".

Comment 43

[Wan-Teh Chang]

(In reply to Wan-Teh Chang from comment #42)
> Comment on attachment 8516274 [details] [diff] [review]
> 8514717: bug_1064670 test and quickder changes for NSS; restructure gtests
>
[...]
> Please refrain from making big changes or adding new changes to
> this patch. At this point we need to stabilize this patch.

Just wanted to stress this point: I am fine with you not renaming the
directories and scripts as I suggested in my review comments. If you
agree with the new names, you can rename them later.

Comment 44

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

Attachment: bug_1064670 - be more strict on DER length decoding in quickder.c

(Carrying forward r=wtc from comment 42)

Wan-Teh: I accepted all of your changes on quickder.c, they're here in the patch. 

Per wtc's last comment 43, I've moved everything that isn't the quickder.c changes into Bug 1095118. Doing that last env variable rename spilled out to yet more files (init.sh and cert.sh), so it seemed best to just stabilize the real important part here.

Comment 45

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

Marking checkin-needed.

Comment 46

[Martin Thomson [:mt:]]

https://hg.mozilla.org/projects/nss/rev/a163e09dc4d5

Comment 47

[Daniel Veditz [:dveditz]]

This appears to have landed on the wrong branch, "tip" (3.16.2.3) vs "default" (3.18 beta). Is that what you intended, Martin?

Comment 48

[Lawrence Mandel [:lmandel] (use needinfo)]

dveditz confirmed that this issue is not critical enough to take this late in 34. As such, I'm marking 34 as wontfix.

Comment 49

[Daniel Veditz [:dveditz]]

(In reply to Daniel Veditz [:dveditz] from comment #47)
> This appears to have landed on the wrong branch, "tip" (3.16.2.3)

I guess it doesn't hurt to have it on that branch since we may want this fixed on ESR31, but it needs to land on the 3.18 mainline for sure.

Comment 50

[Martin Thomson [:mt:]]

Thanks for catching that Dan.  I'll land it on default.  (another reason that I shouldn't be allowed access to hg).

Comment 51

[Martin Thomson [:mt:]]

https://hg.mozilla.org/projects/nss/rev/e9a7991380db

Comment 52

[Kai Engert (:KaiE:)]

> This appears to have landed on the wrong branch, "tip" (3.16.2.3)

Clarification:

The patch was added to the 3.16.2 branch, but it was added on the branch after the creation of the 3.16.2.3 release.

It could potentially become part of a 3.16.2.4 release, if we decided to create one.

Comment 53

[Ryan VanderMeulen [:RyanVM]]

Should we update b2g32/b2g34/beta35 from 3.17.2 to 3.17.3 for this fix?

Comment 54

[Lukas Blakk [:lsblakk] use ?needinfo]

Is this safe to uplift?  If it's going to get into 35, it should get nominated by Dec 22.

Comment 55

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

Lukas: It's included in NSS 3.173 (bug 1088969) but that has larger changes that we probably don't want to uplift. After consulting with keeler, it seems that if we want to include this in 35 we'd need to private patch it in. This patch on its own is not dependent on anything else, and should be safe to uplift.

Comment 56

[Lukas Blakk [:lsblakk] use ?needinfo]

Thanks JC. Given that we've wontfixed it for several cycles and we're very late in the 35 cycle, it's not worth the risk to mess around here and instead let it ride to general release in 36.

Comment 57

[Al Billings [:abillings - ex-MoCo]]

Did we ever make a 3.16.2.4 release of NSS for ESR31?

Comment 58

[Kai Engert (:KaiE:)]

(In reply to Al Billings [:abillings] from comment #57)
> Did we ever make a 3.16.2.4 release of NSS for ESR31?

No, nobody asked for it, after I offered it in the above comment.