[Backport 3.6] Defragment incoming TLS handshake messages (context extension pointer) #9949
Conversation
Signed-off-by: Deomid rojer Ryabkov <rojer@rojer.me>
Co-authored-by: minosgalanakis <30719586+minosgalanakis@users.noreply.github.com> Signed-off-by: Deomid Ryabkov <rojer@rojer.me>
Signed-off-by: Deomid rojer Ryabkov <rojer@rojer.me>
Signed-off-by: Deomid rojer Ryabkov <rojer@rojer.me>
Except the first Signed-off-by: Deomid rojer Ryabkov <rojer@rojer.me>
Signed-off-by: Deomid rojer Ryabkov <rojer@rojer.me>
Tests uses openssl s_server with a mix of max_send_frag and split_send_frag options. Signed-off-by: Waleed Elmelegy <waleed.elmelegy@arm.com>
* Add tests for the server side. * Remove restriction for TLS 1.2 so that we can test TLS 1.2 & 1.3. * Use latest version of openSSL to make sure -max_send_frag & -split_send_frag flags are supported. Signed-off-by: Waleed Elmelegy <waleed.elmelegy@arm.com>
Signed-off-by: Waleed Elmelegy <waleed.elmelegy@arm.com>
Signed-off-by: Waleed Elmelegy <waleed.elmelegy@arm.com>
Signed-off-by: Waleed Elmelegy <waleed.elmelegy@arm.com>
Signed-off-by: Waleed Elmelegy <waleed.elmelegy@arm.com>
Signed-off-by: Waleed Elmelegy <waleed.elmelegy@arm.com>
Signed-off-by: Waleed Elmelegy <waleed.elmelegy@arm.com>
…entation tests Signed-off-by: Waleed Elmelegy <waleed.elmelegy@arm.com>
…tation Signed-off-by: Waleed Elmelegy <waleed.elmelegy@arm.com>
Signed-off-by: Waleed Elmelegy <waleed.elmelegy@arm.com>
Signed-off-by: Waleed Elmelegy <waleed.elmelegy@arm.com>
Signed-off-by: Waleed Elmelegy <waleed.elmelegy@arm.com>
Signed-off-by: Waleed Elmelegy <waleed.elmelegy@arm.com>
Not used so far, just the scaffolding. Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
Manually remove it in ssl.h, then: sed -i 's/ssl->in_iv/ssl->in_ext->in_iv/g' library/ssl_*.[ch] Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
Manually remove from ssl.h, then: sed -i 's/ssl->in_hs\(hdr\|fraglen\)/ssl->in_ext->in_hs\1/g' library/ssl_*.[ch] Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
include/mbedtls/ssl.h
Outdated
| @@ -1795,7 +1805,7 @@ struct mbedtls_ssl_context { | |||
| * (the end is marked by in_len). */ | |||
| #endif /* MBEDTLS_SSL_DTLS_CONNECTION_ID */ | |||
| unsigned char *MBEDTLS_PRIVATE(in_len); /*!< two-bytes message length field */ | |||
| unsigned char *MBEDTLS_PRIVATE(in_iv); /*!< ivlen-byte IV */ | |||
| mbedtls_ssl_context_in_ext *MBEDTLS_PRIVATE(in_ext); /*!< extension structure */ | |||
There was a problem hiding this comment.
Note that this is still technically an ABI change, although there's a good chance it works on all platforms where Mbed TLS has been ported. Pointers to different types can have different representations, and even different sizes. I know of C implementations that have different representations for word pointers and char pointers, although they have the same size — the hardware can only address 64-bit words, but C uses 8-bit char and encodes char pointers with the 3 top bits encoding an octet inside the word. I don't know if there are any C99 implementations like this, let alone C99 implementations where char pointers would have a different size (encoding the intra-word offset in a separate word).
There was a problem hiding this comment.
Ah, I wasn't sure, thanks. I think as long as they're the same size we're fine, even if the representations are different: this is a private field, and it's not used in any public inline function, so applications should not be accessing it, hence not be affected by a change in representation.
If the size changed, that would be a problem indeed: if it became larger then it would be game over as that would likely make the whole structure larger, existing applications would not allocate enough size, likely resulting in stack corruption. If it became smaller, then I guess it depends if all fields that follow are private and not used by any public inline function - there's also a chance padding would just fix it for us.
I considered keeping the type to unsigned char *, and casting for each access (likely through a trivial accessor function) - I'm not 100% sure but I seem to remember that all pointers can safely be cast to void * or char * and back?
There was a problem hiding this comment.
Yes, regardless of representation, casting to void* or char* and back results in a pointer that's equal to the original pointer.
|
I'll point out that this passed CI on the first try even though it was prepared quickly, which I think supports my point that this approach has the advantage of being very simple to reason about. |
In principle, pointers to char and pointers to word-aligned structs might have different sizes. However, it's guaranteed that any pointer can be converted to a char pointer and back, so it's OK to store the pointer to struct as a pointer to char. Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
mpg
added
needs-review
| @@ -1795,7 +1805,7 @@ struct mbedtls_ssl_context { | |||
| * (the end is marked by in_len). */ | |||
| #endif /* MBEDTLS_SSL_DTLS_CONNECTION_ID */ | |||
| unsigned char *MBEDTLS_PRIVATE(in_len); /*!< two-bytes message length field */ | |||
| unsigned char *MBEDTLS_PRIVATE(in_iv); /*!< ivlen-byte IV */ | |||
| unsigned char *MBEDTLS_PRIVATE(in_ext); /*!< extension structure (ABI compat) */ | |||
There was a problem hiding this comment.
For the record, I am approving the design using an extension structure to keep the same general structure of the patch but preserve the ABI.
(This is not an approval on GitHub because I haven't done a full code review.)
gilles-peskine-arm
changed the title
|
Closing as superseded by #9981 |
|
Actually close as superseded by #9981 |
Description
This is the 3.6 backport of #9872 and #9928, with a few extra commits to try and preserve ABI compatibility in 3.6.
Status: work in progress pushed for CI feedback and early design review. Outdated see #9981
PR checklist
Please remove the segment/s on either side of the | symbol as appropriate, and add any relevant link/s to the end of the line.
If the provided content is part of the present PR remove the # symbol.
Notes for the submitter
Please refer to the contributing guidelines, especially the
checklist for PR contributors.
Help make review efficient: