The default bug view has changed. See this FAQ.
Bug 641052 (CVE-2011-3640)

NSS_NoDB_Init should not try to open /pkcs11.txt and /secmod.db

RESOLVED FIXED in 3.13

Status

NSS
Libraries
P1
normal
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Wan-Teh Chang, Assigned: Wan-Teh Chang)

Tracking

unspecified
3.13

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:moderate])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
Created attachment 562178 [details] [diff] [review]
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).
Assignee: nobody → wtc
Status: NEW → ASSIGNED
Attachment #562178 - Flags: review?(rrelyea)
(Assignee)

Comment 2

6 years ago
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.
(Assignee)

Updated

6 years ago
Priority: P2 → P1
Target Milestone: --- → 3.13

Comment 3

6 years ago
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

6 years ago
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
Attachment #562178 - Flags: review?(rrelyea) → review-

Comment 5

6 years ago
Created attachment 563870 [details] [diff] [review]
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.
Attachment #562178 - Attachment is obsolete: true
Attachment #563870 - Flags: review?(wtc)
(Assignee)

Comment 6

6 years ago
Created attachment 564058 [details] [diff] [review]
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
Attachment #563870 - Attachment is obsolete: true
Attachment #563870 - Flags: review?(wtc)
(Assignee)

Updated

6 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Comment 7

6 years ago
> 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
(Assignee)

Comment 8

6 years ago
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?
dveditz, can you assign a CVE?
Whiteboard: [sg:moderate]

Comment 10

6 years ago
> 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
Alias: CVE-2011-3640
You need to log in before you can comment on or make changes to this bug.