1382278 - certutil -A creates uninitialised database

Status: RESOLVED FIXED

(NSS :: Tools, defect, P3)

Description

[Hubert Kario]

When certutil is used to import a certificate to a database that does not exist, it creates uninitialised database:

   openssl req -x509 -newkey rsa -keyout localhost.key -out localhost.crt -nodes -batch -subj /CN=localhost
   mkdir nssdb
   certutil -A -n ca -t 'cTC,cTC,cTC' -d nssdb -a -i localhost.crt
   modutil -dbdir nssdb -list 'NSS Internal PKCS #11 Module'

Outputs:

  Slot: NSS User Private Key and Certificate Services
...
  Access: NOT Write Protected
  Login Type: Login required
  User Pin: NOT Initialized


When the database is created as follows:

   mkdir nssdb
   certutil -N -d nssdb --empty-password
   certutil -A -n ca -t 'cTC,cTC,cTC' -d nssdb -a -i localhost.crt
   modutil -dbdir nssdb -list 'NSS Internal PKCS #11 Module'

The output of the last command is like this:

  Slot: NSS User Private Key and Certificate Services
...
  Access: NOT Write Protected
  Login Type: Public (no login required)
  User Pin: Initialized


Finally, if I try to create a new format database at the time of importing a certificate, the whole operation fails:

   mkdir nssdb
   certutil -A -n ca -t 'cTC,cTC,cTC' -d sql:nssdb -a -i localhost.crt

Outputs:

certutil: could not authenticate to token NSS Certificate DB.: SEC_ERROR_IO: An I/O error occurred during security authorization.

Comment 1

[Kai Engert (:KaiE:)]

Hubert, can you please confirm I understand the report correctly?

- when performing this operation using the DBM format,
  the operation works, but the resulting NSS database
  is uninitialized (which is untypical, because certutil
  usually initializes the database)

- when performing this operaton using the SQL format,.
  operation fails

If my understanding is correct, this is a change in NSS behavior that 
should block change the NSS default file format to SQL.

Comment 2

[Hubert Kario]

yes, that's exactly the case

Comment 3

[Robert Relyea]

So looking at this issue this happens because NSS will automatically create a database if one doesn't exist and it's opened RW. I'm not sure we can mess with the underlying NSS database creation semantic (at least at this time).

This can be fixed in the following ways in certutil:

1) Whenever we initialize NSS read/write, always check to see if the database is uninitialized and initialize it if it's not. This has the advantage that certutil will now never create an uninitialized database. The disadvantages are:
   a. certutil would silently change and existing uninitialized database to one initialized to either no password or the password supplied.

2) When initializing NSS rw, first try to initialize read only to see if the database already exists. If it doesn't exist, we then initialize it after we open rw. This will guarantee every cerutil created database will never be uninitialized, but does not modify existing uninitialized databases. Disadvantage: overly complicated. Not a good example or other apps using NSS.

3) In AddCert, check to see if the database is initialized and initialize it if it's not (use the given password if it's available). This will solve the issue above. Disadvantage: It will modify uninitialized databased to initialized databases that already exist.


I'm proposing to implement option 3. I think it is less surprising to the user to have an uninitialised database suddenly initialized because he created a certificate in it (rather than case 1 where it could happen because he listed the database with a -x flag) and not as complicated as option 2. certutil will already initialize the database on key gen.

Comment 4

[Hubert Kario]

Why initialising the database is a bad thing? What use does an uninitialised database have?

I'd say it's surprising to users that the database can be created, store certificates but not be initialised.

The problem is that -A is only the one obvious operation I noticed failing, I didn't perform extensive testing to see if it's the only operation that causes creation of uninitialised database. Thus I'd say that option 1). is a better solution

Comment 5

[Robert Relyea]

When you initialize a database, you decide whether it is has a password or not and what that password is. If you are doing some operation that doesn't include a password, you will be making a decision for the user on the fly without the user's input.

In the case of adding a new cert, the user is expected to supply a password anyway (to update the trust values), so using that password to initialize the database is fine.

This doesn't just affect the NSS database, it also affects other tokens that have an 'uninitialized' state. It's reasonable to expect to initialize a token before you modify it, but if you are just listing it.

It's also a corner case. You have to specifically ask to open a database r/w when doing something like listing the database. So I won't resist either option 1 or 3 (which is why I listed all the options). My preference is option 3.

Also, whatever we do, we do need to deal with uninitialized databases. Application can create them (sometimes by accident), and NSS has already created a number of them, so we do need to deal with them in any case. Having an obscure way of creating them may be a good thing.

bob

Comment 6

[Hubert Kario]

(In reply to Robert Relyea from comment #5)
> When you initialize a database, you decide whether it is has a password or
> not and what that password is. If you are doing some operation that doesn't
> include a password, you will be making a decision for the user on the fly
> without the user's input.

to a large degree an uninitialised database is a database without a password (empty password)

If the user decides later to set a password, he or she can do that even if we did initialise it with empty password.

> In the case of adding a new cert, the user is expected to supply a password
> anyway (to update the trust values), so using that password to initialize
> the database is fine.

I'd go further and say that if we can continue without password and initialise the database without password that would be fine too.

> This doesn't just affect the NSS database, it also affects other tokens that
> have an 'uninitialized' state. It's reasonable to expect to initialize a
> token before you modify it, but if you are just listing it.

yes, for tokens the situation is quite different, but then a regular user is very unlikely to see an uninitialised token (as it would most likely be provisioned by his or her employer)

also, I'm talking about nss databases specifically

> Also, whatever we do, we do need to deal with uninitialized databases.
> Application can create them (sometimes by accident), and NSS has already
> created a number of them, so we do need to deal with them in any case.
> Having an obscure way of creating them may be a good thing.

yes, we will need that for testing, but then creation of uninitialized databases should be explicit and we should define the semantics when the database is left unmodified and when it becomes initialised after making changes

Comment 7

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

Bulk change per wleung.

Comment 8

[Robert Relyea]

Attachment: Initialize the database before attempint a write operation

Comment 9

[Kai Engert (:KaiE:)]

Comment on attachment 8908827 [details] [diff] [review]
Initialize the database before attempint a write operation

I've confirmed that you're adding this code to a good location inside certutil, and you're reusing equivalent function calls from other places that set or change the password.

The PW_PLAINTEXT code seems unnecessary currently, because apparently doesn't offer a way to provide the password directly as a parameter. But your code looks correct, so it's OK to keep it, in case this is ever supported in the future.

I've only one small functional change request.

Instead of "free", we should use the same cleanup functions that other code uses, in particular SECU_ChangePW2 does this:
    if (newpw) {
        PORT_Memset(newpw, 0, PL_strlen(newpw));
        PORT_Free(newpw);
    }

Besides that, a few whitespace changes are required to make the code comply with our clang-format tool.

I'll make these trivial changes for you.

Comment 10

[Kai Engert (:KaiE:)]

Attachment: 1382278-v2.patch

This changes Bob's patch to use different free code, and adjusts whitespace.

Comment 11

[Kai Engert (:KaiE:)]

Attachment: 1382278-test.patch

I suggest to add this test.

(It fails without the patch.)

Comment 12

[Kai Engert (:KaiE:)]

Comment on attachment 8909289 [details] [diff] [review]
1382278-v2.patch

https://hg.mozilla.org/projects/nss/rev/18edd4ad8389

Comment 13

[Kai Engert (:KaiE:)]

Bob, do you think the new init code should be executed for these?
- cmd_CreateNewCert
- cmd_CertReq

Comment 14

[Robert Relyea]

I was thinking that those would already have initialization since you need to initialize the database before you can create or install keys. That said they probably both just fail currently so maybe it would make sense to initialize the database before they are executed.

bob

RE changes you made: r+
I'll review the tests separately.

Comment 15

[Robert Relyea]

Comment on attachment 8909290 [details] [diff] [review]
1382278-test.patch

Review of attachment 8909290 [details] [diff] [review]:
-----------------------------------------------------------------

Before you r+ this, I wanted to verify that you really wanted to test both dbm and sql separately on each run rather than dbm on the dbm run and sql on the sql run.

::: tests/cert/cert.sh
@@ +1955,5 @@
> +  certu -A -n ca -t 'C,C,C' -d dbm:${IMPLICIT_DBM_DIR} -i "${SERVER_CADIR}/serverCA.ca.cert"
> +
> +  CU_ACTION="Add cert with trust flags to sql db with implicit init"
> +  mkdir ${IMPLICIT_SQL_DIR}
> +  certu -A -n ca -t 'C,C,C' -d sql:${IMPLICIT_SQL_DIR} -i "${SERVER_CADIR}/serverCA.ca.cert"

Did you really want to explicitly label these? our test suites run all the tests with dbm and then with sql separately. I think you can just add one tests.

The other thing is we do have the _P forms of the directories separate from the _DIR forms. This was because the rdb style systems didn't have full paths, but 'profile names'. It's probably not important, and I suspect the _P forms have rotted since we haven't supported rdb style systems in a long time, so really it's a bikeshedding comment, but I thought I'd point it out.

Comment 16

[Robert Relyea]

The above should be 'Before I r+ this, I want...'

Kai, did you really intend to explicitly test dbm and sql cases for this in a single test, rather than having a single test for the functionality and letting dbm and sql cases be covered in the respective dbm and sql runs?

Comment 17

[Kai Engert (:KaiE:)]

Good point, I agree to run just one test in each cycle, with the default format of the cycle, and let the cycles cover both the dbm and sql scenarios.


Regarding _P forms: I found the P_R_ directories like P_R_CADIR, and P_ dirs like P_SERVER_CADIR.

Those seem to have the option to use "multiaccess" combined with a "domain".
The multiaccess is used, only, if environment variable MULTIACCESS_DBM is defined.
However, it seems we don't ever define MULTIACCESS_DBM anywhere in the tests scripts, so this seems to have rotted and no longer in use.

When not using multiaccess, the difference of the P_R_ directories is that the P_R_ dirs are relative path names, and the P_ dirs are identical to the versions without P_ (absolute).

I think you're asking me to use the same approach, that could allow the implicit init test to be executed through multiaccess, if it was ever enabled.

It seems simple to do, so I'll try to add it.

Comment 18

[Kai Engert (:KaiE:)]

Attachment: 1382278-test-v3.patch

Comment 19

[Robert Relyea]

Thanks. I was just making you aware of what MULTIACCESS case. The old rdb code (multiaccess) used a central server which NSS talked to through a rpc. The actual location of the database was opaque to NSS itself, it just supplied a 'domain' which the rdb server would map to some location under it's tree. I'm not sure if the code is still intact since we haven't used rdb in a long time, but it could be useful if we decide we need to do some 'installable' database in the future (rdb would open an external dll to access the rpc).

Comment 20

[Kai Engert (:KaiE:)]

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