Closed
Bug 641052
(CVE-2011-3640)
Opened 13 years ago
Closed 13 years ago
NSS_NoDB_Init should not try to open /pkcs11.txt and /secmod.db
Categories
(NSS :: Libraries, defect, P1)
NSS
Libraries
Tracking
(Not tracked)
RESOLVED
FIXED
3.13
People
(Reporter: wtc, Assigned: wtc)
Details
(Whiteboard: [sg:moderate])
Attachments
(1 file, 2 obsolete files)
4.60 KB,
patch
|
Details | Diff | Splinter Review |
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•13 years ago
|
||
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 | ||
Comment 2•13 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•13 years ago
|
Priority: P2 → P1
Target Milestone: --- → 3.13
Comment 3•13 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•13 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•13 years ago
|
||
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•13 years ago
|
||
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•13 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Comment 7•13 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•13 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?
Comment 10•13 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
Updated•13 years ago
|
Alias: CVE-2011-3640
You need to log in
before you can comment on or make changes to this bug.
Description
•