1767590 - (CVE-2022-31741) Uninitialized variable leads to invalid/arbitrary memory read in S/MIME decryption

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[yaniv]

Attachment: POC file

This is a bug I encountered in the decryption process of CMS messages in NSS, triggerable by passing a malicious message to NSS_CMSDecoder_Update. This flow is used by Thunderbird and Evolution when attempting to read S/MIME encrypted emails.

In NSS_CMSDigestContext_FinishSingle, variable dp is supposed to be initialized by a successful call to NSS_CMSDigestContext_FinishMultiple, and then dereferenced and passed to SECITEM_CopyItem as the third argument:

SECStatus
NSS_CMSDigestContext_FinishSingle(NSSCMSDigestContext *cmsdigcx,
                                  PLArenaPool *poolp,
                                  SECItem *digest)
{
    ...
    SECItem **dp;
    ...
    /* get the digests into arena, then copy the first digest into poolp */
    rv = NSS_CMSDigestContext_FinishMultiple(cmsdigcx, arena, &dp);
    if (rv == SECSuccess) {
        /* now copy it into poolp */
        rv = SECITEM_CopyItem(poolp, digest, dp[0]);
    }
    ...

However, NSS_CMSDigestContext_FinishMultiple can return SECSuccess without initializing the passed dp argument. This happens in case cmsdigcx->saw_contents == 0:

    /* no contents? do not finish digests */  
    if (digestsp == NULL || !cmsdigcx->saw_contents) {
        rv = SECSuccess;
        goto cleanup;
    }

I'm not sure exactly what in the input causes the program to arrive to this state, but it seems to be some bug in the ASN.1 parsing code.

When this happens, dp remains uninitialized when SECITEM_CopyItem is called, and one of following may happen:
1. Crash in NSS_CMSDigestContext_FinishSingle when attempting to read dp[0].
2. Crash in SECITEM_CopyItem when attempting to read from pointer dp[0]->data.
3. If stack memory is properly groomed by an attacker, valid memory of dp[0]->len bytes will be read from pointer dp[0]->data into an object.

I reproduced this on Thunderbird and Evolution (.p7m email attachment), and on cmsutil compiled from trunk.

Steps to reproduce:
$ cmsutil -D -d . -i poc.cms

MSan log:

==3958692==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0x7ffff75a00eb in NSS_CMSDigestContext_FinishSingle ./lib/smime/cmsdigest.c:258:46
    #1 0x7ffff759b3f0 in NSS_CMSDigestedData_Decode_AfterData ./lib/smime/cmsdigdata.c:187:14
    #2 0x7ffff7594f84 in nss_cms_after_data ./lib/smime/cmsdecode.c:367:18
    #3 0x7ffff758da0a in nss_cms_decoder_notify ./lib/smime/cmsdecode.c:185:21
    #4 0x7ffff77eed10 in sec_asn1d_notify_after ./lib/util/secasn1d.c:437:5
    #5 0x7ffff77cea44 in sec_asn1d_next_in_sequence ./lib/util/secasn1d.c:2078:5
    #6 0x7ffff77b2e35 in SEC_ASN1DecoderUpdate_Util ./lib/util/secasn1d.c:2823:17
    #7 0x7ffff758e7a2 in NSS_CMSDecoder_Update ./lib/smime/cmsdecode.c:683:14
    #8 0x4ab1c3 in decode ./cmd/smimetools/cmsutil.c:191:10
    #9 0x4a678a in main ./cmd/smimetools/cmsutil.c:1455:24
    #10 0x7ffff6e35fcf in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
    #11 0x7ffff6e3607c in __libc_start_main csu/../csu/libc-start.c:409:3
    #12 0x420db4 in _start (./dist/Debug/bin/cmsutil+0x420db4)

  Uninitialized value was created by an allocation of 'dp' in the stack frame of function 'NSS_CMSDigestContext_FinishSingle'
    #0 0x7ffff759fa90 in NSS_CMSDigestContext_FinishSingle ./lib/smime/cmsdigest.c:242

SUMMARY: MemorySanitizer: use-of-uninitialized-value ./lib/smime/cmsdigest.c:258:46 in NSS_CMSDigestContext_FinishSingle

Comment 1

[Dennis Jackson]

Hi Yaniv, thank you for the report. I've confirmed the PoC works and I'm taking a look now.

Comment 2

[Dennis Jackson]

This issue may also impact Firefox, as we call into NSS_CMSMessage_CreateFromDER when verifying a PKCS#7 signature on an addon.

As the reporter notes NSS_CMSDigestContext_FinishMultiple can return SECSuccess but leave its out parameter digestsp unchanged, which is a memory safety violation for code which assumes it will be set if the function succeeds. The function contract is pretty ugly.digestsp is a pointer to an array of SECItems* and the array is variously:

• is left as-is (if the digest context hasn't seen any data)
• is set, but points to a single element which is NULL (if we did see data, but digest count is set to 0),
• is set, points to a NULL-terminated array of SECItem pointers (otherwise).

Changing the callers (cmsdigest.c:NSS_CMSDigestContext_FinishSingle, cmsutil.c:decode) to properly initialise the outparam is sufficient to fix the test case, but I'm concerned about NSS_CMSSignedData_Decode_AfterData and NSS_CMSSignedData_Encode_AfterData where the outparam comes from and lives on in a struct (NSSCMSSignedData).

I'd prefer to update NSS_CMSDigestContext_FinishMultiple to ensure that the outparam is set to NULL if the function fails. This ensures that if that if there is a consumer of NSSCMSSignedData which is missing a null-check, the attacker gets a null pointer dereference rather than an arbitrary read. I'd also like to promote cmsdigcx->saw_contents being false to a failure, but I'm unsure what might be relying on that behaviour.

I've attached a patch which does all three. It could do with a lookover from someone that's more familiar with this codebase.

Comment 3

[Dennis Jackson]

Attachment: Bug 1767590 - Initialize pointers passed to NSS_CMSDigestContext_FinishMultiple r=#nss-reviewers,kaie

Comment 4

[Daniel Veditz [:dveditz]]

Adding the bounty flag at the reporter's request.

Comment 5

[Kai Engert [:KaiE:]]

(In reply to Dennis Jackson from comment #2)

It could do with a lookover from someone that's more familiar with this codebase.

I'm afraid we don't have any active contributors who are familiar with this code.

Comment 6

[Dennis Jackson]

I found some time to take a further look at the smime codebase. I used weggli to audit for similar code patterns where the address of uninitialized values was being passed as a parameter which didn't turn up any new issues.

With respect to the bug in NSS_CMSDigestContext_FinishSingle, its only reachable when parsing DigestedData content and support for that content type has never properly been implemented (code) and isn't part of the smime standard. However, the code is still reachable from Firefox and Thunderbird because we fully parse the CMS data in NSS_CMSMessage_CreateFromDER and use the templates in cmsasn1.c prior to validating the format.

John pointed out that we can't NULL out digestsp if there's an error as the encoder is relying on the existing behaviour. As we're currently investigating if we can remove this code entirely, I've updated the patch to the sufficient fix outlined my earlier comment. With the fix, the missing data is noticed here and the parser raises a decodeError.

Comment 7

[Kai Engert [:KaiE:]]

Do you plan to create a NSS 3.68.x release and uplift to ESR 91.x ?

Comment 8

[John Schanck [:jschanck]]

I found a memory leak in an adjacent codepath.

$ ascii2der <<EOF | cmsutil -D -d .  
SEQUENCE {
  # digestedData
  OBJECT_IDENTIFIER { 1.2.840.113549.1.7.5 }
  [0] {
    SEQUENCE {
      INTEGER { 0 }
      SEQUENCE {
        # sha256
        OBJECT_IDENTIFIER { 2.16.840.1.101.3.4.2.1 }
      }
      SEQUENCE {
        # signedData
        OBJECT_IDENTIFIER { 1.2.840.113549.1.7.2 }
        OCTET_STRING { \`aa\` }
      }
    }
  }
}
EOF
cmsutil: failed to decode message.
cmsutil: problem decoding: SEC_ERROR_BAD_DER: security library: improperly formatted DER-encoded message.

=================================================================
==468819==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 72 byte(s) in 1 object(s) allocated from:
    #0 0x7fc457c5d037 in __interceptor_calloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:154
    #1 0x7fc4574758c8 in PR_Calloc ../../../../pr/src/malloc/prmem.c:458
    #2 0x7fc45796d096 in PORT_ZAlloc_Util ../../lib/util/secport.c:116
    #3 0x7fc4578bcd4d in nss_cms_before_data ../../lib/smime/cmsdecode.c:253
    #4 0x7fc4578bc845 in nss_cms_decoder_notify ../../lib/smime/cmsdecode.c:169
    #5 0x7fc457955f1d in sec_asn1d_notify_before ../../lib/util/secasn1d.c:425
    #6 0x7fc45795e49c in sec_asn1d_next_in_sequence ../../lib/util/secasn1d.c:2175
    #7 0x7fc45796166f in SEC_ASN1DecoderUpdate_Util ../../lib/util/secasn1d.c:2832
    #8 0x7fc4578be73a in NSS_CMSDecoder_Update ../../lib/smime/cmsdecode.c:663
    #9 0x5651d59e980a in decode ../../cmd/smimetools/cmsutil.c:191
    #10 0x5651d59f0e92 in main ../../cmd/smimetools/cmsutil.c:1455
    #11 0x7fc45752fd09 in __libc_start_main ../csu/libc-start.c:308

SUMMARY: AddressSanitizer: 72 byte(s) leaked in 1 allocation(s).

If you change the final object identifier from PKCS#7 signedData (1.2.840.113549.1.7.2) to PKCS#7 data (1.2.840.113549.1.7.1) this example triggers the original bug.

Comment 9

[Dennis Jackson]

Comment on attachment 9274964 [details]
Bug 1767590 - Initialize pointers passed to NSS_CMSDigestContext_FinishMultiple r=#nss-reviewers,kaie

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Fairly easily. The changes clearly indicate an uninitialized read which can be reached from the addons signature verification code path in Firefox or from processing an SMIME message in Thunderbird.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes
• Which older supported branches are affected by this flaw?: all
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?:
• How likely is this patch to cause regressions; how much testing does it need?: Very unlikely to cause regressions as we're just initialising previously unset pointers.
• Is Android affected?: Unknown

Comment 10

[Dennis Jackson]

[Tracking Requested - why for this release]:
This a sec-high memory safety bug in NSS which impacts Firefox, ESR & Thunderbird. Security approval has been requested.

Comment 11

[Tom Ritter [:tjr]]

Comment on attachment 9274964 [details]
Bug 1767590 - Initialize pointers passed to NSS_CMSDigestContext_FinishMultiple r=#nss-reviewers,kaie

Approved to land and request uplift

Comment 12

[Dennis Jackson]

https://hg.mozilla.org/projects/nss/rev/8bb2f26245b91be5e51a0278cf446814d620e05b

Comment 13

[Dennis Jackson]

Note on timelines: This will be fixed in the upcoming Firefox 101, ESR 91 and Thunderbird releases on May 31st. We'll also announce a new release of NSS on May 31st with the included bug fix and associated release notes.

Comment 14

[Tom Ritter [:tjr]]

Attachment: advisory.txt

Comment 15

[yaniv]

Can you please give credit to "Cybeats PSI Team" instead?