Last Comment Bug 168309 - certutil fails with list everything in the hardware token
: certutil fails with list everything in the hardware token
Status: RESOLVED FIXED
:
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: 3.4.2
: x86 Windows 2000
: P1 normal (vote)
: 3.4.3
Assigned To: Ian McGreer
: Bishakha Banerjee
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2002-09-12 13:05 PDT by thomask
Modified: 2002-09-17 15:24 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch for 3.4 branch (1.03 KB, patch)
2002-09-13 07:12 PDT, Ian McGreer
wtc: review+
Details | Diff | Splinter Review
alternative patch - remove the maximum (739 bytes, patch)
2002-09-13 11:19 PDT, Ian McGreer
bugz: review+
Details | Diff | Splinter Review
Remove the unused macro definition (924 bytes, patch)
2002-09-13 15:11 PDT, Wan-Teh Chang
no flags Details | Diff | Splinter Review

Description thomask 2002-09-12 13:05:03 PDT
In pki3hack.c

cache_token_cert(NSSCertificate *c, void *arg)
{
    NSSToken *token = (NSSToken *)arg;
    NSSTrustDomain *td = STAN_GetDefaultTrustDomain();
    NSSCertificate *cp = nssCertificate_AddRef(c);
    if (nssList_Count(token->certList) > NSSTOKEN_MAX_LOCAL_CERTS) {
        nssToken_DestroyCertList(token, PR_TRUE);
       /* terminate the traversal */
       return PR_FAILURE;
    }

When NSS is used with certutil to list certificates in hardware
token, certutil fails to list everything when there is more than
NSSTOKEN_MAX_LOCAL_CERTS.

it is because the local cert got destroyed.
Comment 1 Wan-Teh Chang 2002-09-12 16:12:20 PDT
Assigned the bug to Ian.
Comment 2 Ian McGreer 2002-09-13 07:12:43 PDT
Created attachment 99053 [details] [diff] [review]
patch for 3.4 branch


This cert cache code was replaced in NSS 3.5+, so this patch is only for the
3.4 branch.
Comment 3 Wan-Teh Chang 2002-09-13 10:28:33 PDT
Comment on attachment 99053 [details] [diff] [review]
patch for 3.4 branch

r=wtc.	Bob, could you review this patch too?
Comment 4 thomask 2002-09-13 10:55:10 PDT
Applied the patch to my tree, and it core dumped:

(/tools/ns/workshop-6.2/bin/../WS6U2/bin/sparcv9/dbx) where
current thread: t@1
=>[1] nssList_Count(list = (nil)), line 310 in "list.c"
  [2] cache_token_cert(c = 0x3436c0, arg = 0x2ceaa8), line 115 in "pki3hack.c"
  [3] retrieve_cert(t = 0x2ceaa8, session = 0x2ceaf0, h = 2181039210U, arg =
0xffbef7f8), line 573 in "devobject.c"
  [4] traverse_objects_by_template(tok = 0x2ceaa8, sessionOpt = (nil),
obj_template = 0xffbef76c, otsize = 2U, callback = 0x179118 =
&`certutil`devobject.c`retrieve_cert(struct NSSTokenStr *t, struct nssSessionStr
*session, CK_OBJECT_HANDLE h, void *arg), arg = 0xffbef7f8), line 267 in
"devobject.c"
  [5] nssToken_TraverseCertificates(token = 0x2ceaa8, sessionOpt = (nil), search
= 0xffbef7f8), line 610 in "devobject.c"
  [6] nssToken_LoadCerts(token = 0x2ceaa8), line 269 in "pki3hack.c"
  [7] STAN_LoadDefaultNSS3TrustDomain(), line 378 in "pki3hack.c"
  [8] nss_Init(configdir = 0x2a7160 ".", certPrefix = 0x28d4a8 "", keyPrefix =
0x28d4a8 "", secmodName = 0x262210 "secmod.db", readOnly = 1, noCertDB = 0,
noModDB = 0, forceOpen = 0, noRootInit = 0), line 454 in "nssinit.c"
  [9] NSS_Initialize(configdir = 0x2a7160 ".", certPrefix = 0x28d4a8 "",
keyPrefix = 0x28d4a8 "", secmodName = 0x262210 "secmod.db", flags = 1U), line
512 in "nssinit.c"
  [10] main(argc = 6, argv = 0xffbefb44), line 2650 in "certutil.c"
(/tools/ns/workshop-6.2/bin/../WS6U2/bin/sparcv9/dbx)


Then, I put 

static PRStatus
cache_token_cert(NSSCertificate *c, void *arg)
{
    NSSToken *token = (NSSToken *)arg;
    NSSTrustDomain *td = STAN_GetDefaultTrustDomain();
    NSSCertificate *cp = nssCertificate_AddRef(c);
 +   if (token->certList == 0) 
 +     return PR_FAILURE;
    if (nssList_Count(token->certList) > NSSTOKEN_MAX_LOCAL_CERTS) {
        nssToken_DestroyCertList(token, PR_TRUE);

Then, it core dumped in


(/tools/ns/workshop-6.2/bin/../WS6U2/bin/sparcv9/dbx) where
current thread: t@1
=>[1] nssList_Clone(list = (nil)), line 337 in "list.c"
  [2] nssList_CreateIterator(list = (nil)), line 364 in "list.c"
  [3] nssToken_UpdateTrustForCerts(token = 0x2cead8), line 301 in "pki3hack.c"
  [4] pk11_CheckPassword(slot = 0x321828, pw = 0x2fdc08 "userpin"), line 681 in
"pk11slot.c"
  [5] PK11_DoPassword(slot = 0x321828, loadCerts = 1, wincx = 0xffbefa8c), line
1158 in "pk11slot.c"
  [6] PK11_Authenticate(slot = 0x321828, loadCerts = 1, wincx = 0xffbefa8c),
line 853 in "pk11slot.c"
  [7] listCerts(handle = 0x2ce820, name = (nil), slot = 0x321828, raw = 0, ascii
= 0, outfile = 0x2abc70, pwarg = 0xffbefa8c), line 646 in "certutil.c"
  [8] ListCerts(handle = 0x2ce820, name = (nil), slot = 0x321828, raw = 0, ascii
= 0, outfile = 0x2abc70, pwdata = 0xffbefa8c), line 703 in "certutil.c"
  [9] main(argc = 6, argv = 0xffbefb44), line 2680 in "certutil.c"
(/tools/ns/workshop-6.2/bin/../WS6U2/bin/sparcv9/dbx)
Comment 5 Ian McGreer 2002-09-13 11:04:58 PDT
Thomas-

Did you do a clean rebuild?  Dependencies don't work at all in NSS 3.4, and
certutil is linked statically.
Comment 6 Ian McGreer 2002-09-13 11:10:43 PDT
Nevermind, I see the problem now.  The hardware token is not friendly, correct?
 (that is, you must login to see the certs)
Comment 7 Ian McGreer 2002-09-13 11:16:16 PDT
The second crash you reported is due to the fact that some code is assuming that
all hardware token certs are cached.

I could fix that piece of code, but I'm worried about how many other parts of
the code make similar assumptions.  I believe it has not been a problem to date
because we haven't tested with a device that has more than 10 certs (the current
maximum).

So, another less risky way to fix this bug is to remove the maximum, so that
hardware certs are always cached.  Provided we aren't faced with a hardware
token that has millions of certs, that shouldn't be a problem.
Comment 8 Ian McGreer 2002-09-13 11:19:05 PDT
Created attachment 99086 [details] [diff] [review]
alternative patch - remove the maximum


This patch completely removes the limit of cached token certs.
Comment 9 thomask 2002-09-13 12:51:14 PDT
Yes, I agree that removes the limit is a less risky fix. I did try to
change the limit to 100, and it fixed the problem. Thanks for your
help Ian!
Comment 10 Ian McGreer 2002-09-13 13:40:09 PDT
Comment on attachment 99086 [details] [diff] [review]
alternative patch - remove the maximum


patch checked in, reviewed by wtc over IRC
Comment 11 Ian McGreer 2002-09-13 13:41:20 PDT
marking fixed per comment #9 (leaving milestone open, should be 3.4.3)
Comment 12 Wan-Teh Chang 2002-09-13 14:02:43 PDT
The definition of the NSSTOKEN_MAX_LOCAL_CERTS macro
should also be removed.

Thanks, Ian!
Comment 13 Wan-Teh Chang 2002-09-13 15:11:01 PDT
Created attachment 99139 [details] [diff] [review]
Remove the unused macro definition

I've checked in this patch on the NSS_3_4_BRANCH.
Comment 14 Robert Relyea 2002-09-17 15:24:19 PDT
We should test 3.6 with a token with more than 10 certs. I would rather fix the
problems we find there than remove the limit.

bob

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