1245528 - (CVE-2016-1950) NSS Heap buffer overflow vulnerability in ASN1 certificate parsing

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Francis Gabriel]

Attachment: repro.tgz

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/48.0.2564.97 Safari/537.36

Steps to reproduce:

This vulnerability affects (at least) the latest NSS code available
from the mercurial.

By giving a malformed certificate (DER format) to the commonly used
SEC_ASN1DecodeItem() function, the parser crashes as shown in
the stacktrace attached to this post.

The joined archive file contains a simple C program and the invalid certificate
causing the issue.


Actual results:

The free of a pointer is crashing due to a previous memory corruption
caused by the memcpy() call in the sec_asn1d_parse_leaf() (secasn1d.c)
function.

After debugging, one can see the arena memory is effectively corrupted by
hundreds of bytes contained in the certificate.

----
Program received signal SIGSEGV, Segmentation fault.
0x00007ffff65fd5b8 in _int_free (av=0x7ffff6929620 <main_arena>, p=<optimized out>, have_lock=0) at malloc.c:3996
3996    malloc.c: No such file or directory.
(gdb) bt
#0  0x00007ffff65fd5b8 in _int_free (av=0x7ffff6929620 <main_arena>, p=<optimized out>, have_lock=0) at malloc.c:3996
#1  0x00007ffff6d6ae52 in PR_Free (ptr=0xfa78e0) at ../../../../pr/src/malloc/prmem.c:458
#2  0x00007ffff6d7e261 in PR_DestroyLock (lock=0xfa78e0) at ../../../../pr/src/pthreads/ptsynch.c:167
#3  0x00007ffff73c5a11 in PORT_FreeArena_Util (arena=0xfa7880, zero=1) at secport.c:338
#4  0x00007ffff73c0869 in SEC_ASN1DecoderFinish_Util (cx=0xfa79b0) at secasn1d.c:2982
#5  0x00007ffff73c0afd in SEC_ASN1Decode_Util (poolp=0xfa6eb0, dest=0xfa7090, theTemplate=0x7ffff793c580 <CERT_SignedDataTemplate>,
    buf=0x61b700 <in_cert> "0\202\002\001\060\201\001A0\r\006\tCCCCCCCCC\005", len=1023) at secasn1d.c:3093
#6  0x00007ffff73c0b50 in SEC_ASN1DecodeItem_Util (poolp=0xfa6eb0, dest=0xfa7090,
    theTemplate=0x7ffff793c580 <CERT_SignedDataTemplate>, src=0x7fffffffe4a0) at secasn1d.c:3107
#7  0x000000000040864d in main () at certutil.c:52
---



Expected results:

The expected result is the stop of the invalid certificate parsing with an error value contained in the return of SEC_ASN1DecodeItem()  function.

Comment 1

[Daniel Veditz [:dveditz]]

cmdtmc: you tell us which version of Chrome you're using in comment 0 but not which version of NSS you're testing. We did fix some bugs in this area in NSS 3.20.1 (shipped with Firefox 42) so presumably you're testing a newer version than that, but if you're testing "the NSS on my Linux Distro" it could be 3.19 or even earlier.

sec_asn1d_parse_leaf() makes me think of bug 1192028 in particular, and also bug 1202868 (both fixed in that version).

Keeler: can you take a look at this please?

Comment 2

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

Looks like decoding constructed BIT STRINGs is quite broken. For instance, this line:

  https://dxr.mozilla.org/mozilla-central/rev/584870f1cbc5d060a57e147ce249f736956e2b62/security/nss/lib/util/secasn1d.c#1679

    item->len = (item->len << 3) - state->bit_string_unused_bits;

interacts very poorly with this line:

  https://dxr.mozilla.org/mozilla-central/rev/584870f1cbc5d060a57e147ce249f736956e2b62/security/nss/lib/util/secasn1d.c#1606

    PORT_Memcpy (item->data + item->len, buf, len);

(That's right, this code converts the length of one bitstring component from bytes to bits and then interprets it as a byte offset when copying in the next bitstring component.)

I'll attach a reduced STR.

Comment 3

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

Attachment: vfychain.c

(I replaced cmd/vfychain since checkcert has been removed, I think.)

Comment 4

[Francis Gabriel]

Daniel: For the version of chrome, sorry about that, Bugzilla automatically included it in the report.

I'm not using NSS like "Linux on my distro", but a very recent one pulled (i built myself) from the official Mercurial at this commit:

---
changeset:   11825:7bbcda0719dc
tag:         tip
user:        Martin Thomson <[e-mail]>
date:        Mon Jan 04 17:29:17 2016 +1100
summary:     Bug 1226179 - Using -Werror on all unix platforms, r=gaston
---

This a very recent version, i think around 3.23, and that's why i selected the "trunk" option in the "Version:" combobox of this report header.

Comment 5

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

Attachment: 1245528-BIT_STRING-fix.diff

Ryan, this is another ASN.1 decoding bug, this time with BIT STRING. I had a quick look at the history of changes around where the bug is, and unless I'm missing some context, this has been present since it was written (or at least imported into mercurial).

Basically, sec_asn1d_parse_leaf wasn't treating the length of the output SECItem for BIT STRINGs as bits, so it would write past the end of allocated memory. At the same time, the bytes-to-bits conversion in sec_asn1d_parse_more_bit_string was unnecessary (and indeed, incorrect).

You can test this with asan and the modified vfychain.c (attachment 8715576 [details]) (and as a bit of a correction, where that file says "DER" I of course meant "BER").

Comment 6

[Ryan Sleevi]

Comment on attachment 8716476 [details] [diff] [review]
1245528-BIT_STRING-fix.diff

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

Just a quick remark; I'll try to page in more of the code later today and give a more thorough review (I want to see more of the code in context, and of course Splinter is awful for that, and I don't want to upload it to Rieldveld for the obvious reasons)

::: lib/util/secasn1d.c
@@ +1614,5 @@
> +            }
> +        }
> +       PORT_Memcpy (item->data + offset, buf, len);
> +        if (state->underlying_kind == SEC_ASN1_BIT_STRING) {
> +            item->len += (len << 3) - state->bit_string_unused_bits;

What about overflow here?

Comment 7

[Ryan Sleevi]

So in reading through, I think there's still a bug for the case where the outermost layer is indefinite, since that's going to trigger the subitem append case (Namely, case #2 / case #5 mentioned at http://mxr.mozilla.org/nss/source/lib/util/secasn1d.c#1799 ), because this will trigger the subitem append in http://mxr.mozilla.org/nss/source/lib/util/secasn1d.c#1898 , which is going to botch on the bitstring (due to line 1883) due to removing the shifts in sec_asn1d_parse_more_bit_string , at least as I read it.

You can see this manifest during concatenation in http://mxr.mozilla.org/nss/source/lib/util/secasn1d.c#2191

I *think* the right solution is to leave the shifting in sec_asn1d_parse_more_bit_string , but to make sure that sec_asn1d_parse_leaf is aware of the substring concatenation rules, similar to how sec_asn1d_concat_substrings is.

Note: I haven't tested this, so I could be wildly off, but when reading the code and looking at how the data flow for item->len was in the case of BITSTRING, it seemed like we would still end up botching the math. Let me know if I've misread...

Comment 8

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

Thanks for the review. I think I see what you're saying with sec_asn1d_add_to_subitems getting it wrong in the bitstring case - the passed-in len will be in bits, not bytes. If it were to copy the data, it would read past the end of the data buffer. However, since it looks like copy_data is only true in the case of an underlying kind of ANY, I'm not sure that's an issue here (it's definitely fragile code, though).

Also, I don't think having the shifting in sec_asn1d_parse_more_bit_string is going to work, because if sec_asn1d_parse_more_bit_string is called when some of the bitstring has already been read, it will take something that's already in bits and shift it again, which is wrong.

Finally, I found another preexisting error - in sec_asn1d_concat_substrings, the check for unused bits is incorrect at http://mxr.mozilla.org/nss/source/lib/util/secasn1d.c#2211:

2211 	    if (is_bit_string && (substring->next == NULL)
2212 			      && (substring->len & 0x7)) {
2213 		PORT_SetError (SEC_ERROR_BAD_DER);
2214 		state->top->status = decodeError;
2215 		return;
2216 	    }

Checking substring->next == NULL means that this will only be enforced for the last substring, which is the exact opposite of what should happen.

Comment 9

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

Attachment: 1245528-BIT_STRING-fix.diff v2

Comment 10

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

Attachment: vfychain.c

Comment 11

[Ryan Sleevi]

Comment on attachment 8718131 [details] [diff] [review]
1245528-BIT_STRING-fix.diff v2

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

::: lib/util/secasn1d.c
@@ +1605,5 @@
> +        unsigned long offset = item->len;
> +        if (state->underlying_kind == SEC_ASN1_BIT_STRING) {
> +            // If this is a BIT STRING, the length is bits, not bytes.
> +            offset = item->len >> 3;
> +            // The previous BIT STRING must have no unused bits.

Documentation: Either BITSTRING, bit-string, or bit string is what this file uses - let's not add a fourth permutation :)

Style: I would suggest putting the error control check (with the early return) earlier - that is, swapping lines 1609 - 1614 with line 1607-1608

@@ +1615,5 @@
> +        }
> +        PORT_Memcpy (item->data + offset, buf, len);
> +        if (state->underlying_kind == SEC_ASN1_BIT_STRING) {
> +            // Protect against overflow during the bytes-to-bits conversion.
> +            if (len >= 0x20000000) {

I believe this should be 0x20000000UL, right?

Alternatively,
if (len >= (1UL << 29))

or

if (len >= (ULONG_MAX >> 3)+1)

(right?)

Just trying to find a less magical magic value :)

@@ +1620,5 @@
> +                PORT_SetError (SEC_ERROR_BAD_DER);
> +                state->top->status = decodeError;
> +                return 0;
> +            }
> +            item->len += (len << 3) - state->bit_string_unused_bits;

This can still overflow, right? Is there a reason why this is safe?

Please, double check my math, but

if (len >= [magic]) {
  PORT_SetError (SEC_ERROR_BAD_DER);
  ...
}
unsigned long len_in_bits = (len << 3) - state->bit_string_unused_bits;
if (ULONG_MAX - item->len < len_in_bits) {
  PORT_SetError (...);
}
item->len += len_in_bits;

Comment 12

[Ryan Sleevi]

(In reply to David Keeler [:keeler] (use needinfo?) from comment #8)
> Thanks for the review. I think I see what you're saying with
> sec_asn1d_add_to_subitems getting it wrong in the bitstring case - the
> passed-in len will be in bits, not bytes. If it were to copy the data, it
> would read past the end of the data buffer. However, since it looks like
> copy_data is only true in the case of an underlying kind of ANY, I'm not
> sure that's an issue here (it's definitely fragile code, though).

Isn't this hit when you set the template to SEC_ASN1_GET(SEC_AnyTemplate) ? (FWIW, the { 0 } in the template[] is unnecessary, because that's only used for SEQUENCEs & SETs)

That is, if you give an ANY template the _ber_2 / _ber_3 case, I would expect them to trigger this, because it's going to concat all the bitstrings, and then attempt to copy the data. You may also run into this with an ANY/SAVE combo.

I'll try to give a better run at this tomorrow - I have some tweaks that make NSS significantly easier to fuzz that I want to discuss with Bob, which may make it easier to be more confident for this.

> Also, I don't think having the shifting in sec_asn1d_parse_more_bit_string
> is going to work, because if sec_asn1d_parse_more_bit_string is called when
> some of the bitstring has already been read, it will take something that's
> already in bits and shift it again, which is wrong.

The only time it adjusts that length is when state->place has been updated from duringBitString to beforeEndOfContents during the processing of sec_asn1d_parse_leaf, and that transition should only happen when all of state->pending has been consumed for that leaf (that is, it only tweaks it at the end of the bitstring processing). That is, I'm specifically thinking of http://mxr.mozilla.org/nss/source/lib/util/secasn1d.c#1610 as the only place in which it transitions out

Granted, I haven't traced this code. The only scenario I could think (and maybe you could confirm if this is what you meant) was the case where you have a definite-length constructed bitstring parent (thus causing a pre-allocation of |state->dest| based on how http://mxr.mozilla.org/nss/source/lib/util/secasn1d.c#1799 explains it), and we're processing definite-length subitems and storing directly into it.

In any event, to be explicit: Having re-read the code, I believe it's OK to do what you're doing, it was just not entirely obvious WHY you were doing it :)

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 47 since this is sec critical. Which other versions are affected?

Comment 15

[Daniel Veditz [:dveditz]]

Decoder: looks like we may need to tweak our cert fuzzer.

Comment 16

[Daniel Veditz [:dveditz]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #14)
> Tracking for 47 since this is sec critical. Which other versions are affected?

All of them according to comment 5.

Comment 17

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

(In reply to Ryan Sleevi from comment #11)
> ::: lib/util/secasn1d.c
> @@ +1605,5 @@
...
> Documentation: Either BITSTRING, bit-string, or bit string is what this file
> uses - let's not add a fourth permutation :)

Heh - sounds good. "bit string" seemed most common, so that's what I went with.

> Style: I would suggest putting the error control check (with the early
> return) earlier - that is, swapping lines 1609 - 1614 with line 1607-1608

Ok - swapped.

> @@ +1615,5 @@
...
> or
> 
> if (len >= (ULONG_MAX >> 3)+1)
> 
> (right?)
> 
> Just trying to find a less magical magic value :)

Good call. The ULONG_MAX one made the most sense to me.

> @@ +1620,5 @@
...
> > +            item->len += (len << 3) - state->bit_string_unused_bits;
> 
> This can still overflow, right? Is there a reason why this is safe?
> 
> Please, double check my math, but
> 
> if (len >= [magic]) {
>   PORT_SetError (SEC_ERROR_BAD_DER);
>   ...
> }
> unsigned long len_in_bits = (len << 3) - state->bit_string_unused_bits;
> if (ULONG_MAX - item->len < len_in_bits) {
>   PORT_SetError (...);
> }
> item->len += len_in_bits;

Ah, I see. Yes, This is necessary and correct. (This raises a related question - there are other places where this code adds lengths together (as bytes, not bits) - do we need to worry about overflow in those cases? This would seem to require inputs > ~4GB, if I understand correctly. Is there some earlier step that is filtering out such input?)

(In reply to Ryan Sleevi from comment #12)
> (In reply to David Keeler [:keeler] (use needinfo?) from comment #8)
> > Thanks for the review. I think I see what you're saying with
> > sec_asn1d_add_to_subitems getting it wrong in the bitstring case - the
> > passed-in len will be in bits, not bytes. If it were to copy the data, it
> > would read past the end of the data buffer. However, since it looks like
> > copy_data is only true in the case of an underlying kind of ANY, I'm not
> > sure that's an issue here (it's definitely fragile code, though).
> 
> Isn't this hit when you set the template to SEC_ASN1_GET(SEC_AnyTemplate) ?
> (FWIW, the { 0 } in the template[] is unnecessary, because that's only used
> for SEQUENCEs & SETs)
> 
> That is, if you give an ANY template the _ber_2 / _ber_3 case, I would
> expect them to trigger this, because it's going to concat all the
> bitstrings, and then attempt to copy the data. You may also run into this
> with an ANY/SAVE combo.

Looks like yes, except in that case the lengths are in bytes, so I don't think the code has the same issue (plus, the underlying_kind is not bitstring). (That is, unless I set up the test case incorrectly - I just replaced the template argument with SEC_ASN1_GET(SEC_AnyTemplate).)

> > Also, I don't think having the shifting in sec_asn1d_parse_more_bit_string
> > is going to work, because if sec_asn1d_parse_more_bit_string is called when
> > some of the bitstring has already been read, it will take something that's
> > already in bits and shift it again, which is wrong.
> 
> The only time it adjusts that length is when state->place has been updated
> from duringBitString to beforeEndOfContents during the processing of
> sec_asn1d_parse_leaf, and that transition should only happen when all of
> state->pending has been consumed for that leaf (that is, it only tweaks it
> at the end of the bitstring processing). That is, I'm specifically thinking
> of http://mxr.mozilla.org/nss/source/lib/util/secasn1d.c#1610 as the only
> place in which it transitions out
> 
> Granted, I haven't traced this code. The only scenario I could think (and
> maybe you could confirm if this is what you meant) was the case where you
> have a definite-length constructed bitstring parent (thus causing a
> pre-allocation of |state->dest| based on how
> http://mxr.mozilla.org/nss/source/lib/util/secasn1d.c#1799 explains it), and
> we're processing definite-length subitems and storing directly into it.

Right - this is the case where doing the bytes-to-bits conversion in sec_asn1d_parse_more_bit_string goes wrong. From what I can tell, that transition happens at the end of each sub-string, and it re-uses the same pre-allocated destination, so the overall length keeps getting shifted up 3 places.

> In any event, to be explicit: Having re-read the code, I believe it's OK to
> do what you're doing, it was just not entirely obvious WHY you were doing it
> :)

Ah, I see. Should I add a comment to the effect of the result of this discussion?

Comment 18

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

Attachment: 1245528-BIT_STRING-fix.diff v3

Comment 19

[Wan-Teh Chang]

Comment on attachment 8718953 [details] [diff] [review]
1245528-BIT_STRING-fix.diff v3

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

::: lib/util/secasn1d.c
@@ +1624,5 @@
> +                return 0;
> +            }
> +            // Protect against overflow when computing the total length in bits.
> +            unsigned long len_in_bits = (len << 3) - state->bit_string_unused_bits;
> +            if (ULONG_MAX - item->len < len_in_bits) {

item->len is an unsigned int, so we should use UINT_MAX instead of ULONG_MAX here.

Note: it is correct to use ULONG_MAX with |len| because |len| is an unsigned long.

@@ +1631,5 @@
> +                return 0;
> +            }
> +            item->len += len_in_bits;
> +        } else {
> +            item->len += len;

It seems that this addition should also be protected from integer overflow.

Comment 20

[Christian Holler (:decoder)]

Canceling needinfo as tsmith is working on ASN.1 fuzzing now.

Comment 21

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

Attachment: 1245528-BIT_STRING-fix.diff v4

Same again, but with more overflow protection. Also addressed Wan-Teh's comment regarding ULONG_MAX vs. UINT_MAX.

Comment 22

[Ryan Sleevi]

Comment on attachment 8720405 [details] [diff] [review]
1245528-BIT_STRING-fix.diff v4

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

Thanks!

Should we also add tests to the DER gtests?

Comment 23

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

Thanks for the reviews!

(In reply to Ryan Sleevi from comment #22)
> Should we also add tests to the DER gtests?

Good idea. I'll go ahead and write some for this bug and the others from last fall, but in a follow-up bug, since I think there's a bit of time pressure to get this bug wrapped up.

Comment 24

[Kai Engert (:KaiE:)]

What is the suggested timing for checking in the fix?
Can we go ahead and into hg/nss immediately, for immediate testing with nss buildbot?

Or would you like to postpone landing to be closer to a release?

Comment 25

[Ryan Sleevi]

Oh, totally was thinking for follow-up.

Comment 26

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

Comment on attachment 8720405 [details] [diff] [review]
1245528-BIT_STRING-fix.diff v4

[Security approval request comment]
How easily could an exploit be constructed based on the patch? Probably relatively easily.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? Well, not quite a bulls-eye, but near enough.

Which older supported branches are affected by this flaw? All of them.

If not all supported branches, which bug introduced the flaw? n/a

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? This patch should apply to all branches.

How likely is this patch to cause regressions; how much testing does it need? :tsmith mentioned he's already done some fuzzing with this patch applied and didn't find any issues with it. This probably don't need any special testing - existing mozilla-central tests are likely to catch regressions.

Comment 27

[Ryan Sleevi]

On testing, I should add that I've had a fuzzer running using libfuzzer with the patch from comment #9 and haven't picked anything up in the past week (and it found this bug in 10s)

Comment 28

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

Comment on attachment 8720405 [details] [diff] [review]
1245528-BIT_STRING-fix.diff v4

sec-approval+.

We'll want Aurora, Beta, and ESR38 patches too.

Comment 29

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

Ryan, are there any coordination concerns wrt. landing this? Looks like we've got the go-ahead on our end.

Comment 30

[Ryan Sleevi]

I spoke with our TPMs; given our limited exposure to this (gated on either user confirmation or based on local filesystem), we're treating it as lower severity. We'll land fixes around the same time, but don't require explicit coordination.

Mozilla may wish to give Apple a heads-up though, I believe this affects them as well (They call SEC_ASN1Decode via SecAsn1Decode)

Comment 31

[Kai Engert (:KaiE:)]

(In reply to Al Billings [:abillings] from comment #28)
> Comment on attachment 8720405 [details] [diff] [review]
> 1245528-BIT_STRING-fix.diff v4
> 
> sec-approval+.
> 
> We'll want Aurora, Beta, and ESR38 patches too.

ESR 38 uses NSS 3.19.2.2, it will require a NSS 3.19.2.3

Beta uses NSS 3.21, it will require a 3.21.1

Aurora is planned to be upgradd to NSS 3.22.1, it will need a NSS 3.22.2

That means we'll have to create three new versions of NSS.

Comment 32

[Kai Engert (:KaiE:)]

The fix seems to be isolated to the nss/lib/util part of NSS.
It seems that for Linux distributions that ship libnssutil3.so separately, rebuilding that library will be sufficient.

Comment 33

[Kai Engert (:KaiE:)]

(In reply to Kai Engert (:kaie) from comment #24)
> Can we go ahead and into hg/nss immediately, for immediate testing with nss
> buildbot?

Who wants to answer that question?

We should get some testing, prior to creating all the NSS branch releases, right?

Comment 34

[Kai Engert (:KaiE:)]

And another question is, should we hurry and get this fix landed for all releases scheduled for 2016-03-07 ?

If yes, what's the deadline for landing the patches into the respective branches, aurora, beta, ESR38 ?

If yes I could work on creating NSS releases tomorrow (Friday).

Comment 35

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

We need release management involvement here. I can't tell you the deadlines that they will accept for an NSS update.

Comment 36

[Kai Engert (:KaiE:)]

To clarify my statement from comment 31:

I'm offering to give you NSS releases, that have ONLY this one bugfix on top of the versions that the respective Mozilla branches are currently using.

Comment 37

[Daniel Veditz [:dveditz]]

The deadline for nightly/aurora is March 6. The deadline for beta is release candidates which are usually a week before, so Feb 27 maybe? but then there's no testing so the week before that is better, Feb 21-ish. ESR deadline, is probably more around Feb 26/7.

I'd very much want this fix in beta for this release rather than wait 6 more weeks (really 8 or 9 from now) because this affects products other than Firefox so it will likely become known in that time gap.

Please check into the nss branch for testing and start making releases.

Comment 38

[Daniel Veditz [:dveditz]]

If this didn't involve a "NSS release" (which sounds scary) we would certainly allow this single patch into beta at this time in the release cycle. If the NSS release is a point release with only this patch, as Kai described, this should not trouble anyone.

Comment 39

[Kai Engert (:KaiE:)]

Yes, I don't want to scare anyone, and there's no need to be scared.

"NSS release" here means "one NSS patch plus new NSS version numbers", that's all.

It's just that delivering patches for NSS always require that we create a new NSS releases, because NSS is a universal library, that is consumed by other applications, too.

Comment 40

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Kai Engert (:kaie) from comment #34)
> And another question is, should we hurry and get this fix landed for all
> releases scheduled for 2016-03-07 ?
> 
> If yes, what's the deadline for landing the patches into the respective
> branches, aurora, beta, ESR38 ?
> 
> If yes I could work on creating NSS releases tomorrow (Friday).

Beta: Ideally we want the NSS updates in the middle of a beta cycle but for Beta45 we are already at Beta7. So we need it asap as we gtb beta8 Monday (2/22). RC begins 2/29 and I really doubt we would take it then.

Aurora: It would be good to uplift next week, sooner the better.

ESR38: On Tuesday, 2/23, I will send an email to request enterprise users to smoke test ESR. Just so we get maximum testing, it would be ideal if the NSS update patch lands on esr38 branch by Monday noon PST.

Sylvestre, Liz, please add anything that I may have missed.

Comment 41

[Kai Engert (:KaiE:)]

Agreed with Al on text for NSS release notes:
  "Bug 1245528, fixed a bug in the ASN1 certificate parser".

Al will write more detailed notes when 45 and ESR 38.x ship.

We can update the NSS release notes to be more detailed on that day, too.

Comment 42

[Kai Engert (:KaiE:)]

https://hg.mozilla.org/projects/nss/rev/1fbe0c8dc9ba

Comment 43

[Kai Engert (:KaiE:)]

Landed the usual, obvious bustage fix on top of it, for older C compilers that require variable declaration at the beginning of the block.
https://hg.mozilla.org/projects/nss/rev/6294c234cdab

Comment 44

[Kai Engert (:KaiE:)]

(In reply to Kai Engert (:kaie) from comment #43)
> Landed the usual, obvious bustage fix on top of it, for older C compilers
> that require variable declaration at the beginning of the block.
> https://hg.mozilla.org/projects/nss/rev/6294c234cdab

Sigh, although I had searched, I had missed another place...:
https://hg.mozilla.org/projects/nss/rev/10dd89468e07

Comment 45

[Kai Engert (:KaiE:)]

(In reply to Ritu Kothari (:ritu) from comment #40)
> 
> Beta: Ideally we want the NSS updates in the middle of a beta cycle but for
> Beta45 we are already at Beta7. So we need it asap as we gtb beta8 Monday
> (2/22). RC begins 2/29 and I really doubt we would take it then.
> 
> Aurora: It would be good to uplift next week, sooner the better.
> 
> ESR38: On Tuesday, 2/23, I will send an email to request enterprise users to
> smoke test ESR. Just so we get maximum testing, it would be ideal if the NSS
> update patch lands on esr38 branch by Monday noon PST.

I should be able to upgrade ESR38 and Beta45 tomorrow, pending approvals.

For Aurora 46, we are waiting for root CA changes that Kathleen Wilson wants in Firefox 46. They are scheduled to be ready next week. Ideally I'd like to combine this fix with the CA updates for Aurora in a single NSS 3.22.2 release.

Comment 46

[Kai Engert (:KaiE:)]

Attachment: upgrade mozilla-esr38 to NSS 3.19.2.3 to pick up this fix

Comment 47

[Kai Engert (:KaiE:)]

Attachment: upgrade beta45 to NSS 3.21.1 to pick up this fix

Comment 48

[Kai Engert (:KaiE:)]

Attachment: upgrade aurora46 to NSS 3.22.2 to pick up this fix

Comment 49

[Huzaifa Sidhpurwala]

(In reply to Ryan Sleevi from comment #30)
> I spoke with our TPMs; given our limited exposure to this (gated on either
> user confirmation or based on local filesystem), we're treating it as lower
> severity. We'll land fixes around the same time, but don't require explicit
> coordination.
> 
Should this be downgraded from sec-critical then?

Comment 50

[Kai Engert (:KaiE:)]

(In reply to Huzaifa Sidhpurwala from comment #49)
> (In reply to Ryan Sleevi from comment #30)
> > I spoke with our TPMs; given our limited exposure to this (gated on either
> > user confirmation or based on local filesystem), we're treating it as lower
> > severity. We'll land fixes around the same time, but don't require explicit
> > coordination.
> > 
> Should this be downgraded from sec-critical then?

IIUC Ryan comment explains the Chromium perspective. My understanding is they have application level code that makes the issue less serious for them. The issue is critical for Firefox.

Comment 51

[Kai Engert (:KaiE:)]

(In reply to Ritu Kothari (:ritu) from comment #40)
> 
> Beta: Ideally we want the NSS updates in the middle of a beta cycle but for
> Beta45 we are already at Beta7. So we need it asap as we gtb beta8 Monday
> (2/22). RC begins 2/29 and I really doubt we would take it then.
> 
> Aurora: It would be good to uplift next week, sooner the better.
> 
> ESR38: On Tuesday, 2/23, I will send an email to request enterprise users to
> smoke test ESR. Just so we get maximum testing, it would be ideal if the NSS
> update patch lands on esr38 branch by Monday noon PST.

Please note that I've been ready to land since Friday.

I'm waiting for approvals.

Comment 52

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8720980 [details]
upgrade mozilla-esr38 to NSS 3.19.2.3 to pick up this fix

Taking it on all branches. Should be in 45 beta 9.

Comment 53

[Kai Engert (:KaiE:)]

Comment on attachment 8720980 [details]
upgrade mozilla-esr38 to NSS 3.19.2.3 to pick up this fix

https://hg.mozilla.org/releases/mozilla-esr38/rev/b03db72e32f6

Comment 54

[Kai Engert (:KaiE:)]

Comment on attachment 8720981 [details]
upgrade beta45 to NSS 3.21.1 to pick up this fix

https://hg.mozilla.org/releases/mozilla-beta/rev/e2b86793693f

Comment 55

[Kai Engert (:KaiE:)]

Comment on attachment 8720984 [details]
upgrade aurora46 to NSS 3.22.2 to pick up this fix

Landed a NSS 3.22.2-beta1 tag into aurora for immediate testing.
https://hg.mozilla.org/releases/mozilla-aurora/rev/fb1d0012abdc

The reason is we are waiting for the root CA changes prior to finalizing the 3.22.2 release. This should happen this week.

Comment 56

[Kai Engert (:KaiE:)]

I think NSS 3.23 should be the release that primarily announces this fix.

Comment 57

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Al, Dan: Fx47 is now on Beta channel (starting from today). I think now is a good time as any to ask when do we plan to update NSS version for Beta47 and what version will we be updating to? Has this already been planned?

Comment 58

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

I'm not sure why I'm being asked. I don't control NSS updates.

This fixed bug isn't really a good place to discuss NSS versions.

Firefox 46 has NSS version 3.22.3 and 47 already has 3.23 per https://kuix.de/mozilla/versions/ and comment 55 says this landed in 3.22.2, which is why this bug is fixed in 45 (a release ago).

Comment 59

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Al Billings [:abillings] from comment #58)
> I'm not sure why I'm being asked. I don't control NSS updates.
> 
> This fixed bug isn't really a good place to discuss NSS versions.
> 
> Firefox 46 has NSS version 3.22.3 and 47 already has 3.23 per
> https://kuix.de/mozilla/versions/ and comment 55 says this landed in 3.22.2,
> which is why this bug is fixed in 45 (a release ago).

Great! Thanks. I will update the status flags to reflect that.