Fix NULL+0 undefined behavior in ECB encryption and decryption

psa_cipher_encrypt() and psa_cipher_decrypt() sometimes add a zero offset to
a null pointer when the cipher does not use an IV. This is undefined
behavior, although it works as naively expected on most platforms. This
can cause a crash with modern Clang+ASan (depending on compiler optimizations).

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

ChangeLog.d/psa-ecb-ub.txt

@@ -0,0 +1,3 @@
+Bugfix
+   * Fix undefined behavior (typically harmless in practice) in PSA ECB
+     encryption and decryption.

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,42 @@ extern void (*mbedtls_test_hook_test_fail)( const char * test, int line, const c
  */
 #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.
+ *
+ * \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.
+ *
+ * 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.
+ *
+ * \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

library/psa_crypto.c

@@ -3454,8 +3454,8 @@ psa_status_t psa_cipher_encrypt( mbedtls_svc_key_id_t key,
     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,
+        mbedtls_buffer_offset( output, default_iv_length ),
-        output_length );
+        output_size - default_iv_length, output_length );
 
 exit:
     unlock_status = psa_unlock_key_slot( slot );

library/psa_crypto_cipher.c

@@ -516,10 +516,10 @@ psa_status_t mbedtls_psa_cipher_encrypt(
     if( status != PSA_SUCCESS )
         goto exit;
 
-    status = mbedtls_psa_cipher_finish( &operation,
+    status = mbedtls_psa_cipher_finish(
-                                        output + update_output_length,
+        &operation,
-                                        output_size - update_output_length,
+        mbedtls_buffer_offset( output, update_output_length ),
-                                        &finish_output_length );
+        output_size - update_output_length, &finish_output_length );
     if( status != PSA_SUCCESS )
         goto exit;
 
@@ -563,7 +563,9 @@ psa_status_t mbedtls_psa_cipher_decrypt(
             goto exit;
     }
 
-    status = mbedtls_psa_cipher_update( &operation, input + operation.iv_length,
+    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 )
@@ -571,9 +573,10 @@ psa_status_t mbedtls_psa_cipher_decrypt(
 
     accumulated_length = olength;
 
-    status = mbedtls_psa_cipher_finish( &operation, output + accumulated_length,
+    status = mbedtls_psa_cipher_finish(
-                                        output_size - accumulated_length,
+        &operation,
-                                        &olength );
+        mbedtls_buffer_offset( output, accumulated_length ),
+        output_size - accumulated_length, &olength );
     if( status != PSA_SUCCESS )
         goto exit;
 

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 @@ void cipher_alg_without_iv( int alg_arg, int key_type_arg, data_t *key_data,
     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 @@ void cipher_alg_without_iv( int alg_arg, int key_type_arg, data_t *key_data,
     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;