641052 - (CVE-2011-3640) NSS_NoDB_Init should not try to open /pkcs11.txt and /secmod.db

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Wan-Teh Chang]

Although NSS_NoDB_Init takes a 'configdir' argument, it ignores
the argument:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/nss/nssinit.c&rev=1.106&mark=838#834

A coworker of mine ran strace on a simple program that calls
NSS_NoDB_Init(NULL).  strace showed that NSS_NoDB_Init tries
to open /pkcs11.txt and /secmod.db.

NSS_NoDB_Init should not try to open /pkcs11.txt and /secmod.db
because those are not the intended file paths.

Comment 1

[Wan-Teh Chang]

Attachment: Proposed patch: make NSS_NoDB_Init try to open "" and "" instead of "/pkcs11.txt" and "/secmod.db"

Bob, please review this patch.

This bug was rediscovered and reported in Chromium bug 97426:
http://code.google.com/p/chromium/issues/detail?id=97426

This patch makes NSS_NoDB_Init try to open "" and "" (empty
string pathnames) instead of "/pkcs11.txt" and "/secmod.db".
Ideally NSS_NoDB_Init should not try to open the pkcs11 and
secmod databases at all, but I don't know the code well enough
to do that.

The patch does two things:
1. Make sftk_getSecmodName return "" and "" as the pkcs11
and secmod database pathnames if the noModDB flag is specified.
2. Make sftk_getOldSecmodName work correctly if dbname does
not contain any path separator (i.e., it contains just a file
name or is an empty string).

Comment 2

[Wan-Teh Chang]

Comment on attachment 562178 [details] [diff] [review]
Proposed patch: make NSS_NoDB_Init try to open "" and "" instead of "/pkcs11.txt" and "/secmod.db"

Two changes in this patch need extra explanation.

>-#ifdef WINDOWS
>+#ifdef _WIN32

coreconf/WIN32.mk defines _WINDOWS, not WINDOWS.
But I use _WIN32 for consistency (used elsewhere
in the same file).

>-   if (lconfigdir) {
>+   if (noModDB) {
>+	value = PR_smprintf("");
>+   } else if (lconfigdir && lconfigdir[0] != '\0') {
> 	value = PR_smprintf("%s" PATH_SEPARATOR "%s",lconfigdir,secmodName);
>    } else {
> 	value = PR_smprintf("%s",secmodName);
>    }

For a NSS_NoDB_Init(NULL) call, lconfigdir is "" as opposed to NULL.
(This is why we ended up with "/pkcs11.txt".)   So we also need to
test for an empty string.

Comment 3

[Robert Relyea]

Ah... We handle the regular databases correctly, but we don't handle secomd/pkcs11.txt...

I would go ahead and return NULL for the filename and value from sftk_getSecmodName(), then check dbname for NULL in each of the following entry points (before the legacy DB call):


sftkdb_DeleteSecmodDB - return SECFailure
sftkdb_AddSecmodDB - return SECFailure
sftkdb_ReleaseSecmodDBData - skip the legacy if and continue.. (I.E.

-if ((dbType == SDB_LEGACY) || (dbType == SDB_MULTIACCESS)) {
+if ((dbname != NULL) && ((dbType == SDB_LEGACY) || (dbType == SDB_MULTIACCESS))) {

sftkdb_ReadSecmodDB - skip the entire function to the if which reads...
    if (!moduleList[0]) {


--------------------------------

Other comments on the patch..
+    /* XXX is this a good idea? when we can't open a sql: cert database,
+     * do we also look for the old cert database? */

yes, it's required. The semantics of sql: is to fall back to the oldDB it the sql db doesn't exist and the old one does. If the DB was open rw, the database is updated.

For the NSS_NoDB_Init() case, however, we aren't going to open any of these, so we need to skip past this check in that case.

The rest of the patch seems fine...

Comment 4

[Robert Relyea]

Comment on attachment 562178 [details] [diff] [review]
Proposed patch: make NSS_NoDB_Init try to open "" and "" instead of "/pkcs11.txt" and "/secmod.db"

r- see comments in comment 3
The patch is pretty close.

bob

Comment 5

[Robert Relyea]

Attachment: Update wtc's patch to use

wtc, you can r+ and check in this version, or replace it with your own and request a review from me.

Comment 6

[Wan-Teh Chang]

Attachment: Patch v3

Bob, I've reviewed and tested your patch.  Thanks a lot!
The only change I made is to reformat the comment block.

For sftkdb_DeleteSecmodDB and sftkdb_AddSecmodDB, I assume
it is an error for dbname to be NULL, so it would be
inappropriate for these functions to be a no-op and return
SECSuccess.  Should they set an error code before returning
SECFailure?

Patch checked in on the NSS trunk (NSS 3.13).

Checking in sftkmod.c;
/cvsroot/mozilla/security/nss/lib/softoken/sftkmod.c,v  <--  sftkmod.c
new revision: 1.9; previous revision: 1.8
done
Checking in sftkpars.c;
/cvsroot/mozilla/security/nss/lib/softoken/sftkpars.c,v  <--  sftkpars.c
new revision: 1.12; previous revision: 1.11
done

Comment 7

[Robert Relyea]

> For sftkdb_DeleteSecmodDB and sftkdb_AddSecmodDB, I assume
> it is an error for dbname to be NULL, so it would be
> inappropriate for these functions to be a no-op and return
> SECSuccess.  Should they set an error code before returning
> SECFailure?

yes

Comment 8

[Wan-Teh Chang]

Bob: which error code do you recommend?  SEC_ERROR_LIBRARY_FAILURE
(because this seems like a programming error) or SEC_ERROR_INVALID_ARGS?
Or SEC_ERROR_JS_DEL_MOD_FAILURE and SEC_ERROR_JS_ADD_MOD_FAILURE?

Comment 9

[Reed Loden [:reed]]

dveditz, can you assign a CVE?

Comment 10

[Robert Relyea]

> Bob: which error code do you recommend?  SEC_ERROR_LIBRARY_FAILURE
> (because this seems like a programming error) or SEC_ERROR_INVALID_ARGS?
> Or SEC_ERROR_JS_DEL_MOD_FAILURE and SEC_ERROR_JS_ADD_MOD_FAILURE?

SEC_ERROR_INVALID_ARGS

bob