168309 - certutil fails with list everything in the hardware token

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[thomask]

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]

Assigned the bug to Ian.

Comment 2

[Ian McGreer]

Attachment: 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]

Comment on attachment 99053 [details] [diff] [review]
patch for 3.4 branch

r=wtc.	Bob, could you review this patch too?

Comment 4

[thomask]

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]

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]

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]

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]

Attachment: alternative patch - remove the maximum

This patch completely removes the limit of cached token certs.

Comment 9

[thomask]

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]

Comment on attachment 99086 [details] [diff] [review]
alternative patch - remove the maximum


patch checked in, reviewed by wtc over IRC

Comment 11

[Ian McGreer]

marking fixed per comment #9 (leaving milestone open, should be 3.4.3)

Comment 12

[Wan-Teh Chang]

The definition of the NSSTOKEN_MAX_LOCAL_CERTS macro
should also be removed.

Thanks, Ian!

Comment 13

[Wan-Teh Chang]

Attachment: Remove the unused macro definition

I've checked in this patch on the NSS_3_4_BRANCH.

Comment 14

[Robert Relyea]

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