Fixes for pkcs12 with NULL and/or zero length password #5155

paul-elliott-arm · 2021-11-11T19:28:02Z

Description

Previously passing a NULL or zero length password into either mbedtls_pkcs12_pbe() or mbedtls_pkcs12_derive() could cause an infinate loop, and it was also possible to pass a NULL password, with a non-zero length, which would cause memory corruption. Unfortunately the documentation said that either was acceptable.

I have fixed these issues, and improved the documentation to reflect the changes and further explain what is expected of the inputs.

This closes #5136 and #4928.

Status

READY

Requires Backporting

Yes
2.x (#5310)
2.16 (#5311)
(issue exists in all branches)

Migrations

NO

Todos

Tests
Changelog updated
Backported

Steps to test or reproduce

Call either mbedtls_pkcs12_pbe() or mbedtls_pkcs12_derive() with a null password, the library should now handle this.

Previously passing a NULL or zero length password into either mbedtls_pkcs12_pbe() or mbedtls_pkcs12_derive() could cause an infinate loop, and it was also possible to pass a NULL password, with a non-zero length, which would cause memory corruption. I have fixed these errors, and improved the documentation to reflect the changes and further explain what is expected of the inputs. Signed-off-by: Paul Elliott <paul.elliott@arm.com>

include/mbedtls/pkcs12.h

library/pkcs12.c

gilles-peskine-arm · 2021-11-15T22:35:47Z

include/mbedtls/pkcs12.h

- * \param pwd        password to use (may be NULL if no password is used)
+ * \param datalen    length of buffer to fill
+ * \param pwd        Null terminated BMPString password to use (may be NULL if
+ *                   no password is used, but not if pwdlen is non-zero)
 * \param pwdlen     length of the password (may be 0)


Actually, looking at our implementation, I don't think this function is correct when pwdlen==0 or saltlen==0. This is unrelated to pointer nullability. Our code does:

if( hlen <= 32 ) v = 64; else v = 128; … pkcs12_fill_buffer( salt_block, v, salt, saltlen ); pkcs12_fill_buffer( pwd_block, v, pwd, pwdlen ); … 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;

But PKCS#12 §B.2 steps 2 and 3 specify that if the salt or password is empty, what gets hashed is an empty string, not v bytes.

mbedtls_pkcs12_derivation already errors out when pwdlen > 64 || saltlen > 64, both cases where the calculation quoted above would be incorrect because it would need to hash multiple blocks of v bytes. (64 could be raised to 128 when the hash is one with 128-byte blocks.) We could simply make it error out when pwdlen == 0 || saltlen == 0 for the same reason.

Per RFC 128, pwdlen is at least 2 bytes (a UCS2 null byte). saltlen is an arbitrary byte string, and could in theory be empty. A salt is necessary for security when the password is actually a password, but not when using an empty password (pwdlen==2).

Erroring out would break backward compability, but it would eliminate a non-compliance with the standard. So I'm not sure what to do. At least in LTS branches, we should maintain backward compatibility, but document what's going on. In non-LTS branches, I would prefer to either error out or fix the functional bug. Fixing the invalid pointer dereference is urgent, fixing the functional bug isn't, so we don't need to fix the functional bug now, but if we decide not to make it an error, we should at least document the current behavior, either as something we guarantee or something we may fix in the future.

Ah, ok. I took this documentation to mean that the password and salt buffers still had to be v bytes long, but if the password or salt was empty, then so would the buffer be (hence the memset to zero)

Hang on, though. How are we supposed to do step 6C from the documentation above if the password and salt blocks are not of length v

Step 6 doesn't use the password and salt directly, only I. I is always a concatenation of v-bit blocks. The number of blocks can be 0; this only happens if both the salt and the password are empty. If the number of blocks is 0, step 6C doesn't actually modify I. This might hurt for the security analysis of the algorithm, but that's not really a concern when both the administrator and the user decided to have no effective security (0-length salt and empty password).

ChangeLog.d/fix-pkcs12-null-password.txt

include/mbedtls/pkcs12.h

ChangeLog.d/fix-pkcs12-null-password.txt

Signed-off-by: Paul Elliott <paul.elliott@arm.com>

Remove surplus _test suffix. Change labeling from Pcks12 to PCKS#12 as it should be. Signed-off-by: Paul Elliott <paul.elliott@arm.com>

Also ensure they are used in test data rather than values Signed-off-by: Paul Elliott <paul.elliott@arm.com>

Signed-off-by: Paul Elliott <paul.elliott@arm.com>

Incorrect interpretation of 'empty' Signed-off-by: Paul Elliott <paul.elliott@arm.com>

gilles-peskine-arm

Please amend the commit message of "Add expected output for tests":

To accurately reflect the fact that for (pwd="", salt=""), the output is what our code generated, not from openssl implementation.
To include the C code you used to generate the test vectors with openssl. (Just paste the C code into the commit message, it's simple enough that it doesn't need a makefile or anything.)

I'm not completely sure what the intended output is when the password and the salt are both empty according to RFC 7292. Our code doesn't have a special case and we're validating that it doesn't do anything bad, and that's good enough for me. We could reject this special case on the basis that it's invalid, but I'm not sure it's supposed to be invalid, although OpenSSL seems to think it is.

My remark about the use variables is not a blocker.

gilles-peskine-arm · 2021-12-10T15:34:36Z

library/pkcs12.c

+    use_password = ( pwd && pwdlen != 0 );
+    use_salt = ( salt && saltlen != 0 );


Specification point of view: the conditions that use use_password below are actually “iterate n times where n is the number of password blocks rounded up” (first if) and “if the password is between 1 and block_size bytes long” (second if). Since passwords longer than block_size bytes are rejected, the condition simplifies to pwdlen != 0.

Implementation point of view: if pwdlen != 0 then pwd can't be null at this point, so use_password could be simplified to pwdlen != 0.

So the indirection via use_password isn't really useful, we could just write if( pwdlen != 0 ) below to match the specification more closely.

Same for the salt.

agreed, we either have pwdlen == 0 (and password not to be used) or pwdlen != 0 && pwd != NULL. So this is equivalent to use_password = (pwdlen != 0) and if( use_password != 0 ) is equivalent to if( pwd_len != 0 ).

same for the salt.

if it's worth changing this, there are a couple of minor style changes I've mentioned. however, if it's not worth changing, the whole thing is fine

Expected output generated by OpenSSL (see below) apart from the case where both password and salt are either NULL or zero length, as OpenSSL does not support this. For these test cases we have had to use our own output as that which is expected. Code to generate test cases is as follows: #include <openssl/pkcs12.h> #include <openssl/evp.h> #include <string.h> int Keygen_Uni( const char * test_name, unsigned char *pass, int passlen, unsigned char *salt, int saltlen, int id, int iter, int n, unsigned char *out, const EVP_MD *md_type ) { size_t index; printf( "%s\n", test_name ); int ret = PKCS12_key_gen_uni( pass, passlen, salt, saltlen, id, iter, n, out, md_type ); if( ret != 1 ) { printf( "Key generation returned %d\n", ret ); } else { for( index = 0; index < n; ++index ) { printf( "%02x", out[index] ); } printf( "\n" ); } printf( "\n" ); } int main(void) { unsigned char out_buf[48]; unsigned char pass[64]; int pass_len; unsigned char salt[64]; int salt_len; /* If ID=1, then the pseudorandom bits being produced are to be used as key material for performing encryption or decryption. If ID=2, then the pseudorandom bits being produced are to be used as an IV (Initial Value) for encryption or decryption. If ID=3, then the pseudorandom bits being produced are to be used as an integrity key for MACing. */ int id = 1; int iter = 3; memset( out_buf, 0, sizeof( out_buf ) ); memset( pass, 0, sizeof( pass ) ); memset( salt, 0, sizeof( salt ) ); Keygen_Uni( "Zero length pass and salt", pass, 0, salt, 0, id, iter, sizeof(out_buf), out_buf, EVP_md5( ) ); memset( out_buf, 0, sizeof( out_buf ) ); Keygen_Uni( "NULL pass and salt", NULL, 0, NULL, 0, id, iter, sizeof(out_buf), out_buf, EVP_md5( ) ); memset( out_buf, 0, sizeof( out_buf ) ); salt[0] = 0x01; salt[1] = 0x23; salt[2] = 0x45; salt[3] = 0x67; salt[4] = 0x89; salt[5] = 0xab; salt[6] = 0xcd; salt[7] = 0xef; Keygen_Uni( "Zero length pass", pass, 0, salt, 8, id, iter, sizeof(out_buf), out_buf, EVP_md5( ) ); memset( out_buf, 0, sizeof( out_buf ) ); Keygen_Uni( "NULL pass", NULL, 0, salt, 8, id, iter, sizeof(out_buf), out_buf, EVP_md5( ) ); memset( out_buf, 0, sizeof( out_buf ) ); memset( salt, 0, sizeof( salt ) ); pass[0] = 0x01; pass[1] = 0x23; pass[2] = 0x45; pass[3] = 0x67; pass[4] = 0x89; pass[5] = 0xab; pass[6] = 0xcd; pass[7] = 0xef; Keygen_Uni( "Zero length salt", pass, 8, salt, 0, id, iter, sizeof(out_buf), out_buf, EVP_md5( ) ); memset( out_buf, 0, sizeof( out_buf ) ); Keygen_Uni( "NULL salt", pass, 8, NULL, 0, id, iter, sizeof(out_buf), out_buf, EVP_md5( ) ); memset( out_buf, 0, sizeof( out_buf ) ); salt[0] = 0x01; salt[1] = 0x23; salt[2] = 0x45; salt[3] = 0x67; salt[4] = 0x89; salt[5] = 0xab; salt[6] = 0xcd; salt[7] = 0xef; Keygen_Uni( "Valid pass and salt", pass, 8, salt, 8, id, iter, sizeof(out_buf), out_buf, EVP_md5( ) ); return 0; } Signed-off-by: Paul Elliott <paul.elliott@arm.com>

tom-cosgrove-arm · 2021-12-13T13:34:18Z

library/pkcs12.c

-    pkcs12_fill_buffer( pwd_block,  v, pwd,  pwdlen  );
+    if( use_salt != 0 )
+    {
+        pkcs12_fill_buffer( salt_block, v, salt, saltlen );


note: these two single-statement blocks within ifs use braces, unlike the surrounding code. however, probably not worth while changing at this point

tom-cosgrove-arm · 2021-12-13T13:40:56Z

tests/suites/test_suite_pkcs12.function

+                        data_t* expected_output, int expected_status )
+
+{
+   int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED;


note: this appears to be indented by 3 spaces, not the usual 4. again, probably not worthwhile going through another review cycle to fix

tom-cosgrove-arm

LGTM

borrrden · 2022-11-28T22:49:29Z

The CVE for this issue still lists affected versions as "all versions before 3.1.0" but this ticket indicated they were backported. Does 2.28.1 have the fix for this?

gilles-peskine-arm · 2022-11-29T09:00:04Z

@borrrden Yes, this was backported to 2.28.1 (and 2.16.12).

paul-elliott-arm self-assigned this Nov 11, 2021

paul-elliott-arm requested a review from gilles-peskine-arm November 11, 2021 19:34

gilles-peskine-arm requested changes Nov 15, 2021

View reviewed changes

gilles-peskine-arm added needs-work component-crypto Crypto primitives and low-level interfaces and removed needs-review Every commit must be reviewed by at least two team members, needs: changelog needs-reviewer This PR needs someone to pick it up for review component-x509 labels Nov 15, 2021

Make changelog more specific

0b7d5a8

Signed-off-by: Paul Elliott <paul.elliott@arm.com>

paul-elliott-arm added 6 commits November 30, 2021 16:21

Remove incorrect test dependency

3584ae4

Signed-off-by: Paul Elliott <paul.elliott@arm.com>

Rename (and relabel) pkcs12 test case

df00695

Remove surplus _test suffix. Change labeling from Pcks12 to PCKS#12 as it should be. Signed-off-by: Paul Elliott <paul.elliott@arm.com>

Simplify Input usage macros

4768a30

Also ensure they are used in test data rather than values Signed-off-by: Paul Elliott <paul.elliott@arm.com>

Delete unneccesary changelog entry

117282f

Signed-off-by: Paul Elliott <paul.elliott@arm.com>

Add explanation for safety in function

7298bef

Signed-off-by: Paul Elliott <paul.elliott@arm.com>

Remove incorrect hashing

2fd6b61

Incorrect interpretation of 'empty' Signed-off-by: Paul Elliott <paul.elliott@arm.com>

paul-elliott-arm requested a review from gilles-peskine-arm December 8, 2021 14:52

gilles-peskine-arm requested changes Dec 10, 2021

View reviewed changes

paul-elliott-arm force-pushed the pcks12_fix branch from c22580d to 7fa7383 Compare December 10, 2021 17:46

paul-elliott-arm mentioned this pull request Dec 10, 2021

Backport 2.x: Fixes for pkcs12 with NULL and/or zero length password #5310

Merged

3 tasks

paul-elliott-arm removed the needs-work label Dec 10, 2021

paul-elliott-arm added size-m Estimated task size: medium (~1w) and removed size-s Estimated task size: small (~2d) labels Dec 10, 2021

paul-elliott-arm force-pushed the pcks12_fix branch from 7fa7383 to 6e7deb1 Compare December 10, 2021 20:54

gilles-peskine-arm approved these changes Dec 10, 2021

View reviewed changes

gilles-peskine-arm added needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review labels Dec 10, 2021

tom-cosgrove-arm reviewed Dec 13, 2021

View reviewed changes

tom-cosgrove-arm approved these changes Dec 13, 2021

View reviewed changes

gilles-peskine-arm merged commit a5c1851 into Mbed-TLS:development Dec 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes for pkcs12 with NULL and/or zero length password #5155

Fixes for pkcs12 with NULL and/or zero length password #5155

paul-elliott-arm commented Nov 11, 2021 •

edited

Loading

gilles-peskine-arm Nov 15, 2021

paul-elliott-arm Nov 18, 2021 •

edited

Loading

paul-elliott-arm Nov 18, 2021

gilles-peskine-arm Nov 18, 2021

gilles-peskine-arm left a comment •

edited

Loading

gilles-peskine-arm Dec 10, 2021

tom-cosgrove-arm Dec 13, 2021

tom-cosgrove-arm Dec 13, 2021

tom-cosgrove-arm Dec 13, 2021

tom-cosgrove-arm left a comment

borrrden commented Nov 28, 2022

gilles-peskine-arm commented Nov 29, 2022

		use_password = ( pwd && pwdlen != 0 );
		use_salt = ( salt && saltlen != 0 );

Fixes for pkcs12 with NULL and/or zero length password #5155

Fixes for pkcs12 with NULL and/or zero length password #5155

Conversation

paul-elliott-arm commented Nov 11, 2021 • edited Loading

Description

Status

Requires Backporting

Migrations

Todos

Steps to test or reproduce

gilles-peskine-arm Nov 15, 2021

Choose a reason for hiding this comment

paul-elliott-arm Nov 18, 2021 • edited Loading

Choose a reason for hiding this comment

paul-elliott-arm Nov 18, 2021

Choose a reason for hiding this comment

gilles-peskine-arm Nov 18, 2021

Choose a reason for hiding this comment

gilles-peskine-arm left a comment • edited Loading

Choose a reason for hiding this comment

gilles-peskine-arm Dec 10, 2021

Choose a reason for hiding this comment

tom-cosgrove-arm Dec 13, 2021

Choose a reason for hiding this comment

tom-cosgrove-arm Dec 13, 2021

Choose a reason for hiding this comment

tom-cosgrove-arm Dec 13, 2021

Choose a reason for hiding this comment

tom-cosgrove-arm left a comment

Choose a reason for hiding this comment

borrrden commented Nov 28, 2022

gilles-peskine-arm commented Nov 29, 2022

paul-elliott-arm commented Nov 11, 2021 •

edited

Loading

paul-elliott-arm Nov 18, 2021 •

edited

Loading

gilles-peskine-arm left a comment •

edited

Loading