This bug was originally reported as Bugscape bug 55713. Several certutil commands accept a "-h <token>" argument. However, if the token specified is not present in the secmod.db specified (note that one will be created if it does not already exist in the specified directory), a Segmentation Violation will occur. A specific example of this behavior occurred on Red Hat Linux AS 2.1: % certutil -T -d . -h foobar % Segmentation fault This is due to the fact that the return value from the call "PK11_FindSlotByName()" is not checked to see if it is null. The following *.c files under nss/cmds use PK11_FindSlotByName: ./certutil/certutil.c: slot = PK11_FindSlotByName(slotname); ./modutil/pk11.c: slot = PK11_FindSlotByName(tokenName); ./pk12util/pk12util.c: slot = PK11_FindSlotByName(slotname); ./signtool/certgen.c: slot = PK11_FindSlotByName(token); ./tests/remtest.c: slot = PK11_FindSlotByName(tokenName); ./symkeyutil/symkeyutil.c: slot = PK11_FindSlotByName(slotname); ./symkeyutil/symkeyutil.c: target = PK11_FindSlotByName(targetName); Only certutil/certutil.c and symkeyutil/symkeyutil.c (the first call to PK11_FindSlotByName) fail to check the return value of PK11_FindSlotByName.

I'd like to ask for a little more diagnosis of the problem before it is fixed. Perhaps SOME paths in certutil crash when -h token sepecifies a non-existant token, but not all paths crash. I'd like to see the specific certutil commands that crash be enumerated, and only those commands fixed. In particular, I would not want to cert certutil changed so that it NEVER allowed the -h argument to specify the name of a nonexisted token, because that would break commands like the following: certutil -L -d dbdir -h all certutil -L -d dbdir -h nosuchtoken Each of those commands lists the contents of all tokens, without crashing, even though no "all" token and no "nosuchtoken" exists. The code does not special case the word "all" or the word "nosuchtoken". Any non-existed token name will suffice to get certutil -L to list the contents of all tokens. So, I want the fix to be specific enough to the problem that it eliminates the crash without stopping other desirable behavior, such as that I stated above, from working.

Good point, Nelson. I found that in certutil.c, the ListCerts and ListKeys functions both treat a null 'slot' argument as meaning "all slots". If we blindly test the return value of PK11_FindSlotByName for null, we will break the support of -h all in some commands.

Narrowing summary to match actual reported error.

*** Bug 339179 has been marked as a duplicate of this bug. ***

Retargetting all P2s to 3.11.3 .

ALexei, please fix this bug. Note comment 1 & comment 2.

The options of certutil that can give this crash with a non-existent token are: 1)certutil -T (as reported already in this bug description) 2)certutil -A (see Bug 339179) or -E or -S 3)certutil -W 4)certutil -N certutil -T calls PK11_ResetToken(slot,sso_pass) and this function straightaway goes to access a member of slot structure(PK11SlotInfo) certutil -A (or -E or -S) calls AddCert() which again calls PK11_IsFriendly(slot) and this last function again tries to access a slot member without checking. certutil -W calls SECU_ChangePW() which calls PK11_NeedUserInit(slot) and this last function tries to access one member of slot. certutil -N calls SECU_ChangePW() and crashes when a null slot is passed. So, I plan to address these particular options only and check for NULL before actually calling the internal functions. This can assure that (1)other paths are not disturbed and (2)only the paths where crash occurs due to a null slot, will not give crashes anymore.

This modifies certutil.c only and checks for slot as NULL only in those cases, where there comes a crash. So,for e.g. "certutil -L -h bogustoken" works as before.If this approach seems OK, my patch for review will not be much different.

One more option (-M) can make certutil crash. I did not mention that in Comment #7 although the patch in Comment #8 covers that case. I do not see any other option that can make certutil crash besides what I have already mentioned.

This patch solves the problems mentioned in comment #7 and comment #8. Hence, all other paths where a wrong slot still did not give a crash, remain untouched. The patch inserts null check for slot only for those options, which can give a crash with a wrong token passed with -h option.

Hence, this patch covers only these options for certutil, namely -T, -W, -M, -N, -A, -E and -S.

Comment on attachment 270507 [details] [diff] [review] For review. The same as the earlier patch except some indentation. Your patch is functionally correct but I'd like to suggest some changes to make it more compact. Check -N, -M, -T, -W right after the call to PK11_FindSlotByName. There are several other places in certutil where parameters in common to several commands are checked for all those commands at one place. Print the error string and the slot name in the error message--you could use SECU_PrintError--certutil has so many options that it's easy to use the wrong letter.

Comment on attachment 270507 [details] [diff] [review] For review. The same as the earlier patch except some indentation. Your patch is functionally correct but I'd like to suggest some changes to make it more compact. Check -N, -M, -T, -W right after the call to PK11_FindSlotByName. There are several other places in certutil where parameters in common to several commands are checked for all those commands at one place. Print the error string and the slot name in the error message--you could use SECU_PrintError--certutil has so many options that it's easy to use the wrong letter.

This is just to show the status. Code is more compact and checks for the options right after the call to PK11_FindSlotByName().

Two observations: (1)certutil -T -d db crashes with new NSS lib but does not crash with the older implemenation. (2)The problem described in the bug is not exactly same as point (1). It says certutil -T -d . -h falsetoken crashes. This happens both in new and old NSS. Analysis: (1) happens because of an ommission in nss/lib/softoken/legacydb/lginit.c. There, sdb->sdb_Reset = lg_Reset is not being done. (2) can easily be solved by checking for NULL in certutil.c For details, read on: I had thought that the problem lies only within certutil but upon analysing the crash of certutil -T -d db, it appears that there is a problem in the function lg_init(), present in nss/lib/softoken/legacydb/lginit.c. There, sdb->sdb_Reset *is not being* set to lg_Reset unlike what is being done for other members of sdb, for example for sdb_Close, sdb_Abort etc. And, that was the reason for certutil -T -db giving a crash. I modified lg_init() to set sdb->sdb_Reset = lg_Reset and compiled and ran the same command. It did not give the crash. I am talking here of NSS checked out from HEAD Branch. But,the earlier NSS( say a few months old. And, http://lxr.mozilla.org/seamonkey/source/security/nss/lib/softoken/pkcs11.c still shows the earlier implementation), had a different implementation of NSC_InitToken() present in nss/lib/softoken/pkcs11.c and did not give a crash to certutil -T -d db. However, it gave then and gives now crash for "certutil -T -d db -h falsetoken". And, that is because we are not checking for NULL when we pass a slot to functions in case of some options in certutil.c. Thus,the difference is: in the first case, slot was "internal" and in the other, slot is NULL. If it is NULL, certutil is going to filter it for some options(-T,-W,-M,-N,-A,-E,-S) and if it is internal, the functions called will have to take care. I had thought first to raise a separate bug for this issue(that is for lginit.c) and make this bug(233932) dependent on this new bug. But, I think I will give a patch which will more or less be same as the earlier patch but now would include the change in lginit.c.

More on Comment #15 , Running "$ dbx certutil core" upon getting crash from certutil -T -d gives: t@null (l@1) program terminated by signal 0 0xffffffff: <bad address 0xffffffff> Current function is sftkdb_ResetDB 722 crv = (*db->sdb_Reset)(db); (dbx 1) up Current function is sftkdb_ResetKeyDB 2549 crv = sftkdb_ResetDB(handle); (dbx 2) up Current function is NSC_InitToken 2885 rv = sftkdb_ResetKeyDB(handle); (dbx 3) up Current function is PK11_ResetToken 2235 (unsigned char *)sso_pwd, sso_pwd ? PORT_Strlen(sso_pwd): 0, tokenName); (dbx 4) up Current function is certutil_main And: Current function is sftkdb_ResetDB 722 crv = (*db->sdb_Reset)(db); (dbx 10) p *db *db = { private = 0x80b1ad0 version = -606348325 sdb_type = SDB_LEGACY sdb_flags = 4 app_private = 0x80b9f70 sdb_FindObjectsInit = 0xce221ff0 = &lg_FindObjectsInit() sdb_FindObjects = 0xce222100 = &lg_FindObjects() sdb_FindObjectsFinal = 0xce2221d0 = &lg_FindObjectsFinal() sdb_GetAttributeValue = 0xce21d300 = &lg_GetAttributeValue() sdb_SetAttributeValue = 0xce21dda0 = &lg_SetAttributeValue() sdb_CreateObject = 0xce21fbc0 = &lg_CreateObject() sdb_DestroyObject = 0xce21fed0 = &lg_DestroyObject() sdb_GetPWEntry = 0xce219a50 = &lg_GetPWEntry() sdb_PutPWEntry = 0xce219ae0 = &lg_PutPWEntry() sdb_Begin = 0xce222a80 = &lg_Begin() sdb_Commit = 0xce222b10 = &lg_Commit() sdb_Abort = 0xce222ba0 = &lg_Abort() sdb_Reset = 0xdbdbdbdb sdb_Close = 0xce223130 = &lg_Close() } So, as you see here, sdb_Reset has not been set to any function.

The extra addition to this patch is a change done in nss/lib/softoken/legacydb/lginit.c. See Comment #15 for details. Thus, this patch is more compact compared to the attachment in Comment #10 and primarily touches certutil.c where it checks for NULL for some options(-T,-W,-M,-N,-A,-E,-S) and adds a single line (sdb->sdb_Reset = lg_Reset) in lginit.c. This is to avoid crash for certutil -T -d db.

Biswatosh, Please file a separate bug about the problem you found in lginit.c, and please attach your patch for that one file to that bug. Please ask Bob Relyea, rrelyea@redhat.com, to review it. I think that was a good find, and that patch should get r+ easily. Your patch to certutil looks about right to me. But I'm going to let Neil review it.

Comment on attachment 271987 [details] [diff] [review] For review. It is a small enhancement on the earlier patch(271512) Bob, Please review the part of this patch that affects lginit.c. Please see prior comments in this bug for an analysis of the problem.

Comment on attachment 271987 [details] [diff] [review] For review. It is a small enhancement on the earlier patch(271512) r+ for the lginit changes. We should probably add a reset test to all.sh as well. bob

Biswatosh, Please go ahead and commit the change to lginit.c on the trunk now. You can forget my request in comment 18 to file another bug.

Committed lginit.c to the HEAD with the single line change ,namely sdb->sdb_Reset = lg_Reset in lg_init() Checking in lginit.c; /cvsroot/mozilla/security/nss/lib/softoken/legacydb/lginit.c,v <-- lginit.c new revision: 1.8; previous revision: 1.7 done

Neil, In the patch that I gave for review, the lg_init.c part is now already committed (as Bob had given a r+ on that) so only the certutil.c part need be reviewed. Thanks

Biswatosh, Thank you for committing the fix to lginit.c. In all future checkin comments, be sure to include the bug number, along with the reviewer information, e.g. "Bug 123456 r=nelson"

Comment on attachment 271987 [details] [diff] [review] For review. It is a small enhancement on the earlier patch(271512) Good.

Commited change in certutil.c to the HEAD. Checking in certutil.c; /cvsroot/mozilla/security/nss/cmd/certutil/certutil.c,v <-- certutil.c new revision: 1.114; previous revision: 1.113 done

Do I need a super review for commiting this patch to the branch as well? Since, Neil, Nelson and Bob looked at the patch(Bob reviewed one file), I guess I can go ahead in commiting.

Biswatosh, within the NSS community, we don't make any distinction between normal review and super review. We say you need two reviews to commit on the branch. That can be a review and a super review, or two reviews. You have two reviews now, so go ahead.

Commiting certutil.c to the branch NSS_3_11_BRANCH ... Checking in certutil.c; /cvsroot/mozilla/security/nss/cmd/certutil/certutil.c,v <-- certutil.c new revision: 1.97.2.10; previous revision: 1.97.2.9 done The patch for softoken/legacydb/lginit.c is not applicable in this branch and hence not committed. It is not applicable because this legacydb is not implemented in the branch.

Closing this bug as the patch has been reviewed and commited to the head and to the branch.