From: maoyufeng <maoyufeng3@huawei.com>
Date: Mon, 23 May 2022 14:57:48 +0800
Subject: [PATCH] Fix issue CVE-2021-43666

Signed-off-by: maoyufeng <maoyufeng3@huawei.com>
---
 ChangeLog.d/fix-pkcs12-null-password.txt |  5 ++
 include/mbedtls/pkcs12.h                 | 34 ++++++----
 library/pkcs12.c                         | 82 ++++++++++++++++++------
 tests/CMakeLists.txt                     |  1 +
 tests/scripts/all.sh                     | 30 +++++++++
 tests/suites/test_suite_pkcs12.data      | 35 ++++++++++
 tests/suites/test_suite_pkcs12.function  | 68 ++++++++++++++++++++
 7 files changed, 223 insertions(+), 32 deletions(-)
 create mode 100644 ChangeLog.d/fix-pkcs12-null-password.txt
 mode change 100755 => 100644 library/pkcs12.c
 create mode 100644 tests/suites/test_suite_pkcs12.data
 create mode 100644 tests/suites/test_suite_pkcs12.function

diff --git a/ChangeLog.d/fix-pkcs12-null-password.txt b/ChangeLog.d/fix-pkcs12-null-password.txt
new file mode 100644
index 0000000000000000000000000000000000000000..a6ce140fdc3479cfac671542692ab646f7c8b5f3
--- /dev/null
+++ b/ChangeLog.d/fix-pkcs12-null-password.txt
@@ -0,0 +1,5 @@
+Bugfix
+   * Fix a potential invalid pointer dereference and infinite loop bugs in
+     pkcs12 functions when the password is empty. Fix the documentation to
+     better describe the inputs to these functions and their possible values.
+     Fixes #5136.
\ No newline at end of file
diff --git a/include/mbedtls/pkcs12.h b/include/mbedtls/pkcs12.h
index 9cbcb1730559bb7d3a22a378467b9f6aa1b1c3fa..9e11e24a8d07b9e377fd49d9206fd300330bcdbf 100755
--- a/include/mbedtls/pkcs12.h
+++ b/include/mbedtls/pkcs12.h
@@ -83,8 +83,9 @@ extern "C" {
  * \brief            PKCS12 Password Based function (encryption / decryption)
  *                   for pbeWithSHAAnd128BitRC4
  *
- * \param pbe_params an ASN1 buffer containing the pkcs-12PbeParams structure
- * \param mode       either MBEDTLS_PKCS12_PBE_ENCRYPT or MBEDTLS_PKCS12_PBE_DECRYPT
+ * \param pbe_params an ASN1 buffer containing the pkcs-12 PbeParams structure
+ * \param mode       either #MBEDTLS_PKCS12_PBE_ENCRYPT or
+ *                   #MBEDTLS_PKCS12_PBE_DECRYPT
  * \param pwd        the password used (may be NULL if no password is used)
  * \param pwdlen     length of the password (may be 0)
  * \param input      the input data
@@ -105,8 +106,9 @@ int mbedtls_pkcs12_pbe_sha1_rc4_128( mbedtls_asn1_buf *pbe_params, int mode,
  * \param pbe_params an ASN1 buffer containing the pkcs-12PbeParams structure
  * \param mode       either MBEDTLS_PKCS12_PBE_ENCRYPT or MBEDTLS_PKCS12_PBE_DECRYPT
  * \param cipher_type the cipher used
- * \param md_type     the mbedtls_md used
- * \param pwd        the password used (may be NULL if no password is used)
+ * \param md_type    the mbedtls_md used
+ * \param pwd        Latin1-encoded password used. This may only be \c NULL when
+ *                   \p pwdlen is 0. No null terminator should be used.
  * \param pwdlen     length of the password (may be 0)
  * \param input      the input data
  * \param len        data length
@@ -127,18 +129,24 @@ int mbedtls_pkcs12_pbe( mbedtls_asn1_buf *pbe_params, int mode,
  *                   to produce pseudo-random bits for a particular "purpose".
  *
  *                   Depending on the given id, this function can produce an
- *                   encryption/decryption key, an nitialization vector or an
+ *                   encryption/decryption key, an initialization vector or an
  *                   integrity key.
  *
  * \param data       buffer to store the derived data in
- * \param datalen    length to fill
- * \param pwd        password to use (may be NULL if no password is used)
- * \param pwdlen     length of the password (may be 0)
- * \param salt       salt buffer to use
- * \param saltlen    length of the salt
- * \param mbedtls_md         mbedtls_md type to use during the derivation
- * \param id         id that describes the purpose (can be MBEDTLS_PKCS12_DERIVE_KEY,
- *                   MBEDTLS_PKCS12_DERIVE_IV or MBEDTLS_PKCS12_DERIVE_MAC_KEY)
+ * \param datalen    length of buffer to fill
+ * \param pwd        The password to use. For compliance with PKCS#12 §B.1, this
+ *                   should be a BMPString, i.e. a Unicode string where each
+ *                   character is encoded as 2 bytes in big-endian order, with
+ *                   no byte order mark and with a null terminator (i.e. the
+ *                   last two bytes should be 0x00 0x00).
+ * \param pwdlen     length of the password (may be 0).
+ * \param salt       Salt buffer to use This may only be \c NULL when
+ *                   \p saltlen is 0.
+ * \param saltlen    length of the salt (may be zero)
+ * \param mbedtls_md mbedtls_md type to use during the derivation
+ * \param id         id that describes the purpose (can be
+ *                   #MBEDTLS_PKCS12_DERIVE_KEY, #MBEDTLS_PKCS12_DERIVE_IV or
+ *                   #MBEDTLS_PKCS12_DERIVE_MAC_KEY)
  * \param iterations number of iterations
  *
  * \return          0 if successful, or a MD, BIGNUM type error.
diff --git a/library/pkcs12.c b/library/pkcs12.c
old mode 100755
new mode 100644
index 3d23d5e354923cd01d69a479fcf572d80af540a6..05ade49e93b3d2cb8e03f7915f0ead4b79e919c4
--- a/library/pkcs12.c
+++ b/library/pkcs12.c
@@ -209,6 +209,9 @@ int mbedtls_pkcs12_pbe( mbedtls_asn1_buf *pbe_params, int mode,
     mbedtls_cipher_context_t cipher_ctx;
     size_t olen = 0;
 
+    if( pwd == NULL && pwdlen != 0 )
+        return( MBEDTLS_ERR_PKCS12_BAD_INPUT_DATA );
+
     cipher_info = mbedtls_cipher_info_from_type( cipher_type );
     if( cipher_info == NULL )
         return( MBEDTLS_ERR_PKCS12_FEATURE_UNAVAILABLE );
@@ -261,12 +264,23 @@ static void pkcs12_fill_buffer( unsigned char *data, size_t data_len,
     unsigned char *p = data;
     size_t use_len;
 
-    while( data_len > 0 )
+    if( filler != NULL && fill_len != 0 )
     {
-        use_len = ( data_len > fill_len ) ? fill_len : data_len;
-        memcpy( p, filler, use_len );
-        p += use_len;
-        data_len -= use_len;
+        while( data_len > 0 )
+        {
+            use_len = ( data_len > fill_len ) ? fill_len : data_len;
+            memcpy( p, filler, use_len );
+            p += use_len;
+            data_len -= use_len;
+        }
+    }
+    else
+    {
+        /* If either of the above are not true then clearly there is nothing
+         * that this function can do. The function should *not* be called
+         * under either of those circumstances, as you could end up with an
+         * incorrect output but for safety's sake, leaving the check in as
+         * otherwise we could end up with memory corruption.*/
     }
 }
 
@@ -283,6 +297,8 @@ int mbedtls_pkcs12_derivation( unsigned char *data, size_t datalen,
     unsigned char hash_output[MBEDTLS_MD_MAX_SIZE];
     unsigned char *p;
     unsigned char c;
+    int           use_password = 0;
+    int           use_salt = 0;
 
     size_t hlen, use_len, v, i;
 
@@ -293,6 +309,15 @@ int mbedtls_pkcs12_derivation( unsigned char *data, size_t datalen,
     if( datalen > 128 || pwdlen > 64 || saltlen > 64 )
         return( MBEDTLS_ERR_PKCS12_BAD_INPUT_DATA );
 
+    if( pwd == NULL && pwdlen != 0 )
+        return( MBEDTLS_ERR_PKCS12_BAD_INPUT_DATA );
+
+    if( salt == NULL && saltlen != 0 )
+        return( MBEDTLS_ERR_PKCS12_BAD_INPUT_DATA );
+
+    use_password = ( pwd && pwdlen != 0 );
+    use_salt = ( salt && saltlen != 0 );
+
     md_info = mbedtls_md_info_from_type( md_type );
     if( md_info == NULL )
         return( MBEDTLS_ERR_PKCS12_FEATURE_UNAVAILABLE );
@@ -310,8 +335,15 @@ int mbedtls_pkcs12_derivation( unsigned char *data, size_t datalen,
 
     memset( diversifier, (unsigned char) id, v );
 
-    pkcs12_fill_buffer( salt_block, v, salt, saltlen );
-    pkcs12_fill_buffer( pwd_block,  v, pwd,  pwdlen  );
+    if( use_salt != 0 )
+    {
+        pkcs12_fill_buffer( salt_block, v, salt, saltlen );
+    }
+
+    if( use_password != 0 )
+    {
+        pkcs12_fill_buffer( pwd_block,  v, pwd,  pwdlen  );
+    }
 
     p = data;
     while( datalen > 0 )
@@ -323,11 +355,17 @@ int mbedtls_pkcs12_derivation( unsigned char *data, size_t datalen,
         if( ( ret = mbedtls_md_update( &md_ctx, diversifier, v ) ) != 0 )
             goto exit;
 
-        if( ( ret = mbedtls_md_update( &md_ctx, salt_block, v ) ) != 0 )
-            goto exit;
+        if( use_salt != 0 )
+        {
+            if( ( ret = mbedtls_md_update( &md_ctx, salt_block, v )) != 0 )
+                goto exit;
+        }
 
-        if( ( ret = mbedtls_md_update( &md_ctx, pwd_block, v ) ) != 0 )
-            goto exit;
+        if( use_password != 0)
+        {
+            if( ( ret = mbedtls_md_update( &md_ctx, pwd_block, v )) != 0 )
+                goto exit;
+        }
 
         if( ( ret = mbedtls_md_finish( &md_ctx, hash_output ) ) != 0 )
             goto exit;
@@ -355,22 +393,28 @@ int mbedtls_pkcs12_derivation( unsigned char *data, size_t datalen,
             if( ++hash_block[i - 1] != 0 )
                 break;
 
-        // salt_block += B
-        c = 0;
-        for( i = v; i > 0; i-- )
+        if( use_salt != 0 )
         {
-            j = salt_block[i - 1] + hash_block[i - 1] + c;
+            // salt_block += B
+            c = 0;
+            for( i = v; i > 0; i-- )
+            {
+                j = salt_block[i - 1] + hash_block[i - 1] + c;
             c = (unsigned char) (j >> 8);
             salt_block[i - 1] = j & 0xFF;
+            }
         }
 
-        // pwd_block  += B
-        c = 0;
-        for( i = v; i > 0; i-- )
+        if( use_password != 0 )
         {
-            j = pwd_block[i - 1] + hash_block[i - 1] + c;
+            // pwd_block  += B
+            c = 0;
+            for( i = v; i > 0; i-- )
+            {
+                j = pwd_block[i - 1] + hash_block[i - 1] + c;
             c = (unsigned char) (j >> 8);
             pwd_block[i - 1] = j & 0xFF;
+            }
         }
     }
 
diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt
index a8e7523e504bc5cff9925648dd1b3bd7fb55f0cf..c5d484f924328bde2d2525b6f89e125974c3b770 100644
--- a/tests/CMakeLists.txt
+++ b/tests/CMakeLists.txt
@@ -120,6 +120,7 @@ add_test_suite(pem)
 add_test_suite(pkcs1_v15)
 add_test_suite(pkcs1_v21)
 add_test_suite(pkcs5)
+add_test_suite(pkcs12)
 add_test_suite(pk)
 add_test_suite(pkparse)
 add_test_suite(pkwrite)
diff --git a/tests/scripts/all.sh b/tests/scripts/all.sh
index 1a4de44b665f8df822adbd83bece912119bca98f..00222f379e808d8662967fdf200e18739cbabe61 100755
--- a/tests/scripts/all.sh
+++ b/tests/scripts/all.sh
@@ -1683,6 +1683,36 @@ component_test_valgrind () {
     fi
 }
 
+support_test_cmake_out_of_source () {
+    distrib_id=""
+    distrib_ver=""
+    distrib_ver_minor=""
+    distrib_ver_major=""
+
+    # Attempt to parse lsb-release to find out distribution and version. If not
+    # found this should fail safe (test is supported).
+    if [[ -f /etc/lsb-release ]]; then
+
+        while read -r lsb_line; do
+            case "$lsb_line" in
+                "DISTRIB_ID"*) distrib_id=${lsb_line/#DISTRIB_ID=};;
+                "DISTRIB_RELEASE"*) distrib_ver=${lsb_line/#DISTRIB_RELEASE=};;
+            esac
+        done < /etc/lsb-release
+
+        distrib_ver_major="${distrib_ver%%.*}"
+        distrib_ver="${distrib_ver#*.}"
+        distrib_ver_minor="${distrib_ver%%.*}"
+    fi
+
+    # Running the out of source CMake test on Ubuntu 16.04 using more than one
+    # processor (as the CI does) can create a race condition whereby the build
+    # fails to see a generated file, despite that file actually having been
+    # generated. This problem appears to go away with 18.04 or newer, so make
+    # the out of source tests unsupported on Ubuntu 16.04.
+    [ "$distrib_id" != "Ubuntu" ] || [ "$distrib_ver_major" -gt 16 ]
+}
+
 component_test_cmake_out_of_source () {
     msg "build: cmake 'out-of-source' build"
     MBEDTLS_ROOT_DIR="$PWD"
diff --git a/tests/suites/test_suite_pkcs12.data b/tests/suites/test_suite_pkcs12.data
new file mode 100644
index 0000000000000000000000000000000000000000..bda7d9921caad35c1835e0093df8a9644d4bd9cb
--- /dev/null
+++ b/tests/suites/test_suite_pkcs12.data
@@ -0,0 +1,35 @@
+PKCS#12 derive key : MD5: Zero length password and hash
+depends_on:MBEDTLS_MD5_C
+pkcs12_derive_key:MBEDTLS_MD_MD5:48:"":USE_GIVEN_INPUT:"":USE_GIVEN_INPUT:3:"6afdcbd5ebf943272134f1c3de2dc11b6afdcbd5ebf943272134f1c3de2dc11b6afdcbd5ebf943272134f1c3de2dc11b":0
+
+PKCS#12 derive key: MD5: NULL password and hash
+depends_on:MBEDTLS_MD5_C
+pkcs12_derive_key:MBEDTLS_MD_MD5:48:"":USE_NULL_INPUT:"":USE_NULL_INPUT:3:"6afdcbd5ebf943272134f1c3de2dc11b6afdcbd5ebf943272134f1c3de2dc11b6afdcbd5ebf943272134f1c3de2dc11b":0
+
+PKCS#12 derive key: MD5: Zero length password
+depends_on:MBEDTLS_MD5_C
+pkcs12_derive_key:MBEDTLS_MD_MD5:48:"":USE_GIVEN_INPUT:"0123456789abcdef":USE_GIVEN_INPUT:3:"832d8502114fcccfd3de0c2b2863b1c45fb92a8db2ed1e704727b324adc267bdd66ae4918a81fa2d1ba15febfb9e6c4e":0
+
+PKCS#12 derive key: MD5: NULL password
+depends_on:MBEDTLS_MD5_C
+pkcs12_derive_key:MBEDTLS_MD_MD5:48:"":USE_NULL_INPUT:"0123456789abcdef":USE_GIVEN_INPUT:3:"832d8502114fcccfd3de0c2b2863b1c45fb92a8db2ed1e704727b324adc267bdd66ae4918a81fa2d1ba15febfb9e6c4e":0
+
+PKCS#12 derive key: MD5: Invalid length NULL password
+depends_on:MBEDTLS_MD5_C
+pkcs12_derive_key:MBEDTLS_MD_MD5:48:"0123456789abcdef":USE_NULL_INPUT:"0123456789abcdef":USE_GIVEN_INPUT:3:"":MBEDTLS_ERR_PKCS12_BAD_INPUT_DATA
+
+PKCS#12 derive key: MD5: Zero length salt
+depends_on:MBEDTLS_MD5_C
+pkcs12_derive_key:MBEDTLS_MD_MD5:48:"0123456789abcdef":USE_GIVEN_INPUT:"":USE_GIVEN_INPUT:3:"832d8502114fcccfd3de0c2b2863b1c45fb92a8db2ed1e704727b324adc267bdd66ae4918a81fa2d1ba15febfb9e6c4e":0
+
+PKCS#12 derive key: MD5: NULL salt
+depends_on:MBEDTLS_MD5_C
+pkcs12_derive_key:MBEDTLS_MD_MD5:48:"0123456789abcdef":USE_GIVEN_INPUT:"":USE_NULL_INPUT:3:"832d8502114fcccfd3de0c2b2863b1c45fb92a8db2ed1e704727b324adc267bdd66ae4918a81fa2d1ba15febfb9e6c4e":0
+
+PKCS#12 derive key: MD5: Invalid length NULL salt
+depends_on:MBEDTLS_MD5_C
+pkcs12_derive_key:MBEDTLS_MD_MD5:48:"0123456789abcdef":USE_GIVEN_INPUT:"0123456789abcdef":USE_NULL_INPUT:3:"":MBEDTLS_ERR_PKCS12_BAD_INPUT_DATA
+
+PKCS#12 derive key: MD5: Valid password and salt
+depends_on:MBEDTLS_MD5_C
+pkcs12_derive_key:MBEDTLS_MD_MD5:48:"0123456789abcdef":USE_GIVEN_INPUT:"0123456789abcdef":USE_GIVEN_INPUT:3:"46559deeee036836ab1b633ec620178d4c70eacf42f72a2ad7360c812efa09ca3d7567b489a109050345c2dc6a262995":0
diff --git a/tests/suites/test_suite_pkcs12.function b/tests/suites/test_suite_pkcs12.function
new file mode 100644
index 0000000000000000000000000000000000000000..56b896c8253b5d9734d27b0bd7cd315b3cbc7ee7
--- /dev/null
+++ b/tests/suites/test_suite_pkcs12.function
@@ -0,0 +1,68 @@
+/* BEGIN_HEADER */
+#include "mbedtls/pkcs12.h"
+
+typedef enum
+{
+   USE_NULL_INPUT = 0,
+   USE_GIVEN_INPUT = 1,
+} input_usage_method_t;
+
+/* END_HEADER */
+
+/* BEGIN_DEPENDENCIES
+ * depends_on:MBEDTLS_PKCS12_C
+ * END_DEPENDENCIES
+ */
+
+/* BEGIN_CASE */
+void pkcs12_derive_key( int md_type, int key_size_arg,
+                        data_t *password_arg, int password_usage,
+                        data_t *salt_arg, int salt_usage,
+                        int iterations,
+                        data_t* expected_output, int expected_status )
+
+{
+   int ret = 0;
+   unsigned char *output_data = NULL;
+
+   unsigned char *password = NULL;
+   size_t password_len = 0;
+   unsigned char *salt = NULL;
+   size_t salt_len = 0;
+   size_t key_size = key_size_arg;
+
+   if( password_usage == USE_GIVEN_INPUT )
+      password = password_arg->x;
+
+   password_len = password_arg->len;
+
+   if( salt_usage == USE_GIVEN_INPUT )
+      salt = salt_arg->x;
+
+   salt_len = salt_arg->len;
+
+   ASSERT_ALLOC( output_data, key_size );
+
+   ret = mbedtls_pkcs12_derivation( output_data,
+                                    key_size,
+                                    password,
+                                    password_len,
+                                    salt,
+                                    salt_len,
+                                    md_type,
+                                    MBEDTLS_PKCS12_DERIVE_KEY,
+                                    iterations );
+
+   TEST_EQUAL( ret, expected_status );
+
+   if( expected_status == 0 )
+   {
+      ASSERT_COMPARE( expected_output->x, expected_output->len,
+                      output_data, key_size );
+   }
+
+exit:
+   mbedtls_free( output_data );
+
+}
+/* END_CASE */
\ No newline at end of file