Closed Bug 465709 Opened 12 years ago Closed 12 years ago

NSS_InitWithMerge creates a broken database if it didn't exist before

Categories

(NSS :: Libraries, defect)

3.12.1
x86
Linux
defect
Not set
major

Tracking

(Not tracked)

RESOLVED FIXED
3.12.3

People

(Reporter: wolfiR, Assigned: rrelyea)

Details

Attachments

(4 files, 2 obsolete files)

Attached file testcase
NSS_InitWithMerge creates a broken database if it didn't exist before.

It makes no difference if there is a cert8.db or not before NSS_InitWithMerge() is called. The resulting database seems to be broken differently though.
In both cases Firefox can't use the new cert9.db.

The attached testcase is derived from https://wiki.mozilla.org/NSS_Shared_DB
I've only tested databases w/o a password so far FWIW.
I also can confirm that the testcase merges correctly if a good cert9.db already existed.
In dev-tech-crypto, Wolfgang Rosenauer wrote:

> I'm saying that if the database fails to migrate the first time,
> and leaves an empty database, then subsequent merges to that database
> also fail. However, if the first merge is successful (creating the
> database and populating it), then subsequent merges always work.

Thanks, Wolfgang.  I think that additional info is helpful.  
I'd like to understand a little more about the first failure that leaves
an empty cert9.db.  There is some logic that attempts to detect when a 
cert8 DB has already been merged into a cert9.db and avoid doing it again.  
I wonder if that is part of the issue here.
Assignee: nobody → rrelyea
In the single cert8/key3 update to cert9/key4 (as opposed to a merge update where you are merging from cert8/key3 to a cert9/key4 in a shared location) case, which is what we are talking about here, there is a 2 step update. The second step requires a password. The code is supposed to know if the update has only half completed (I had some problem with early version of the shared DB code), where the databases are created, but the tables have not been. When it comes time to do the actual migrate, the tables are then created. If this is the case that's failing, we can simulate it with certutil as follows:

Start it a dbm database with a password.

certutil -L -X -d sql:.

This command does not require a password, so the databases will only partially update (databases are created, but not populated).

certutil -K -x -d sql:.
<supply the password>

This should successfully update the database.
I've just tried this and it failed. So this looks like a good test to reproduce the problem. I'll attach it to the bug.

bob
Summary: NSS_InitWithMerge creates a broken database if it didn't exist before → NSS_Init creates a broken database if it didn't exist before
I've updated the comments to be more precise. In all the cases we are talking about, NSS_InitWithMerge is not yet active. Kai is still working on a mozilla patch with NSS_InitWithMerge, and certutil uses InitWithMerge in the certutil --upgrade-merge.
Sample test...

bobs-laptop(127) ls -l
total 128
-rw-r----- 1 rrelyea rrelyea   602 Nov  3 16:02 Alice.cert
-rw------- 1 rrelyea rrelyea 65536 Nov 20 16:51 cert8.db
-rw------- 1 rrelyea rrelyea 16384 Nov 20 16:51 key3.db
-rw-r----- 1 rrelyea rrelyea   450 Nov  3 16:02 req
-rw------- 1 rrelyea rrelyea 16384 Nov  3 16:02 secmod.db
bobs-laptop(128) certutil -L -d dbm:.

Certificate Nickname                                         Trust Attributes
                                                             SSL,S/MIME,JAR/XPI

Alice                                                        u,u,u
bob@bogus.com                                                p,p,p
dave@bogus.com                                               p,p,p
eve@bogus.com                                                p,p,p
TestCA                                                       CT,C,C
bobs-laptop(129) certutil -K -d dbm:. -f ../password_file 
certutil: Checking token "NSS Certificate DB" in slot "NSS User Private Key and Certificate Services"
< 0> rsa      914ee50d6b02109543b0d1b6b01b0a92b3209fb4   alice@bogus.com
bobs-laptop(130) certutil -L -X -d sql:.

Certificate Nickname                                         Trust Attributes
                                                             SSL,S/MIME,JAR/XPI

Alice                                                        u,u,u
bob@bogus.com                                                p,p,p
dave@bogus.com                                               p,p,p
eve@bogus.com                                                p,p,p
TestCA                                                       CT,C,C
bobs-laptop(131) ls -l
total 168
-rw-r----- 1 rrelyea rrelyea   602 Nov  3 16:02 Alice.cert
-rw------- 1 rrelyea rrelyea 65536 Nov 20 16:58 cert8.db
-rw-r--r-- 1 rrelyea rrelyea  9216 Nov 20 16:58 cert9.db
-rw------- 1 rrelyea rrelyea 16384 Nov 20 16:58 key3.db
-rw-r--r-- 1 rrelyea rrelyea  9216 Nov 20 16:58 key4.db
-rw-r--r-- 1 rrelyea rrelyea   521 Nov 20 16:58 pkcs11.txt
-rw-r----- 1 rrelyea rrelyea   450 Nov  3 16:02 req
-rw------- 1 rrelyea rrelyea 16384 Nov  3 16:02 secmod.db
bobs-laptop(132) certutil -K -d sql:. -f ../password_file 
certutil: Checking token "NSS Certificate DB" in slot "NSS User Private Key and Certificate Services"
certutil: no keys found
bobs-laptop(133) certutil -K -X -d sql:. -f ../password_file
certutil: Checking token "NSS Certificate DB" in slot "NSS User Private Key and Certificate Services"
certutil: no keys found
bobs-laptop(134)
here's what the resulting databases look like:

bobs-laptop(136) sqlite3 cert9.db
SQLite version 3.3.13
Enter ".help" for instructions
sqlite> .dump
BEGIN TRANSACTION;
CREATE TABLE nssPublic (id PRIMARY KEY UNIQUE ON CONFLICT ABORT, a0, a1, a2, a3, a10, a11, a12, a80, a81, a82, a83, a84, a85, a86, a87, a88, a89, a8a, a8b, a90, a100, a101, a102, a103, a104, a105, a106, a107, a108, a109, a10a, a10b, a10c, a110, a111, a120, a121, a122, a123, a124, a125, a126, a127, a128, a130, a131, a132, a133, a134, a160, a161, a162, a163, a164, a165, a166, a170, a180, a181, a200, a201, a202, a210, a40000211, a40000212, a300, a301, a302, a400, a401, a402, a403, a404, a405, a406, a480, a481, a482, a500, a501, a502, a503, ace534351, ace534352, ace534353, ace534354, ace534355, ace534356, ace534357, ace534358, ace534364, ace534365, ace534366, ace534367, ace534368, ace536351, ace536352, ace536353, ace536354, ace536355, ace536356, ace536357, ace536358, ace536359, ace53635a, ace53635b, ace53635c, ace53635d, ace53635e, ace53635f, ace536360, ace5363b4, ace5363b5, ad5a0db00, a80000001, ace534369);
CREATE INDEX issuer ON nssPublic (a81);
CREATE INDEX subject ON nssPublic (a101);
CREATE INDEX label ON nssPublic (a3);
CREATE INDEX ckaid ON nssPublic (a102);
COMMIT;
sqlite> .quit
bobs-laptop(137) sqlite3 key4.db
SQLite version 3.3.13
Enter ".help" for instructions
sqlite> .dump
BEGIN TRANSACTION;
CREATE TABLE nssPrivate (id PRIMARY KEY UNIQUE ON CONFLICT ABORT, a0, a1, a2, a3, a10, a11, a12, a80, a81, a82, a83, a84, a85, a86, a87, a88, a89, a8a, a8b, a90, a100, a101, a102, a103, a104, a105, a106, a107, a108, a109, a10a, a10b, a10c, a110, a111, a120, a121, a122, a123, a124, a125, a126, a127, a128, a130, a131, a132, a133, a134, a160, a161, a162, a163, a164, a165, a166, a170, a180, a181, a200, a201, a202, a210, a40000211, a40000212, a300, a301, a302, a400, a401, a402, a403, a404, a405, a406, a480, a481, a482, a500, a501, a502, a503, ace534351, ace534352, ace534353, ace534354, ace534355, ace534356, ace534357, ace534358, ace534364, ace534365, ace534366, ace534367, ace534368, ace536351, ace536352, ace536353, ace536354, ace536355, ace536356, ace536357, ace536358, ace536359, ace53635a, ace53635b, ace53635c, ace53635d, ace53635e, ace53635f, ace536360, ace5363b4, ace5363b5, ad5a0db00, a80000001, ace534369);
CREATE TABLE metaData (id PRIMARY KEY UNIQUE ON CONFLICT REPLACE, item1, item2);
INSERT INTO "metaData" VALUES('password',X'B4B427A9687A74558523C2E0F25680EE6661D227',X'303C3028060B2A864886F70D010C05010330190414366C310DF396DB1F1AFEF8B854F9F54DD7EEA7DA020101041024EEBA8FA467D8EF98244305C49E14AA');
CREATE INDEX issuer ON nssPrivate (a81);
CREATE INDEX subject ON nssPrivate (a101);
CREATE INDEX label ON nssPrivate (a3);
CREATE INDEX ckaid ON nssPrivate (a102);
COMMIT;
sqlite> .quit
However: If I try again without trying to open the database read only in the middle....

bobs-laptop(138) rm cert9.db key4.db pkcs11.txt 
bobs-laptop(139) certutil -L -X -d sql:.

Certificate Nickname                                         Trust Attributes
                                                             SSL,S/MIME,JAR/XPI

Alice                                                        u,u,u
bob@bogus.com                                                p,p,p
dave@bogus.com                                               p,p,p
eve@bogus.com                                                p,p,p
TestCA                                                       CT,C,C
bobs-laptop(140) certutil -K -X -d sql:. -f ../password_file 
certutil: Checking token "NSS Certificate DB" in slot "NSS User Private Key and Certificate Services"
< 0> rsa      914ee50d6b02109543b0d1b6b01b0a92b3209fb4   alice@bogus.com
More diagnostics.... It appears we are creating the password entry even though the database is opened read/only.

bob


rm cert9.db key4.db pkcs11.txt
bobs-laptop(143) certutil -L -X -d sql:.

Certificate Nickname                                         Trust Attributes
                                                             SSL,S/MIME,JAR/XPI

Alice                                                        u,u,u
bob@bogus.com                                                p,p,p
dave@bogus.com                                               p,p,p
eve@bogus.com                                                p,p,p
TestCA                                                       CT,C,C
bobs-laptop(144) sqlite3 cert9.db
SQLite version 3.3.13
Enter ".help" for instructions
sqlite> .dump
BEGIN TRANSACTION;
CREATE TABLE nssPublic (id PRIMARY KEY UNIQUE ON CONFLICT ABORT, a0, a1, a2, a3, a10, a11, a12, a80, a81, a82, a83, a84, a85, a86, a87, a88, a89, a8a, a8b, a90, a100, a101, a102, a103, a104, a105, a106, a107, a108, a109, a10a, a10b, a10c, a110, a111, a120, a121, a122, a123, a124, a125, a126, a127, a128, a130, a131, a132, a133, a134, a160, a161, a162, a163, a164, a165, a166, a170, a180, a181, a200, a201, a202, a210, a40000211, a40000212, a300, a301, a302, a400, a401, a402, a403, a404, a405, a406, a480, a481, a482, a500, a501, a502, a503, ace534351, ace534352, ace534353, ace534354, ace534355, ace534356, ace534357, ace534358, ace534364, ace534365, ace534366, ace534367, ace534368, ace536351, ace536352, ace536353, ace536354, ace536355, ace536356, ace536357, ace536358, ace536359, ace53635a, ace53635b, ace53635c, ace53635d, ace53635e, ace53635f, ace536360, ace5363b4, ace5363b5, ad5a0db00, a80000001, ace534369);
CREATE INDEX issuer ON nssPublic (a81);
CREATE INDEX subject ON nssPublic (a101);
CREATE INDEX label ON nssPublic (a3);
CREATE INDEX ckaid ON nssPublic (a102);
COMMIT;
sqlite> .quit
bobs-laptop(145) sqlite3 key4.db
SQLite version 3.3.13
Enter ".help" for instructions
sqlite> .dump
BEGIN TRANSACTION;
CREATE TABLE nssPrivate (id PRIMARY KEY UNIQUE ON CONFLICT ABORT, a0, a1, a2, a3, a10, a11, a12, a80, a81, a82, a83, a84, a85, a86, a87, a88, a89, a8a, a8b, a90, a100, a101, a102, a103, a104, a105, a106, a107, a108, a109, a10a, a10b, a10c, a110, a111, a120, a121, a122, a123, a124, a125, a126, a127, a128, a130, a131, a132, a133, a134, a160, a161, a162, a163, a164, a165, a166, a170, a180, a181, a200, a201, a202, a210, a40000211, a40000212, a300, a301, a302, a400, a401, a402, a403, a404, a405, a406, a480, a481, a482, a500, a501, a502, a503, ace534351, ace534352, ace534353, ace534354, ace534355, ace534356, ace534357, ace534358, ace534364, ace534365, ace534366, ace534367, ace534368, ace536351, ace536352, ace536353, ace536354, ace536355, ace536356, ace536357, ace536358, ace536359, ace53635a, ace53635b, ace53635c, ace53635d, ace53635e, ace53635f, ace536360, ace5363b4, ace5363b5, ad5a0db00, a80000001, ace534369);
CREATE INDEX issuer ON nssPrivate (a81);
CREATE INDEX subject ON nssPrivate (a101);
CREATE INDEX label ON nssPrivate (a3);
CREATE INDEX ckaid ON nssPrivate (a102);
COMMIT;
sqlite> .quit
bobs-laptop(146) certutil -K -d sql:. -f ../password_file 
certutil: Checking token "NSS Certificate DB" in slot "NSS User Private Key and Certificate Services"
certutil: no keys found
bobs-laptop(147) sqlite3 cert9.db
SQLite version 3.3.13
Enter ".help" for instructions
sqlite> .dump
BEGIN TRANSACTION;
CREATE TABLE nssPublic (id PRIMARY KEY UNIQUE ON CONFLICT ABORT, a0, a1, a2, a3, a10, a11, a12, a80, a81, a82, a83, a84, a85, a86, a87, a88, a89, a8a, a8b, a90, a100, a101, a102, a103, a104, a105, a106, a107, a108, a109, a10a, a10b, a10c, a110, a111, a120, a121, a122, a123, a124, a125, a126, a127, a128, a130, a131, a132, a133, a134, a160, a161, a162, a163, a164, a165, a166, a170, a180, a181, a200, a201, a202, a210, a40000211, a40000212, a300, a301, a302, a400, a401, a402, a403, a404, a405, a406, a480, a481, a482, a500, a501, a502, a503, ace534351, ace534352, ace534353, ace534354, ace534355, ace534356, ace534357, ace534358, ace534364, ace534365, ace534366, ace534367, ace534368, ace536351, ace536352, ace536353, ace536354, ace536355, ace536356, ace536357, ace536358, ace536359, ace53635a, ace53635b, ace53635c, ace53635d, ace53635e, ace53635f, ace536360, ace5363b4, ace5363b5, ad5a0db00, a80000001, ace534369);
CREATE INDEX issuer ON nssPublic (a81);
CREATE INDEX subject ON nssPublic (a101);
CREATE INDEX label ON nssPublic (a3);
CREATE INDEX ckaid ON nssPublic (a102);
COMMIT;
sqlite> .quit
bobs-laptop(148) sqlite3 key4.db
SQLite version 3.3.13
Enter ".help" for instructions
sqlite> .dump
BEGIN TRANSACTION;
CREATE TABLE nssPrivate (id PRIMARY KEY UNIQUE ON CONFLICT ABORT, a0, a1, a2, a3, a10, a11, a12, a80, a81, a82, a83, a84, a85, a86, a87, a88, a89, a8a, a8b, a90, a100, a101, a102, a103, a104, a105, a106, a107, a108, a109, a10a, a10b, a10c, a110, a111, a120, a121, a122, a123, a124, a125, a126, a127, a128, a130, a131, a132, a133, a134, a160, a161, a162, a163, a164, a165, a166, a170, a180, a181, a200, a201, a202, a210, a40000211, a40000212, a300, a301, a302, a400, a401, a402, a403, a404, a405, a406, a480, a481, a482, a500, a501, a502, a503, ace534351, ace534352, ace534353, ace534354, ace534355, ace534356, ace534357, ace534358, ace534364, ace534365, ace534366, ace534367, ace534368, ace536351, ace536352, ace536353, ace536354, ace536355, ace536356, ace536357, ace536358, ace536359, ace53635a, ace53635b, ace53635c, ace53635d, ace53635e, ace53635f, ace536360, ace5363b4, ace5363b5, ad5a0db00, a80000001, ace534369);
CREATE TABLE metaData (id PRIMARY KEY UNIQUE ON CONFLICT REPLACE, item1, item2);
INSERT INTO "metaData" VALUES('password',X'B4B427A9687A74558523C2E0F25680EE6661D227',X'303C3028060B2A864886F70D010C05010330190414366C310DF396DB1F1AFEF8B854F9F54DD7EEA7DA020101041024EEBA8FA467D8EF98244305C49E14AA');
CREATE INDEX issuer ON nssPrivate (a81);
CREATE INDEX subject ON nssPrivate (a101);
CREATE INDEX label ON nssPrivate (a3);
CREATE INDEX ckaid ON nssPrivate (a102);
COMMIT;
sqlite> .quit
Bob, I wonder if you've found, and are addressing, a different problem 
from the one originally reported.  

In the newsgroup, the problem originally reported by Hans Petter Jansson 
concerned a program that tried to create a cert9.db (from scratch) using 
NSS_InitWithMerge.  I asked Hans to confirm that that was what he was doing, 
and he did confirm that.  I think that's the problem that Wolfgang first 
reported in this bug.  (I gather that Hans and Wolfgang are colleagues 
working on the same project.)

I think you may have identified an additional bug, one with NSS_Init 
(which is supposed to be read-only, and is not ever supposed to create
or write to any DB files).  But I kinda doubt that is the same problem
as the one where NSS_InitWithMerge was used to create the cert9.db file.

If these are separate problems, I think the (new) problem of NSS_Init 
creating new DBs should be a separate bug, and this bug should go back to 
be about creating DBs with NSS_InitWithMerge.

But I could have this all wrong.  Maybe Wolfgang and Hans Petter Jansson can help clarify.
Nelson is right. We might have more than one issue but the "first" issue which is confirmed and needs to be solved is that my provided testcase (using NSS_InitWithMerge) doesn't create a correctly updated/merged sql db while it seems that it should by documentation and what Nelson said.

In addition all my tests so far were always with an empty master password.

What I try to achieve is that Firefox upgrades and merges the legacy db to a shared location using the sql db when started. NSS_InitWithMerge seems to be the correct (and only) choice to do that. And that fails currently if there isn't already a shared db at the shared location.
Well, I'm not sure whether NSS_InitWithMerge is defined such that it should
create a cert9.db when none exists, or if it should fail and return an error
in that case.  But I am sure it should not return a success indication if 
it actually fails, and I feel strongly that it must not create and leave 
on disk a corrupt DB file.
Agreed, 
and if it's not defined to create one from scratch I'm wondering what the proposal is to achieve what I wrote before.

If a db exists NSS_InitWithMerge can be used (upgrade-merge) but how can I create a shared database from scratch preserving the old data (just upgrade (to another location))? Is there an API for that or do I need to use NSS_Initialize with an sql parameter and just move the resulting DB files around manually?
I don't know what you're trying to accomplish, but if you just want to 
create a new cert9 from the contents of an existing cert8, you can use 
NSS_InitReadWrite.  If you need the cert9 in a different directory than
the cert 8, you can either move/link the cert9 after, or move/link the 
cert8 before (and key DBs too).
So Here are the basic scenarios:

1) You have a new application and you want to use shared datbases -- Just use NSS_Init() with the "sql:" prefix.

2) You have an application that uses old (dbm) databases and you want to just start using the shared (sql) database in the same old directory you have been using. -- Just use NSS_Init() with the "sql:" prefix. NSS will automatically detect your old dbm database and upgrade it to the the sql database.

3) You have an application that uses the old (dbm) databases and you want to transition to using a shared (sql) database in a common location in which several different apps will share (and those different applications may already have their own copy of old (dbm) databases). -- Use NSS_InitWithMerge() to open NSS and follow the documented initialization procedure.

4) You have want to manage find-grain control of any merging (that is you only want to merge data as a result of User intervention, or under full control of the application). Open your 'main' database (the one you intend on continuing to use) with NSS_Init() as you would normally. Use SECMOD_OpenUserDB to open the database you want to merge. Then call PK11_MergeTokens(). PK11_MergeTokens() will always try to merge the entries from the source database into the target. It has no notion of 'already merged'. If there were some entries that could not be merged for some reason, PK11_MergeTokens will return an error. It will optionlly return an error log of those entries that failed to merge and 'why'. NOTE that PK11_MergeTokens() is a general merge tool. It will take as source or targets any sort of database or token. You could even specify merging a database into a smart card (though the smart card is not likely to support all the object types a database can).


NSS_InitWithMerge is designed to have the same semantics as NSS_Init did with older databases. NSS_Init tried to automatically detect the existence of older databases when the new NSS database was created. The idea is applications could call NSS_InitWithMerge as their normal way of initializing NSS. The InitWithMerge call would be a no-op if the source database has already been merged in (just like NSS_Init would not look for older databases if it finds the newer one). If NSS_InitWithMerge cannot open the old database, it will operate as if the old one didn't exist (which is a possible scenario), and create a new empty sql database, and say the application id has been updated. The idea here is that no source database is probably a *very* common scenario, while an existing old database that can't be opened is rare. The latter can be handled by falling about to an explicit call to PK11_MergeTokens().

All that being said, There's still the issue, is the new database 'usable'? You should be able to init that new database as normal, and then be able to set passwords, load certs, etc. You should also be able to merge other databases from other sources into this new database.

bob
>The latter can be handled by falling about to an explicit call to PK11_MergeTokens().

should read:

The latter can be handled by falling back to an explicit call to PK11_MergeTokens().
Thanks for the detailed description. That's how I understood it and this bug is about 3) not working for the initial creation of the new shared database.

The new database is not 'usable' for me. I'm able to init that new database but it's not fully working. (That's why Firefox' password manager fails which was the initial symptom I reported in the newsgroup).
Summary: NSS_Init creates a broken database if it didn't exist before → NSS_InitWithMerge creates a broken database if it didn't exist before
Finally getting back to this I've discovered the following:

1) There is a bug in the SharedDB page which I have fixed. This bug is reflected in the test case. The first if should be:

 if (PK11_IsRemovable (slot) && PK11_NeedLogin(slot))

and not

 if (PK11_IsRemovable (slot) && !PK11_NeedLogin(slot))

The resulting database is only a partially merged database.
Once the change is made, update fails for the following reason:

> nss-migrate
First auth call failed: -8177.

this is because the nss-migrate.c program does not define a function to fetch the password.


2) The database created with the following commands:

(start with an NSS database is keys already created)
certutil -L -X -d sql:.
certutil -K -d sql:.

incorrectly puts the password record in the key database even though the key database is readOnly.

I'll include a patch for the latter issue shortly. This may have been compounding the above problem.

3) There is also an issue with upgrade merge where we should be able to update with only one password (either because the target database is new, or it has the same password as the source). I have a patch for that issue as well.
This fixed the problem where you can hose your key database by trying to 'update' when in read only mode.

The basic problem is sqlite3 (the version we have anyway) does not allow you to specify if you want to open the databased readonly or rw. It will automatically fall back to readonly if the database can't be open rw. This means we have to enforce readonly on our own. we missed one write case, which is the write of the meta data. This patch fixes this.

bob
Update-merge is designed to upgrade the database with the minimal amount of data input from the user. 

No input is needed in the following cases:

1. The source database has no password or is uninitialized and the target database has no password or is uninitialized.

One password is needed in the following cases:

2. The source database has no password or is uninitialize and the target has a password (need the target password).
3. The source database has a password and the target database has no password, (need source password).
4. The source database has a password and the target is uninitialized (need source password, target password becomes the source password).
5. The source database has a password and it matches the target password. (need source password which matches target password).

Case where we need 2 password.

6. The source database has a password which is different from the target password (need target password).

The problem is without this patch, both cases 4 and 5 prompt for the password twice. What is happening is the underlying password checking code has detected that the old database (source) has a password and has arranged for the slot to have the slot name of the old database so applications can control what gets prompted at this point. Once we have all the source password, we need to switch to using a new token name. We signal by simulating a token removal. For cases 4 and 5 we have arranged for the new token to already be logged in so the user does not get a prompt. Unfortunately a side effect of closing all the sessions with the close all sessions call is to logout the token (normal PKCS #11 semantics). This patch prevents that logout in the case where we are simulating a token removal.

bob
Issue 2 was turned up because of this issue. We still want to prevent the writing of meta data on a readonly database, so that patch for issue 2 is necessary.

This patch is necessary because without it any application that opens an unupdated database readonly will effectively stop accessing this database once it has authenticated. You can see this with the following commands.

certutil -L -d sql:.

will happily list all the certs in a dbm database in '.' if the sql database has not yet been updated.

certutil -K -d sql:.

will fail to list the keys (because you have supplied the password), and with Issue 2, leave the resulting sql database uninitialized, but with the appearance of being initialized.

If issue2 is fixed, or if you haven't yet used the above command, the following will work.

certutil -K -X -d sql:.

With this patch the cerutil -K -d sql:. will successfully list the keys.

bob
Attachment #367883 - Flags: review?(wtc)
Comment on attachment 367883 [details] [diff] [review]
Issue 2: stop writing of password record if the database was opened read only

see comments for description.
Attachment #367890 - Flags: review?(wtc)
Attachment #367894 - Flags: review?
wtc, these are 3 corner cases in shared db, mostly around update. The patches are relatively small, hopefully I've included enough info for review.

(review by any other NSS team member is also welcome).

bob
Attachment #367894 - Flags: review? → review?(wtc)
Comment on attachment 367894 [details] [diff] [review]
Final Issue: Trying to update a readonly database.

This is the wrong patch. It's identical the Issue 3 patch.
Attachment #367894 - Attachment is obsolete: true
Attachment #367894 - Flags: review?(wtc)
Attachment #367896 - Flags: review?(wtc)
Comment on attachment 367883 [details] [diff] [review]
Issue 2: stop writing of password record if the database was opened read only

r=wtc.  Nit: you added two extra blank lines (one above
and one below the new code).
Attachment #367883 - Flags: review?(wtc) → review+
Comment on attachment 367890 [details] [diff] [review]
Issue 3: prevent logout when switching tokens.

r=wtc.

Nit: add a space before some of the PR_FALSE arguments passed
to sftk_CloseAllSessions.

>+    /* special case - if we are in a middle of upgrade, we want to close the
>+     * sessions to show the card has been removed, but we don't want to 
>+     * explicity logout in case we can upgrade with the existing password
>+     */

The "card" in this comment confuses me.  We're upgrading the NSS database.
What's the (smart)card?  Did you mean the (soft)token?
Attachment #367890 - Flags: review?(wtc) → review+
Comment on attachment 367896 [details] [diff] [review]
Final Issue (v2): Trying to update a readonly database.

r=wtc.  One question:

>-	if (keydb->update) {
>+	if (keydb->flags != SDB_RDONLY && keydb->update) {

Since there are three other SDB_xxx bit flags, should we
rewrite the test
    keydb->flags != SDB_RDONLY
as
    (keydb->flags & SDB_RDONLY) == 0
or perhaps
    (keydb->flags & SDB_RDWR) != 0
?
Attachment #367896 - Flags: review?(wtc) → review+
Thanks for the reviews wan-teh. I've corrected the nits for your first two reviews.

To answer the your question, it's a virtual card/token removal. I've updated the comment to make that more clear.


As for the last issue, Your review pointed out a better way of doing this patch, which I'll attach shortly...
The sftkdb code uses SDB_RDONLY as a discrete value, not as a flag, so I checked the flag use that wtc pointed out. It turns out the flags are stored in the sdb data structure and are available to sftkdb code (we access them to determine if a given database can support meta data, for instance). This simplifies the patch by using those flags rather than trying to remember what flags we passed to the underlying sdb.

In short, this patch has the same effect as the previous patch, only more simply. The notion came directly from wtc's review.

bob
Attachment #367896 - Attachment is obsolete: true
Attachment #368285 - Flags: review?(wtc)
Comment on attachment 368285 [details] [diff] [review]
Final Issue (v3): Trying to update a readonly database.

r=wtc.  Do you think it's more robust to test
    (keydb->db->sdb_flags & SDB_RDWR) != 0
instead, just in case neither SDB_RDONLY nor SDB_RDWR is set?

LXR showed that we are currently using the test
    (keydb->db->sdb_flags & SDB_RDONLY) == 0)
elsewhere, so you should check this in as is for consistency.
The LXR also shows three flags == SDB_RDONLY tests that should
probably be changed:
http://mxr.mozilla.org/security/ident?i=SDB_RDONLY
http://mxr.mozilla.org/security/ident?i=SDB_RDWR
Attachment #368285 - Flags: review?(wtc) → review+
>     (keydb->db->sdb_flags & SDB_RDWR) != 0
> instead, just in case neither SDB_RDONLY nor SDB_RDWR is set?

No, because as far as I can tell, SDB_RDWR is never set. SDB_RDONLY is always set
on read only databases

> The LXR also shows three flags == SDB_RDONLY tests that should
> probably be changed:

Those are cases where SDB_RDONY are not used as a flag. We probably should use different defines rather than overloading these defines.

bob
Should we remove SDB_RDWR then, to avoid confusion?  LXR shows
we still set SDB_RDWR in one place:
http://mxr.mozilla.org/security/ident?i=SDB_RDWR
2009-03-19 11:21	rrelyea%redhat.com 	/cvsroot/mozilla/security/nss/lib/softoken/sftkpwd.c 	1.8 	  	1/1  	
Bug 465709 - NSS_InitWithMerge creates a broken database if it didn't exist before

Final patch for this bug. Don't try to update a read only database, even after authentication.

r=wtc
2009-03-18 16:41	rrelyea%redhat.com 	/cvsroot/mozilla/security/nss/lib/softoken/pkcs11.c 	1.163 	  	2/2  	

Fix one of the review nits wtc pointed (poor formatting of a couple of PR_FALSE statements).

bob
2009-03-18 16:38	rrelyea%redhat.com 	/cvsroot/mozilla/security/nss/lib/softoken/sdb.c 	1.11 	  	4/0  	

Bug 465709 NSS_InitWithMerge creates a broken database if it didn't exist before

Patches 2 & 3 r=wtc.

1) don't allow writes to the meta data cache if we opened the database
read only.
2) preserve the login state over virtual tokenremovals so updatemerges can complete with a single password in all cases where a single password merge update is possible.
2009-03-18 16:38	rrelyea%redhat.com 	/cvsroot/mozilla/security/nss/lib/softoken/pkcs11.c 	1.162 	  	24/16 
2009-03-18 16:38	rrelyea%redhat.com 	/cvsroot/mozilla/security/nss/lib/softoken/pkcs11i.h 	1.51 	  	1/1
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.12.3
You need to log in before you can comment on or make changes to this bug.