diff options
author | Robert Relyea <rrelyea@redhat.com> | 2022-03-18 15:27:06 -0700 |
---|---|---|
committer | Robert Relyea <rrelyea@redhat.com> | 2022-03-18 15:27:06 -0700 |
commit | d4a3a6f7e7a163cae9b409851c80f5ee397094b8 (patch) | |
tree | d93f19dfc684d1fba5063c5b2bb11de0a10d5d33 | |
parent | 51a26d202145f53876a74fb60267f631bc0567ad (diff) | |
download | nss-hg-d4a3a6f7e7a163cae9b409851c80f5ee397094b8.tar.gz |
Bug 1757075 NSS does not properly import or export pkcs12 files with large passwords and pkcs5v2 encoding.
Don't use NULL when encoding UTF8 with pkcs5v2. Fix a bug here when converting from UCS2 to UTF8 we would add a double NULL when adding a NULL.
Differential Revision: https://phabricator.services.mozilla.com/D141538
-rw-r--r-- | cmd/pk12util/pk12util.c | 11 | ||||
-rw-r--r-- | lib/pkcs12/p12local.c | 60 | ||||
-rw-r--r-- | tests/common/init.sh | 3 | ||||
-rwxr-xr-x | tests/tools/tools.sh | 24 |
4 files changed, 78 insertions, 20 deletions
diff --git a/cmd/pk12util/pk12util.c b/cmd/pk12util/pk12util.c index b0c4ffd7b..a591309fd 100644 --- a/cmd/pk12util/pk12util.c +++ b/cmd/pk12util/pk12util.c @@ -665,6 +665,17 @@ P12U_ExportPKCS12Object(char *nn, char *outfile, PK11SlotInfo *inSlot, goto loser; } + /* we are passing UTF8, drop the NULL in the normal password value. + * UCS2 conversion will add it back if necessary. This only affects + * password > Blocksize of the Hash function and pkcs5v2 pbe (if password + * <=Blocksize then the password is zero padded anyway, so an extra NULL + * at the end has not effect). This is allows us to work with openssl and + * gnutls. Older versions of NSS already fail to decrypt long passwords + * in this case, so we aren't breaking anyone with this code */ + if ((pwitem->len > 0) && (!pwitem->data[pwitem->len - 1])) { + pwitem->len--; + } + p12cxt = p12u_InitContext(PR_FALSE, outfile); if (!p12cxt) { SECU_PrintError(progName, "Initialization failed: %s", outfile); diff --git a/lib/pkcs12/p12local.c b/lib/pkcs12/p12local.c index 900a0dbce..11fab5308 100644 --- a/lib/pkcs12/p12local.c +++ b/lib/pkcs12/p12local.c @@ -908,8 +908,12 @@ sec_pkcs12_find_object(SEC_PKCS12SafeContents *safe, return NULL; } -/* this function converts a password to unicode and encures that the - * required double 0 byte be placed at the end of the string +/* this function converts a password to UCS2 and ensures that the + * required double 0 byte be placed at the end of the string (if zeroTerm + * is set), or the 0 bytes at the end are dropped (if zeroTerm is not set). + * If toUnicode is false, we convert from UCS2 to UTF8/ASCII (latter is a + * proper subset of the former) depending on the state of the asciiCovert + * flag) */ PRBool sec_pkcs12_convert_item_to_unicode(PLArenaPool *arena, SECItem *dest, @@ -917,12 +921,15 @@ sec_pkcs12_convert_item_to_unicode(PLArenaPool *arena, SECItem *dest, PRBool asciiConvert, PRBool toUnicode) { PRBool success = PR_FALSE; + int bufferSize; + if (!src || !dest) { PORT_SetError(SEC_ERROR_INVALID_ARGS); return PR_FALSE; } - dest->len = src->len * 3 + 2; + bufferSize = src->len * 3 + 2; + dest->len = bufferSize; if (arena) { dest->data = (unsigned char *)PORT_ArenaZAlloc(arena, dest->len); } else { @@ -956,24 +963,36 @@ sec_pkcs12_convert_item_to_unicode(PLArenaPool *arena, SECItem *dest, return PR_FALSE; } - if ((dest->len >= 2) && - (dest->data[dest->len - 1] || dest->data[dest->len - 2]) && zeroTerm) { - if (dest->len + 2 > 3 * src->len) { - if (arena) { - dest->data = (unsigned char *)PORT_ArenaGrow(arena, - dest->data, dest->len, - dest->len + 2); - } else { - dest->data = (unsigned char *)PORT_Realloc(dest->data, - dest->len + 2); + /* in some cases we need to add NULL terminations and in others + * we need to drop null terminations */ + if (zeroTerm) { + /* unicode adds two nulls at the end */ + if (toUnicode) { + if ((dest->len >= 2) && + (dest->data[dest->len - 1] || dest->data[dest->len - 2])) { + /* we've already allocated space for these new NULLs */ + PORT_Assert(dest->len + 2 <= bufferSize); + dest->len += 2; + dest->data[dest->len - 1] = dest->data[dest->len - 2] = 0; } - - if (!dest->data) { - return PR_FALSE; + /* ascii/utf-8 adds just 1 */ + } else if ((dest->len >= 1) && dest->data[dest->len - 1]) { + PORT_Assert(dest->len + 1 <= bufferSize); + dest->len++; + dest->data[dest->len - 1] = 0; + } + } else { + /* handle the drop case, no need to do any allocations here. */ + if (toUnicode) { + while ((dest->len >= 2) && !dest->data[dest->len - 1] && + !dest->data[dest->len - 2]) { + dest->len -= 2; + } + } else { + while (dest->len && !dest->data[dest->len - 1]) { + dest->len--; } } - dest->len += 2; - dest->data[dest->len - 1] = dest->data[dest->len - 2] = 0; } return PR_TRUE; @@ -1011,7 +1030,8 @@ sec_pkcs12_is_pkcs12_pbe_algorithm(SECOidTag algorithm) * * we assume that the pwitem is already encoded in Unicode by the * caller. if the encryption scheme is not the one defined in PKCS - * #12, decode the pwitem back into UTF-8. */ + * #12, decode the pwitem back into UTF-8. NOTE: UTF-8 strings are + * used in the PRF without the trailing NULL */ PRBool sec_pkcs12_decode_password(PLArenaPool *arena, SECItem *result, @@ -1021,7 +1041,7 @@ sec_pkcs12_decode_password(PLArenaPool *arena, if (!sec_pkcs12_is_pkcs12_pbe_algorithm(algorithm)) return sec_pkcs12_convert_item_to_unicode(arena, result, (SECItem *)pwitem, - PR_TRUE, PR_FALSE, PR_FALSE); + PR_FALSE, PR_FALSE, PR_FALSE); return SECITEM_CopyItem(arena, result, pwitem) == SECSuccess; } diff --git a/tests/common/init.sh b/tests/common/init.sh index 6d74a6241..561c72544 100644 --- a/tests/common/init.sh +++ b/tests/common/init.sh @@ -83,6 +83,7 @@ if [ -z "${INIT_SOURCED}" -o "${INIT_SOURCED}" != "TRUE" ]; then GTESTDIR=${HOSTDIR}/gtests PWFILE=${HOSTDIR}/tests.pw + LONGPWFILE=${HOSTDIR}/tests.longpw EMPTY_FILE=${HOSTDIR}/tests_empty NOISE_FILE=${HOSTDIR}/tests_noise CORELIST_FILE=${HOSTDIR}/clist @@ -92,6 +93,7 @@ if [ -z "${INIT_SOURCED}" -o "${INIT_SOURCED}" != "TRUE" ]; then FIPSP12PWFILE=${HOSTDIR}/tests.fipsp12pw echo nss > ${PWFILE} + echo "nss123456789012345678901234567890123456789012345678901234567890_" > ${LONGPWFILE} echo > ${EMPTY_FILE} echo "fIps140" > ${FIPSPWFILE} echo "fips104" > ${FIPSBADPWFILE} @@ -661,6 +663,7 @@ if [ -z "${INIT_SOURCED}" -o "${INIT_SOURCED}" != "TRUE" ]; then fi R_PWFILE=../tests.pw + R_LONGPWFILE=../tests.longpw R_EMPTY_FILE=../tests_empty R_NOISE_FILE=../tests_noise diff --git a/tests/tools/tools.sh b/tests/tools/tools.sh index 19d8b1903..15a41e39c 100755 --- a/tests/tools/tools.sh +++ b/tests/tools/tools.sh @@ -387,6 +387,30 @@ tools_p12_export_list_import_with_default_ciphers() ret=$? html_msg $ret 0 "Listing Alice's pk12 EC file (pk12util -l)" check_tmpfile + + echo "$SCRIPTNAME: Exporting Alice's email EC cert & key with long pw------" + echo "pk12util -o Alice-ec-long.p12 -n \"Alice-ec\" -d ${P_R_ALICEDIR} -k ${R_PWFILE} \\" + echo " -w ${R_LONGPWFILE}" + ${BINDIR}/pk12util -o Alice-ec-long.p12 -n "Alice-ec" -d ${P_R_ALICEDIR} -k ${R_PWFILE} \ + -w ${R_LONGPWFILE} 2>&1 + ret=$? + html_msg $ret 0 "Exporting Alice's email EC cert & key with long pw (pk12util -o)" + check_tmpfile + verify_p12 Alice-ec-long.p12 "default" "default" "default" + + echo "$SCRIPTNAME: Importing Alice's email EC cert & key with long pw-----" + echo "pk12util -i Alice-ec-long.p12 -d ${P_R_COPYDIR} -k ${R_PWFILE} -w ${R_LONGPWFILE}" + ${BINDIR}/pk12util -i Alice-ec-long.p12 -d ${P_R_COPYDIR} -k ${R_PWFILE} -w ${R_LONGPWFILE} 2>&1 + ret=$? + html_msg $ret 0 "Importing Alice's email EC cert & key with long pw (pk12util -i)" + check_tmpfile + + echo "$SCRIPTNAME: Listing Alice's pk12 EC file with long pw ------------" + echo "pk12util -l Alice-ec-long.p12 -w ${R_LONGPWFILE}" + ${BINDIR}/pk12util -l Alice-ec-long.p12 -w ${R_LONGPWFILE} 2>&1 + ret=$? + html_msg $ret 0 "Listing Alice's pk12 EC file with long pw (pk12util -l)" + check_tmpfile } tools_p12_import_old_files() |