What are the steps to test this in 1.1.1?
Is there a test case?

@mattcaswell Is this still relevant or is it superceded by your new fixes?

@mattcaswell Is this still relevant or is it superceded by your new fixes?

It needs updating in light of the new fixes

It needs updating in light of the new fixes

I've now updated this PR. Please take a look.

What are the steps to test this in 1.1.1?
Is there a test case?

It's quite difficult to test this independently. If you remove the fix from this PR, but just add the assert (modified to match the original reserved amount), i.e. :

                || !ossl_assert(origlen + SSL_RT_MAX_CIPHER_BLOCK_SIZE >= thiswr->length)

Then that is sufficient to make the existing tests fail.

Ah, now I see, here I have the necessary steps to reproduce the issue
they are as follows:

$ cat test.conf 
openssl_conf = openssl_init

[openssl_init]
ssl_conf = ssl_config

[ssl_config]
server = server_conf
client = client_conf

[server_conf]
Options = -EncryptThenMac

[client_conf]
Options = -EncryptThenMac

$ OPENSSL_CONF=test.conf ../util/shlib_wrap.sh ./openssl s_server -ssl_config server -cipher "ECDHE-RSA-AES256-SHA384" -tls1_2

vs.

$ ../util/shlib_wrap.sh ./openssl s_client

So it is easy to debug it, the assertion happens in s_client in the 4th time the WPACKET_reserve_bytes
is executed in line 991.

In the case where the assertion does not hold, I see that mac_size == 0.
In all other cases mac_size is != 0 and the assertion holds.
So since the worst case is AES256-SHA384, the max. HASH in this case would be 48.

Therefore I would suggest something like the following:

diff --git a/ssl/record/rec_layer_s3.c b/ssl/record/rec_layer_s3.c
index fe4abd6..1ca7958 100644
--- a/ssl/record/rec_layer_s3.c
+++ b/ssl/record/rec_layer_s3.c
@@ -16,6 +16,7 @@
 #include <openssl/rand.h>
 #include "record_local.h"
 #include "../packet_local.h"
+#include "internal/cryptlib.h"
 
 #if     defined(OPENSSL_SMALL_FOOTPRINT) || \
         !(      defined(AESNI_ASM) &&   ( \
@@ -987,7 +988,7 @@ int do_ssl3_write(SSL *s, int type, const unsigned char *buf,
          * This will be at most one cipher block or the tag length if using
          * AEAD. SSL_RT_MAX_CIPHER_BLOCK_SIZE covers either case.
          */
-        if (!WPACKET_reserve_bytes(thispkt, SSL_RT_MAX_CIPHER_BLOCK_SIZE,
+        if (!WPACKET_reserve_bytes(thispkt, (mac_size == 0 ? 48 : 0) + SSL_RT_MAX_CIPHER_BLOCK_SIZE,
                                    NULL)
                    /*
                     * We also need next the amount of bytes written to this
@@ -1037,6 +1038,7 @@ int do_ssl3_write(SSL *s, int type, const unsigned char *buf,
 
         /* Allocate bytes for the encryption overhead */
         if (!WPACKET_get_length(thispkt, &origlen)
+               || !ossl_assert(origlen + (mac_size == 0 ? 48 : 0) + SSL_RT_MAX_CIPHER_BLOCK_SIZE >= thiswr->length)
                    /* Encryption should never shrink the data! */
                 || origlen > thiswr->length
                 || (thiswr->length > origlen

would look more conservative and more correct to me.
Of course there is an easy way to find out if the cipher is a true AEAD cipher it would be
better to add zero headroom in this case.

Slightly better would indeed be adding (mac_size == 0 && !clear ? 48 : 0)
after some debugging I think now that only EVP_CTRL_AEAD_TLS1_AAD knows
if it is AEAD (always 16 byte) or AES-CBC-HMAC (1-16 byte padding + either 20, 32 or 48 byte hash len).
Where the case 48 bytes is not implemented and uses the explicit code where mac_size is != 0.

would look more conservative and more correct to me.

I don't think that is the correct fix. We must be careful to only reserve the right amount. If we reserve too much then we could exceed the amount allocated in the underlying buffer and therefore the WPACKET_reserve_bytes call could fail. If we don't reserve enough then the encryption could grow more than we have reserved.

The right amount needs to be relative to the amount that we allocated in the original underlying buffer for encryption growth:

So, what we originally allocate is SSL3_RT_SEND_MAX_ENCRYPTED_OVERHEAD bytes for the encryption growth. This macro is defined as:

# define SSL3_RT_SEND_MAX_ENCRYPTED_OVERHEAD \
                        (SSL_RT_MAX_CIPHER_BLOCK_SIZE + SSL3_RT_MAX_MD_SIZE)

Here SSL3_RT_MAX_MD_SIZE is defined as 64 (so slightly more than the 48 you have in your proposed changes).

So given that we have allowed SSL3_RT_SEND_MAX_ENCRYPTED_OVERHEAD bytes in the underlying buffer for encryption growth, then that's how much we should allow for when reserving bytes for that growth. However that encryption growth comes from two sources: the MAC and any growth due to cipher block padding.

For some ciphersuites we do those 2 things separately, i.e. add the encrypted data and then add the MAC (for Encrypt-then-Mac), or sometimes add the MAC and then encrypt (Mac-then-Encrypt). Encrypt-then-MAC isn't that interesting because by the time we add the MAC we already know how much the encryption itself grew by. So we could just reserve SSL3_RT_SEND_MAX_ENCRYPTED_OVERHEAD bytes and we know it will be safe. Also safe is to reserve SSL3_RT_SEND_MAX_ENCRYPTED_OVERHEAD - mac_size bytes because we know the encryption cipher won't be adding those MAC bytes. With Mac-then-Encrypt we have already consumed mac_size bytes by the time we do then encryption, so we must not reserve any more than SSL3_RT_SEND_MAX_ENCRYPTED_OVERHEAD - mac_size bytes because otherwise we could go beyond our allocated buffer and the reserve operation will therefore fail. Since SSL3_RT_SEND_MAX_ENCRYPTED_OVERHEAD - mac_size is safe in both the Encrypt-then-MAC and MAC-then-Encrypt cases we can just always use that value.

The other case is where we have a stitched ciphersuite or an AEAD ciphersuite. In that case mac_size is 0, but the encryption overhead could be anything up to the maximum of SSL3_RT_SEND_MAX_ENCRYPTED_OVERHEAD. Since mac_size is 0, then SSL3_RT_SEND_MAX_ENCRYPTED_OVERHEAD - mac_size still works.

So, in all cases, SSL3_RT_SEND_MAX_ENCRYPTED_OVERHEAD - mac_size is correct. It should never exceed the allocated buffer, but should always be sufficient for the encryption growth.

I have only a problem with the assertion, because an engine-based aes-cbc-sha-x cipher might have a good reason to not use the minimal padding, for instance as a counter measure against the CRIME attack.
It was new to me, that the available length in the packet buffer was not being checked before the padding is added, but that
might easily be used by an existing engine.
I tried it just now, and I think it is pretty easy to modify the dasync engine to add any padding up to 256 bytes,
that is safe, because the packet buffer is always at least twice as large.
here is my experiment:

diff --git a/crypto/evp/e_aes_cbc_hmac_sha1.c b/crypto/evp/e_aes_cbc_hmac_sha1.c
index 27c36b46e7..a36a01c566 100644
--- a/crypto/evp/e_aes_cbc_hmac_sha1.c
+++ b/crypto/evp/e_aes_cbc_hmac_sha1.c
@@ -421,10 +421,12 @@ static int aesni_cbc_hmac_sha1_cipher(EVP_CIPHER_CTX *ctx, unsigned char *out,
     if (EVP_CIPHER_CTX_encrypting(ctx)) {
         if (plen == NO_PAYLOAD_LENGTH)
             plen = len;
+/*
         else if (len !=
                  ((plen + SHA_DIGEST_LENGTH +
                    AES_BLOCK_SIZE) & -AES_BLOCK_SIZE))
             return 0;
+*/
         else if (key->aux.tls_ver >= TLS1_1_VERSION)
             iv = AES_BLOCK_SIZE;

diff --git a/engines/e_dasync.c b/engines/e_dasync.c
index 9ad043b1bd..0806faff7c 100644
--- a/engines/e_dasync.c
+++ b/engines/e_dasync.c
@@ -625,7 +625,8 @@ static int dasync_cipher_ctrl_helper(EVP_CIPHER_CTX *ctx, int type, int arg,
                 }

                 return ((len + SHA_DIGEST_LENGTH + AES_BLOCK_SIZE)
-                        & -AES_BLOCK_SIZE) - len;
+                        & -AES_BLOCK_SIZE) - len
+                        + (len && len < 128 ? 128 : 0);
             } else {
                 return SHA_DIGEST_LENGTH;
             }

test.conf:

openssl_conf = openssl_init

[openssl_init]
ssl_conf = ssl_config

[ssl_config]
server = server_conf
client = client_conf

[server_conf]
Options = -EncryptThenMac

[client_conf]
Options = -EncryptThenMac

OPENSSL_ENGINES=../engines OPENSSL_CONF=test.conf ../util/shlib_wrap.sh ./openssl s_server -engine dasync -ssl_config server -cipher "ECDHE-RSA-AES128-SHA" -tls1

OPENSSL_ENGINES=../engines OPENSSL_CONF=test.conf ../util/shlib_wrap.sh ./openssl s_client -engine dasync -ssl_config client -cipher "ECDHE-RSA-AES128-SHA" -tls1

I have only a problem with the assertion, because an engine-based aes-cbc-sha-x cipher might have a good reason to not use the minimal padding, for instance as a counter measure against the CRIME attack.

Doing that is simply not safe - even before this PR. Libssl assumes minimal padding and that encryption will never grow by more than SSL3_RT_SEND_MAX_ENCRYPTED_OVERHEAD - because that's how much of the underlying buffer is allocated for it. If an engine grows by more than that it is likely to cause a crash in existing 1.1.1 when processing maximally sized records (e.g. records of size SSL3_RT_MAX_PLAIN_LENGTH). After this PR such excessive growth will be spotted by the assert with smaller records.

that is safe, because the packet buffer is always at least twice as large.

Err no? The amount allocated for encryption growth is exactly SSL3_RT_SEND_MAX_ENCRYPTED_OVERHEAD

Err no? The amount allocated for encryption growth is exactly SSL3_RT_SEND_MAX_ENCRYPTED_OVERHEAD

Note: my example above is really working for small data size, and arbitrarily large ones as well.

That is the case because the memory for the write buffer is allocated in ssl3_setup_write_buffer:

        len = ssl_get_max_send_fragment(s)
            + SSL3_RT_SEND_MAX_ENCRYPTED_OVERHEAD + headerlen + align;
#ifndef OPENSSL_NO_COMP
        if (ssl_allow_compression(s))
            len += SSL3_RT_MAX_COMPRESSED_OVERHEAD;
#endif
        if (!(s->options & SSL_OP_DONT_INSERT_EMPTY_FRAGMENTS))
            len += headerlen + align + SSL3_RT_SEND_MAX_ENCRYPTED_OVERHEAD;
    }

    wb = RECORD_LAYER_get_wbuf(&s->rlayer);
    for (currpipe = 0; currpipe < numwpipes; currpipe++) {
        SSL3_BUFFER *thiswb = &wb[currpipe];

        if (thiswb->buf != NULL && thiswb->len != len) {
            OPENSSL_free(thiswb->buf);
            thiswb->buf = NULL;         /* force reallocation */
        }

        if (thiswb->buf == NULL) {
            p = OPENSSL_malloc(len);

and ssl_get_max_send_fragment() is always greater than 512. As my working code
example above prooves, it is possible to add arbitrary padding to small packets,
see the + (len && len < 128 ? 128 : 0);, this method can be used to make all small
packets using roughly the same size, len, is the size of the application data, which is
used as a side channel in the CRIME attack as we all know. Of course I have to let
the zero length application packets alone, since they are used for need_empty_fragments,
therefore the len && in the above padding adjustment expression, but that cant be a
compressed datagram, so is not interesting for CRIME attack counter measures.

and ssl_get_max_send_fragment() is always greater than 512.

So? This is the memory allocated for the plaintext record. If the length of the plaintext record is equal to ssl_get_max_send_fragment() (which is normally SSL3_RT_MAX_PLAIN_LENGTH, or 16384 bytes - but can be configured to something smaller) then all of this part of the buffer will already be consumed by the time you get to encryption.

Assuming compression is not allowed, then the remaining buffer is made up of allowances for the header and alignment bytes (which will also be consumed by the time you get to encryption)....which leaves SSL3_RT_SEND_MAX_ENCRYPTED_OVERHEAD for the encryption growth. If you exceed that with excessive padding, and all the rest of the buffer has been used then you will run over the end of the buffer.

it is possible to add arbitrary padding to small packets

You would get away with it with small packets but only by accident - and as soon as you encrypt a full length packet you will get into trouble. Libssl assumes minimal padding. Anything other than that is definitely not supported. I would be very surprised if any existing engine is actually doing that.

You would get away with it with small packets but only by accident - and as soon as you encrypt a full length packet you will get into trouble.

Yeah, that is clear, increasing a maximum size packet would be a rather silly idea, because it would add
a new side channel instead of eliminating one.
But hiding the exact size of the plain text record is a pretty cool feature IMHO.

I would be very surprised if any existing engine is actually doing that.

All I can say, that I was very surprised when the 1.1.1r was withdrawn.
And pretty much everything can be used in ways we did not expect.
After I learned that the padding is de-facto not limited, it took me just half an hour
to develop a way to use this feature in an engine for something interesting.

But hiding the exact size of the plain text record is a pretty cool feature IMHO.

There is a supported way to do this with TLSv1.3 (i.e. via SSL_CTX_set_record_padding_callback() and similar functions).

IMO, in TLSv1.2 and below this is just not supported and never has been. You may have been able to subvert the ENGINE infrastructure to do this for smaller records, but this worked only by accident because we never validated the encryption growth amount. The fact that we didn't do this is a clear bug IMO and we need to fix it.

Sorry, but if you want to do hardening, why don't you check the padding length against
the underlying packet buffer's available length here:

before the overwrite happens?

We could potentially do that, but there is no access to the WPACKET at that point in the code. It would require refactoring for that to happen. That's probably not appropriate for 1.1.1.

It would just need one additional parameter to pass wr and pkt to the enc funtion, that does not look too hard.

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

You'd need to do that for all enc functions and modify the SSL_ENC_METHOD structure definition accordingly. IMO that is not appropriate for 1.1.1 and certainly isn't with the scope of this PR.

IMO that is not appropriate for 1.1.1

Yeah probably, but adding an assertion at this place is neither appropriate.
As I see it, it is possible to exceed the padding limit either on purpose and get away with it,
because the engine knows or at least correctly guessed a lower limit for the record size limit.
Or exceed the padding limit by accident and get away with it by chance,
but due to the added assertion this will now fail, while it did work previously.
Or exceed the limit and get a crash, but the assertion wont help much, because it is after the
memory overwrite happens, and the ossl_asssert is normally silent, so it is not helpful at all.

To make that clear, I see no problem with reserving the reserved amount of memory.
And Yes, I want to fix that issue as well.

So just for reference: #19710 is how I would like to fix it in 1.1.1

As I see it, it is possible to exceed the padding limit either on purpose and get away with it,
because the engine knows or at least correctly guessed a lower limit for the record size limit.
Or exceed the padding limit by accident and get away with it by chance,
but due to the added assertion this will now fail, while it did work previously.
Or exceed the limit and get a crash, but the assertion wont help much, because it is after the
memory overwrite happens,

I just completely and strongly disagree with you. Any engine that deliberately exceeds the normal encryption growth expectations would have to be making assumptions about the size of the buffer that it has available which it has no business doing. This is very dangerous for such an engine to do. Consider the case where max_fragment_length has been negotiated, or the application has explicitly called SSL_set_max_send_fragment(). In such a case ssl_get_max_send_fragment(s) could return a much smaller number than the maximal SSL3_RT_MAX_PLAIN_LENGTH value during buffer allocation and the engine would have no idea about it - leading to the scenario that it would exceed the buffer even with small record sizes.

Any engine doing that is dangerously broken IMO and having libssl fail in such a case would help to identify it and get it fixed.

and the ossl_asssert is normally silent, so it is not helpful at all.

It's not silent. In a production build the assert is still assessed and returns failure (as opposed to crashing as it does in a debug build).

The ossl_assert is a valuable part of this change because it should cause the existing test suite to fail in the case that a cipher exceeds the growth expectations.

Since we clearly have a disagreement here I am placing an OTC hold on this PR.

OTC Question: Should we include the ossl_assert in this PR to confirm that the cipher had not exceeded the encryption growth expectations?

So just for reference: #19710 is how I would like to fix it in 1.1.1

I'd consider that approach to be orthogonal to this one.

OTC: This is OK to go in 1.1.1 as it is.

Pushed. Thanks.

When applying this patch to my own environment, I was getting errors when calling the SSL_write function. The function's call stack fails at the WPACKET_reserve_bytes function.

#0  WPACKET_reserve_bytes (allocbytes=0x0, len=80, pkt=0xffffffffd158) at ssl/packet.c:46
#1  WPACKET_reserve_bytes (pkt=pkt@entry=0xffffffffd158, len=len@entry=80, allocbytes=allocbytes@entry=0x0) at ssl/packet.c:40
#2  0x0000fffff7e63530 in do_ssl3_write (s=s@entry=0x5a1bd0, type=type@entry=23,
    buf=buf@entry=0x5c9ed0 "HTTP/1.1 200 OK\r\nServer: nginx/1.15.8\r\nDate: Thu, 19 Jan 2023 07:08:54 GMT\r\nContent-Type: text/html\r\nLast-Modified: Wed, 18 Jan 2023 11:50:52 GMT\r\nTransfer-Encoding: chunked\r\nConnection: keep-alive\r\nE"..., pipelens=pipelens@entry=0xffffffffe1d8, numpipes=1,
    create_empty_fragment=create_empty_fragment@entry=0, written=written@entry=0xffffffffe1d0) at ssl/record/rec_layer_s3.c:992
#3  0x0000fffff7e63e20 in ssl3_write_bytes (s=0x5a1bd0, type=23, buf_=0x5c9ed0, len=<optimized out>, written=0xffffffffe2f0)
    at ssl/record/rec_layer_s3.c:633
#4  0x0000fffff7e753bc in SSL_write (s=<optimized out>, buf=buf@entry=0x5c9ed0, num=num@entry=16384) at ssl/ssl_lib.c:1990
#5  0x0000000000446dc0 in ngx_ssl_write (c=c@entry=0xfffff599b200,
    data=0x5c9ed0 "HTTP/1.1 200 OK\r\nServer: nginx/1.15.8\r\nDate: Thu, 19 Jan 2023 07:08:54 GMT\r\nContent-Type: text/html\r\nLast-Modified: Wed, 18 Jan 2023 11:50:52 GMT\r\nTransfer-Encoding: chunked\r\nConnection: keep-alive\r\nE"..., size=size@entry=16384) at src/event/ngx_event_openssl.c:2228
#6  0x0000000000447848 in ngx_ssl_send_chain (c=0xfffff599b200, in=0x5d5ed0, limit=2147479551) at src/event/ngx_event_openssl.c:2173

I took a closer look at the WPACKET_reserve_bytes function and found that if (pkt->maxsize - pkt->written < len) caused the error.

Perhaps SSL3_RT_SEND_MAX_ENCRYPTED_OVERHEAD - mac_size is unreasonable, and the value needs to be modified.
@mattcaswell

@bangcheng - are you able to tell me the value of eivlen, thiswr->length and align in frame 2 (do_ssl3_write) of that stack trace?

Also can you confirm what protocol version has been negotiated in this scenario?

I've been able to recreate what is probably the same issue. Fixes in #20085, #20086 and #20087