1387919 - (CVE-2022-34476) "consumed == child->consumed" assert failed in sec_asn1d_reuse_encoding

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P3)

Description

[Gustavo Grieco]

Attachment: crash.asn1

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:54.0) Gecko/20100101 Firefox/54.0
Build ID: 20170630112252

Steps to reproduce:

Hi,

We found a failed assertion in the last revision of nss (a0a4e05dcdd5) when we try to print a certificate using pp.

To reproduce execute: ./pp -t c -i crash.asn1


Actual results:

It aborts:

Certificate:
Assertion failure: consumed == child->consumed, at secasn1d.c:1565

Using gdb, we can obtain the backtrace:

Program received signal SIGABRT, Aborted.
0x00007f66a124f8c0 in raise () from /usr/lib/libc.so.6
(gdb) bt
#0  0x00007f66a124f8c0 in raise () from /usr/lib/libc.so.6
#1  0x00007f66a1250f72 in abort () from /usr/lib/libc.so.6
#2  0x00007f66a22148b2 in PR_Assert (s=0x7f66a28879ee "consumed == child->consumed", file=0x7f66a2887480 "secasn1d.c", ln=1565)
    at ../../../../pr/src/io/prlog.c:553
#3  0x00007f66a28741f0 in sec_asn1d_reuse_encoding (state=0x100231908) at secasn1d.c:1565
#4  0x00007f66a287616c in SEC_ASN1DecoderUpdate_Util (cx=0x100231810, buf=0x100230597 "0\200\030", '\060' <repeats 49 times>, "\030\001\060", len=78)
    at secasn1d.c:2804
#5  0x00007f66a287681e in SEC_ASN1Decode_Util (poolp=0x100230dc0, dest=0x100230ef0, theTemplate=0x7f66a1d545e0 <CERT_CertificateTemplate>, 
    buf=0x100230490 "0\200\002\002\200", len=341) at secasn1d.c:3107
#6  0x00007f66a2876880 in SEC_ASN1DecodeItem_Util (poolp=0x100230dc0, dest=0x100230ef0, theTemplate=0x7f66a1d545e0 <CERT_CertificateTemplate>, 
    src=0x1002301c0) at secasn1d.c:3121
#7  0x000000010000cb3a in SECU_PrintCertificate (out=0x7f66a15bd580 <_IO_2_1_stdout_>, der=0x1002301c0, m=0x10001250b "Data", level=1)
    at secutil.c:2384
#8  0x000000010000f205 in secu_PrintSignedDataSigOpt (out=0x7f66a15bd580 <_IO_2_1_stdout_>, der=0x7fffffffdf50, m=0x10001175b "Certificate", level=0, 
    inner=0x10000cab6 <SECU_PrintCertificate>, withSignature=withSignature) at secutil.c:3194
#9  0x000000010000f2e4 in SECU_PrintSignedData (out=0x7f66a15bd580 <_IO_2_1_stdout_>, der=0x7fffffffdf50, m=0x10001175b "Certificate", level=0, 
    inner=0x10000cab6 <SECU_PrintCertificate>) at secutil.c:3212
#10 0x0000000100004b96 in main (argc=5, argv=0x7fffffffe058) at pp.c:150


Expected results:

It shouldn't abort.

Comment 1

[Daniel Veditz [:dveditz]]

Aren't PR_Assert()s always fatal at runtime? If so this is probably not exploitable in any way.

Comment 2

[Franziskus Kiefer [:franziskus]]

PR_Assert() are no-ops in release builds. So this works fine in release builds but will fail in debug builds, which is what we want I think. I'll have a look if there's anything here that we should fix but the assert is what we want.

Comment 3

[Daniel Veditz [:dveditz]]

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #2)
> PR_Assert() are no-ops in release builds.

PR_ASSERT() are no-ops, but PR_Assert() always fire. But this location turns out to be PORT_Assert() which is mapped to PR_ASSERT() and not PR_Assert() as I would naively expect from the lowercase letters.

> works fine in release builds but will fail in debug builds, which is what we
> want I think. I'll have a look if there's anything here that we should fix but
> the assert is what we want.

Please close or unhide the bug as appropriate when you're done with that investigation.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

We'll take a closer look at this in the near future. Let's keep it hidden until then.

Comment 5

[Daniel Veditz [:dveditz]]

Leaving the needinfo on Tim to assign an appropriate security rating.

Comment 6

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

Emailing Tim to follow up.

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

Let's call this sec-moderate. I'm inclined to think that this would mostly confuse the parser and cause it to either skip or re-read parts of the input and thus fail to parse invalid ASN.1. It's hard to exactly tell if there might be other ramifications, given the complexity of the parser.

Comment 8

[John Schanck [:jschanck]]

Took a look at this today, but did not finish. Some notes below.

Here's a der-ascii formatted reproducer.

SEQUENCE {
  SEQUENCE {
    INTEGER { 0 }
    SEQUENCE {
      OBJECT_IDENTIFIER { 0 }
    }
    SEQUENCE indefinite {
      SET indefinite {
        SEQUENCE indefinite {
          OBJECT_IDENTIFIER { 2.5.4.10 }
          PrintableString { "xxxxxxxx" }
        }
      }
    }
    SEQUENCE {
      UTCTime { "000000000000Z" }
      UTCTime { "999999999999Z" }
    }
  }
  SEQUENCE {
    OBJECT_IDENTIFIER { 2.5.4.6 }
  }
  BIT_STRING { 0 }
}

The issue is in the handling of

    SEQUENCE indefinite {
      SET indefinite {
        SEQUENCE indefinite {
          OBJECT_IDENTIFIER { 2.5.4.10 }
          PrintableString { "xxxxxxxx" }
        }
      }
    }

We read this chunk twice. We first save the entire sequence into a CERTCertificate derIssuer field. Then we parse it into the issuer field. On the second pass we seem to lose the "indefinite" flag while decoding the outer sequence and we leave an EOC unconsumed.
Apart from the assertion, this doesn't cause any problems. That's because we update the buffer position based on the number of bytes consumed in the first pass, consumed, rather than in the second child->consumed.

    PORT_Assert(consumed == child->consumed);
[...]
    state->consumed += consumed;

My sense right now is that this is a bug rather than a security issue.

Of course there's clearly a correctness problem, and it's possible that it can manifest in a more dangerous way.

Comment 9

[John Schanck [:jschanck]]

An error occurs when parsing an indefinite-length encoded SEQUENCE that is inside an indefinite-length encoded GROUP (SEQUENCE-OF or SET-OF).

tldr; the parser interprets a single EOC as ending both the SEQUENCE and the GROUP containing it.

In an iteration over elements of a GROUP (sec_asn1d_next_in_group), the child of the current state is responsible for parsing the GROUP's end-of-contents octets---a call to sec_asn1d_parse_end_of_contents(state->child) sets the endofcontents flag for state->child and a later call to sec_asn1d_next_in_group checks state->child->endofcontents and terminates the iteration (here).

An iteration over elements of a SEQUENCE (sec_asn1d_next_in_sequence), on the other hand, terminates when the child's template points to a template terminator (here). The current state, not its child, handles the end-of-contents octets, and a call to sec_asn1d_parse_end_of_contents(state) sets the endofcontents flag for state even though this is not needed to signal the end of iteration over the SEQUENCE.

The error occurs when state points to an indefinite-length encoded GROUP and state->child points to an indefinite-length encoded SEQUENCE. The call to sec_asn1d_parse_end_of_contents to read the SEQUENCE's end-of-contents octets sets the endofcontents flag for state->child, and this is misinterpreted as an end-of-iteration signal for the surrounding GROUP.

When the GROUP's end-of-contents octets are present, this always leads to a decoding error. When they are not present, the decoder can mistakenly accept malformed ASN.1.

Comment 10

[John Schanck [:jschanck]]

Attachment: Bug 1387919 - Fix secasn1d parsing of indefinite SEQUENCE inside indefinite GROUP. r=keeler

Comment 11

[Gustavo Grieco]

I'm the original reporter of this issue and I'm happy to see some activity 5 years after this was found.

When they are not present, the decoder can mistakenly accept malformed ASN.1.

Giver your analysis, I'm wondering if this affects certificates parsed by Firefox or it is just localized in the specific calls of the NSS printer.

Comment 12

[John Schanck [:jschanck]]

This affects a legacy decoder that is not used to parse WebPKI X.509 certificates in Firefox. We need to do some further investigation to rule out an impact on PKCS#7 and PKCS#12 as used in Firefox. Our current assessment is that any possible impact is quite limited. To expand on my "accept malformed ASN.1" comment: I suspect the worst-case here is that the decoder admits an item that is missing a trailing EOC.

Comment 14

[John Schanck [:jschanck]]

https://hg.mozilla.org/projects/nss/rev/1811eec2499702da599293e49a8827bccda77ca4

Comment 15

[Tom Ritter [:tjr]]

Attachment: advisory.txt

Comment 16

[Daniel Veditz [:dveditz]]

We are awarding a bounty for this issue, but it's a bit on the small end I'm afraid. Our bounty program only covers NSS to the extent it's used in Firefox and this one doesn't seem to.