423839 - Add multiple PKCS#11 token password command line option to NSS tools.

Status: RESOLVED FIXED

(NSS :: Tools, enhancement, P2)

Description

[Slavomir Katuscak]

Many of NSS tools requires two passwords for some PKSC#11 tokens (Solaris Crypto Framework,...) - DB password and PKCS#11 token password. Our tools only supports DB password as command line option. Token password is not supported yet and this can be significant problem for test automation.

Comment 1

[Slavomir Katuscak]

There is function SECU_GetModulePassword in secutil.c where this password is expected in some cases ('Enter Password or Pin for "Sun Metaslot":'). Is there some way how can I set the source of password ? (See switch (pwdata->source) section.)

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Slavo, the DB password *IS* the token password.  

Comment 3

[Julien Pierre]

Nelson,

I think Slavo was saying that he needs to feed the password for all the tokens that are installed in secmod.db and that need login. And the command-line options only allow setting a single token password. At this time, I think the only option is to feed the passwords through stdin .

We could also devise a scheme for a password file in the future. The web server uses a file with the format :

token:password

Right now the password file we can pass in to some of our tools doesn't specify which token the password is for.

Comment 4

[Julien Pierre]

Right now, many tools use different password callbacks .

./SSLsample/client.c:   PK11_SetPasswordFunc(myPasswd);
./SSLsample/server.c:   PK11_SetPasswordFunc(myPasswd);
./certcgi/certcgi.c:    PK11_SetPasswordFunc(return_dbpasswd);
./certutil/certutil.c:    PK11_SetPasswordFunc(SECU_GetModulePassword);
./crlutil/crlutil.c:    PK11_SetPasswordFunc(SECU_GetModulePassword);
./crmf-cgi/crmfcgi.c:  PK11_SetPasswordFunc(passwordCallback);
./crmftest/testcrmf.c:    PK11_SetPasswordFunc(SECU_GetModulePassword);
./dbtest/dbtest.c:          PK11_SetPasswordFunc(getPassword);
./modutil/pk11.c:    PK11_SetPasswordFunc(SECU_GetModulePassword);
./p7content/p7content.c:    PK11_SetPasswordFunc (MyPK11PasswordFunc);
./p7sign/p7sign.c:    PK11_SetPasswordFunc (MyPK11PasswordFunc);
./pk12util/pk12util.c:    PK11_SetPasswordFunc(SECU_GetModulePassword);
./pwdecrypt/pwdecrypt.c:    PK11_SetPasswordFunc(SECU_GetModulePassword);
./rsaperf/rsaperf.c:    PK11_SetPasswordFunc(SECU_GetModulePassword);
./sdrtest/sdrtest.c:    PK11_SetPasswordFunc(SECU_GetModulePassword);
./selfserv/selfserv.c:    PK11_SetPasswordFunc( passwd ? ownPasswd : SECU_GetModulePassword);
./shlibsign/shlibsign.c:    PK11_SetPasswordFunc(SECU_GetModulePassword);
./signtool/util.c:          PK11_SetPasswordFunc(pk11_password_hardcode);
./signtool/util.c:          PK11_SetPasswordFunc(SECU_GetModulePassword);
./smimetools/cmsutil.c:    PK11_SetPasswordFunc(&SECU_GetModulePassword);
./strsclnt/strsclnt.c:  PK11_SetPasswordFunc(ownPasswd);
./strsclnt/strsclnt.c:  PK11_SetPasswordFunc(SECU_GetModulePassword);
./symkeyutil/symkeyutil.c:    PK11_SetPasswordFunc(SECU_GetModulePassword);
./tests/remtest.c:    PK11_SetPasswordFunc(SECU_GetModulePassword);
./tstclnt/tstclnt.c:    PK11_SetPasswordFunc(ownPasswd);
./tstclnt/tstclnt.c:            PK11_SetPasswordFunc(SECU_GetModulePassword);
./vfychain/vfychain.c:    PK11_SetPasswordFunc(myPasswd);
./vfyserv/vfyserv.c:    PK11_SetPasswordFunc(myPasswd);

We should unify that and make them all use the SECU version if possible, and then enhance SECU_GetModulePassword to support an extended syntax that includes multiple tokens in a single string. This way we won't need to add multiple options to any tools.

The syntax of the password string could be something like

tokenname:password,tokenname:password,...

The old syntax of just a single password would also be supported for compatibility.

Comment 5

[Julien Pierre]

Attachment: support multiple passwords

This changes the password file reader to support either the old syntax (single password) or the new syntax, which is tokenname:password .

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 324392 [details] [diff] [review]
support multiple passwords

\
>+    do
>+    {

>+        if (nb == 0) {
>+            fprintf(stderr,"password file contains no data\n");
>+            break;
>+        }

This test should be done before the loop starts.  It doesn't need to be 
done every time through the loop.  nb doesn't change, so it will have the
same value every time.


>+        phrase = &phrase[tokenLen+2];

Shouldn't that be tokenLen+1 ?

Comment 7

[Julien Pierre]

Attachment: incorporate Nelson's feedback

1) The test was put in the loop on purpose in order to avoid having an extra return statement, which would have required free'ing phrases separately. But it's not so desirable after all, since phrase was left uninitialized. So I have added that extra return path in this patch.

I also fixed a leak of phases when PR_Open fails.

2) you are right, it was supposed to be tokenLen+2 .

Comment 8

[Julien Pierre]

I meant : it was supposed to read :

phrase = &phrase[tokenLen+1];

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 324720 [details] [diff] [review]
incorporate Nelson's feedback

Has this been tested?

Comment 10

[Julien Pierre]

Nelson,

Yes. The patch passes all.sh, and I am able to login to the Sun metaslot with certutil using a password  file containing the string "Sun metaslot:password".

Comment 11

[Julien Pierre]

Thanks for the review, Nelson. I checked in attachment 324720 [details] [diff] [review] to the trunk :

Checking in secutil.c;
/cvsroot/mozilla/security/nss/cmd/lib/secutil.c,v  <--  secutil.c
new revision: 1.87; previous revision: 1.86
done

Further changes are still needed for the tools that do not currently use SECU_GetModulePassword as their callback. The list is provided in comment 4. I will review those tools to see why they need an alternate callback.

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

I am about to back this patch out.  
It reliably crashes in pk12util on some platforms.  
(It should crash on ALL platforms, and it's a mystery why it doesn't.)
I should have detected this in review. :(

Here's the problem.  The patched function, SECU_FilePassword, takes a
slot pointer argument.  Prior to this patch, that argument was unused.
This patch uses it, and requires that it be non-NULL.  But there are 
places that legitimately call it with NULL slot pointers.  Not all places 
that call it, asking for a password are looking for a slot password. 
pk12util calls it to get a password for the PKCS#12 file, which is not a 
slot.  pk12util passes a hard-coded NULL for the SLOT pointer in the cases
where it's looking for a pkcs12 file password, as it should.  

In the patched function, we see these lines:

    tokenName = PK11_GetTokenName(slot);
    tokenLen = PORT_Strlen(tokenName);

When slot is NULL, PK11_GetTokenName doesn't crash, but returns NULL.
Then, PORT_Strlen crashes when passed a NULL pointer.
Why it only crashes in some builds on some platforms, and not others, 
is a mystery that may be worthwhile to understand.  

So, this function needs to be able to work when slot is NULL.  It should
just skip the slot name comparison tests in that case, and assume that 
the first line's string in the password.  

Comment 13

[Julien Pierre]

My bad. I had only run ssl.sh, not all.sh .
It seems the only platforms that passed where the "memory leak" tinderboxes.
I think the subset of tests being checked for leaks is only SSL tests, so these tinderboxes were not affected by this change.

Comment 14

[Julien Pierre]

Attachment: fix crash

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 325017 [details] [diff] [review]
fix crash

r=nelson, with one more fix:

>     if (slot) {
>         tokenName = PK11_GetTokenName(slot);
>-        tokenLen = PORT_Strlen(tokenName);
>+        if (tokenName)
>+            tokenLen = PORT_Strlen(tokenName);
>     }

Comment 16

[Julien Pierre]

Nelson,

I checked in attachment 325017 [details] [diff] [review] with your change to the trunk .
Checking in secutil.c;
/cvsroot/mozilla/security/nss/cmd/lib/secutil.c,v  <--  secutil.c
new revision: 1.89; previous revision: 1.88
done

I will update this bug with my review of the tools usage of the password handlers.

Comment 17

[Julien Pierre]

Attachment: Adds password file support to 4 tools

This is an additional patch to support the proper callback and password files in tools that did not.

The 2 tests that tools need to pass to support the password file are :

a) it must use SECU_GetModulePassword
b) ist must have the ability to set PW_FROMFILE

certcgi fails both tests. This program is unused, and the password handler it uses is already broken (always returns "foo" . There is clearly more work to do to fix that tool than the password handler. So I'm going to skip it.

certutil is OK

crlutil is OK

crmf-cgi fails both tests. It is unused . The password handler tries to get the password from the CGI input, but also only supports one token . SECU_GetModulePassword is not a suitable replacement. Since this prgogram is unused, I will also skip this tool.

crmftest passes a, but fails b. Support is added in the patch.

dbtest : fails both tests. this test exists for the purpose of checking the internal DB only. There is no need to support tokens at the same time in this tool. The test script should ensure that it doesn't run this program against a secmod.db containing external tokens requiring a login

modutil doesn't actually need a password handler at all. One is only set in the changepw command, and the user is prompted explicitly for it. I have determined through debugging that the password handler is never invoked. Thus, the patch removes the unnecessary call to PK11_SetPasswordFunc.

p7content : fails both tests. This patch adds support.

p7sign : fails both tests. This patch adds support.

I still have a few tools to go through, but these are ready for review.

Comment 18

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 332008 [details] [diff] [review]
Adds password file support to  4 tools

r=nelson

Comment 19

[Julien Pierre]

Nelson,

Thanks for the review .

I checked the following to the trunk :

Checking in crmftest/testcrmf.c;
/cvsroot/mozilla/security/nss/cmd/crmftest/testcrmf.c,v  <--  testcrmf.c
new revision: 1.11; previous revision: 1.10
done
Checking in modutil/pk11.c;
/cvsroot/mozilla/security/nss/cmd/modutil/pk11.c,v  <--  pk11.c
new revision: 1.27; previous revision: 1.26
done
Checking in p7sign/p7sign.c;
/cvsroot/mozilla/security/nss/cmd/p7sign/p7sign.c,v  <--  p7sign.c
new revision: 1.14; previous revision: 1.13
done
Checking in p7content/p7content.c;
/cvsroot/mozilla/security/nss/cmd/p7content/p7content.c,v  <--  p7content.c
new revision: 1.12; previous revision: 1.11
done

Comment 20

[Julien Pierre]

Attachment: Add support for more tools

Continuing the tools review :

pk12util : OK

pwdecrypt : passes a, fails b. Support is added in this patch. This is currently unused by our QA test suite, so the patch is untested.

rsaperf : passes a, fails b. Support is added in this patch.

sdrtest : OK

selfserv : this sometimes SECU_GetModulePassword, and other times not (single password for all tokens, if -w is used). But it never supports a password file. Support has been added in this patch.

shlibsign : fails b . Patch adds support.

signtool : this is broken the same way that selfserv is. No password file support. Also wincx is NULL so further fixes are needed. Patch adds support.

smimetools : passes a, fails b. Support is added in this patch.

strsclnt : fails both tests. Support is added in this patch

symkeyutil : OK

remtests : fails b. doesn't use any API that takes wincx . Support could be provided with a wrapper for SECU_GetModulePassword. This progrma is currently not used by the QA test suite.

tstclnt : fails both. support added in this patch.

vfychain : fails both. Support added in this patch.

vfyserv : fails both. Support added in this patch.

This patch also removes references to libsec from tools.

Comment 21

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 332303 [details] [diff] [review]
Add support for more tools

This patch is 90% r+
I'm giving review marks for each of the files in this patch, individually.
files marked + can be committed without further review, however, one 
change is required in some cases.  See below.

+ lib/secutil.h
+ p7env/p7env.c
+ p7verify/p7verify.c
+ rsaperf/rsaperf.c
+ selfserv/selfserv.c
+ signtool/certgen.c
+ signtool/list.c
+ signtool/sign.c
+ signtool/signtool.c
+ signtool/signtool.h
+ signtool/util.c
+ smimetools/cmsutil.c
+ strsclnt/strsclnt.c
+ tests/remtest.c         (is this a dead file?)
+ vfychain/vfychain.c 
+ vfyserv/vfyserv.c

- pwdecrypt/pwdecrypt.c  (patch extends usage message past 80 columns)
- shlibsign/shlibsign.c  (ditto)

Required change for r+ files:

There are several commands where the patch adds one or more option 
letters to the string of option letters passed to PL_CreateOptState.
In most cases, the option characters in those strings were carefully
arranged to be in ASCII order (numerals first, then capitals, then 
lower case).  In all cases, this patch adds letters without regard
to maintaining the sorting order.  

Please fix all calls to PL_CreateOptState in this patch, so that the
sorting order is restored.  

One  other comment.  The patch to vfyserv contains one change that 
I would have expected to be in many of the files, not just that one
file.  It is:
-	secStatus = SSL_SetPKCS11PinArg(sslSocket, password);
+	secStatus = SSL_SetPKCS11PinArg(sslSocket, &pwdata);
I'm surprised this is required in more files.  
But I'm assuming that this patch was tested (all the programs were tested)
and approving it on that basis.  If some programs were not, please do not
commit them until they are tested.

Comment 22

[Julien Pierre]

Nelson,

Thanks for your review.

I will fix the PL_CreateOptState before checkin and the 80 columns issue.

As far as testing goes, I ran all.sh and it passed. As mentioned in my review comments, some programs are not used in all.sh . I did not attempt to test those, with or without the changes. Not all programs use SSL, so that may explain why the call to SSL_SetPKCS11PinArg isn't required. However, I see that even many that do, like selfserv, strsclnt and tstlcnt, passed the QA without this call. So, it may be that this call just isn't needed.

Comment 23

[Nelson Bolyard (seldom reads bugmail)]

If the only testing was all.sh, that's not enough.
All.sh will typically test one of the ways of providing a password, 
but not more than one.
At a minimum, we need to ensure that all the programs we ship on Solaris
continue to work with all 3 forms of password input 
- pw on command line
- pw from file
- pw obtained interactively (prompting the user).

Comment 24

[Julien Pierre]

Nelson,

All three methods of providing the password are enabled by the same password handler function, SECU_GetModulePassword, which is now used in nearly every tool with this patch .

The requirements for each of the 3 methods to work are :

1) pw on the command-line : pwdata structure must be initialized with source = PW_PLAINTEXT, and passed around in NSS calls as the "wincx" argument. This is the method tested by all.sh .

2) pw from file : only differs from 1) by initializing the structure with source = PW_FROMFILE .

3) pw obtained interactively : no additional requirements other than using SECU_GetModulePassword . pwdata structure is initialized with source = PW_NONE, or not passed in.

In the patch, I only seeked to add method 2) where it was previously unavailable (that is, everywhere). I did not attempt to add method 1) if it wasn't already provided - but I changed the implementation of command-line plaintext password if it wasn't already using SECU_GetModulePassword 

The fact that all.sh works with method 1 ensures that wincx is properly set for all the tools that it exercises.

Method 2 will work equally well for the same tools as long as pwdata.source is initialized with PW_FILE, since we know wincx is being properly set to point to the structure .

Method 3 is always available by not providing a password or password file on the command-line for all the tools. These options are never required for any tools.

I did manually test that all 3 methods work correctly with certutil. I did not repeat the same test with every tool. I think that's unnecessary given what I stated above.

Comment 25

[Nelson Bolyard (seldom reads bugmail)]

Julien, you've got r+.  If you're sure it's going to work, go for it.

Comment 26

[Julien Pierre]

Re: SSL_SetPKCS11PinArg, this call is only needed in programs that call
SSL_RevealPinArg .

There are only 2 such calls :
1) in vfyutil.c . This is matched by the set call in vfyserv.c

2) in lib/ssl/authcert.c, in NSS_GetClientAuthData . This is the default
handler for selecting the client certificate and private key to use in an SSL
client.

This function is only invoked in SSL client programs that perform client
authentication and don't override this callback.

It turns out we have 3 SSL clients that do SSL client auth :
strsclnt, tstlcnt and vfyserv

a) tstclnt uses the default callback, and calls SSL_SetPKCS11PinArg
b) strsclnt has its own callback, and stuffs the wincx into its own
Cert_And_Key structure, so it doesn't call SSL_SetPKCS11PinArg
c) vfyserv has its own callback, and is calling SSL_SetPKCS11PinArg .

In summary, the patch is correct.

Comment 27

[Julien Pierre]

I checked the patch in to the trunk with the PL_CreateOptState change and fixed usage messages.

Checking in certutil/certutil.c;
/cvsroot/mozilla/security/nss/cmd/certutil/certutil.c,v  <--  certutil.c
new revision: 1.140; previous revision: 1.139
done
Checking in lib/secutil.h;
/cvsroot/mozilla/security/nss/cmd/lib/secutil.h,v  <--  secutil.h
new revision: 1.29; previous revision: 1.28
done
Checking in p7env/p7env.c;
/cvsroot/mozilla/security/nss/cmd/p7env/p7env.c,v  <--  p7env.c
new revision: 1.9; previous revision: 1.8
done
Checking in p7verify/p7verify.c;
/cvsroot/mozilla/security/nss/cmd/p7verify/p7verify.c,v  <--  p7verify.c
new revision: 1.10; previous revision: 1.9
done
Checking in pwdecrypt/pwdecrypt.c;
/cvsroot/mozilla/security/nss/cmd/pwdecrypt/pwdecrypt.c,v  <--  pwdecrypt.c
new revision: 1.5; previous revision: 1.4
done
Checking in rsaperf/rsaperf.c;
/cvsroot/mozilla/security/nss/cmd/rsaperf/rsaperf.c,v  <--  rsaperf.c
new revision: 1.20; previous revision: 1.19
done
Checking in selfserv/selfserv.c;
/cvsroot/mozilla/security/nss/cmd/selfserv/selfserv.c,v  <--  selfserv.c
new revision: 1.87; previous revision: 1.86
done
Checking in shlibsign/shlibsign.c;
/cvsroot/mozilla/security/nss/cmd/shlibsign/shlibsign.c,v  <--  shlibsign.c
new revision: 1.16; previous revision: 1.15
done
Checking in signtool/certgen.c;
/cvsroot/mozilla/security/nss/cmd/signtool/certgen.c,v  <--  certgen.c
new revision: 1.13; previous revision: 1.12
done
Checking in signtool/list.c;
/cvsroot/mozilla/security/nss/cmd/signtool/list.c,v  <--  list.c
new revision: 1.10; previous revision: 1.9
done
Checking in signtool/sign.c;
/cvsroot/mozilla/security/nss/cmd/signtool/sign.c,v  <--  sign.c
new revision: 1.15; previous revision: 1.14
done
Checking in signtool/signtool.c;
/cvsroot/mozilla/security/nss/cmd/signtool/signtool.c,v  <--  signtool.c
new revision: 1.14; previous revision: 1.13
done
Checking in signtool/signtool.h;
/cvsroot/mozilla/security/nss/cmd/signtool/signtool.h,v  <--  signtool.h
new revision: 1.13; previous revision: 1.12
done
Checking in signtool/util.c;
/cvsroot/mozilla/security/nss/cmd/signtool/util.c,v  <--  util.c
new revision: 1.27; previous revision: 1.26
done
Checking in smimetools/cmsutil.c;
/cvsroot/mozilla/security/nss/cmd/smimetools/cmsutil.c,v  <--  cmsutil.c
new revision: 1.54; previous revision: 1.53
done
Checking in strsclnt/strsclnt.c;
/cvsroot/mozilla/security/nss/cmd/strsclnt/strsclnt.c,v  <--  strsclnt.c
new revision: 1.62; previous revision: 1.61
done
Checking in tests/remtest.c;
/cvsroot/mozilla/security/nss/cmd/tests/remtest.c,v  <--  remtest.c
new revision: 1.5; previous revision: 1.4
done
Checking in tstclnt/tstclnt.c;
/cvsroot/mozilla/security/nss/cmd/tstclnt/tstclnt.c,v  <--  tstclnt.c
new revision: 1.51; previous revision: 1.50
done
Checking in vfychain/vfychain.c;
/cvsroot/mozilla/security/nss/cmd/vfychain/vfychain.c,v  <--  vfychain.c
new revision: 1.26; previous revision: 1.25
done
Checking in vfyserv/vfyserv.c;
/cvsroot/mozilla/security/nss/cmd/vfyserv/vfyserv.c,v  <--  vfyserv.c
new revision: 1.17; previous revision: 1.16
done

Comment 28

[Slavomir Katuscak]

Latest patch caused Tinderbox hangs:

memleak.sh: -------- Stopping server:
tstclnt -p 8441 -h ciotat.red.iplanet.com -c j -f -d /export/tinderbox/SunOS_5.9/mozilla/tests_results/security/ciotat.1/pkix/client_memleak -w nss -o < /export/tinderbox/SunOS_5.9/mozilla/security/nss/tests/memleak/sslreq.dat
Enter Password or Pin for "NSS FIPS 140-2 Certificate DB":

Tstclnt asks for passsword also when it's set on command line. I let Tinderboxes in state waiting for password, feel free to debug (they are running in screen).

Comment 29

[Nelson Bolyard (seldom reads bugmail)]

tstclnt is hanging/failing on ciotat and aquash in the same place (same
command).  At a minimum, we must back out this bug's patch to tstclnt,
or fix it, before 2 PM today (California time).  Today our focus must be
on stabilizing 3.12.1 for tonight's release candidate builds.

Comment 30

[Nelson Bolyard (seldom reads bugmail)]

I backed out the last checkin to tstclnt.c
Checking in tstclnt.c; new revision: 1.52; previous revision: 1.51
We'll have to watch tinderbox to see if there are any other failures.

Comment 31

[Julien Pierre]

Sorry, it looks like I didn't include tstclnt.c in the diff that I submitted for review. But I still don't see how the changes could have broken it. I ran the full all.sh with my patch and didn't see any problem. Is this problem intermittent, or happening on certain platforms only ?

Comment 32

[Julien Pierre]

Slavo,

A stack trace from the hung tinderboxes would be very useful, if you can collect one with pstack . I guess not since the patch was backed out. I haven't been able to reproduce this issue yet. I tested on Solaris 10 on my own system.
Was this seen only on the memory leak tinderboxes or others too ?

Comment 33

[Julien Pierre]

It seemed the issue was only on the memory leak tinderboxes.
I ran memleak.sh on my solaris machine with the patch. I didn't see any failure or hang.

Comment 34

[Slavomir Katuscak]

Stack from dopushups machine:

bash-3.00$ pstack 14915
#0  0x005877a2 in _dl_sysinfo_int80 () from /lib/ld-linux.so.2
#1  0x001ca073 in __read_nocancel () from /lib/tls/libc.so.6
#2  0x0016ef68 in _IO_file_read_internal () from /lib/tls/libc.so.6
#3  0x0016dcee in _IO_new_file_underflow () from /lib/tls/libc.so.6
#4  0x0017030b in _IO_default_uflow_internal () from /lib/tls/libc.so.6
#5  0x001700fd in __uflow () from /lib/tls/libc.so.6
#6  0x001650a0 in _IO_getline_info_internal () from /lib/tls/libc.so.6
#7  0x00164fdf in _IO_getline_internal () from /lib/tls/libc.so.6
#8  0x00163e93 in fgets () from /lib/tls/libc.so.6
#9  0x08054e3f in SEC_GetPassword ()
#10 0x0804d8b5 in SECU_GetPasswordString ()
#11 0x0804dbf6 in SECU_GetModulePassword ()
#12 0x00876ee0 in PK11_DoPassword ()
#13 0x00876ff3 in PK11_Authenticate ()
#14 0x0088ee44 in PK11_GetBestSlotMultiple ()
#15 0x0088ef71 in PK11_GetBestSlot ()
#16 0x008828a4 in PK11_VerifyRecover ()
#17 0x00871c45 in DecryptSigBlock ()
#18 0x00872272 in vfy_CreateContext ()
#19 0x00872806 in vfy_VerifyData ()
#20 0x0087295e in VFY_VerifyDataWithAlgorithmID ()
#21 0x008695ab in CERT_VerifySignedDataWithPublicKey ()
#22 0x008f74ea in PKIX_PL_Cert_VerifySignature ()
#23 0x008de8b8 in pkix_BuildForwardDepthFirstSearch ()
#24 0x008e0720 in pkix_Build_InitiateBuildChain ()
#25 0x008e26cb in PKIX_BuildChain ()
#26 0x0086c554 in cert_VerifyCertChainPkix ()
#27 0x00869d7c in cert_VerifyCertChain ()
#28 0x0086a654 in CERT_VerifyCertChain ()
#29 0x0086b01b in CERT_VerifyCert ()
#30 0x0086b1e0 in CERT_VerifyCertNow ()
#31 0x00d57d08 in SSL_AuthCertificate ()
#32 0x00d55e18 in ssl3_HandleHandshakeMessage ()
#33 0x00d56a93 in ssl3_HandleRecord ()
#34 0x00d57788 in ssl3_GatherCompleteHandshake ()
#35 0x00d58d0d in ssl_GatherRecord1stHandshake ()
#36 0x00d5f2c0 in ssl_Do1stHandshake ()
#37 0x00d60a98 in ssl_SecureSend ()
#38 0x00d646df in ssl_Send ()
#39 0x0026e7d3 in PR_Send ()
#40 0x0804d507 in main ()

One more important info - always hanged in PKIX cycle (NSS_ENABLE_PKIX_VERIFY="1").

Comment 35

[Julien Pierre]

Here is the stack I got on Solaris, with all the arguments included .
We can see from it that wincx is NULL between frames 6 and 17.
However, it is non-NULL between frames 24 and 28.

So, the proper value of wincx is getting lost between frames 18 and 23, which is the libpkix code.

current thread: t@1
  [1] _read(0x7, 0x80c95a4, 0x2000), at 0xfeac52a7
  [2] __filbuf(0xfeaf1c40), at 0xfeaa81a4
  [3] fgets(0x8044050, 0xc7, 0xfeaf1c40), at 0xfeaaa16e
  [4] SEC_GetPassword(input = 0xfeaf1c40, output = 0xfeaf1c50, prompt = 0x8044191 "Enter Password or Pin for "NSS FIPS 140-2 Certificate DB":", ok = 0x8062a60 = &SEC_BlindCheckPassword(char *cp)), line 109 in "secpwd.c"
  [5] SECU_GetPasswordString(arg = (nil), prompt = 0x8044191 "Enter Password or Pin for "NSS FIPS 140-2 Certificate DB":"), line 184 in "secutil.c"
=>[6] SECU_GetModulePassword(slot = 0x808c890, retry = 0, arg = (nil)), line 317 in "secutil.c"
  [7] pk11_GetPassword(slot = 0x808c890, retry = 0, wincx = (nil)), line 517 in "pk11auth.c"
  [8] PK11_DoPassword(slot = 0x808c890, loadCerts = 1, wincx = (nil)), line 587 in "pk11auth.c"
  [9] PK11_Authenticate(slot = 0x808c890, loadCerts = 1, wincx = (nil)), line 319 in "pk11auth.c"
  [10] PK11_GetBestSlotMultiple(type = 0x8044368, mech_count = 1, wincx = (nil)), line 1909 in "pk11slot.c"
  [11] PK11_GetBestSlot(type = 1U, wincx = (nil)), line 1930 in "pk11slot.c"
  [12] PK11_VerifyRecover(key = 0x80c2c18, sig = 0x80444e8, dsig = 0x80443d0, wincx = (nil)), line 641 in "pk11obj.c"
  [13] DecryptSigBlock(tagp = 0x80c2ab8, digest = 0x80c2ac0 "", digestlen = 0x80c2b50, maxdigestlen = 64U, key = 0x80c2c18, sig = 0x80444e8, wincx = (nil)), line 79 in "secvfy.c"
  [14] vfy_CreateContext(key = 0x80c22a8, sig = 0x80444e8, encAlg = SEC_OID_PKCS1_RSA_ENCRYPTION, hashAlg = SEC_OID_SHA1, hash = (nil), wincx = (nil)), line 402 in "secvfy.c"
  [15] vfy_VerifyData(buf = 0x80b48ac "0\x82^A\xe4\xa0^C^B^A^B^B^Ad0^M^F^I*\x86H\x86\xf7^M^A^A^E^E", len = 488, key = 0x80c22a8, sig = 0x80444e8, encAlg = SEC_OID_PKCS1_RSA_ENCRYPTION, hashAlg = SEC_OID_SHA1, hash = (nil), wincx = (nil)), line 699 in "secvfy.c"
  [16] VFY_VerifyDataWithAlgorithmID(buf = 0x80b48ac "0\x82^A\xe4\xa0^C^B^A^B^B^Ad0^M^F^I*\x86H\x86\xf7^M^A^A^E^E", len = 488, key = 0x80c22a8, sig = 0x80444e8, sigAlgorithm = 0x80b4738, hash = (nil), wincx = (nil)), line 749 in "secvfy.c"
  [17] CERT_VerifySignedDataWithPublicKey(sd = 0x80b472c, pubKey = 0x80c22a8, wincx = (nil)), line 91 in "certvfy.c"
  [18] PKIX_PL_Cert_VerifySignature(cert = 0x80bb5d4, pubKey = 0x807f2c4, plContext = 0x8086038), line 2937 in "pkix_pl_cert.c"
  [19] pkix_Build_VerifyCertificate(state = 0x80bfe6c, userCheckers = 0x80bd5c4, revocationChecking = 1, pTrusted = 0x80446f8, pNeedsCRLChecking = 0x80446e4, verifyNode = 0x80c1c8c, plContext = 0x8086038), line 1220 in "pkix_build.c"
  [20] pkix_BuildForwardDepthFirstSearch(pNBIOContext = 0x80447cc, state = 0x80bfe6c, pValResult = 0x804477c, plContext = 0x8086038), line 2697 in "pkix_build.c"
  [21] pkix_Build_InitiateBuildChain(procParams = 0x80bb0f4, pNBIOContext = 0x8044888, pState = 0x8044890, pBuildResult = 0x804488c, pVerifyNode = 0x8044904, plContext = 0x8086038), line 4262 in "pkix_build.c"
  [22] PKIX_BuildChain(procParams = 0x80bb0f4, pNBIOContext = 0x8044900, pState = 0x80448fc, pBuildResult = 0x8044908, pVerifyNode = 0x8044904, plContext = 0x8086038), line 4435 in "pkix_build.c"
  [23] cert_BuildAndValidateChain(procParams = 0x80bb0f4, pResult = 0x8044948, pVerifyNode = 0x8044944, plContext = 0x8086038), line 790 in "certvfypkix.c"
  [24] cert_VerifyCertChainPkix(cert = 0x80b4720, checkSig = 1, requiredUsage = certUsageSSLServer, time = 1218585383724518LL, wincx = 0x807acc4, log = (nil), pSigerror = (nil), pRevoked = (nil)), line 1228 in "certvfypkix.c"
  [25] cert_VerifyCertChain(handle = 0x808fe60, cert = 0x80b4720, checkSig = 1, sigerror = (nil), certUsage = certUsageSSLServer, t = 1218585383724518LL, wincx = 0x807acc4, log = (nil), revoked = (nil)), line 870 in "certvfy.c"
  [26] CERT_VerifyCertChain(handle = 0x808fe60, cert = 0x80b4720, checkSig = 1, certUsage = certUsageSSLServer, t = 1218585383724518LL, wincx = 0x807acc4, log = (nil)), line 882 in "certvfy.c"
  [27] CERT_VerifyCert(handle = 0x808fe60, cert = 0x80b4720, checkSig = 1, certUsage = certUsageSSLServer, t = 1218585383724518LL, wincx = 0x807acc4, log = (nil)), line 1478 in "certvfy.c"
  [28] CERT_VerifyCertNow(handle = 0x808fe60, cert = 0x80b4720, checkSig = 1, certUsage = certUsageSSLServer, wincx = 0x807acc4), line 1529 in "certvfy.c"
  [29] SSL_AuthCertificate(arg = 0x808fe60, fd = 0x807e6a8, checkSig = 1, isServer = 0), line 255 in "sslauth.c"
  [30] ssl3_HandleCertificate(ss = 0x8096130, b = 0x809dae2 "^N", length = 0), line 7260 in "ssl3con.c"
  [31] ssl3_HandleHandshakeMessage(ss = 0x8096130, b = 0x809d5e6 "", length = 1276U), line 7938 in "ssl3con.c"
  [32] ssl3_HandleHandshake(ss = 0x8096130, origBuf = 0x8096374), line 8062 in "ssl3con.c"
  [33] ssl3_HandleRecord(ss = 0x8096130, cText = 0x8044c9c, databuf = 0x8096374), line 8325 in "ssl3con.c"
  [34] ssl3_GatherCompleteHandshake(ss = 0x8096130, flags = 0), line 206 in "ssl3gthr.c"
  [35] ssl_GatherRecord1stHandshake(ss = 0x8096130), line 1258 in "sslcon.c"
  [36] ssl_Do1stHandshake(ss = 0x8096130), line 151 in "sslsecur.c"
  [37] ssl_SecureSend(ss = 0x8096130, buf = 0x8044dd8 "GET /stop HTTP/1.0^M\n^M\n", len = 22, flags = 0), line 1152 in "sslsecur.c"
  [38] ssl_Send(fd = 0x807e6a8, buf = 0x8044dd8, len = 22, flags = 0, timeout = 4294967295U), line 1447 in "sslsock.c"
  [39] PR_Send(fd = 0x807e6a8, buf = 0x8044dd8, amount = 22, flags = 0, timeout = 4294967295U), line 226 in "priometh.c"
  [40] main(argc = 13, argv = 0x8045f94), line 981 in "tstclnt.c"

Comment 36

[Julien Pierre]

Attachment: Set wincx argument correctly in libpkix

This solves the problem encountered with tstclnt with libpkix during memory leak checking . I am not sure why this code path was only exercised in that particular test case.

Comment 37

[Julien Pierre]

Attachment: Also fix the same wincx problem for CRLs

The additional CRL code path was found through code inspection. It was not triggered in any test, probably because the token is logged in first while searching for certs.

Comment 38

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 333638 [details] [diff] [review]
Also fix the same wincx problem for CRLs

I wish Alexei could review this, but here's r+.

Comment 39

[Julien Pierre]

Thanks, Nelson. I will wait until the tree is open to checkin.

Comment 40

[Julien Pierre]

Now that the trunk is open, I checked in the libpkix patch and restored the tstclnt from the earlier patch .

Checking in pkix_pl_nss/pki/pkix_pl_cert.c;
/cvsroot/mozilla/security/nss/lib/libpkix/pkix_pl_nss/pki/pkix_pl_cert.c,v  <--  pkix_pl_cert.c
new revision: 1.19; previous revision: 1.18
done
Checking in pkix_pl_nss/pki/pkix_pl_crl.c;
/cvsroot/mozilla/security/nss/lib/libpkix/pkix_pl_nss/pki/pkix_pl_crl.c,v  <--  pkix_pl_crl.c
new revision: 1.10; previous revision: 1.9
done

Checking in tstclnt.c;
/cvsroot/mozilla/security/nss/cmd/tstclnt/tstclnt.c,v  <--  tstclnt.c
new revision: 1.53; previous revision: 1.52
done