Bug 1720226 integrity checks in key4.db not happening on private components with AES_CBC

When we added support for AES, we also added support for integrity checks on the encrypted components.

It turns out the code that verifies the integrity checks was broken in 2 ways:

    1. it wasn't accurately operating when AES was being used (the if statement wasn't actually triggering for AES_CBC because we were looking for AES in the wrong field).
    2. password update did not update the integrity checks in the correct location, meaning any database which AES encrypted keys, and which had their password updated will not be able to validate their keys.

While we found this in a previous rebase, the patch had not been pushed upstream.


The attached patch needs sqlite3 to run the tests.

Differential Revision: https://phabricator.services.mozilla.com/D120011

5 files changed, 98 insertions, 13 deletions

diff --git a/automation/taskcluster/docker-builds/Dockerfile b/automation/taskcluster/docker-builds/Dockerfile
index 97436902c..82e829d87 100644
--- a/automation/taskcluster/docker-builds/Dockerfile
+++ b/automation/taskcluster/docker-builds/Dockerfile
@@ -35,6 +35,7 @@ RUN apt-get update \
     valgrind \
     zlib1g-dev \
     clang-format-3.9 \
+    sqlite3 \
  && rm -rf /var/lib/apt/lists/* \
  && apt-get autoremove -y && apt-get clean -y
 
diff --git a/automation/taskcluster/docker-gcc-4.4/Dockerfile b/automation/taskcluster/docker-gcc-4.4/Dockerfile
index 55344e567..866e8066c 100644
--- a/automation/taskcluster/docker-gcc-4.4/Dockerfile
+++ b/automation/taskcluster/docker-gcc-4.4/Dockerfile
@@ -11,6 +11,7 @@ RUN apt-get update \
     make \
     patch \
     mercurial \
+    sqlite3 \
     zlib1g-dev \
  && rm -rf /var/lib/apt/lists/* \
  && apt-get autoremove -y && apt-get clean -y
diff --git a/automation/taskcluster/docker/Dockerfile b/automation/taskcluster/docker/Dockerfile
index 6df17c5e1..859b5bd11 100644
--- a/automation/taskcluster/docker/Dockerfile
+++ b/automation/taskcluster/docker/Dockerfile
@@ -20,6 +20,7 @@ RUN apt-get update \
     mercurial \
     ninja-build \
     pkg-config \
+    sqlite3 \
     zlib1g-dev \
  && rm -rf /var/lib/apt/lists/* \
  && apt-get autoremove -y && apt-get clean -y
diff --git a/lib/softoken/sftkpwd.c b/lib/softoken/sftkpwd.c
index e0ff76f08..3bc4e57e1 100644
--- a/lib/softoken/sftkpwd.c
+++ b/lib/softoken/sftkpwd.c
@@ -287,9 +287,12 @@ sftkdb_DecryptAttribute(SFTKDBHandle *handle, SECItem *passKey,
     }
 
     /* If we are using aes 256, we need to check authentication as well.*/
-    if ((type != CKT_INVALID_TYPE) && (cipherValue.alg == SEC_OID_AES_256_CBC)) {
+    if ((type != CKT_INVALID_TYPE) &&
+        (cipherValue.alg == SEC_OID_PKCS5_PBES2) &&
+        (cipherValue.param->encAlg == SEC_OID_AES_256_CBC)) {
         SECItem signature;
         unsigned char signData[SDB_MAX_META_DATA_LEN];
+        CK_RV crv;
 
         /* if we get here from the old legacy db, there is clearly an
          * error, don't return the plaintext */
@@ -301,15 +304,28 @@ sftkdb_DecryptAttribute(SFTKDBHandle *handle, SECItem *passKey,
 
         signature.data = signData;
         signature.len = sizeof(signData);
-        rv = sftkdb_GetAttributeSignature(handle, handle, id, type,
-                                          &signature);
-        if (rv != SECSuccess) {
-            goto loser;
+        rv = SECFailure;
+        /* sign sftkdb_GetAttriibuteSignature returns a crv, not an rv */
+        crv = sftkdb_GetAttributeSignature(handle, handle, id, type,
+                                           &signature);
+        if (crv == CKR_OK) {
+            rv = sftkdb_VerifyAttribute(handle, passKey, CK_INVALID_HANDLE,
+                                        type, *plain, &signature);
         }
-        rv = sftkdb_VerifyAttribute(handle, passKey, CK_INVALID_HANDLE, type,
-                                    *plain, &signature);
         if (rv != SECSuccess) {
-            goto loser;
+            /*  handle bug 1720226 where old versions of NSS misfiled the signature
+             *  attribute on password update */
+            id |= SFTK_KEYDB_TYPE | SFTK_TOKEN_TYPE;
+            signature.len = sizeof(signData);
+            crv = sftkdb_GetAttributeSignature(handle, handle, id, type,
+                                               &signature);
+            if (crv != CKR_OK) {
+                rv = SECFailure;
+                PORT_SetError(SEC_ERROR_BAD_SIGNATURE);
+                goto loser;
+            }
+            rv = sftkdb_VerifyAttribute(handle, passKey, CK_INVALID_HANDLE,
+                                        type, *plain, &signature);
         }
     }
 
@@ -1198,6 +1214,7 @@ sftk_updateEncrypted(PLArenaPool *arena, SFTKDBHandle *keydb,
     unsigned int i;
     for (i = 0; i < privAttrCount; i++) {
         // Read the old attribute in the clear.
+        CK_OBJECT_HANDLE sdbId = id & SFTK_OBJ_ID_MASK;
         CK_ATTRIBUTE privAttr = { privAttrTypes[i], NULL, 0 };
         CK_RV crv = sftkdb_GetAttributeValue(keydb, id, &privAttr, 1);
         if (crv != CKR_OK) {
@@ -1222,7 +1239,7 @@ sftk_updateEncrypted(PLArenaPool *arena, SFTKDBHandle *keydb,
         plainText.data = privAttr.pValue;
         plainText.len = privAttr.ulValueLen;
         if (sftkdb_EncryptAttribute(arena, keydb, keydb->db, newKey,
-                                    iterationCount, id, privAttr.type,
+                                    iterationCount, sdbId, privAttr.type,
                                     &plainText, &result) != SECSuccess) {
             return CKR_GENERAL_ERROR;
         }
@@ -1232,10 +1249,9 @@ sftk_updateEncrypted(PLArenaPool *arena, SFTKDBHandle *keydb,
         PORT_Memset(plainText.data, 0, plainText.len);
 
         // Write the newly encrypted attributes out directly.
-        CK_OBJECT_HANDLE newId = id & SFTK_OBJ_ID_MASK;
         keydb->newKey = newKey;
         keydb->newDefaultIterationCount = iterationCount;
-        crv = (*keydb->db->sdb_SetAttributeValue)(keydb->db, newId, &privAttr, 1);
+        crv = (*keydb->db->sdb_SetAttributeValue)(keydb->db, sdbId, &privAttr, 1);
         keydb->newKey = NULL;
         if (crv != CKR_OK) {
             return crv;
diff --git a/tests/dbtests/dbtests.sh b/tests/dbtests/dbtests.sh
index 61a3888c6..00740d092 100755
--- a/tests/dbtests/dbtests.sh
+++ b/tests/dbtests/dbtests.sh
@@ -75,6 +75,7 @@ Echo()
     echo "| $*"
     echo "---------------------------------------------------------------"
 }
+
 dbtest_main()
 {
     cd ${HOSTDIR}
@@ -249,9 +250,9 @@ dbtest_main()
     ${BINDIR}/certutil -L -n bob -d ${CONFLICT_DIR}
     ret=$?
     if [ $ret -ne 0 ]; then
-      html_failed "Nicknane conflict test-setting nickname conflict incorrectly worked"
+      html_failed "Nickname conflict test-setting nickname conflict incorrectly worked"
     else
-      html_passed "Nicknane conflict test-setting nickname conflict was correctly rejected"
+      html_passed "Nickname conflict test-setting nickname conflict was correctly rejected"
     fi
     # import a token private key and make sure the corresponding public key is
     # created
@@ -262,6 +263,71 @@ dbtest_main()
     else
       html_passed "Importing Token Private Key correctly creates the corrresponding Public Key"
     fi
+    #
+    # If we are testing an sqlite db, make sure we detect corrupted attributes.
+    # This test only runs if 1) we have sqlite3 (the command line sqlite diagnostic
+    # tool) and 2) we are using the sql database (rather than the dbm).
+    #
+    which sqlite3
+    ret=$?
+    KEYDB=${CONFLICT_DIR}/key4.db
+    # make sure sql database is bing used.
+    if [ ! -f ${KEYDB} ]; then
+      Echo "skipping key corruption test: requires sql database"
+    # make sure sqlite3 is installed.
+    elif [ $ret -ne 0 ]; then
+      Echo "skipping key corruption test: sqlite3 command not installed"
+    else
+      # we are going to mangle this key database in multiple tests, save a copy
+      # so that we can restore it later.
+      cp ${KEYDB} ${KEYDB}.save
+      # dump the keys in the log for diagnostic purposes
+      ${BINDIR}/certutil -K -d ${CONFLICT_DIR} -f ${R_PWFILE}
+      # fetch the rsa and ec key ids
+      rsa_id=$(${BINDIR}/certutil -K -d ${CONFLICT_DIR} -f ${R_PWFILE} | grep rsa | awk '{ print $4}')
+      ec_id=$(${BINDIR}/certutil -K -d ${CONFLICT_DIR} -f ${R_PWFILE} | grep ' ec ' | awk '{ print $4}')
+      # now loop through all the private attributes and mangle them one at a time
+      for i in 120 122 123 124 125 126 127 128 011
+      do
+        Echo "testing if key corruption is detected in attribute $i"
+        cp ${KEYDB}.save ${KEYDB}  # restore the saved keydb
+        # find all the hmacs for this key attribute and mangle each entry
+        sqlite3 ${KEYDB} ".dump metadata" |  sed -e "/sig_key_.*_00000$i/{s/.*VALUES('\\(.*\\)',X'\\(.*\\)',NULL.*/\\1 \\2/;p;};d" | while read sig data
+        do
+          # mangle the last byte of the hmac
+          # The following increments the last nibble by 1 with both F and f
+          # mapping to 0. This mangles both upper and lower case results, so 
+          # it will work on the mac.
+          last=$((${#data}-1))
+          newbyte=$(echo "${data:${last}}" | tr A-Fa-f0-9 B-F0b-f0-9a)
+          mangle=${data::${last}}${newbyte}
+          echo "  amending ${sig} from ${data} to ${mangle}"
+          # write the mangled entry, inserting with a key matching an existing
+          # entry will overwrite the existing entry with the same key (${sig})
+          sqlite3 ${KEYDB} "BEGIN TRANSACTION; INSERT INTO metaData VALUES('${sig}',X'${mangle}',NULL); COMMIT"
+        done
+        # pick the key based on the attribute we are mangling,
+        # only CKA_VALUE (0x011) is not an RSA attribute, so we choose
+        # ec for 0x011 and rsa for all the rest. We could use the dsa
+        # key here, both CKA_VALUE attributes will be modifed in the loop above, but
+        # ec is more common than dsa these days.
+        if [ "$i" = "011" ]; then
+            key_id=$ec_id
+        else
+            key_id=$rsa_id
+        fi
+        # now try to use the mangled key (try to create a cert request with the key).
+        echo "${BINDIR}/certutil -R -k ${key_id} -s 'CN=BadTest, E=bad@mozilla.com, O=BOGUS NSS, L=Mountain View, ST=California, C=US' -d ${CONFLICT_DIR} -f ${R_PWFILE} -a"
+        ${BINDIR}/certutil -R -k ${key_id} -s 'CN=BadTest, E=bad@mozilla.com, O=BOGUS NSS, L=Mountain View, ST=California, C=US' -d ${CONFLICT_DIR} -f ${R_PWFILE} -a
+        ret=$?
+        if [ ${ret} -eq 0 ]; then
+          html_failed "Key attribute $i corruption not detected"
+        else
+          html_passed "Corrupted key attribute $i correctly disabled key"
+        fi
+      done
+      cp ${KEYDB}.save ${KEYDB}  # restore the saved keydb
+    fi
 
 
     if [ "${NSS_DEFAULT_DB_TYPE}" = "sql" ] ; then