Merge pull request #6648 from gilles-peskine-arm/psa-ecb-null-0
Fix NULL+0 undefined behavior in PSA crypto ECB
diff --git a/.travis.yml b/.travis.yml
index 67cb3ca..eaf817a 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -25,8 +25,40 @@
- tests/scripts/all.sh -k build_arm_linux_gnueabi_gcc_arm5vte build_arm_none_eabi_gcc_m0plus
- name: full configuration
+ os: linux
+ dist: focal
+ addons:
+ apt:
+ packages:
+ - clang-10
+ - gnutls-bin
script:
- - tests/scripts/all.sh -k test_full_cmake_gcc_asan
+ # Do a manual build+test sequence rather than using all.sh,
+ # because there's no all.sh component that does what we want,
+ # which is a build with Clang >= 10 and ASan, running all the SSL
+ # testing.
+ # - The clang executable in the default PATH is Clang 7 on
+ # Travis's focal instances, but we want Clang >= 10.
+ # - Running all the SSL testing requires a specific set of
+ # OpenSSL and GnuTLS versions and we don't want to bother
+ # with those on Travis.
+ # So we explicitly select clang-10 as the compiler, and we
+ # have ad hoc restrictions on SSL testing based on what is
+ # passing at the time of writing. We will remove these limitations
+ # gradually.
+ - make generated_files
+ - make CC=clang-10 CFLAGS='-Werror -Wall -Wextra -fsanitize=address,undefined -fno-sanitize-recover=all -O2' LDFLAGS='-Werror -Wall -Wextra -fsanitize=address,undefined -fno-sanitize-recover=all'
+ - make test
+ - programs/test/selftest
+ - tests/scripts/test_psa_constant_names.py
+ - tests/ssl-opt.sh
+ # Modern OpenSSL does not support fixed ECDH or null ciphers.
+ - tests/compat.sh -p OpenSSL -e 'NULL\|ECDH-'
+ - tests/scripts/travis-log-failure.sh
+ # GnuTLS supports CAMELLIA but compat.sh doesn't properly enable it.
+ - tests/compat.sh -p GnuTLS -e 'CAMELLIA'
+ - tests/scripts/travis-log-failure.sh
+ - tests/context-info.sh
- name: Windows
os: windows
diff --git a/ChangeLog.d/psa-ecb-ub.txt b/ChangeLog.d/psa-ecb-ub.txt
new file mode 100644
index 0000000..9d725ac
--- /dev/null
+++ b/ChangeLog.d/psa-ecb-ub.txt
@@ -0,0 +1,3 @@
+Bugfix
+ * Fix undefined behavior (typically harmless in practice) in PSA ECB
+ encryption and decryption.
diff --git a/include/mbedtls/aes.h b/include/mbedtls/aes.h
index c359011..1cd20fe 100644
--- a/include/mbedtls/aes.h
+++ b/include/mbedtls/aes.h
@@ -61,11 +61,6 @@
/** Invalid input data. */
#define MBEDTLS_ERR_AES_BAD_INPUT_DATA -0x0021
-#if ( defined(__ARMCC_VERSION) || defined(_MSC_VER) ) && \
- !defined(inline) && !defined(__cplusplus)
-#define inline __inline
-#endif
-
#ifdef __cplusplus
extern "C" {
#endif
diff --git a/include/mbedtls/build_info.h b/include/mbedtls/build_info.h
index 170cbeb..362ce2f 100644
--- a/include/mbedtls/build_info.h
+++ b/include/mbedtls/build_info.h
@@ -53,6 +53,12 @@
#define _CRT_SECURE_NO_DEPRECATE 1
#endif
+/* Define `inline` on some non-C99-compliant compilers. */
+#if ( defined(__ARMCC_VERSION) || defined(_MSC_VER) ) && \
+ !defined(inline) && !defined(__cplusplus)
+#define inline __inline
+#endif
+
#if !defined(MBEDTLS_CONFIG_FILE)
#include "mbedtls/mbedtls_config.h"
#else
diff --git a/include/mbedtls/cipher.h b/include/mbedtls/cipher.h
index a3f52ea..151da1d 100644
--- a/include/mbedtls/cipher.h
+++ b/include/mbedtls/cipher.h
@@ -46,11 +46,6 @@
#define MBEDTLS_CIPHER_MODE_STREAM
#endif
-#if ( defined(__ARMCC_VERSION) || defined(_MSC_VER) ) && \
- !defined(inline) && !defined(__cplusplus)
-#define inline __inline
-#endif
-
/** The selected feature is not available. */
#define MBEDTLS_ERR_CIPHER_FEATURE_UNAVAILABLE -0x6080
/** Bad input parameters. */
diff --git a/include/mbedtls/error.h b/include/mbedtls/error.h
index 841b75b..4a97d65 100644
--- a/include/mbedtls/error.h
+++ b/include/mbedtls/error.h
@@ -26,11 +26,6 @@
#include <stddef.h>
-#if ( defined(__ARMCC_VERSION) || defined(_MSC_VER) ) && \
- !defined(inline) && !defined(__cplusplus)
-#define inline __inline
-#endif
-
/**
* Error code layout.
*
diff --git a/include/mbedtls/pem.h b/include/mbedtls/pem.h
index c75a124..a4c6fb8 100644
--- a/include/mbedtls/pem.h
+++ b/include/mbedtls/pem.h
@@ -27,11 +27,6 @@
#include <stddef.h>
-#if ( defined(__ARMCC_VERSION) || defined(_MSC_VER) ) && \
- !defined(inline) && !defined(__cplusplus)
-#define inline __inline
-#endif
-
/**
* \name PEM Error codes
* These error codes are returned in case of errors reading the
diff --git a/include/mbedtls/pk.h b/include/mbedtls/pk.h
index 867961d..db0bfac 100644
--- a/include/mbedtls/pk.h
+++ b/include/mbedtls/pk.h
@@ -44,11 +44,6 @@
#include "psa/crypto.h"
#endif
-#if ( defined(__ARMCC_VERSION) || defined(_MSC_VER) ) && \
- !defined(inline) && !defined(__cplusplus)
-#define inline __inline
-#endif
-
/** Memory allocation failed. */
#define MBEDTLS_ERR_PK_ALLOC_FAILED -0x3F80
/** Type mismatch, eg attempt to encrypt with an ECDSA key */
diff --git a/include/psa/crypto_platform.h b/include/psa/crypto_platform.h
index 47ab1cf..573b33c 100644
--- a/include/psa/crypto_platform.h
+++ b/include/psa/crypto_platform.h
@@ -45,11 +45,6 @@
/* PSA requires several types which C99 provides in stdint.h. */
#include <stdint.h>
-#if ( defined(__ARMCC_VERSION) || defined(_MSC_VER) ) && \
- !defined(inline) && !defined(__cplusplus)
-#define inline __inline
-#endif
-
#if defined(MBEDTLS_PSA_CRYPTO_KEY_ID_ENCODES_OWNER)
/* Building for the PSA Crypto service on a PSA platform, a key owner is a PSA
diff --git a/library/aria.c b/library/aria.c
index 924f952..5e52eea 100644
--- a/library/aria.c
+++ b/library/aria.c
@@ -37,11 +37,6 @@
#include "mbedtls/platform_util.h"
-#if ( defined(__ARMCC_VERSION) || defined(_MSC_VER) ) && \
- !defined(inline) && !defined(__cplusplus)
-#define inline __inline
-#endif
-
/* Parameter validation macros */
#define ARIA_VALIDATE_RET( cond ) \
MBEDTLS_INTERNAL_VALIDATE_RET( cond, MBEDTLS_ERR_ARIA_BAD_INPUT_DATA )
diff --git a/library/chacha20.c b/library/chacha20.c
index e53eb82..85d7461 100644
--- a/library/chacha20.c
+++ b/library/chacha20.c
@@ -36,11 +36,6 @@
#if !defined(MBEDTLS_CHACHA20_ALT)
-#if ( defined(__ARMCC_VERSION) || defined(_MSC_VER) ) && \
- !defined(inline) && !defined(__cplusplus)
-#define inline __inline
-#endif
-
#define ROTL32( value, amount ) \
( (uint32_t) ( (value) << (amount) ) | ( (value) >> ( 32 - (amount) ) ) )
diff --git a/library/common.h b/library/common.h
index a630fcc..25d5294 100644
--- a/library/common.h
+++ b/library/common.h
@@ -25,6 +25,7 @@
#include "mbedtls/build_info.h"
+#include <stddef.h>
#include <stdint.h>
/** Helper to define a function as static except when building invasive tests.
@@ -68,6 +69,44 @@
*/
#define MBEDTLS_ALLOW_PRIVATE_ACCESS
+/** Return an offset into a buffer.
+ *
+ * This is just the addition of an offset to a pointer, except that this
+ * function also accepts an offset of 0 into a buffer whose pointer is null.
+ * (`p + n` has undefined behavior when `p` is null, even when `n == 0`.
+ * A null pointer is a valid buffer pointer when the size is 0, for example
+ * as the result of `malloc(0)` on some platforms.)
+ *
+ * \param p Pointer to a buffer of at least n bytes.
+ * This may be \p NULL if \p n is zero.
+ * \param n An offset in bytes.
+ * \return Pointer to offset \p n in the buffer \p p.
+ * Note that this is only a valid pointer if the size of the
+ * buffer is at least \p n + 1.
+ */
+static inline unsigned char *mbedtls_buffer_offset(
+ unsigned char *p, size_t n )
+{
+ return( p == NULL ? NULL : p + n );
+}
+
+/** Return an offset into a read-only buffer.
+ *
+ * Similar to mbedtls_buffer_offset(), but for const pointers.
+ *
+ * \param p Pointer to a buffer of at least n bytes.
+ * This may be \p NULL if \p n is zero.
+ * \param n An offset in bytes.
+ * \return Pointer to offset \p n in the buffer \p p.
+ * Note that this is only a valid pointer if the size of the
+ * buffer is at least \p n + 1.
+ */
+static inline const unsigned char *mbedtls_buffer_offset_const(
+ const unsigned char *p, size_t n )
+{
+ return( p == NULL ? NULL : p + n );
+}
+
/** Byte Reading Macros
*
* Given a multi-byte integer \p x, MBEDTLS_BYTE_n retrieves the n-th
diff --git a/library/debug.c b/library/debug.c
index bdbf6dd..6114a46 100644
--- a/library/debug.c
+++ b/library/debug.c
@@ -30,11 +30,6 @@
#include <stdio.h>
#include <string.h>
-#if ( defined(__ARMCC_VERSION) || defined(_MSC_VER) ) && \
- !defined(inline) && !defined(__cplusplus)
-#define inline __inline
-#endif
-
#define DEBUG_BUF_SIZE 512
static int debug_threshold = 0;
diff --git a/library/ecp.c b/library/ecp.c
index 37f6090..cd7d554 100644
--- a/library/ecp.c
+++ b/library/ecp.c
@@ -88,11 +88,6 @@
#include "ecp_internal_alt.h"
-#if ( defined(__ARMCC_VERSION) || defined(_MSC_VER) ) && \
- !defined(inline) && !defined(__cplusplus)
-#define inline __inline
-#endif
-
#if defined(MBEDTLS_SELF_TEST)
/*
* Counts of point addition and doubling, and field multiplications.
diff --git a/library/ecp_curves.c b/library/ecp_curves.c
index 7b14237..5cd2828 100644
--- a/library/ecp_curves.c
+++ b/library/ecp_curves.c
@@ -39,11 +39,6 @@
#define ECP_VALIDATE( cond ) \
MBEDTLS_INTERNAL_VALIDATE( cond )
-#if ( defined(__ARMCC_VERSION) || defined(_MSC_VER) ) && \
- !defined(inline) && !defined(__cplusplus)
-#define inline __inline
-#endif
-
#define ECP_MPI_INIT(s, n, p) {s, (n), (mbedtls_mpi_uint *)(p)}
#define ECP_MPI_INIT_ARRAY(x) \
diff --git a/library/mps_reader.c b/library/mps_reader.c
index 36958b4..6f823bd 100644
--- a/library/mps_reader.c
+++ b/library/mps_reader.c
@@ -29,11 +29,6 @@
#include <string.h>
-#if ( defined(__ARMCC_VERSION) || defined(_MSC_VER) ) && \
- !defined(inline) && !defined(__cplusplus)
-#define inline __inline
-#endif
-
#if defined(MBEDTLS_MPS_ENABLE_TRACE)
static int mbedtls_mps_trace_id = MBEDTLS_MPS_TRACE_BIT_READER;
#endif /* MBEDTLS_MPS_ENABLE_TRACE */
diff --git a/library/poly1305.c b/library/poly1305.c
index 0850f66..4d0cdee 100644
--- a/library/poly1305.c
+++ b/library/poly1305.c
@@ -32,11 +32,6 @@
#if !defined(MBEDTLS_POLY1305_ALT)
-#if ( defined(__ARMCC_VERSION) || defined(_MSC_VER) ) && \
- !defined(inline) && !defined(__cplusplus)
-#define inline __inline
-#endif
-
#define POLY1305_BLOCK_SIZE_BYTES ( 16U )
/*
diff --git a/library/psa_crypto.c b/library/psa_crypto.c
index 8c9deff..e881f2f 100644
--- a/library/psa_crypto.c
+++ b/library/psa_crypto.c
@@ -3454,8 +3454,8 @@
status = psa_driver_wrapper_cipher_encrypt(
&attributes, slot->key.data, slot->key.bytes,
alg, local_iv, default_iv_length, input, input_length,
- output + default_iv_length, output_size - default_iv_length,
- output_length );
+ mbedtls_buffer_offset( output, default_iv_length ),
+ output_size - default_iv_length, output_length );
exit:
unlock_status = psa_unlock_key_slot( slot );
diff --git a/library/psa_crypto_cipher.c b/library/psa_crypto_cipher.c
index 70dc74d..91a0e3b 100644
--- a/library/psa_crypto_cipher.c
+++ b/library/psa_crypto_cipher.c
@@ -516,10 +516,10 @@
if( status != PSA_SUCCESS )
goto exit;
- status = mbedtls_psa_cipher_finish( &operation,
- output + update_output_length,
- output_size - update_output_length,
- &finish_output_length );
+ status = mbedtls_psa_cipher_finish(
+ &operation,
+ mbedtls_buffer_offset( output, update_output_length ),
+ output_size - update_output_length, &finish_output_length );
if( status != PSA_SUCCESS )
goto exit;
@@ -563,17 +563,20 @@
goto exit;
}
- status = mbedtls_psa_cipher_update( &operation, input + operation.iv_length,
- input_length - operation.iv_length,
- output, output_size, &olength );
+ status = mbedtls_psa_cipher_update(
+ &operation,
+ mbedtls_buffer_offset_const( input, operation.iv_length ),
+ input_length - operation.iv_length,
+ output, output_size, &olength );
if( status != PSA_SUCCESS )
goto exit;
accumulated_length = olength;
- status = mbedtls_psa_cipher_finish( &operation, output + accumulated_length,
- output_size - accumulated_length,
- &olength );
+ status = mbedtls_psa_cipher_finish(
+ &operation,
+ mbedtls_buffer_offset( output, accumulated_length ),
+ output_size - accumulated_length, &olength );
if( status != PSA_SUCCESS )
goto exit;
diff --git a/library/ssl_misc.h b/library/ssl_misc.h
index 9998e5b..1902d71 100644
--- a/library/ssl_misc.h
+++ b/library/ssl_misc.h
@@ -57,11 +57,6 @@
#include "common.h"
-#if ( defined(__ARMCC_VERSION) || defined(_MSC_VER) ) && \
- !defined(inline) && !defined(__cplusplus)
-#define inline __inline
-#endif
-
/* Shorthand for restartable ECC */
#if defined(MBEDTLS_ECP_RESTARTABLE) && \
defined(MBEDTLS_SSL_CLI_C) && \
diff --git a/tests/compat.sh b/tests/compat.sh
index d681217..529c2c5 100755
--- a/tests/compat.sh
+++ b/tests/compat.sh
@@ -595,6 +595,20 @@
G_CLIENT_ARGS="-p $PORT --debug 3 $G_MODE"
G_CLIENT_PRIO="NONE:$G_PRIO_MODE:+COMP-NULL:+CURVE-ALL:+SIGN-ALL"
+ # Newer versions of OpenSSL have a syntax to enable all "ciphers", even
+ # low-security ones. This covers not just cipher suites but also protocol
+ # versions. It is necessary, for example, to use (D)TLS 1.0/1.1 on
+ # OpenSSL 1.1.1f from Ubuntu 20.04. The syntax was only introduced in
+ # OpenSSL 1.1.0 (21e0c1d23afff48601eb93135defddae51f7e2e3) and I can't find
+ # a way to discover it from -help, so check the openssl version.
+ case $($OPENSSL_CMD version) in
+ "OpenSSL 0"*|"OpenSSL 1.0"*) :;;
+ *)
+ O_CLIENT_ARGS="$O_CLIENT_ARGS -cipher ALL@SECLEVEL=0"
+ O_SERVER_ARGS="$O_SERVER_ARGS -cipher ALL@SECLEVEL=0"
+ ;;
+ esac
+
if [ "X$VERIFY" = "XYES" ];
then
M_SERVER_ARGS="$M_SERVER_ARGS ca_file=data_files/test-ca_cat12.crt auth_mode=required"
diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh
index b460c67..c6f6e29 100755
--- a/tests/ssl-opt.sh
+++ b/tests/ssl-opt.sh
@@ -1689,6 +1689,20 @@
O_LEGACY_CLI="$O_LEGACY_CLI -connect 127.0.0.1:+SRV_PORT"
fi
+# Newer versions of OpenSSL have a syntax to enable all "ciphers", even
+# low-security ones. This covers not just cipher suites but also protocol
+# versions. It is necessary, for example, to use (D)TLS 1.0/1.1 on
+# OpenSSL 1.1.1f from Ubuntu 20.04. The syntax was only introduced in
+# OpenSSL 1.1.0 (21e0c1d23afff48601eb93135defddae51f7e2e3) and I can't find
+# a way to discover it from -help, so check the openssl version.
+case $($OPENSSL_CMD version) in
+ "OpenSSL 0"*|"OpenSSL 1.0"*) :;;
+ *)
+ O_CLI="$O_CLI -cipher ALL@SECLEVEL=0"
+ O_SRV="$O_SRV -cipher ALL@SECLEVEL=0"
+ ;;
+esac
+
if [ -n "${OPENSSL_NEXT:-}" ]; then
O_NEXT_SRV="$O_NEXT_SRV -accept $SRV_PORT"
O_NEXT_SRV_NO_CERT="$O_NEXT_SRV_NO_CERT -accept $SRV_PORT"
diff --git a/tests/suites/test_suite_psa_crypto.function b/tests/suites/test_suite_psa_crypto.function
index ca1614b..1f3b3b6 100644
--- a/tests/suites/test_suite_psa_crypto.function
+++ b/tests/suites/test_suite_psa_crypto.function
@@ -4,6 +4,7 @@
#include "mbedtls/asn1.h"
#include "mbedtls/asn1write.h"
#include "mbedtls/oid.h"
+#include "common.h"
/* For MBEDTLS_CTR_DRBG_MAX_REQUEST, knowing that psa_generate_random()
* uses mbedtls_ctr_drbg internally. */
@@ -3983,7 +3984,7 @@
TEST_LE_U( length, output_buffer_size );
output_length += length;
PSA_ASSERT( psa_cipher_finish( &operation,
- output + output_length,
+ mbedtls_buffer_offset( output, output_length ),
output_buffer_size - output_length,
&length ) );
output_length += length;
@@ -4001,7 +4002,7 @@
TEST_LE_U( length, output_buffer_size );
output_length += length;
PSA_ASSERT( psa_cipher_finish( &operation,
- output + output_length,
+ mbedtls_buffer_offset( output, output_length ),
output_buffer_size - output_length,
&length ) );
output_length += length;