mbedtls_ssl_send_alert_message() not re-entrant when underlying transport is not ready

[hanno-becker]

Context: Functions returning MBEDTLS_ERR_SSL_WANT_READ/WRITE are supposed to be re-called with the same parameters until they succeed. This is usually implemented by having these functions first check if a flush is currently in progress, and if yes, solely attempt to complete it (in particular: not writing the respective message again). As long as the user does not mix unfinished calls of such functions, this is fine.

Issue: The alert sending function mbedtls_ssl_send_alert_message() does not follow this pattern:

int mbedtls_ssl_send_alert_message( mbedtls_ssl_context *ssl,
                            unsigned char level,
                            unsigned char message )
{
    int ret;

    if( ssl == NULL || ssl->conf == NULL )
        return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA );

    MBEDTLS_SSL_DEBUG_MSG( 2, ( "=> send alert message" ) );
    MBEDTLS_SSL_DEBUG_MSG( 3, ( "send alert level=%u message=%u", level, message ));

    ssl->out_msgtype = MBEDTLS_SSL_MSG_ALERT;
    ssl->out_msglen = 2;
    ssl->out_msg[0] = level;
    ssl->out_msg[1] = message;

    if( ( ret = mbedtls_ssl_write_record( ssl ) ) != 0 )
    {
        MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_ssl_write_record", ret );
        return( ret );
    }
    MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= send alert message" ) );

    return( 0 );
}

This code always writes a new record, regardless of the state of flushing, i.e. ignoring potentially only partially completed previous attempts to send the alert. This can lead to corrupted data being dispatched to the underlying stream transport, as e.g. it is possible to have a first call to mbedtls_ssl_send_alert_message() send a partial record header, followed by a second call which sends an entire alert message again.

The closely related function mbedtls_ssl_close_notify() behaves correctly:

int mbedtls_ssl_close_notify( mbedtls_ssl_context *ssl )
{
    int ret;

    if( ssl == NULL || ssl->conf == NULL )
        return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA );

    MBEDTLS_SSL_DEBUG_MSG( 2, ( "=> write close notify" ) );

    if( ssl->out_left != 0 )
        return( mbedtls_ssl_flush_output( ssl ) );

    if( ssl->state == MBEDTLS_SSL_HANDSHAKE_OVER )
    {
        if( ( ret = mbedtls_ssl_send_alert_message( ssl,
                        MBEDTLS_SSL_ALERT_LEVEL_WARNING,
                        MBEDTLS_SSL_ALERT_MSG_CLOSE_NOTIFY ) ) != 0 )
        {
            MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_ssl_send_alert_message", ret );
            return( ret );
        }
    }

    MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= write close notify" ) );

    return( 0 );
}

Suggestion for fix: Move the code

    if( ssl->out_left != 0 )
        return( mbedtls_ssl_flush_output( ssl ) );

from mbedtls_ssl_close_notify() to mbedtls_ssl_send_alert_message().

[ciarmcom]

ARM Internal Ref: IOTSSL-2464