Mbed-TLS / mbedtls

An open source, portable, easy to use, readable and flexible TLS library, and reference implementation of the PSA Cryptography API. Releases are on a varying cadence, typically around 3 - 6 months between releases.
https://www.trustedfirmware.org/projects/mbed-tls/
Other
5.58k stars 2.61k forks source link

Possible overflow in `mbedtls_x509_set_extension(..)` which leads to a segementation fault #8687

Closed jwinzig-at-hilscher closed 10 months ago

jwinzig-at-hilscher commented 11 months ago

Summary

When calling the function mbedtls_x509_set_extension(..) with the value 0xFFFFFFFF for the parameter val_len, an internal overflow leads to a segmentation fault.

System information

Mbed TLS version (number or commit id): 4aad0ff (current development branch) Operating system and version: Ubuntu 22.04.3 LTS Configuration (if not default, please attach mbedtls_config.h): default Compiler and options (if you used a pre-built binary, please indicate how you obtained it): see "Steps to reproduce" Additional environment information: -

Expected behavior

Return error (e.g. MBEDTLS_ERR_X509_BAD_INPUT_DATA).

Actual behavior

Segmentation fault at line 390, when writing a byte to a NULL-pointer (cur->val.p).

Steps to reproduce

Call mbedtls_x509_set_extension(..) with the value 0xFFFFFFFF for the parameter val_len. In mbedtls_x509_set_extension(..) the value val_len is incremented by 1 when calling mbedtls_asn1_store_named_data(..). Since it overflows, its has the value 0 inside the function. Therefore no buffer is allocated for cur->val.p and the value NULL is assigned to it.

Example programs/x509/set_ext.c:

#include "mbedtls/x509_csr.h"
#include "mbedtls/oid.h"
#include <stdio.h>

#define EXT_KEY_USAGE_TMP_BUF_MAX_LENGTH 12

int x509_set_extension_length_check(int val_len)
{
    int ret = 0;

    mbedtls_x509write_csr ctx;
    mbedtls_x509write_csr_init(&ctx);
    unsigned char buf[EXT_KEY_USAGE_TMP_BUF_MAX_LENGTH] = { 0 };
    unsigned char *p = buf + sizeof(buf);

    ret = mbedtls_x509_set_extension(&(ctx.MBEDTLS_PRIVATE(extensions)),
                                        MBEDTLS_OID_EXTENDED_KEY_USAGE,
                                        MBEDTLS_OID_SIZE(MBEDTLS_OID_EXTENDED_KEY_USAGE),
                                        0,
                                        p,
                                        val_len);

    return ret;
}

int main(void){
    int ret = 0;

    ret = x509_set_extension_length_check(0xFFFFFFFE);      // -0x2880 == MBEDTLS_ERR_X509_ALLOC_FAILED
    printf("val_len: 0xFFFFFFFE -> ret: -0x%04x\n", -ret); 

    ret = x509_set_extension_length_check(0xFFFFFFFF);      // Segementation fault
    printf("val_len: 0xFFFFFFFF -> ret: -0x%04x\n", -ret);
}

Build command:

cc -Wall -Wextra -Wformat=2 -Wno-format-nonliteral -I../tests/include -I../include -D_FILE_OFFSET_BITS=64 -I../3rdparty/everest/include -I../3rdparty/everest/include/everest -I../3rdparty/everest/include/everest/kremlib -I../3rdparty/p256-m/p256-m/include -I../3rdparty/p256-m/p256-m/include/p256-m -I../3rdparty/p256-m/p256-m_driver_interface -O2 x509/set_ext.c    ../tests/src/asn1_helpers.o ../tests/src/bignum_helpers.o ../tests/src/certs.o ../tests/src/fake_external_rng_for_test.o ../tests/src/helpers.o ../tests/src/psa_crypto_helpers.o ../tests/src/psa_exercise_key.o ../tests/src/random.o ../tests/src/threading_helpers.o ../tests/src/drivers/hash.o ../tests/src/drivers/platform_builtin_keys.o ../tests/src/drivers/test_driver_aead.o ../tests/src/drivers/test_driver_asymmetric_encryption.o ../tests/src/drivers/test_driver_cipher.o ../tests/src/drivers/test_driver_key_agreement.o ../tests/src/drivers/test_driver_key_management.o ../tests/src/drivers/test_driver_mac.o ../tests/src/drivers/test_driver_pake.o ../tests/src/drivers/test_driver_signature.o ../tests/src/test_helpers/ssl_helpers.o -L../library -lmbedtls -lmbedx509 -lmbedcrypto  -o x509/set_ext

Execution and Output:

./x509/set_ext 
val_len: 0xFFFFFFFE -> ret: -0x2880
zsh: segmentation fault  ./x509/set_ext

Additional information

-

davidhorstmann-arm commented 11 months ago

Hi,

Testing this on a 64-bit platform, there is a problem in this reproducer is caused by passing in an int rather than a size_t for val_len. Since this is signed (0xFFFFFFFF == -1) it is first promoted to 64-bit -1 (0xFFFFFFFFFFFFFFFF) then converted to a size_t. This value then overflows to zero.

If I try changing val_len in the reproducer to be a size_t I correctly allocate the large amount of memory and then segfault when trying to copy from the (non-existent) buffer.

However, I agree that this will be a problem for 32-bit platforms. I will review the fix.