1379154 - After migrating an uninitialized database (no password set) from DBM to SQL, by deleting a cert, NSS continues to read the contents of the DBM database, even when asked to access using the SQL method

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Kai Engert (:KaiE:)]

NSS appears to read from a cert8.db file, even when cert9.db files are already present, and NSS has been told to use sql: mode.

I'm using the attached database as a starting point.
It's in DBM format, and contains 3 certificates,
one of them with nickname "test".

$ certutil -d dbm:. -L
GeoTrust Primary Certification Authority - G2                ,,   
Let's Encrypt Authority X1                                   ,,   
test                                                         ,,   

I'm executing a delete operation, with path "sql:", which I'd expect to migrate to sql file format, and also delete the test cert.

certutil -d sql:. -D -n test

After this command, I expect the "test" certificate to be gone, but it isn't apparently.

$ certutil -d sql:. -L
GeoTrust Primary Certification Authority - G2                ,,   
Let's Encrypt Authority X1                                   ,,   
test                                                         ,,   

I eventually noticed that the file is indeed gone from cert9.db, the problem is that NSS still reads cert8.db!

Why?

IMHO, after the conversion, if SQL files are present, and NSS is asked to use SQL, then NSS should completely ignore any leftover DBM files.

I believe this is a precondition to changing the default NSS file format to SQL, because applications that get auto conversion to SQL shouldn't get unexpected side effects from leftover DBM files.

Comment 1

[Kai Engert (:KaiE:)]

Attachment: db.zip

Comment 2

[Kai Engert (:KaiE:)]

Bob, yesterday you explained to me why we get this effect.

You said, it's because no database password had yet been provided, NSS wasn't able yet to fully convert the key database. This "incomplete conversion" state is memorized in the database file. The database will get fully converted after having provided the database password.

But I cannot confirm your theory yet.

First, the database password is "empty". It seems very unusual having to provide a database password, when it's at the default.

Second, I did what you said anyway, but I still see the bug. Because the database password is empty, I created an "empty" database file, and gave it to certutil with parameter -f.
Nevertheless, the "convert triggered by delete" still produces the same state, where "certutil -d sql: -L" still lists the deleted certificate.

(I tried both an completely empty password file, and a password file containing just a newline.)

Comment 3

[Kai Engert (:KaiE:)]

Bob, another problem/inconsistency.

You said, it's now required to authenticate.

But apparently, when the database password is "empty", it's not even POSSIBLE to authenticate!

I tried to change certutil, to always call PK11_Authenticate() after NSS init.

But that fails. PK11_Authenticate() calls PK11_NeedUserInit(), which returns true, and therefore PK11_Authenticate ends with an error.

How can the SQL file format be used with an "empty" password?

It seems it cannot be used in equivalent simply ways as with the past DBM file format.

Comment 4

[Kai Engert (:KaiE:)]

I have a new theory on how things are working.

Apparently "with SQL file format it's necessary to authenticate" is an incomplete explanation.

Apparently SQL format now requires to call "PK11_InitPin" once, prior making modifications. (This wasn't necessary with DBM.)

I have an experimental patch for certutil, which adds the following function, and calls it prior to any modification:

static void
prepare_for_modification(PK11SlotInfo *slot)
{
    if (!PK11_NeedLogin(slot))
        return;

    if (PK11_NeedUserInit(slot)) {
        PK11_InitPin(slot, (char *)NULL, "");
    }
    PK11_Authenticate(slot, PR_TRUE, NULL);
}

If this is the correct approach, then any application making modifications to the NSS cert database must add similar code, to ensure the application will conitnue to function with SQL format.

Comment 5

[Kai Engert (:KaiE:)]

Attachment: 1379154-v1.patch

Comment 6

[Kai Engert (:KaiE:)]

It seems this code works with DBM databases, too.

If the code makes sense, we should check it in first, prior to changing the default to SQL.

Comment 7

[Kai Engert (:KaiE:)]

Attachment: 1379154-v2.patch

I had to remove the call to PK11_Authenticate from prepare_for_modification, it's not necessary, and its presence breaks scenarios with a proper password.

This patch works with the test case, and also passes the NSS test suite.
https://treeherder.mozilla.org/#/jobs?repo=nss-try&revision=5ca2a4c5a886b438ab8ef18fd9fc844bd605b934

Comment 8

[Robert Relyea]

Oh, so the problem is we have an initialized NSS key database. I'm still poking around on that theory. If that's the case, we should be able to get conversion code to work correctly.

Comment 9

[Robert Relyea]

OK, I have a little history for you related to this, and then a possible solution.

NSS has the notion of an 'uninitialized token'. An 'uninitialized token' is one in which the key database had not yet had a password record added. For most applications, the first thing they do is initialize the key database so they they can store keys in it. Certutil always creates initialized databased when it creates new databases. Firefox always creates uninitialized databases when it creates new databases.

The reason for this is firefox often stores certs, but doesn't often store keys. Firefox wants to prompt the user for a new password when it creates a key the first time, but not everytime it creates a new key. The 'uninitialized state' is how firefox knows that it hasn't ever asked the user to create a new password. Sometimes (often), the user chooses to create a database with no password. That database is now initialized and it's password is set to the empty string. NSS treats that as a 'no password' database. keys are still encrypted, but the encryption key is a PBE generated by the well known string "".

OK, so that's uninitialized passwords. Now let's look at the dbm->sql update case. If the dbm database has a "" string password, the update can complete at init time and you will have a fully up to date database. If the dbm database has a non-null password set, then we update the sql cert database, but the key database is left without a metadata table (where the password entry would normally live) until the user supplies a password to update it. If the user never supplies the password and exits the program, the next application needs to know that the key database needs to be updated. The underlying sqlite database code does this by telling NSS that it's metadata-less keydb is a 'newInit', and therefore needs to participate in the update process. When the password is finally supplied the update is then complete.

Now we try to update an uninitialized key database. In that case, we know there isn't any password, so we can 'complete' the update. The issue is since the source database is uninitialized, we don't create the metadata table because we don't have a password entry (definition of uninitialized). Now when we start up again, NSS thinks that it's dealing with a partially updated key database rather than a fully updated uninitialized key database.

there's a simple patch to fix that particular issue. It puts a dummy metatable entry if it's updating a key database from an uninitialized key database:

--- a/lib/softoken/sftkdb.c
+++ b/lib/softoken/sftkdb.c
@@ -2306,16 +2306,21 @@ loser:
         if (crv == CKR_OK) {
             goto done;
         }

         /* nope, update it from the source */
         crv = (*handle->update->sdb_GetMetaData)(handle->update, "password",
                                                  &item1, &item2);
         if (crv != CKR_OK) {
+        /* if we get here, neither the source, nor the target has been initialized
+             * with a password entry. Create a metadata table now so that we don't
+             * mistake this for a partially updated database */
+        item1.data[0] = 0; item2.data[0] = 0; item1.len = item2.len =1;
+        crv = (*handle->update->sdb_PutMetaData(handle->update, "empty", &item1, &item2);
             goto done;
         }
         crv = (*handle->db->sdb_PutMetaData)(handle->db, "password", &item1,
                                              &item2);
         if (crv != CKR_OK) {
             goto done;
         }
     }


The patch needs testing and white space cleanup.

There are still some questions we need to answer like: 1) if we have partially updated, we should mark the cert database as updated and not use the old dbm.

bob

Comment 10

[Kai Engert (:KaiE:)]

Bob said, by attached patch v2 might not be necessary. The fix from Bob's comment might be enough. I'll also have to check if this change is sufficient for Firefox.

Comment 11

[Kai Engert (:KaiE:)]

(In reply to Robert Relyea from comment #9)
> there's a simple patch to fix that particular issue. It puts a dummy
> metatable entry if it's updating a key database from an uninitialized key
> database:

Bob, for the case described in this bug, your patch from comment 9 doesn't work, because the code isn't reached.

We arrive at line 2293, and "handle->type" is zero, so the following test fails, 
    if (handle->type == SFTK_KEYDB_TYPE) {
and the entire block is skipped.

https://hg.mozilla.org/projects/nss/file/f212be04f3d0/lib/softoken/sftkdb.c#l2292

Comment 12

[Kai Engert (:KaiE:)]

(In reply to Kai Engert (:kaie:) from comment #11)
> 
> Bob, for the case described in this bug, your patch from comment 9 doesn't
> work, because the code isn't reached.

Sorry, the code doesn't work, but I gave an incorrect failure reasons.

When this function is reached the second time (for handling the key database), your new code is reached, but we fail inside lg_PutMetaData (softoken/legacydb/keydb.c):

    if (PORT_Strcmp(id, "password") != 0) {
        /* shouldn't happen */
        return CKR_GENERAL_ERROR; /* no extra data stored */
    }

https://hg.mozilla.org/projects/nss/file/f212be04f3d0/lib/softoken/legacydb/keydb.c#l2222

I assume you want that to be changed to
    if (PORT_Strcmp(id, "password") != 0 && PORT_Strcmp(id, "empty") != 0) {

This still doesn't work, because inside lg_PutMetaData we reach nsslowkey_PutPWCheckEntry, which then fails.

Comment 13

[Kai Engert (:KaiE:)]

I think if we want to make progress on this bug, we'd need a tested patch from Bob.

Comment 14

[Kai Engert (:KaiE:)]

I still believe this should be a blocker for changing the NSS default to SQL.

Apparently it's a fully supported scenario, that an application may store only certificates, and never calls PK11_InitPin (database wasn't created by certutil).

With this bug present, the certificate management of such an application would malfunction after an automatic conversion to SQL, with the old DBM files still being present in the same directory.

Comment 15

[Kai Engert (:KaiE:)]

(In reply to Robert Relyea from comment #9)
> NSS has the notion of an 'uninitialized token'. An 'uninitialized token' is
> one in which the key database had not yet had a password record added. For
> most applications, the first thing they do is initialize the key database so
> they they can store keys in it. Certutil always creates initialized
> databased when it creates new databases. Firefox always creates
> uninitialized databases when it creates new databases.
> 
> The reason for this is firefox often stores certs, but doesn't often store
> keys. Firefox wants to prompt the user for a new password when it creates a
> key the first time, but not everytime it creates a new key. The
> 'uninitialized state' is how firefox knows that it hasn't ever asked the
> user to create a new password. Sometimes (often), the user chooses to create
> a database with no password.

Just for the record, and I mentioned this already to Bob in a call:

The above description of Firefox behavior is no longer true. (It might have been correct in much older versions.)

Today's versions of Firefox never prompt the user suggesting to set a master password. (If the user wants a master password, the user must take the initiative and go to the preferences to set it.)

Comment 16

[Kai Engert (:KaiE:)]

(In reply to Robert Relyea from comment #9)
> 
>          /* nope, update it from the source */
>          crv = (*handle->update->sdb_GetMetaData)(handle->update, "password",
>                                                   &item1, &item2);
>          if (crv != CKR_OK) {
> +        /* if we get here, neither the source, nor the target has been
> initialized
> +             * with a password entry. Create a metadata table now so that
> we don't
> +             * mistake this for a partially updated database */
> +        item1.data[0] = 0; item2.data[0] = 0; item1.len = item2.len =1;
> +        crv = (*handle->update->sdb_PutMetaData(handle->update, "empty",
> &item1, &item2);
>              goto done;
>          }
>          crv = (*handle->db->sdb_PutMetaData)(handle->db, "password", &item1,
>                                               &item2);
> 
> The patch needs testing and white space cleanup.

Just for the record, if you want to continue to explore this approach, here is the fixed version of the above snippet which compiles (but doesn't work):

+            /* if we get here, neither the source, nor the target has been initialized
+             * with a password entry. Create a metadata table now so that we don't
+             * mistake this for a partially updated database */
+            item1.data[0] = 0;
+            item2.data[0] = 0;
+            item1.len = item2.len = 1;
+            crv = (*handle->update->sdb_PutMetaData)(handle->update, "empty", &item1, &item2);

Comment 17

[Robert Relyea]

> When this function is reached the second time (for handling the key database), your new code is reached, but we fail inside 
> lg_PutMetaData (softoken/legacydb/keydb.c):

Hmm this is the legacy put, function. Why are getting here? This is drying to update the dbm database. I wonder if I have the wrong handles in the above code.

Ah I do have the wrong handles:
 
crv = (*handle->update->sdb_PutMetaData)(handle->update, "empty", &item1, &item2);

It should be:

crv = (*handle->db->sdb_PutMetaData)(handle->db, "empty", &item1, &item2);

We may wanto to add the short circuit in the legacay lg_PutMetaData() to ignore the empty label (just return if we try to write an empy label).

bob

Comment 18

[Kai Engert (:KaiE:)]

Attachment: 1379154-v3.patch (from Bob)

This is the patch that Bob has provided in comment 9 plus comment 16.

A first test seems to confirm it fixes the issue.+
I've started an NSS try build to test for regressions.
https://treeherder.mozilla.org/#/jobs?repo=nss-try&revision=ecc1a8c76fe34613a63a560f75aa9a45df3643df

Comment 19

[Kai Engert (:KaiE:)]

Comment on attachment 8904569 [details] [diff] [review]
1379154-v3.patch (from Bob)

Regression tests look good. r=kaie

Comment 20

[Kai Engert (:KaiE:)]

https://hg.mozilla.org/projects/nss/rev/ab0ed74adde1