Open Bug 325376 Opened 18 years ago Updated 1 year ago

change "security library: bad database." mesage for errors that are not related to db failure

Categories

(NSS :: Tools, defect, P3)

3.11

Tracking

(Not tracked)

3.15.5

People

(Reporter: alvolkov.bgs, Unassigned)

Details

Attachments

(3 files, 5 obsolete files)

Our tools mislead users about condition of cert db by saying "bad database"
for any unsuccessful operations that somehow were releated to db read or write.

For exmaple if user list a cert that is not in db he gets the following error:

$ certutil -d . -L -n "test"
certutil: Could not find: test
: security library: bad database.

Nelson's comments:
There's nothing "bad" about the database, just because it doesn't contain the data you wanted to find there.  I agree that the error message should be changed to reflect what really happened, and not make a value judgement about the condition of the database.
Alexei, thanks for finding these bugs.  
Since you've done the hard work (finding the bugs), 
please attach patches to fix them.
Assignee: wtchang → alexei.volkov.bugs
Target Milestone: --- → 3.11.1
Please fix this for 3.11.1.
The message should say something like:
"Data base record not found, or duplicate record creation disallowed."
Priority: -- → P2
QA Contact: jason.m.reid → tools
Priority: P2 → P3
The target milestone is already released. Resetting target milestone.
Target Milestone: 3.11.1 → ---
Assignee: alvolkov.bgs → emaldona
Target Milestone: --- → 3.14
The problem seems to be the non-existent directory doesn't get detected until we are in the lower levels and the pkcs #11 module returns an error that the higher levels of NSS have a hard time interpreting correctly and thus mapping it in a meaningful way. nss_Init(with all the arguments) is the function that all the NSS_Init, with the expection of NSSInitNoDB, in turn call. We could do some parameter validation there and let the caller know that the path is non-existent.
Comment on attachment 658917 [details] [diff] [review]
Validate configdir path and return error informing caller it's non-existent

From a self-review and some testing
1. Change
+    PORT_SetError(PR_FILE_NOT_FOUND_ERROR); 
to
+/* No need to set the error nspr has already it */
and its actually PR_FILE_NOT_FOUND_ERROR)
2. configdir of NULL or "" are a posibility
3. The patch is incomplete. Modifications to dbtests.sh are needed.

I'm trying to take care solely of handling the case of non existing directories and this bug probably requires more than that. Perhaps a more narraowly focued bug is needed. For the time being I'll submit the revised patch to Bug 788973.
I'll submit the revised patch here as originally planned.
Ensures user gets an accurate error message when a non existing directory path is detected.
Attachment #658917 - Attachment is obsolete: true
Attachment #659575 - Flags: review?(kaie)
Comment on attachment 659575 [details] [diff] [review]
Validate configdir parameter to detect non existent paths: V2

Adding Ryan as Kai may be unavailable these days.
Attachment #659575 - Flags: review?(ryan.sleevi)
Comment on attachment 659575 [details] [diff] [review]
Validate configdir parameter to detect non existent paths: V2

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

I'm possibly a poor person to review this, in that I haven't quite fully grasped NSS's goals regarding "API compatibility" and how much "'feature'-or-bug compatibility" that entails.

It would seem like with this patch, you're forcing a validation of the path that would normally be left up to the individual SECMOD modules, and likely not until the first time they are accessed. For example, given that the sqlite implementation will re-read the sqlite DB every time you attempt to access the store (at least, for non-cached data), it seems like a perfectly valid condition to supply a configdir for a path that doesn't quite exist yet, but this patch prevents that.

That said, it looks correct, and I think the number of people who are relying on such behaviours - especially since trying to init a module without actually having the config dir present may lead to an incomplete init - that the patch is correct, but I kicked it up to wtc just to confirm.

::: mozilla/security/nss/lib/nss/nssinit.c
@@ +335,5 @@
> +dbpath_from_configdir(const char *configdir)
> +{
> +    /* handle supported database modifiers */
> +    if (strncmp(configdir, "sql:", 4) == 0) {
> +    return configdir + 4;

It appears the indentation has been lost here. Could you please ensure it's indented correctly for whatever the local file is (I'm guessing spaces for the if, tab for the return)

@@ +384,5 @@
>      }
>  
> +    if (configdir && configdir[0] != 0) {
> +	const char *dbpath = dbpath_from_configdir(configdir);
> +	if (dbpath && PR_Access(dbpath, PR_ACCESS_EXISTS) == PR_FAILURE) {

I think the choice of PR_ACCESS_EXISTS is fine here, but I'm curious why you didn't go with PR_ACCESS_READ_OK.
Attachment #659575 - Flags: superreview?(wtc)
Attachment #659575 - Flags: review?(ryan.sleevi)
Attachment #659575 - Flags: review+
Status: NEW → ASSIGNED
Comment on attachment 659575 [details] [diff] [review]
Validate configdir parameter to detect non existent paths: V2

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

Elio: thanks for the patch. Your approach seems good, but I think
it requires more thought given the issue that Ryan pointed out.
It would be nice to ask Bob to take a look.

Also, I think we can also fix this bug by improving the error
message for SEC_ERROR_BAD_DATABASE without changing the behavior.
That seems to be what Alexei and Nelson asked for. Perhaps we
can change the error message to "Security library: could not open database".

::: mozilla/security/nss/lib/nss/nssinit.c
@@ +336,5 @@
> +{
> +    /* handle supported database modifiers */
> +    if (strncmp(configdir, "sql:", 4) == 0) {
> +    return configdir + 4;
> +    } else if (strncmp(configdir, "dbm:", 4) == 0) {

Please omit "else" after return statements.

@@ +385,5 @@
>  
> +    if (configdir && configdir[0] != 0) {
> +	const char *dbpath = dbpath_from_configdir(configdir);
> +	if (dbpath && PR_Access(dbpath, PR_ACCESS_EXISTS) == PR_FAILURE) {
> +	    /* the error has already been set by nspr */

Nit: change "nspr" to "PR_Access".

::: mozilla/security/nss/tests/dbtests/dbtests.sh
@@ -88,5 @@
>        html_passed "Certutil didn't work in a nonexisting dir $ret" 
>      fi
>      ${BINDIR}/dbtest -r -d ./non_existent_dir
>      ret=$?
> -    if [ $ret -ne 46 ]; then

Is it important for dbtest to fail with a particular exit status?
Attachment #659575 - Flags: superreview?(wtc) → superreview-
Target Milestone: 3.14 → 3.14.1
Target Milestone: 3.14.1 → 3.14.2
Comment on attachment 659575 [details] [diff] [review]
Validate configdir parameter to detect non existent paths: V2

Clearing the request for my review. I assume we're waiting for Elio to address wtc's comments.
Attachment #659575 - Flags: review?(kaie)
Address Ryan's and Wan-Teh's review comments.
$ certutil -L -d non-existent-dir know results in
certutil: function failed: PR_FILE_NOT_FOUND_ERROR: File not found
Attachment #8356168 - Flags: review?(wtc)
Target Milestone: 3.14.2 → 3.15.5
Comment on attachment 8356168 [details] [diff] [review]
Validate configdir parameter to detect inaccessible paths: V3

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

Thank you for the patch. Most of the comments are just nits.
But please note the two comments marked with "IMPORTANT".

::: lib/nss/nssinit.c
@@ +328,5 @@
>  	return;
>  }
>  
> +static const char *
> +dbpath_from_configdir(const char *configdir)

By our naming convention, this function should be named nss_DBPathFromConfigDir.

@@ +333,5 @@
> +{
> +    /* handle supported database modifiers */
> +    if (strncmp(configdir, "sql:", 4) == 0) {
> +	return configdir + 4;
> +    } 

Nit: the code review tool shows a space character at the end of this line.

@@ +340,5 @@
> +    }
> +    if (strncmp(configdir, "extern:", 7) == 0) {
> +	return configdir + 7;
> +    }
> +    if(strncmp(configdir, "rdb:", 4) == 0) {

Nit: add a space between "if" and "(".

@@ +341,5 @@
> +    if (strncmp(configdir, "extern:", 7) == 0) {
> +	return configdir + 7;
> +    }
> +    if(strncmp(configdir, "rdb:", 4) == 0) {
> +    /* if rdb: is specified, the configdir isn't really a path. Skip it */

Nit: this comment should be aligned with the "return NULL" statement.

@@ +382,5 @@
>  	PORT_SetError(SEC_ERROR_NO_MEMORY);
>  	return rv;
>      }
>  
> +    if (configdir && configdir[0] != 0) {

IMPORTANT: it seems that this test should be performed when we know we are going to open the DB, i.e., noCertDB is false.

For example, if I call NSS_NoDB_Init, I think this test should be skipped.

@@ +384,5 @@
>      }
>  
> +    if (configdir && configdir[0] != 0) {
> +	const char *dbpath = dbpath_from_configdir(configdir);
> +	if (dbpath && PR_Access(dbpath, PR_ACCESS_READ_OK) == PR_FAILURE) {

Would it be enough to just test that the DB directory exists (PR_ACCESS_EXISTS)?

::: tests/dbtests/dbtests.sh
@@ +88,5 @@
>        html_passed "Certutil didn't work in a nonexisting dir $ret" 
>      fi
>      ${BINDIR}/dbtest -r -d ./non_existent_dir
>      ret=$?
> +    if [ $ret -eq 0 ]; then

The original code for line 85 and line 92 seems like a stronger test because we expect a particular exit code, rather than any nonzero exit code. I am not familiar with these tests, so I just wanted to make sure you intended to make these changes.

@@ +97,5 @@
>  
>      Echo "test force opening the database in a nonexisting directory"
>      ${BINDIR}/dbtest -f -d ./non_existent_dir
>      ret=$?
> +    if [ $ret -ne 46 ]; then

IMPORTANT: Why did you change this line to expect a particular exit code?

But more important, changing a zero exit code to mean "test failed" seems wrong. Force open means NSS initialization should succeed even if the DB cannot be opened. I suspect it is because you perform the dbpath existence test at the beginning of nss_InitModules, with no regard to the noCertDB and forceOpen parameter.
I'm still ruminating on the last comment specially about the part on force open. For a moment I think I understand it a few seconds later I am not so sure.
Attachment #8371077 - Flags: review?(wtc)
Attachment #659575 - Attachment is obsolete: true
Attachment #8356168 - Attachment is obsolete: true
Attachment #8356168 - Flags: review?(wtc)
Attachment #8371077 - Attachment is obsolete: true
Attachment #8371077 - Flags: review?(wtc)
Attachment #8397392 - Flags: review?(wtc)
Attachment #8397392 - Flags: review?(rrelyea)
Comment on attachment 8397392 [details] [diff] [review]
Validate configdir parameter to detect inaccessible paths: V5

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

r=wtc. Please note the comment about forceOpen that I marked with
"IMPORTANT".

I will try to come up with an alternative patch, which solves the
problem at the softoken level.

::: lib/nss/nssinit.c
@@ +382,5 @@
>  	PORT_SetError(SEC_ERROR_NO_MEMORY);
>  	return rv;
>      }
>  
> +    if (!noCertDB && configdir && configdir[0] != 0) {

Nit: use
    configdir[0] != '\0'
instead of
    configdir[0] != 0

@@ +384,5 @@
>      }
>  
> +    if (!noCertDB && configdir && configdir[0] != 0) {
> +	const char *dbpath = nss_DBPathFromConfigDir(configdir);
> +	if (dbpath && PR_Access(dbpath, PR_ACCESS_EXISTS) == PR_FAILURE) {

Nit: it's slightly faster to test != PR_SUCCESS than == PR_FAILURE because PR_SUCCESS is 0.

@@ +386,5 @@
> +    if (!noCertDB && configdir && configdir[0] != 0) {
> +	const char *dbpath = nss_DBPathFromConfigDir(configdir);
> +	if (dbpath && PR_Access(dbpath, PR_ACCESS_EXISTS) == PR_FAILURE) {
> +	    /* the error has already been set by PR_Access */
> +	    return rv;

IMPORTANT: I believe it is wrong to return SECFailure here if forceOpen is true. Does the force-open dbtest still pass?

It seems that the test on line 386 should also consider forceOpen,
like this:

    if (!noCertDB && !forceOpen && configdir && configdir[0] != '\0') {
Attachment #8397392 - Flags: review?(wtc) → review+
Please review. I think this is a better approach. However,
it is more work to verify the patch is complete.

Also, is it important to fix this bug? If I get the
SEC_ERROR_BAD_DATABASE error, I can easily check if the
database path name is wrong. Perhaps we should just rename
the error code to "SEC_ERROR_CANNOT_OPEN_DATABASE"?
Attachment #8397517 - Flags: superreview?(rrelyea)
Attachment #8397517 - Flags: review?(emaldona)
Comment on attachment 8397517 [details] [diff] [review]
Alternative approach: let the softoken report the "file not found" error

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

::: lib/pk11wrap/pk11pars.c
@@ +1060,2 @@
>  	    SECMOD_FreeModuleSpecList(module,moduleSpecList);
> +	    PORT_SetError(err);

This change is useful in its own right. I found that
SECMOD_FreeModuleSpecList modifies the error code, so
it is necessary to save and restore the error code
across the SECMOD_FreeModuleSpecList call.

::: lib/softoken/legacydb/pk11db.c
@@ +539,4 @@
>  	 pkcs11db = dbopen( dbName, NO_CREATE, 0600, DB_HASH, 0 );
> +    }
> +    if (pkcs11db) {
> +	(*pkcs11db->sync)(pkcs11db, 0);

Please ignore the changes in this file. They are not necessary
for this bug, and I found that the change here is wrong.
Elio:

I found that the original bug that Alexei reported is not that
the NSS database file is not found. The original bug is that
if a certificate is not found in a good certificate database,
NSS reports the error SEC_ERROR_BAD_DATABASE.

This patch shows where the SEC_ERROR_BAD_DATABASE is set in
the softoken. The softoken NSC_FindObjectsXXX functions
actually succeed, so the SEC_ERROR_BAD_DATABASE error is not
mapped to a CKR_ error code. The problem is that the
PK11_FindCertFromNickname doesn't set an error when the
NSC_FindObjectsXXX functions find nothing, so the residual
SEC_ERROR_BAD_DATABASE error is reported.

I found that the Stan code invoked by PK11_FindCertFromNickname
calls nss_SetError(NSS_ERROR_NOT_FOUND), so I added a
CERT_MapStanError call to PK11_FindCertFromNickname. Then
I ran into the strange problem that NSS_DestroyArena clears
the Stan error stack, so CERT_MapStanError needs to be called
before we call NSS_DestroyArena (indirectly).

Finally, CERT_MapStanError contains a bug (the initial value
of |error| is wrong) and its mapping for NSS_ERROR_NOT_FOUND
is inappropriate (SEC_ERROR_LIBRARY_FAILURE).
Comment on attachment 8397565 [details] [diff] [review]
Patch that illustrates the original bug Alexei reported and a partial fix

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

::: cmd/lib/secutil.c
@@ +3593,4 @@
>          SECItem item = {0, NULL, 0};
>          PRFileDesc* fd = PR_Open(name, PR_RDONLY, 0777); 
>          if (!fd) {
> +            PORT_SetError(err);

This change is required to reproduce the original problem
reported by Alexei, otherwise the failed PR_Open call will
change the error code to PR_FILE_NOT_FOUND_ERROR.

::: lib/certdb/stanpcertdb.c
@@ +190,5 @@
>  			SEC_ERROR_UNKNOWN_ISSUER)
>  	STAN_MAP_ERROR(NSS_ERROR_INVALID_CERTIFICATE, SEC_ERROR_CERT_NOT_VALID)
>  	STAN_MAP_ERROR(NSS_ERROR_INVALID_UTF8, SEC_ERROR_BAD_DATA)
>  	STAN_MAP_ERROR(NSS_ERROR_INVALID_NSSOID, SEC_ERROR_BAD_DATA)
> +	STAN_MAP_ERROR(NSS_ERROR_NOT_FOUND, SEC_ERROR_CRL_NOT_FOUND)

There is no SEC_ERROR_ITEM_NOT_FOUND or SEC_ERROR_CERT_NOT_FOUND
error, so I'm using SEC_ERROR_CRL_NOT_FOUND as a placeholder.
Comment on attachment 8397392 [details] [diff] [review]
Validate configdir parameter to detect inaccessible paths: V5

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

::: lib/nss/nssinit.c
@@ +382,5 @@
>  	PORT_SetError(SEC_ERROR_NO_MEMORY);
>  	return rv;
>      }
>  
> +    if (!noCertDB && configdir && configdir[0] != 0) {

Elio:

Given my new understanding of what this bug is about, I now suggest
that you exclude this change (lines 386-393) from the patch and check
in the rest of the patch, which consists of cleanup and refactoring.

I think it is not necessary to report a more fine-grained error code
than SEC_ERROR_BAD_DATABASE for a nonexistent cert or key database,
unless you have got this request from your customers.

I think the original bug reported by Alexei is more important to fix,
and you can use my patch (attachment 8397565 [details] [diff] [review]) as a starting point.
Comment on attachment 8397392 [details] [diff] [review]
Validate configdir parameter to detect inaccessible paths: V5

r- I like wtc's alternative. We already have too much knowledge about what softoken is doing at the NSS init layer. I prefer to not expand that knowledge if we can help it.
Attachment #8397392 - Flags: review?(rrelyea) → review-
Comment on attachment 8397517 [details] [diff] [review]
Alternative approach: let the softoken report the "file not found" error

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

::: lib/pk11wrap/pk11pars.c
@@ +1045,5 @@
>  		}
>  		child = SECMOD_LoadModule(*index,module,PR_TRUE);
>  		if (!child) break;
>  		if (child->isCritical && !child->loaded) {
> +		    err = PORT_GetError();

Hmm if we aren't checking the error from either SECMOD_DestroyModule() or SECMOD_FreeModuleSpecList() maybe we should be saving the error code inside the functions themselves.

I wonder if Softoken is messing us up here (we don't expect C_ calls to change the value of err, but softoken does, since it uses PORT_SetError and PORT_GetError internally...
Attachment #8397517 - Flags: superreview?(rrelyea) → superreview+
Comment on attachment 8397517 [details] [diff] [review]
Alternative approach: let the softoken report the "file not found" error

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

::: lib/pk11wrap/pk11pars.c
@@ +1045,5 @@
>  		}
>  		child = SECMOD_LoadModule(*index,module,PR_TRUE);
>  		if (!child) break;
>  		if (child->isCritical && !child->loaded) {
> +		    err = PORT_GetError();

Bob Relyea wrote:
> Hmm if we aren't checking the error from either SECMOD_DestroyModule() or
> SECMOD_FreeModuleSpecList() maybe we should be saving the error code inside the
> functions themselves.

That's an interesting idea. SECMOD_DestroyModule returns void,
so it is definitely OK for SECMOD_DestroyModule to save and
restore the error code.

> I wonder if Softoken is messing us up here (we don't expect C_ calls to change the value
> of err, but softoken does, since it uses PORT_SetError and PORT_GetError internally...

In the case of SECMOD_FreeModuleSpecList, I believe softoken's
module DB function needs to call lib/util's module DB function
to determine if the database is the "legacy database". lib/util's
module DB function uses the error code SEC_ERROR_LEGACY_DATABASE
to convey that info. We can try modifying softoken's module DB
function so that it saves and restores the error code across
the call to lib/util's module DB function.
In this revision of the patch, I improved my changes in
lib/softoken/legacydb/pk11db.c slightly.

I carried forward Bob's superreview+ on this patch. However,
unless Elio received a separate request from his customers to
report a more fine-grained error ("file not found" instead of
"bad security database"), I won't check this in.
Attachment #8397517 - Attachment is obsolete: true
Attachment #8397517 - Flags: review?(emaldona)
Attachment #8400139 - Flags: superreview+
Thank you Wan-Teh for that effort. I haven't received any request from any Red Hat customers. It's just me and possibly Ales may have brought it up to me as well. The "bad security database" is annoying and misleading until after a while one realises that it was because one had provided the wrong path to the configuration directory which ended up in a non-existent directory.
Severity: minor → S4

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Assignee: elio.maldonado.batiz → nobody
Status: ASSIGNED → NEW
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: