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

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()