579822 - potential OOB read of plaintext->buf[-1] in in ssl3_HandleRecord

Status: RESOLVED WONTFIX

(NSS :: Libraries, defect)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US) AppleWebKit/533.4 (KHTML, like Gecko) Chrome/5.0.375.99 Safari/533.4
Build Identifier: 

[Sorry if this bug report looks strange. I had to report it with the Bugzilla Helper because the normal bug reporting UI for bugzilla.mozilla.org for NSS is broken right now.]

When libssl is configured to use a PKCS#11 module for bulk decryption operations it (indirectly) calls the PKCS#11 module’s C_DecryptUpdate function to do the actual decryption. There is some ambiguity in the PKCS#11 specification about what should happen when C_DecryptUpdate and similar functions are given inputs which are not integral multiples of the block cipher’s block size. Some implementations, like NSS’s softoken, will return an error code. Other implementations may decrypt all but the last (partial) block and return SUCCESS. libssl requires the softoken-like behavior and doesn’t detect the case where the PKCS#11 module hasn’t fully decrypted the input. 

For example, let’s say the bulk cipher is AES (which has 16-byte blocks) with SHA1 (20 bytes) for the HMAC and the input is 47 bytes long. libssl will ask the PKCS#11 module to decrypt the 47 bytes, but the PKCS#11 module may only output 32 bytes and return success. But, libssl will assume it returned 47 bytes of output. When libssl checks the padding and MAC, it will be reading from memory that was either unintialized (if this was the first record, or the longest record so far) or which contains the plaintext fragment/MAC/padding from a previous record or records.

Could somebody exploit this? Well, the MAC in a TLS record only protects the unpadded length of the plaintext, not the padded length. So, in theory an attacker could shorten a valid TLS record in transit, and if libssl doesn’t reject it, it can learn that some previous record had a plaintext and/or padding where a certain section contained a sequence (e.g. 0E0E0E0E-0E0E0E0E-0E0E0E0E-0E0E0E in this case) that could be interpreted as valid padding for the current record. For example, let’s say the plaintext of each record could be either “yes” or “no”. The padding for the previous record would be valid for the current record if they were the same, and invalid for the current record if they were different, because the plaintexts are different lengths. An attacker could truncate the current record by one byte, and if libssl doesn’t reject the record, then it knows that the padding for the previous record was accepted as padding for the current record, or vice versa. Variations of this apply when some previous record was longer than the current record (then libssl would use part of the plaintext of the previous record as the padding for the current record) and when the peer TLS implementation was attempting to take advantage of TLS’s ability to prevent some forms of traffic-analysis with padding that is longer than the minimum required by the protocol. Perhaps there are also other, more generally useful, ways to exploit this bug. I am not sure, as I do not know that much about cryptography.

Note that recently there was a thread on mozilla.dev.tech.crypto “C_EncryptUpdate( inlen != N * blksize ) for CBC, ECB cipher modes”, started by Konstantin Andreev, that pushed me to check into this. In his email, he asks for softoken to be changed from the behavior that libssl depends on for correctness into the behavior that libssl would fail with, in order to be more compatible with other implementations. So, it is likely that other people who follow that list also were indirectly alerted to this problem.

Also, on the RSA cryptoki list, back in 2002, there was a message from a PKCS#11 module implementer:
Subject: Re: Do unpadded block ciphers allow odd length updates?
To: Nelson Bolyard
Cc: PKCS 11 discussion list 
From: "Simon McMahon"
“For the sake of usability, and ignoring the spec for a moment, such
mechanisms should allow odd length Updates as long as the length when Final
is called satisfies the length restrictions. It is easy enough to do and my
implementation, as well as plenty of others I'm sure, do this already. The
only justification I can think of not to do it might be performance.”

So, presumably even in 2002 there were many PKCS#11 implementations with which libssl would have the erroneous behavior. Also, PKCS#11 has added the dual-mode operations such as C_DigestEncryptUpdate, which particularly for SSL implementations, the non-NSS-softoken semantics are clearly more desirable than the NSS-softoken semantics that libssl relies on, because the non-NSS-softoken semantics require fewer calls into the PKCS#11 module per record than are required for the NSS-softoken semantics. So, it could be the case that libssl exhibits this bug with a very large number of third-party PKCS#11 modules.

Reproducible: Always

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Oops, this bug report is not correct. The version of ssl3con.c I was reading was a different one with an uncommitted patch. The actual committed code in ssl3con.c does correctly use the output length returned by the PKCS#11 module, even when the PKCS#11 module doesn't decrypt the whole record. And, in that case, I believe that there is no real possibility of harm; libssl will be interpreting part of the plaintext of the *current* record as the padding/MAC, but in that case the padding/MAC decryption will always fail (I think), and in any case the result doesn't depend on the contents of the previous record at all. But I will leave the bug report open so somebody else can verify that it is indeed harmless.

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

I did find one small issue. When the input ciphertext is less than the block length, C_DecryptUpdate could return SECSuccess with zero output. Then, in ssl3_HandleRecord, plaintext->len would be zero, and subsequently the memory *(plaintext->buf + plaintext->len - 1) == plaintext->buf[-1] would be read as the padding length, which could (rarely) result in an access violation. But, in the cases where it doesn't result in an access violation, it is completely harmless, as immediately ssl3_HandleRecord will see that the padding is bad.

Comment 3

[Daniel Veditz [:dveditz]]

Re-summarizing according to comment 2 (please adjust if I've gotten it wrong).

Comment 4

[Kai Engert (:KaiE:)]

Adding Wan-Teh as he recently works on SSL implementation code.

Comment 5

[Wan-Teh Chang]

(In reply to comment #2)
> I did find one small issue. When the input ciphertext is less than the block
> length, C_DecryptUpdate could return SECSuccess with zero output.

Using AES as the block cipher, based on code inspection
of AES_Decrypt and PK11_CipherOp (which ultimately calls
AES_Decrypt), I believe the input must be a multiple of
the block length:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/freebl/rijndael.c&rev=1.25&mark=1171,1182-1185#1170

So the only problematic input length seems to be 0.

Since I may have missed something in my code inspection,
please doublecheck this.

Assuming I'm right, can an SSL record contain a cText
buffer of zero length?

By code inspection, I think ssl3_GatherData can't deal
with an SSL record with a header and zero-length data.
It'll be handled as EOF.

Comment 6

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

I agree with you as far as softoken goes. The problem is that softoken might not be the PKCS#11 module in use.

Comment 7

[Franziskus Kiefer [:franziskus]]

Closing this as the code looks different now and there hasn't been any activity for 7 years.