Improve & test legacy mbedtls_pkcs5_pbe2

* Prevent pkcs5_pbe2 encryption when PKCS7 padding has been
  disabled since this not part of the specs.
* Allow decryption when PKCS7 padding is disabled for legacy
  reasons, However, invalid padding is not checked.
* Add tests to check these scenarios. Test data has been
  reused but with changing padding data in last block to
  check for valid/invalid padding.
* Document new behaviour, known limitations and possible
  security concerns.

Signed-off-by: Waleed Elmelegy <waleed.elmelegy@arm.com>
diff --git a/include/mbedtls/pkcs5.h b/include/mbedtls/pkcs5.h
index 12dec05..8896714 100644
--- a/include/mbedtls/pkcs5.h
+++ b/include/mbedtls/pkcs5.h
@@ -57,13 +57,36 @@
 /**
  * \brief          PKCS#5 PBES2 function
  *
+ * \note           When encrypting, #MBEDTLS_CIPHER_PADDING_PKCS7 must
+ *                 be enabled at compile time.
+ *
+ * \warning        When decrypting:
+ *                 - if #MBEDTLS_CIPHER_PADDING_PKCS7 is enabled at compile
+ *                   time, this function validates the CBC padding and returns
+ *                   #MBEDTLS_ERR_PKCS5_PASSWORD_MISMATCH if the padding is
+ *                   invalid. Note that this can help active adversaries
+ *                   attempting to brute-forcing the password. Note also that
+ *                   there is no guarantee that an invalid password will be
+ *                   detected (the chances of a valid padding with a random
+ *                   password are about 1/255).
+ *                 - if #MBEDTLS_CIPHER_PADDING_PKCS7 is disabled at compile
+ *                   time, this function does not validate the CBC padding.
+ *
  * \param pbe_params the ASN.1 algorithm parameters
  * \param mode       either MBEDTLS_PKCS5_DECRYPT or MBEDTLS_PKCS5_ENCRYPT
  * \param pwd        password to use when generating key
  * \param pwdlen     length of password
  * \param data       data to process
  * \param datalen    length of data
- * \param output     output buffer
+ * \param output     Output buffer.
+ *                   On success, it contains the decrypted data, possibly
+ *                   followed by the CBC padding.
+ *                   On failure, the content is indetermidate.
+ *                   For decryption, there must be enough room for \p datalen
+ *                   bytes.
+ *                   For encryption, there must be enough room for
+ *                   \p datalen + 1 bytes, rounded up to the block size of
+ *                   the block cipher identified by \p pbe_params.
  *
  * \returns        0 on success, or a MBEDTLS_ERR_XXX code if verification fails.
  */
diff --git a/library/pkcs5.c b/library/pkcs5.c
index 52f1a0d..6a4b3b0 100644
--- a/library/pkcs5.c
+++ b/library/pkcs5.c
@@ -211,6 +211,24 @@
         goto exit;
     }
 
+    /* PKCS5 uses CBC with PKCS7 padding (which is the same as
+     * "PKCS5 padding" except that it's typically only called PKCS5
+     * with 64-bit-block ciphers).
+     */
+    mbedtls_cipher_padding_t padding = MBEDTLS_PADDING_PKCS7;
+#if !defined(MBEDTLS_CIPHER_PADDING_PKCS7)
+    /* For historical reasons, when decrypting, this function works when
+     * decrypting even when support for PKCS7 padding is disabled. In this
+     * case, it ignores the padding, and so will never report a
+     * password mismatch.
+     */
+    if (mode == MBEDTLS_DECRYPT)
+        padding = MBEDTLS_PADDING_NONE;
+#endif
+    if ((ret = mbedtls_cipher_set_padding_mode(&cipher_ctx, padding)) != 0) {
+        goto exit;
+    }
+
     if ((ret = mbedtls_cipher_crypt(&cipher_ctx, iv, enc_scheme_params.len,
                                     data, datalen, output, &olen)) != 0) {
         ret = MBEDTLS_ERR_PKCS5_PASSWORD_MISMATCH;
diff --git a/tests/suites/test_suite_pkcs5.data b/tests/suites/test_suite_pkcs5.data
index bd251f7..03f2cf2 100644
--- a/tests/suites/test_suite_pkcs5.data
+++ b/tests/suites/test_suite_pkcs5.data
@@ -106,10 +106,34 @@
 depends_on:MBEDTLS_SHA512_C
 pbkdf2_hmac:MBEDTLS_MD_SHA512:"7061737300776f7264":"7361006c74":4096:16:"9d9e9c4cd21fe4be24d5b8244c759665"
 
+PBES2 Encrypt, pad=6 (OK)
+depends_on:MBEDTLS_MD_CAN_SHA1:MBEDTLS_DES_C:MBEDTLS_CIPHER_MODE_CBC:MBEDTLS_CIPHER_PADDING_PKCS7
+pbes2_encrypt:MBEDTLS_ASN1_CONSTRUCTED | MBEDTLS_ASN1_SEQUENCE:"301B06092A864886F70D01050C300E04082ED7F24A1D516DD702020800301406082A864886F70D030704088A4FCC9DCC394910":"70617373776f7264":"308187020100301306072A8648CE3D020106082A8648CE3D030107046D306B0201010420F12A1320760270A83CBFFD53F6031EF76A5D86C8A204F2C30CA9EBF51F0F0EA7A1440342000437CC56D976091E5A723EC7592DFF206EEE7CF9069174D0AD14B5F768225962924EE500D82311FFEA2FD2345D5D16BD8A88C26B770D55CD8A2A0EFA01C8B4EDFF":0:"1B60098D4834CA752D37B430E70B7A085CFF86E21F4849F969DD1DF623342662443F8BD1252BF83CEF6917551B08EF55A69C8F2BFFC93BCB2DFE2E354DA28F896D1BD1BFB972A1251219A6EC7183B0A4CF2C4998449ED786CAE2138437289EB2203974000C38619DA57A4E685D29649284602BD1806131772DA11A682674DC22B2CF109128DDB7FD980E1C5741FC0DB7"
+
+PBES2 Encrypt, pad=16 (OK)
+depends_on:MBEDTLS_MD_CAN_SHA1:MBEDTLS_DES_C:MBEDTLS_CIPHER_MODE_CBC:MBEDTLS_CIPHER_PADDING_PKCS7
+pbes2_encrypt:MBEDTLS_ASN1_CONSTRUCTED | MBEDTLS_ASN1_SEQUENCE:"301B06092A864886F70D01050C300E04082ED7F24A1D516DD702020800301406082A864886F70D030704088A4FCC9DCC394910":"70617373776f7264":"308187020100301306072A8648CE3D020106082A8648CE3D030107046D306B0201010420F12A1320760270A83CBFFD53F6031EF76A5D86C8A204F2C30CA9EBF51F0F0EA7A1440342000437CC56D976091E5A723EC7592DFF206EEE7CF9069174D0AD14B5F768225962924EE500D82311FFEA2FD2345D5D16BD8A88C26B770D5510101010101010101010101010101010":0:"1B60098D4834CA752D37B430E70B7A085CFF86E21F4849F969DD1DF623342662443F8BD1252BF83CEF6917551B08EF55A69C8F2BFFC93BCB2DFE2E354DA28F896D1BD1BFB972A1251219A6EC7183B0A4CF2C4998449ED786CAE2138437289EB2203974000C38619DA57A4E685D29649284602BD1806131772DA11A682674DC22D8D337E00CB5D1B5B76BE4AE39341405"
+
+PBES2 Encrypt, pad=6 (PKCS7 padding disabled)
+depends_on:MBEDTLS_MD_CAN_SHA1:MBEDTLS_DES_C:MBEDTLS_CIPHER_MODE_CBC:!MBEDTLS_CIPHER_PADDING_PKCS7
+pbes2_encrypt:MBEDTLS_ASN1_CONSTRUCTED | MBEDTLS_ASN1_SEQUENCE:"301B06092A864886F70D01050C300E04082ED7F24A1D516DD702020800301406082A864886F70D030704088A4FCC9DCC394910":"70617373776f7264":"308187020100301306072A8648CE3D020106082A8648CE3D030107046D306B0201010420F12A1320760270A83CBFFD53F6031EF76A5D86C8A204F2C30CA9EBF51F0F0EA7A1440342000437CC56D976091E5A723EC7592DFF206EEE7CF9069174D0AD14B5F768225962924EE500D82311FFEA2FD2345D5D16BD8A88C26B770D55CD8A2A0EFA01C8B4EDFF":MBEDTLS_ERR_CIPHER_FEATURE_UNAVAILABLE:""
+
+PBES2 Encrypt, pad=16 (PKCS7 padding disabled)
+depends_on:MBEDTLS_MD_CAN_SHA1:MBEDTLS_DES_C:MBEDTLS_CIPHER_MODE_CBC:!MBEDTLS_CIPHER_PADDING_PKCS7
+pbes2_encrypt:MBEDTLS_ASN1_CONSTRUCTED | MBEDTLS_ASN1_SEQUENCE:"301B06092A864886F70D01050C300E04082ED7F24A1D516DD702020800301406082A864886F70D030704088A4FCC9DCC394910":"70617373776f7264":"308187020100301306072A8648CE3D020106082A8648CE3D030107046D306B0201010420F12A1320760270A83CBFFD53F6031EF76A5D86C8A204F2C30CA9EBF51F0F0EA7A1440342000437CC56D976091E5A723EC7592DFF206EEE7CF9069174D0AD14B5F768225962924EE500D82311FFEA2FD2345D5D16BD8A88C26B770D5510101010101010101010101010101010":MBEDTLS_ERR_CIPHER_FEATURE_UNAVAILABLE:""
+
 PBES2 Decrypt (OK)
 depends_on:MBEDTLS_SHA1_C:MBEDTLS_DES_C:MBEDTLS_CIPHER_MODE_CBC
 mbedtls_pkcs5_pbes2:MBEDTLS_ASN1_CONSTRUCTED | MBEDTLS_ASN1_SEQUENCE:"301B06092A864886F70D01050C300E04082ED7F24A1D516DD702020800301406082A864886F70D030704088A4FCC9DCC394910":"70617373776f7264":"1B60098D4834CA752D37B430E70B7A085CFF86E21F4849F969DD1DF623342662443F8BD1252BF83CEF6917551B08EF55A69C8F2BFFC93BCB2DFE2E354DA28F896D1BD1BFB972A1251219A6EC7183B0A4CF2C4998449ED786CAE2138437289EB2203974000C38619DA57A4E685D29649284602BD1806131772DA11A682674DC22B2CF109128DDB7FD980E1C5741FC0DB7":0:"308187020100301306072A8648CE3D020106082A8648CE3D030107046D306B0201010420F12A1320760270A83CBFFD53F6031EF76A5D86C8A204F2C30CA9EBF51F0F0EA7A1440342000437CC56D976091E5A723EC7592DFF206EEE7CF9069174D0AD14B5F768225962924EE500D82311FFEA2FD2345D5D16BD8A88C26B770D55CD8A2A0EFA01C8B4EDFF060606060606"
 
+PBES2 Decrypt (Invalid padding & PKCS7 padding disabled)
+depends_on:MBEDTLS_MD_CAN_SHA1:MBEDTLS_DES_C:MBEDTLS_CIPHER_MODE_CBC:!MBEDTLS_CIPHER_PADDING_PKCS7
+mbedtls_pkcs5_pbes2:MBEDTLS_ASN1_CONSTRUCTED | MBEDTLS_ASN1_SEQUENCE:"301B06092A864886F70D01050C300E04082ED7F24A1D516DD702020800301406082A864886F70D030704088A4FCC9DCC394910":"70617373776f7264":"1B60098D4834CA752D37B430E70B7A085CFF86E21F4849F969DD1DF623342662443F8BD1252BF83CEF6917551B08EF55A69C8F2BFFC93BCB2DFE2E354DA28F896D1BD1BFB972A1251219A6EC7183B0A4CF2C4998449ED786CAE2138437289EB2203974000C38619DA57A4E685D29649284602BD1806131772DA11A682674DC22B2CF109128DDB7FDA3488A7144097565":0:"308187020100301306072A8648CE3D020106082A8648CE3D030107046D306B0201010420F12A1320760270A83CBFFD53F6031EF76A5D86C8A204F2C30CA9EBF51F0F0EA7A1440342000437CC56D976091E5A723EC7592DFF206EEE7CF9069174D0AD14B5F768225962924EE500D82311FFEA2FD2345D5D16BD8A88C26B770D55CD8A2A0EFA01C8B4EDFF060606060607"
+
+PBES2 Decrypt (Invalid padding & PKCS7 padding enabled)
+depends_on:MBEDTLS_MD_CAN_SHA1:MBEDTLS_DES_C:MBEDTLS_CIPHER_MODE_CBC:MBEDTLS_CIPHER_PADDING_PKCS7
+mbedtls_pkcs5_pbes2:MBEDTLS_ASN1_CONSTRUCTED | MBEDTLS_ASN1_SEQUENCE:"301B06092A864886F70D01050C300E04082ED7F24A1D516DD702020800301406082A864886F70D030704088A4FCC9DCC394910":"70617373776f7264":"1B60098D4834CA752D37B430E70B7A085CFF86E21F4849F969DD1DF623342662443F8BD1252BF83CEF6917551B08EF55A69C8F2BFFC93BCB2DFE2E354DA28F896D1BD1BFB972A1251219A6EC7183B0A4CF2C4998449ED786CAE2138437289EB2203974000C38619DA57A4E685D29649284602BD1806131772DA11A682674DC22B2CF109128DDB7FDA3488A7144097565":MBEDTLS_ERR_PKCS5_PASSWORD_MISMATCH:"308187020100301306072A8648CE3D020106082A8648CE3D030107046D306B0201010420F12A1320760270A83CBFFD53F6031EF76A5D86C8A204F2C30CA9EBF51F0F0EA7A1440342000437CC56D976091E5A723EC7592DFF206EEE7CF9069174D0AD14B5F768225962924EE500D82311FFEA2FD2345D5D16BD8A88C26B770D55CD8A2A0EFA01C8B4EDFF060606060607"
+
 PBES2 Decrypt (bad params tag)
 depends_on:MBEDTLS_SHA1_C:MBEDTLS_DES_C
 mbedtls_pkcs5_pbes2:MBEDTLS_ASN1_SEQUENCE:"":"":"":MBEDTLS_ERR_PKCS5_INVALID_FORMAT + MBEDTLS_ERR_ASN1_UNEXPECTED_TAG:""
diff --git a/tests/suites/test_suite_pkcs5.function b/tests/suites/test_suite_pkcs5.function
index 5d10da4..3ff000c 100644
--- a/tests/suites/test_suite_pkcs5.function
+++ b/tests/suites/test_suite_pkcs5.function
@@ -33,6 +33,36 @@
 /* END_CASE */
 
 /* BEGIN_CASE depends_on:MBEDTLS_ASN1_PARSE_C */
+void pbes2_encrypt(int params_tag, data_t *params_hex, data_t *pw,
+                   data_t *data, int ref_ret, data_t *ref_out)
+{
+    int my_ret;
+    mbedtls_asn1_buf params;
+    unsigned char *my_out = NULL;
+
+    MD_PSA_INIT();
+
+    params.tag = params_tag;
+    params.p = params_hex->x;
+    params.len = params_hex->len;
+
+    ASSERT_ALLOC(my_out, ref_out->len);
+
+    my_ret = mbedtls_pkcs5_pbes2(&params, MBEDTLS_PKCS5_ENCRYPT,
+                                 pw->x, pw->len, data->x, data->len, my_out);
+    TEST_EQUAL(my_ret, ref_ret);
+    if (ref_ret == 0) {
+        ASSERT_COMPARE(my_out, ref_out->len,
+                       ref_out->x, ref_out->len);
+    }
+
+exit:
+    mbedtls_free(my_out);
+    MD_PSA_DONE();
+}
+/* END_CASE */
+
+/* BEGIN_CASE depends_on:MBEDTLS_ASN1_PARSE_C */
 void mbedtls_pkcs5_pbes2(int params_tag, data_t *params_hex, data_t *pw,
                          data_t *data, int ref_ret, data_t *ref_out)
 {
@@ -44,14 +74,14 @@
     params.p = params_hex->x;
     params.len = params_hex->len;
 
-    my_out = mbedtls_test_zero_alloc(ref_out->len);
+    ASSERT_ALLOC(my_out, ref_out->len);
 
     my_ret = mbedtls_pkcs5_pbes2(&params, MBEDTLS_PKCS5_DECRYPT,
                                  pw->x, pw->len, data->x, data->len, my_out);
-    TEST_ASSERT(my_ret == ref_ret);
-
+    TEST_EQUAL(my_ret, ref_ret);
     if (ref_ret == 0) {
-        TEST_ASSERT(memcmp(my_out, ref_out->x, ref_out->len) == 0);
+        ASSERT_COMPARE(my_out, ref_out->len,
+                       ref_out->x, ref_out->len);
     }
 
 exit: