Ensure our buffer allocation allows for the Explicit IV (1.1.1) by mattcaswell · Pull Request #20087 · openssl/openssl

mattcaswell · 2023-01-19T15:30:12Z

Some ciphers/protocol versions have an explicit IV. We need to make sure we
have sufficient room for it in the underlying buffer.

This is a backport of #20085.

This fixes a regression introduced by #19585 and should be merged before any 1.1.1t is released.

Test that sending large app data records works correctly.

Some ciphers/protocol versions have an explicit IV. We need to make sure we have sufficient room for it in the underlying buffer.

bernd-edlinger · 2023-01-19T15:53:30Z

How does this relate to #19710 ?

mattcaswell · 2023-01-19T16:47:05Z

The CI failures here are quite strange and apparently unrelated - but there are a lot of them.

mattcaswell · 2023-01-19T16:48:22Z

How does this relate to #19710 ?

Unclear

bernd-edlinger · 2023-01-19T16:48:35Z

#19930 would fix the CI failures.

bernd-edlinger · 2023-01-19T16:54:08Z

How does this relate to #19710 ?

Unclear

I feel like I knew this would not hold, and I told you before...

I mean how about reverting #19585 and using #19710 instead, maybe add the test case from this PR,

It seems to work together with #19710, right?

mattcaswell · 2023-01-19T17:00:02Z

I mean how about reverting #19585 and using #19710 instead, maybe add the test case from this PR,

IMO there is nothing fundamentally wrong with the approach in #19585 and #19710 is not a suitable replacement for it. There is no reason to revert it.

bernd-edlinger · 2023-01-19T20:24:50Z

I mean how about reverting #19585 and using #19710 instead, maybe add the test case from this PR,

IMO there is nothing fundamentally wrong with the approach in #19585 and #19710 is not a suitable replacement for it. There is no reason to revert it.

But #19585 does not address the similar issue with DTLS,
while this PR increases the allocation size for TLS and DTLS,
although the test case fails to test DTLS.
Moreover explicit IV is a TLS1.3 feature and !ETM is a TLS1.2 feature,
and the only possible stitched ciphers are listed here:

openssl/ssl/ssl_ciph.c

Lines 560 to 579 in d90907d

    
           if (c->algorithm_enc == SSL_RC4 && 
        
               c->algorithm_mac == SSL_MD5 && 
        
               (evp = EVP_get_cipherbyname("RC4-HMAC-MD5"))) 
        
               *enc = evp, *md = NULL; 
        
           else if (c->algorithm_enc == SSL_AES128 && 
        
                    c->algorithm_mac == SSL_SHA1 && 
        
                    (evp = EVP_get_cipherbyname("AES-128-CBC-HMAC-SHA1"))) 
        
               *enc = evp, *md = NULL; 
        
           else if (c->algorithm_enc == SSL_AES256 && 
        
                    c->algorithm_mac == SSL_SHA1 && 
        
                    (evp = EVP_get_cipherbyname("AES-256-CBC-HMAC-SHA1"))) 
        
               *enc = evp, *md = NULL; 
        
           else if (c->algorithm_enc == SSL_AES128 && 
        
                    c->algorithm_mac == SSL_SHA256 && 
        
                    (evp = EVP_get_cipherbyname("AES-128-CBC-HMAC-SHA256"))) 
        
               *enc = evp, *md = NULL; 
        
           else if (c->algorithm_enc == SSL_AES256 && 
        
                    c->algorithm_mac == SSL_SHA256 && 
        
                    (evp = EVP_get_cipherbyname("AES-256-CBC-HMAC-SHA256"))) 
        
               *enc = evp, *md = NULL;

so at least as for the 1.1.1 version is concerned, that is RC4-HMAC-MD5, AES-128-CBC-HMAC-SHA1,
AES-256-CBC-HMAC-SHA1, AES-128-CBC-HMAC-SHA256, and AES-256-CBC-HMAC-SHA256,
So I think there was no real issue, at all, at least for 1.1.1 as you can see that #19710 passes
even with your new test case and with #19585 reverted.

On the other hand #19710 addresses TLS and DTLS,
and checks the available memory before the overwrite happens.

So to me this looks like first the reservation size is made larger by #19585, and
now the allocation size has to be increased just to fix the newly added assertion,
so net effect: more memory is needed, and probably never used, otherwise #19710
would fail now, once the test case from this PR has been added.

Well additionally, #19585 has quite some history:

It was first merged to master as 830eae6
then had to be fixed urgently as ecacbc5
and now again caused a regression that will also have to be fixed urgently.

Sorry, but how much more do you guys need to consider a revert?

mattcaswell · 2023-01-20T12:09:14Z

#19585 and #19710 have different objectives to each other. One is not a replacement for the other.

#19585 is about addressing a bug in the WPACKET accounting logic. The WPACKET_reserve_bytes and WPACKET_allocate_bytes calls are intended to be used in tandem with each other. When performing an operation which will fill a buffer with bytes that you don't know the size of beforehand you call WPACKET_reserve_bytes to set a ceiling on the maximum number of bytes that you will use, and then afterwards you call WPACKET_allocate_bytes to set the number of bytes that you actually used. It should never be the case that the allocate call uses more bytes than were originally reserved. The whole point of the reserve call is to verify that you have sufficient space left in your buffer compared to what you actually want to use. If you allocate more than you reserve then you could inadvertently overflow the buffer.

#19585 addresses a bug where insufficient bytes are reserved and the allocate call uses more than was reserved. #19710 does not address this WPACKET accounting error. Instead it adds checks in the encryption code to ensure that it does not overflow the underlying buffer before it writes out those bytes. Adding overflow checks before writing out the bytes is not a bad idea IMO, but it should do so using the mechanism that we have implemented for this: i.e. WPACKET. By checking the underlying buffer directly you are just avoiding the mechanism we have put in place for this task and so doesn't seem like the right solution to me. If you checked in tls1_enc that the number of bytes didn't exceed the amount reserved in the WPACKET then that would be a much better solution - but then you would immediately find that you would still need #19585 in place to fix the reservation bug. #19710 does not fix a bug, whereas #19585 does. #19710 does add some useful additional hardening (but in the wrong way IMO). I'm not sure that #19710 can be justified in a stable release (and less so in 1.1.1).

But #19585 does not address the similar issue with DTLS,

The DTLS code does not have the WPACKET accounting error bug that the TLS code has because it does not use WPACKET. This is a bad thing IMO, but not something to be addressed in 1.1.1. It has been addressed in master where the TLS and DTLS code has been harmonised and WPACKET is used for both. Since the DTLS code does not have the bug that #19585 fixes, it is not relevant there. The additional hardening introduced by #19710 could still be useful (but see my comments above) - but it does not address a bug. The hardening in #19710 does not cover TLSv1.3 or SSLv3.

while this PR increases the allocation size for TLS and DTLS,

This PR actually addresses a slightly different problem to #19585. I consider it to be fixing a latent bug that the changes in #19585 highlighted. The underlying buffer is sized based on certain assumptions, i.e. the buffer includes an allowance for:

the payload data itself
growth due to the compression algorithm (if relevant)
the record header
any alignment
any growth due to the encryption algorithm

The last factor includes an allowance for:

growth due to padding
growth due to the MAC/a tag

The bug here is that there is no allowance made for an explicit IV. We've been lucky enough to get away with it by virtue of the fact that the allowance for the MAC/tag is sized at 64 bytes. However the biggest MAC that we ever actually use is SHA-384 - which requires 48 bytes. That gives us 16 spare, which just happens to be the size of an explicit IV. But this is an accident and works by luck.

An alternative fix to that proposed in this PR is to adjust the assumptions that we make and redefine SSL3_RT_SEND_MAX_ENCRYPTED_OVERHEAD so that it only incorporates 48 bytes for the MAC/tag and adds 16 for the explicit IV: ending up at the same value as it has now. We'd then have to modify the WPACKET_reserve_bytes logic to take account of this, and remove the 16 bytes used by the explicit IV. This would have the benefit of meaning that the underlying buffer size does not grow, but IMO complicates things unnecessarily (especially since SSL3_RT_SEND_MAX_ENCRYPTED_OVERHEAD is a public macro). We could just leave the definition of SSL3_RT_SEND_MAX_ENCRYPTED_OVERHEAD alone and just add some comments explaining that the 64 bytes for the MAC actually only needs 48, and the other 16 are for the explicit IV. We'd still need to adjust the reserve logic in this case.

IMO just adding an additional 16 bytes to the underlying buffer is much simpler and the better solution - even though they are strictly speaking redundant. Having an extra 16 bytes in case of "accidents" is probably not a bad thing anyway.

#19710 does not address this problem at all. It works by virtue of the fact that the underlying buffer just happens to be big enough by chance due to the reasons explained above.

although the test case fails to test DTLS.

This is a good point. It wouldn't be a bad idea to extend the test to DTLS as well.

Moreover explicit IV is a TLS1.3 feature and !ETM is a TLS1.2 feature,

This is not correct. Explicit IV is relevant to TLS 1.1, TLS 1.2 and DTLS, but not TLSv1.3. It was introduced in TLS 1.1 due to a known vulnerability in TLSv1.0/SSLv3 where the IV for each record is the last block of the preceding record (meaning it is predictable). This later gave rise to the BEAST attack.

Explicit IV is only applicable to CBC ciphersuites. Since TLSv1.3 doesn't have any of those it is not relevant there.

However, the buffers (in 1.1.1) are allocated early in the handshake before we know what protocol version/ciphersuites we are going to be using. So allocating a 16 byte allowance for the explicit IV in case we subsequently negotiate something that requires it seems sensible.

mattcaswell · 2023-01-20T14:41:54Z

although the test case fails to test DTLS.

This is a good point. It wouldn't be a bad idea to extend the test to DTLS as well.

I've added a commit to do that.

bernd-edlinger · 2023-01-20T17:22:45Z

although the test case fails to test DTLS.

This is a good point. It wouldn't be a bad idea to extend the test to DTLS as well.

I've added a commit to do that.

Thanks, I've added your test to #19710 and it works there too.

bernd-edlinger · 2023-01-20T18:06:35Z

The hardening in #19710 does not cover TLSv1.3 or SSLv3.

No, that is a complete misunderstanding.
TLSv1.3 and SSLv3 had no issue at all before #19585 broke them,
therefore those enc functions do not use the new parameter, if they turn out to exceed
the minimum reservation that could be addressed easily but currently I only see one place
where there was a real issue: in tls1_enc near EVP_AEAD_TLS1_AAD_LEN.
The idea of #19710 as follows:
All enc functions have the natural right to assume 16 bytes to be available for padding
or adding an AEAD tag value, that is guaranteed by a reservation of SSL_RT_MAX_CIPHER_BLOCK_SIZE,
as it was done before #19585 increased this value.
But in some cases the padding and a HMAC hash is needed, and #19710 adds a side-channel
to check the available memory only in this case. And this is only TLS1 not SSL3 and not TLS1.3.
It relies on the fact that the subsequent packet allocation is guaranteed to succeed and not
modifiy the already filled in mac value, but that is not a real issue IMHO.
I thought that would be clear since I wrote this in the original commit message:

This adds a new parameter to the enc functions,
that is optional, and if given can be used to prevent
buffer overflows due to excessive padding before they happen.
A minimum space reservation of 16 bytes is assumed to
be available without checking.
Additionally this fixes a bug, that prevented DTLS
to use aes_cbc_hmac_sha1 and aes_cbc_hmac_sha256.

So yes, the memory reservation currently reilies heavily on the fact that CBC ciphers
dont play together with SHA512 and explicit IV. That wont change ever, but
might deserve a comment.

openssl-machine · 2023-01-24T10:00:19Z

This pull request is ready to merge

hlandau · 2023-01-24T10:25:37Z

Merged to 1.1.1. Thank you.

Test that sending large app data records works correctly. Reviewed-by: Paul Dale <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: Hugo Landau <[email protected]> (Merged from #20087)

Some ciphers/protocol versions have an explicit IV. We need to make sure we have sufficient room for it in the underlying buffer. Reviewed-by: Paul Dale <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: Hugo Landau <[email protected]> (Merged from #20087)

Reviewed-by: Paul Dale <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: Hugo Landau <[email protected]> (Merged from #20087)

Test that sending large app data records works correctly. Reviewed-by: Paul Dale <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: Hugo Landau <[email protected]> (Merged from openssl#20087)

Some ciphers/protocol versions have an explicit IV. We need to make sure we have sufficient room for it in the underlying buffer. Reviewed-by: Paul Dale <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: Hugo Landau <[email protected]> (Merged from openssl#20087)

Reviewed-by: Paul Dale <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: Hugo Landau <[email protected]> (Merged from openssl#20087)

mattcaswell added 2 commits January 19, 2023 15:05

Add a test for large app data

0c72e97

Test that sending large app data records works correctly.

Ensure our buffer allocation allows for the Explicit IV

fb9794c

Some ciphers/protocol versions have an explicit IV. We need to make sure we have sufficient room for it in the underlying buffer.

mattcaswell added approval: review pending This pull request needs review by a committer branch: 1.1.1 Applies to OpenSSL_1_1_1-stable branch (EOL) approval: otc review pending severity: important Important bugs affecting a released version labels Jan 19, 2023

mattcaswell changed the title ~~Ensure our buffer allocation allows for the Explicit IV~~ Ensure our buffer allocation allows for the Explicit IV (1.1.1) Jan 19, 2023

fixup! Add a test for large app data

e8f6e9e

mattcaswell mentioned this pull request Jan 19, 2023

Fix the ceiling on how much encryption growth we can have (1.1.1) #19585

Closed

2 tasks

t8m added triaged: bug The issue/pr is/fixes a bug severity: regression The issue/pr is a regression from previous released version tests: present The PR has suitable tests present labels Jan 19, 2023

mattcaswell closed this Jan 19, 2023

mattcaswell reopened this Jan 19, 2023

Add DTLS support to the large app data test

206c568

paulidale approved these changes Jan 22, 2023

View reviewed changes

paulidale removed the approval: otc review pending label Jan 22, 2023

t8m approved these changes Jan 23, 2023

View reviewed changes

t8m removed the approval: review pending This pull request needs review by a committer label Jan 23, 2023

t8m added the approval: done This pull request has the required number of approvals label Jan 23, 2023

openssl-machine added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Jan 24, 2023

hlandau closed this Jan 24, 2023

openssl-machine pushed a commit that referenced this pull request Jan 24, 2023

Add DTLS support to the large app data test

adc3cfe

Reviewed-by: Paul Dale <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: Hugo Landau <[email protected]> (Merged from #20087)

Uh oh!

Comments

Conversation

mattcaswell commented Jan 19, 2023

Uh oh!

bernd-edlinger commented Jan 19, 2023

Uh oh!

mattcaswell commented Jan 19, 2023

Uh oh!

mattcaswell commented Jan 19, 2023

Uh oh!

bernd-edlinger commented Jan 19, 2023

Uh oh!

bernd-edlinger commented Jan 19, 2023

Uh oh!

mattcaswell commented Jan 19, 2023

Uh oh!

bernd-edlinger commented Jan 19, 2023

Uh oh!

mattcaswell commented Jan 20, 2023

Uh oh!

mattcaswell commented Jan 20, 2023

Uh oh!

bernd-edlinger commented Jan 20, 2023

Uh oh!

bernd-edlinger commented Jan 20, 2023

Uh oh!

openssl-machine commented Jan 24, 2023

Uh oh!

hlandau commented Jan 24, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants