Last Comment Bug 641052 - (CVE-2011-3640) NSS_NoDB_Init should not try to open /pkcs11.txt and /secmod.db
(CVE-2011-3640)
: NSS_NoDB_Init should not try to open /pkcs11.txt and /secmod.db
Status: RESOLVED FIXED
[sg:moderate]
:
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: unspecified
: All All
: P1 normal (vote)
: 3.13
Assigned To: Wan-Teh Chang
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-03-11 12:08 PST by Wan-Teh Chang
Modified: 2011-10-27 12:05 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Proposed patch: make NSS_NoDB_Init try to open "" and "" instead of "/pkcs11.txt" and "/secmod.db" (3.52 KB, patch)
2011-09-23 15:35 PDT, Wan-Teh Chang
rrelyea: review-
Details | Diff | Review
Update wtc's patch to use (4.47 KB, patch)
2011-09-30 15:39 PDT, Robert Relyea
no flags Details | Diff | Review
Patch v3 (4.60 KB, patch)
2011-10-02 10:15 PDT, Wan-Teh Chang
no flags Details | Diff | Review

Description Wan-Teh Chang 2011-03-11 12:08:25 PST
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 2011-09-23 15:35:10 PDT
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).
Comment 2 Wan-Teh Chang 2011-09-23 15:40:22 PDT
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 2011-09-28 17:16:01 PDT
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 2011-09-30 13:10:09 PDT
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 2011-09-30 15:39:42 PDT
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.
Comment 6 Wan-Teh Chang 2011-10-02 10:15:30 PDT
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
Comment 7 Robert Relyea 2011-10-04 13:44:57 PDT
> 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 2011-10-04 14:06:04 PDT
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] (use needinfo?) 2011-10-24 03:44:00 PDT
dveditz, can you assign a CVE?
Comment 10 Robert Relyea 2011-10-24 09:51:40 PDT
> 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

Note You need to log in before you can comment on or make changes to this bug.