mbedtls_ssl_write_real: buffer overflow if MBEDTLS_SSL_MAX_FRAGMENT_LENGTH not defined

[sjorsdewit]

I wrote a simple client based on the client1 example, combined with a rather minimal config (similar to config-mini-tls1_1.h). I noticed that i got segfaults if i tried to send more than a few kb of data with mbedtls_ssl_write().

The documentation for this function states:

If the requested length is greater than the maximum fragment length (either the built-in limit or the one set or negotiated with the peer), then:
with TLS, less bytes than requested are written.

However, it appears that the length passed to mbedtls_ssl_write() is only checked if MBEDTLS_SSL_MAX_FRAGMENT_LENGTH is defined in the config file. The length from mbedtls_ssl_write() is passed to ssl_write_real(), which does the following check:

#if defined(MBEDTLS_SSL_MAX_FRAGMENT_LENGTH)
    size_t max_len = mbedtls_ssl_get_max_frag_len( ssl );

    if( len > max_len )
    {
#if defined(MBEDTLS_SSL_PROTO_DTLS)
        if( ssl->conf->transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM )
        {
            MBEDTLS_SSL_DEBUG_MSG( 1, ( "fragment larger than the (negotiated) "
                                "maximum fragment length: %d > %d",
                                len, max_len ) );
            return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA );
        }
        else
#endif
            len = max_len;
    }
#endif /* MBEDTLS_SSL_MAX_FRAGMENT_LENGTH */

If MBEDTLS_SSL_MAX_FRAGMENT_LENGTH is not defined, the user-specified length is passed straight to memcpy( ssl->out_msg, buf, len ); a few lines below that, while ssl->out_msg may not be large enough.

Apart from the default config, almost none of the example configs define this MBEDTLS_SSL_MAX_FRAGMENT_LENGTH option. Therefore i did not have this option in my config. This results in a segfault if calling mbedtls_ssl_write() with a buffer larger than the mbedtls library has allocated.

I think the length should always be checked, at least to ensure that the ssl->out_msg buffer does not overflow. Or the documentation + examples could be updated to make it very clear that the caller should check the length. I am not sure about the security implications, but i could imagine this may be problematic. In my case the certificates were overwritten by the ssl->out_msg.

[sthagen]

... link to "memcpy( ssl->out_msg, buf, len ); a few lines below that" ...

[simonbutcher]

Thanks for raising the issue. We'll take a look.

[ciarmcom]

ARM Internal Ref: IOTSSL-1131

[gilles-peskine-arm]

Thanks for reporting this issue! I believe it is fixed by #1094, which is now merged to development, therefore I am closing this issue.

[sthagen]

Thanks!