1663091 - assert failure in sec_asn1d_next_in_group when parsing asn1

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[slei.casper]

Attachment: poc

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/85.0.4183.83 Safari/537.36

Steps to reproduce:

1. compile the following code with nss using asan
2. run compiled program with poc

#include "secutil.h"
#include "nss.h"
#include "pk11pub.h"

int main(int argc, char **argv){
    if (NSS_NoDB_Init(NULL) != SECSuccess) {
        return 1;
    }

    PRFileDesc* file = PR_Open(argv[1], PR_RDONLY, 0);

    SECItem data;
    memset(&data, 0, sizeof(data));
    if (SECU_ReadDERFromFile(&data, file, PR_FALSE, PR_FALSE) != SECSuccess) {
        return 1;
    }

    PK11SlotInfo* slot = PK11_GetInternalSlot();
    if (!slot) {
        return 1;
    }

    SECKEYPrivateKey* privKey;
    if (PK11_ImportDERPrivateKeyInfoAndReturnKey(slot, &data, NULL, NULL, PR_FALSE, PR_FALSE, KU_ALL, &privKey, NULL) != SECSuccess) {
        return 1;
    }

    return 0;
}

Actual results:

result:

#0  __GI_raise (sig=sig@entry=0x6) at ../sysdeps/unix/sysv/linux/raise.c:51
#1  0x00007ffff5a958b1 in __GI_abort () at abort.c:79
#2  0x00007ffff7afc9fa in PR_Assert (s=0x7ffff73b8000 <.str.28> "state->indefinite", file=0x7ffff73b7800 <.str.1> "../../lib/util/secasn1d.c", ln=0x7c1) at ../../../../pr/src/io/prlog.c:571
#3  0x00007ffff736eaf3 in sec_asn1d_next_in_group (state=<optimized out>) at ../../lib/util/secasn1d.c:1985
#4  SEC_ASN1DecoderUpdate_Util (cx=<optimized out>, buf=0x61a000001973 "", len=0x3ca) at ../../lib/util/secasn1d.c:2795
#5  0x00007ffff737fd9a in SEC_ASN1Decode_Util (poolp=<optimized out>, dest=0x61d000005aa0, theTemplate=0x7ffff7a89000 <SECKEY_PrivateKeyInfoTemplate>, buf=<optimized out>, len=0x4bd) at ../../lib/util/secasn1d.c:3106
#6  SEC_ASN1DecodeItem_Util (poolp=<optimized out>, dest=0x61d000005aa0, theTemplate=0x7ffff7a89000 <SECKEY_PrivateKeyInfoTemplate>, src=<optimized out>) at ../../lib/util/secasn1d.c:3120
#7  0x00007ffff77603b4 in PK11_ImportDERPrivateKeyInfoAndReturnKey (slot=0x618000000080, derPKI=0x7fffffffe1a0, nickname=0x0, publicValue=0x0, isPerm=0x0, isPrivate=0x0, keyUsage=0xff, privk=0x7fffffffe1e0, wincx=0x0)
    at ../../lib/pk11wrap/pk11pk12.c:280
#8  0x00000000004eef1a in main (argc=<optimized out>, argv=<optimized out>) at ../fuzzsrc/bintest.c:28
#9  0x00007ffff5a76b97 in __libc_start_main (main=0x4eed50 <main>, argc=0x2, argv=0x7fffffffe368, init=<optimized out>, fini=<optimized out>, rtld_fini=<optimized out>, stack_end=0x7fffffffe358) at ../csu/libc-start.c:310
#10 0x000000000041df8a in _start ()

Expected results:

related source code

1949   │ static void
1950   │ sec_asn1d_next_in_group(sec_asn1d_state *state)
1951   │ {
1952   │     sec_asn1d_state *child;
1953   │     unsigned long child_consumed;
1954   │
1955   │     PORT_Assert(state->place == duringGroup);
1956   │     PORT_Assert(state->child != NULL);
1957   │
1958   │     child = state->child;
1959   │
1960   │     child_consumed = child->consumed;
1961   │     child->consumed = 0;
1962   │     state->consumed += child_consumed;
1963   │
1964   │     /*
1965   │      * If our child was just our end-of-contents octets, we are done.
1966   │      */
1967   │     if (child->endofcontents) {
1968   │         /* XXX I removed the PORT_Assert (child->dest == NULL) because there
1969   │          * was a bug in that a template that was a sequence of which also had
1970   │          * a child of a sequence of, in an indefinite group was not working
1971   │          * properly.  This fix seems to work, (added the if statement below),
1972   │          * and nothing appears broken, but I am putting this note here just
1973   │          * in case. */
1974   │         /*
1975   │          * XXX No matter how many times I read that comment,
1976   │          * I cannot figure out what case he was fixing.  I believe what he
1977   │          * did was deliberate, so I am loathe to touch it.  I need to
1978   │          * understand how it could ever be that child->dest != NULL but
1979   │          * child->endofcontents is true, and why it is important to check
1980   │          * that state->subitems_head is NULL.  This really needs to be
1981   │          * figured out, as I am not sure if the following code should be
1982   │          * compensating for "offset", as is done a little farther below
1983   │          * in the more normal case.
1984   │          */
1985   │         PORT_Assert(state->indefinite);        <--------------------------------- assert
1986   │         PORT_Assert(state->pending == 0);
1987   │         if (child->dest && !state->subitems_head) {
1988   │             sec_asn1d_add_to_subitems(state, child->dest, 0, PR_FALSE);
1989   │             child->dest = NULL;
1990   │         }
1991   │
1992   │         child->place = notInUse;
1993   │         state->place = afterGroup;
1994   │         return;
1995   │     }

Comment 1

[slei.casper]

any updates? I think this is pretty critical. Because Firefox could parse a cert file in TLS stream when connecting to attacker's server.

Comment 2

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

For some reason this didn't appear in my security triage queue, thank you for the needinfo. Adding in the team.

On first pass though, asn1d is not typically used by Firefox; the TLS stack uses QuickDER, which is much more testable/fuzzable than the indefinite decoder. That said, this streaming decoder does make appearances still, particularly by Thunderbirdo, legacy-format addons, and of course PK11_ImportDERPrivateKeyInfoAndReturnKey is an exported function, so all user-side uses are potentially exploitable.

Keeping my needinfo while we figure out who does the impact assessment.

Comment 3

[Kevin Jacobs [:kjacobs]]

Thanks for the report. I'm not seeing a security issue here, rather a couple of misplaced assertions:

• The assertion you point to checks that an indefinite length-encoded value is itself not nested within a definite length-encoded value (when the assertion fails, state refers to the parent value).
• The second assertion, which also fails, checks that the parent group contains no more data after the indefinite child's EndOfContents.

No memory violation occurs when the assertions are removed, and AFAIK, neither of the asserted conditions are required by ASN.1.

I'll attach a more minimal POC and I'd appreciate another set of eyes on this (at least w.r.t. the relevant ASN.1 rules). Barring any differing opinion, I think this can be unhidden.

Comment 4

[Kevin Jacobs [:kjacobs]]

Attachment: poc2

minimized poc

Comment 5

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

After reviewing Kevin's analysis and the code, I agree the impacts here appear to be minimal. The biggest danger would be if in release builds we passed the assert leading to a future memory error at some later point after, and while the code is very complex, there's no obvious path to that.

I think this is probably a case where we could/should replace the assertion with an error path, however this parser was ... not designed to emit errors, among the many reasons we've been reducing the use across the board.

From Firefox's point of view, I think this bug can be unhidden. However, it might have more significant impacts to Thunderbird, so I want to check in with Kai on TB's feelings regarding that, first.

Comment 6

[Kai Engert [:KaiE:]]

Thanks for the ping.

That function is called by PrivateKeyFromPkcs8, which is used by WebCrypto. Isn't that an issue for Firefox? Could some web page using it cause Firefox to crash?

Comment 7

[Kai Engert [:KaiE:]]

If this is causing a crash with remote data, and if it could be used to DoS Thunderbird users, then I'd prefer to keep it hidden until we have a fix.

You said this function isn't supposed to return an error.
What would be the correct graceful behavior when this assertion is hit?
Would it be acceptable to stop all further processing?

The function is given a state pointer. Without knowing the code, could the state be modified in a way that causes the surrounding code to stop processing?

Comment 8

[Kevin Jacobs [:kjacobs]]

The crash only occurs on the assertions. If those are removed (or when they're compiled out in Release), no crash or memory violation occurs.

Comment 9

[Kevin Jacobs [:kjacobs]]

If we do want to block this completely, setting cx->status = decodeError in sec_asn1d_next_in_group will do the trick. However, that might break something since release builds don't treat this as an error.

Comment 10

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

The way the asserts compile, we're pretty much stuck until trying it in release. Still, it's invalid.

We probably should just set the decodeError to prompt the early return and keep the assert.

Comment 11

[Kai Engert [:KaiE:]]

Thanks for the clarification, so it doesn't crash, and no problematic code path is known.

(In reply to J.C. Jones [:jcj] (he/him) [increased latency due to COVID-19] from comment #5)

The biggest danger would be if in release builds we passed the assert leading to a future memory error at some later point after, and while the code is very complex, there's no obvious path to that.

Unsure if I understand - does this mean you're worried that bad input could cause a corrupted memory state later?

Comment 12

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

So firstly, let me say I hate asn.1.

Next, okay you're right, this isn't invalid. QuickDER and pkix::der consider it invalid, but there's plenty of use cases for wrapping infinite sequences inside definite ones. IMO they should not be allowed in the WebPKI, but that's my opinion. (And of course, we don't allow it in most of Firefox, just addons and pkcs8 processing).

(In reply to Kai Engert (:KaiE:) from comment #11)

Unsure if I understand - does this mean you're worried that bad input could cause a corrupted memory state later?

That was my concern, but the more I look at this, I don't see any basis for that concern. Every path I go down seems to handle it alright, and of course we haven't seen bugs about this in release Firefox.

So I think I'm swayed to removing the assert. I imagine it was put there because this is nonsensical for WebPKI uses, but this routine isn't really used in WebPKI contexts anymore.

Comment 13

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

I'll post a patch, but I still think we're OK unhiding this bug.

Comment 14

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

Attachment: Bug 1663091 - Remove unnecessary assertions in the streaming ASN.1 decoder r?kaie,kjacobs

The streaming ASN.1 decoder had an assertion that, on debug builds, blocked
embedding indefinite-length fields inside of definite-length fields/contexts,
however that behavior does work correctly, and is valid ASN.1: it tends to
happen when wrapping a signature around existing ASN.1-encoded data, if that
already-encoded data had an indefinite length.

Really this assertion was just overzealous. The conditional after the assert
handles the case well, and memory sanitizers have not found issue here either.

Comment 15

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

I think we should unhide this and land it in NSS 3.59 early.

Comment 16

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

Unhiding and landing.

Comment 17

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

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