1637222 - CKM_CDMF_CBC out-of-bounds read with undersized IV

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P3)

Description

[Guido Vranken]

Attachment: cdmf_oob_read.cpp

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:76.0) Gecko/20100101 Firefox/76.0

Steps to reproduce:

Compile NSS with ASAN and compile and run the attachment

If you change the IV size from 7 to 8, the bug does not manifest.

Actual results:

ASAN stack trace:

==31293==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x602000156874 at pc 0x0000009f50a9 bp 0x7ffd144ac970 sp 0x7ffd144ac968
READ of size 4 at 0x602000156874 thread T0
#0 0x9f50a8 in DES_InitContext /mnt/2tb/cf-nss/sandbox/nss/out/Debug/../../lib/freebl/desblapi.c:174:13
#1 0x9f6321 in DES_CreateContext /mnt/2tb/cf-nss/sandbox/nss/out/Debug/../../lib/freebl/desblapi.c:205:20
#2 0x8e87d8 in sftk_CryptInit /mnt/2tb/cf-nss/sandbox/nss/out/Debug/../../lib/softoken/pkcs11c.c:1034:35
#3 0x8ee37c in NSC_DecryptInit /mnt/2tb/cf-nss/sandbox/nss/out/Debug/../../lib/softoken/pkcs11c.c:1630:12
#4 0x827c9d in PK11_Decrypt /mnt/2tb/cf-nss/sandbox/nss/out/Debug/../../lib/pk11wrap/pk11obj.c:924:11

0x602000156877 is located 0 bytes to the right of 7-byte region [0x602000156870,0x602000156877)
allocated by thread T0 here:
#0 0x4dfc40 in __interceptor_malloc (/mnt/2tb/cf-nss/cryptofuzz/cryptofuzz+0x4dfc40)
#1 0x7d16e0 in PORT_Alloc_Util /mnt/2tb/cf-nss/sandbox/nss/out/Debug/../../lib/util/secport.c:87:14
#2 0x81b644 in pk11_ParamFromIVWithLen /mnt/2tb/cf-nss/sandbox/nss/out/Debug/../../lib/pk11wrap/pk11mech.c:988:44

Expected results:

No memory violation

Comment 1

[Daniel Veditz [:dveditz]]

We should make NSS more robust, but in practice does anyone call this function with the wrong size IV? And more particularly, it looks like Firefox is fine.

Comment 2

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

We'll just keep getting these, where bad inputs make for bad outcomes, but we for code that isn't deprecated, we should fix them...

Ben, can you do a quick sec analysis here? Basically, not sure if this is actually even a sec-low or just should be unhidden.

Comment 3

[Benjamin Beurdouche [:beurdouche]]

At first glance, looks indeed that the PK11 layer knows that mechanisms have certain parameter sizes fixed and that the DES_CreateContext function will use good defaults. In that context it seems that the API for DES_CreateContext is not defensive, which is fine for a low level API - we do it for HACL* - but should probably be documented in the code.

I would lean to classify unhidden here, but we should take the opportunity to audit the callers of that function before unhidding.

Comment 4

[Benjamin Beurdouche [:beurdouche]]

Actually, something sounded wrong about my initial analysis so I went back to look.
While I still think that having a non-defensive low-level API is fine, any trace that
has a caller of that does not enforce its preconditions has at least a missing defensive
check, if not a bug. And it is the case here according to the reported trace.

So I looked a bit deeper while I audited the callers...
First, notice that their is no guard on the fact the IV is not null, as discussed before,
this is fine as long as the callers respect the preconditions, for the IV, not null and
length of 8 bytes in the case the mode is not NSS_DES: /* DES ECB */
https://searchfox.org/mozilla-central/source/security/nss/lib/freebl/desblapi.c#143
https://searchfox.org/mozilla-central/source/security/nss/lib/freebl/desblapi.c#202

Searching for DES_InitContext then for its main caller DES_CreateContext in searchfox I got
the following calls to DES_CreateContext :

1. https://searchfox.org/mozilla-central/source/security/nss/lib/softoken/lowpbe.c#1224

These calls seem restricted to NSS_DES_EDE3_CBC NSS_DES_CBC which are the two
mechanisms that require an IV. This function seem to check that the IV passed is not null,
but doesn't check the size. I think there might be a problem here, the IV is a SECItem which
has a length field and it is not used to check that it has the proper length here:
https://searchfox.org/mozilla-central/source/security/nss/lib/softoken/lowpbe.c#1198

2. https://searchfox.org/mozilla-central/source/security/nss/lib/softoken/pkcs11c.c#121
3. https://searchfox.org/mozilla-central/source/security/nss/lib/softoken/pkcs11c.c#139

Those calls are for the NSS_DES (ECB) mode which doesn't require an IV, hence passing
NULL seems fine.

4. https://searchfox.org/mozilla-central/source/security/nss/lib/softoken/pkcs11c.c#1034

This function is the one from the trace reported by Guido above. There, for a mechanism
t the argument (unsigned char *)pMechanism->pParameter seem passed as the IV.

Looking at the code above that call reveals this guard:
https://searchfox.org/mozilla-central/source/security/nss/lib/softoken/pkcs11c.c#1008
Since this code is above the finish_des: goto, it won't be called in certain cases.

This is what seems to happen at (the line number doesn't match the trace for some reason):
https://searchfox.org/mozilla-central/source/security/nss/lib/softoken/pkcs11c.c#965
So while the underneath low level API is ok to be non-defensive, there seem to be a logic mistake
here which leads to call the function with a short IV input length provided by the fuzzer
without going through a defensive check (if my understanding of the fuzzing process is correct).
Btw while that check is pMechanism->ulParameterLen < 8 maybe it shoud be != 8 ?

5. https://searchfox.org/mozilla-central/source/security/nss/lib/softoken/pkcs11c.c#7725

In the ECB case, the pointer for the IV is passed null, but should not be used so this seems fine.
In the CBC case it comes from a pMechanism->pParameter struct set by the TLS layer where
the value seem to have a correct length. This code path will be unused in Firefox soon.
https://searchfox.org/mozilla-central/source/security/nss/lib/util/pkcs11t.h#1852

Note that I only did this extended checking of the parameters for the IV and not the other inputs...

Comment 5

[Benjamin Beurdouche [:beurdouche]]

Attachment: Bug 1637222 - Enforce IV length check for DES. r=kjacobs,jcj

Comment 6

[Daniel Veditz [:dveditz]]

While I still think that having a non-defensive low-level API is fine, any trace that has a caller of that does not enforce its preconditions has at least a missing defensive check, if not a bug.

We should at the very least have a debug assert for these assumptions.

Comment 7

[Benjamin Beurdouche [:beurdouche]]

JC, can you please r+ the patch when you have a moment so that we can land it?

Comment 8

[Kevin Jacobs [:kjacobs]]

https://hg.mozilla.org/projects/nss/rev/0c70232cb6d3a3328c72e6de1d1631bfe2cf8943

Comment 9

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

I'm planning to uplift this bug's patches to the 3.53 branch for whenever we make our next build of NSS destined for ESR 78.

Comment 10

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

NSS_3_53_BRANCH
https://hg.mozilla.org/projects/nss/rev/a7c4657f24201710bfd55482927e50b6e7955f25