New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mbedtls_ssl_get_record_expansion() returns wrong result for CBC suites in TLS >= 1.1 #1914

Closed
hanno-arm opened this Issue Aug 3, 2018 · 2 comments

Comments

Projects
None yet
3 participants
@hanno-arm
Contributor

hanno-arm commented Aug 3, 2018

Description

  • Type: Bug
  • Priority: Minor for 2.12.0, Major considering the use of mbedtls_ssl_get_record_expansion() in #1879

Bug

OS
All

Mbed TLS build:
Version: all versions supporting (D)TLS 1.1 or higher.
Configuration: Any configuration allowing (D)TLS 1.1 or higher.

Description
The public function mbedtls_ssl_get_record_expansion() is supposed to return the maximum difference between the length of a protected record and the length of the plaintext it encapsulates, with respect to the currently enabled outgoing record protection.

int mbedtls_ssl_get_record_expansion( const mbedtls_ssl_context *ssl )
{
    size_t transform_expansion;
    const mbedtls_ssl_transform *transform = ssl->transform_out;

#if defined(MBEDTLS_ZLIB_SUPPORT)
    if( ssl->session_out->compression != MBEDTLS_SSL_COMPRESS_NULL )
        return( MBEDTLS_ERR_SSL_FEATURE_UNAVAILABLE );
#endif

    if( transform == NULL )
        return( (int) mbedtls_ssl_hdr_len( ssl ) );

    switch( mbedtls_cipher_get_cipher_mode( &transform->cipher_ctx_enc ) )
    {
        case MBEDTLS_MODE_GCM:
        case MBEDTLS_MODE_CCM:
        case MBEDTLS_MODE_STREAM:
            transform_expansion = transform->minlen;
            break;

        case MBEDTLS_MODE_CBC:
            transform_expansion = transform->maclen
                      + mbedtls_cipher_get_block_size( &transform->cipher_ctx_enc );
            break;

        default:
            MBEDTLS_SSL_DEBUG_MSG( 1, ( "should never happen" ) );
            return( MBEDTLS_ERR_SSL_INTERNAL_ERROR );
    }

    return( (int)( mbedtls_ssl_hdr_len( ssl ) + transform_expansion ) );
}

In (D)TLS 1.1 or higher, CBC ciphersuites use an explicit initialization vector at the beginning of the record which is not taken into account in

        case MBEDTLS_MODE_CBC:
            transform_expansion = transform->maclen
                      + mbedtls_cipher_get_block_size( &transform->cipher_ctx_enc );

In total, there are three components contributing to the expansion: Pre-expansion due to explicit IV, post-expansion due to MAC, post-expansion due to padding.


Impact

Currently, there is no internal impact on the library as Mbed TLS does not use mbedtls_ssl_get_record_expansion(). However, PR #1879 starts to make use of mbedtls_ssl_get_record_expansion() to deduce the maximum plaintext length from the maximum MTU, and miscalculation will lead to Mbed TLS not obeying the MTU.

@ciarmcom

This comment has been minimized.

Member

ciarmcom commented Aug 3, 2018

ARM Internal Ref: IOTSSL-2462

@hanno-arm hanno-arm added fix available and removed tracking labels Aug 3, 2018

mpg added a commit to mpg/mbedtls that referenced this issue Aug 13, 2018

hanno-arm added a commit to hanno-arm/mbedtls that referenced this issue Aug 14, 2018

hanno-arm added a commit to hanno-arm/mbedtls that referenced this issue Aug 17, 2018

Fix mbedtls_ssl_get_record_expansion() for CBC modes
`mbedtls_ssl_get_record_expansion()` is supposed to return the maximum
difference between the size of a protected record and the size of the
encapsulated plaintext.

Previously, it did not correctly estimate the maximum record expansion
in case of CBC ciphersuites in (D)TLS versions 1.1 and higher, in which
case the ciphertext is prefixed by an explicit IV.

This commit fixes this bug. Fixes ARMmbed#1914.

hanno-arm added a commit to hanno-arm/mbedtls that referenced this issue Aug 17, 2018

Fix mbedtls_ssl_get_record_expansion() for CBC modes
`mbedtls_ssl_get_record_expansion()` is supposed to return the maximum
difference between the size of a protected record and the size of the
encapsulated plaintext.

Previously, it did not correctly estimate the maximum record expansion
in case of CBC ciphersuites in (D)TLS versions 1.1 and higher, in which
case the ciphertext is prefixed by an explicit IV.

This commit fixes this bug. Fixes ARMmbed#1914.

hanno-arm added a commit to hanno-arm/mbedtls that referenced this issue Aug 17, 2018

Fix mbedtls_ssl_get_record_expansion() for CBC modes
`mbedtls_ssl_get_record_expansion()` is supposed to return the maximum
difference between the size of a protected record and the size of the
encapsulated plaintext.

Previously, it did not correctly estimate the maximum record expansion
in case of CBC ciphersuites in (D)TLS versions 1.1 and higher, in which
case the ciphertext is prefixed by an explicit IV.

This commit fixes this bug. Fixes ARMmbed#1914.

hanno-arm added a commit to hanno-arm/mbedtls that referenced this issue Aug 17, 2018

Fix mbedtls_ssl_get_record_expansion() for CBC modes
`mbedtls_ssl_get_record_expansion()` is supposed to return the maximum
difference between the size of a protected record and the size of the
encapsulated plaintext.

Previously, it did not correctly estimate the maximum record expansion
in case of CBC ciphersuites in (D)TLS versions 1.1 and higher, in which
case the ciphertext is prefixed by an explicit IV.

This commit fixes this bug. Fixes ARMmbed#1914.
@sbutcher-arm

This comment has been minimized.

Collaborator

sbutcher-arm commented Sep 3, 2018

This issue has been fixed in PR's #1915 and back ports #1959 and #1960, which have now been merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment