233932 - certutil -T crashes if -h <token> specifies a nonexistant token

Status: RESOLVED FIXED

(NSS :: Tools, defect, P2)

Description

[Wan-Teh Chang]

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.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 2

[Wan-Teh Chang]

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.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Narrowing summary to match actual reported error.

Comment 4

[Julien Pierre]

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

Comment 5

[Julien Pierre]

Retargetting all P2s to 3.11.3 .

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

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

Comment 7

[Biswatosh Chakraborty]

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.

Comment 8

[Biswatosh Chakraborty]

Attachment: not for review, a patch to stop the crash.

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.

Comment 9

[Biswatosh Chakraborty]

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.
 

Comment 10

[Biswatosh Chakraborty]

Attachment: For review. The same as the earlier patch except some indentation.

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.

Comment 11

[Biswatosh Chakraborty]

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

Comment 12

[Neil Williams]

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 13

[Neil Williams]

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 14

[Biswatosh Chakraborty]

Attachment: Not for review. Implements the suggestion in Comment #13

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

Comment 15

[Biswatosh Chakraborty]

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.

Comment 16

[Biswatosh Chakraborty]

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.

Comment 17

[Biswatosh Chakraborty]

Attachment: For review. It is a small enhancement on the earlier patch(271512)

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.

Comment 18

[Nelson Bolyard (seldom reads bugmail)]

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, [e-mail], 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 19

[Nelson Bolyard (seldom reads bugmail)]

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 20

[Robert Relyea]

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

Comment 21

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 22

[Biswatosh Chakraborty]

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

Comment 23

[Biswatosh Chakraborty]

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
 

Comment 24

[Nelson Bolyard (seldom reads bugmail)]

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 25

[Neil Williams]

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

Good.

Comment 26

[Biswatosh Chakraborty]

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

Comment 27

[Biswatosh Chakraborty]

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. 

Comment 28

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 29

[Biswatosh Chakraborty]

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.

Comment 30

[Biswatosh Chakraborty]

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