162748 - Signtool fails to sign without using -p option

Status: RESOLVED FIXED

(NSS :: Tools, defect, P2)

Description

[Tim Burgess]

Using a VeriSign certificate (and possibly also a self-signed cert), signtool
fails to sign a jar unless the -p option is explicitly used to specify the
private key password.

signtool 1.3 would prompt the user for the password so this bug is
a regression.

Using a command such as:
 /home/hotu/nss-3.4.2/bin/signtool -c9 -o -xloader -xfake -xtextclient -xCVS -d
/home/tim/.mozilla/tim/xxxxxxxx.slt/ -Zapplet.jar -k"hotU Inc.'s VeriSign, Inc.
ID" classes

causes the following error:

...
Generating zigbert.sf file..
warning - can't find private key for this cert
signtool: PROBLEM signing data (Incorrect password)
the tree "classes" was NOT SUCCESSFULLY SIGNED

The error message is also misleading, causing the signer to think that
there is a private key corruption, and that issuing new certificate
requests for a new key will resolve the problem (we tried this a
number of times over 2 weeks).

Netscape signers who migrate to using mozilla and who have previously
not hardcoded their personal password into signing scripts will
hit this bug.

Comment 1

[Wan-Teh Chang]

Ian, could you take a look at this bug report?

Comment 2

[Ian McGreer]

I looked at the cvs history of signtool, and it appears that signtool has only
worked with hardcoded passwords for as long as NSS has been open source.

This should be fixed in 3.7, so that it works like the other tools.

Comment 3

[Wan-Teh Chang]

Moved to target milestone 3.8 because the original
NSS 3.7 release has been renamed 3.8.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

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

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Remove target milestone of 3.8, since these bugs didn't get into that release.

Comment 6

[Biswatosh Chakraborty]

I am going to work on this.

Comment 7

[Biswatosh Chakraborty]

certutil prompts for a password but signtool doesnot. The reason is ,in certutil,
PK11_SetPasswordFunc(SECU_GetModulePassword) is called whereas in signtool, this is not called. I added this to signtool and on not giving a password(with -p option),it behaves just as certutil does and once correct password is given, it 
signs correctly. And, with this changed code,if signtool -k mytestcert -Z temp.jar -p "somewrongpassword" t1 ... is given ,it gives this warning:
******
warning - can't find private key for this cert
signtool: PROBLEM signing data (Incorrect password)
the tree "t1" was NOT SUCCESSFULLY SIGNED
****** 

Actually, both certutil and signtool eventually calls PK11_Authenticate(),which
eventually calls pk11_GetPassword().
Now,let us look at this function:
****************
pk11_GetPassword(PK11SlotInfo *slot, PRBool retry, void * wincx)
 {
     if (PK11_Global.getPass == NULL) return NULL;
     return (*PK11_Global.getPass)(slot, retry, wincx);
 }
****************
Here, in certutil, as PK11_Global.getPass is NOT NULL, it comes to the next line
and calls (*PK11_Global.getPass)(slot, retry, wincx),which prompts for the password. But,in signtool, PK11_Global.getPass is NULL unlike in certutil,
and it returns NULL. The reason why PK11_Global.getPass is not NULL is that
in certutil, we call PK11_SetPasswordFunc(SECU_GetModulePassword).
And what PK11_SetPasswordFunc() does is this:
************
PK11_SetPasswordFunc(PK11PasswordFunc func)
{
     PK11_Global.getPass = func;
}
*************

So, it looks, as mentioned in the begining, the way to solve this bug is to add this single line viz.
PK11_SetPasswordFunc(SECU_GetModulePassword) in signtool.c.
Or, could someone tell me if there was any specific reason for omitting this
in signtool.c? Or,it was just a slip?

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Biswatosh, Have you tried your proposed one line fix?
Does that fix it?
Does the -p option continue to work with that one line fix in place?
If so, why not attach a patch and request review?

Comment 9

[Biswatosh Chakraborty]

Attachment: Patch for review.

This patch just adds one line in signtool.c, namely PK11_SetPasswordFunc(SECU_GetModulePassword); 
The intention behind adding this line is to enable signtool behave
like certutil in prompting for password if not supplied from
command line directly. 
However, -p "password" option of signtool behaves as before. So, I didn't see
any regression. 
The comparison of certutil and signtool without this change is given in 
Comment #7 . There, it is explained why signtool never prompts for password and why certutil
prompts and why this change in signtool will make it behave like certutil.
The output of signtool on not giving password with -p option is like below:
*********************
[root@localhost testing]# signtool -d db -k MyTestCert -Z test.jar  testdir
using certificate directory: db
Generating testdir/META-INF/manifest.mf file..
--> f1
adding testdir/f1 to test.jar...(deflated 0%)
Generating zigbert.sf file..
Enter Password or Pin for "NSS Certificate DB":
adding testdir/META-INF/manifest.mf to test.jar...(deflated 15%)
adding testdir/META-INF/zigbert.sf to test.jar...(deflated 27%)
adding testdir/META-INF/zigbert.rsa to test.jar...(deflated 33%)
tree "testdir" signed successfully
**********************

Comment 10

[Neil Williams]

Comment on attachment 258626 [details] [diff] [review]
Patch for review. 

Nice and simple. Please change the target release to 3.12 (or 3.11.7 if that is what you intended).

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 258626 [details] [diff] [review]
Patch for review. 

The signtool program already contains a call to PK11_SetPasswordFunc 
in function InitCrypto() in signtool/util.c.  It sets a function 
named pk11_password_hardcode as the password callback function.
That code is currently necessary to make signtool's -p option work.

Rather than calling PK11_SetPasswordFunc twice in signtool, it would
be better to change signtool to call it once, e.g. in util.c:

824         if (password) {
825             PK11_SetPasswordFunc(pk11_password_hardcode);
  +         } else {
  +             PK11_SetPasswordFunc(SECU_GetModulePassword);
826         }

or 

824-         if (password) {
825-             PK11_SetPasswordFunc(pk11_password_hardcode);
826-         }
   +         PK11_SetPasswordFunc(password ? pk11_password_hardcode
   +                                       : SECU_GetModulePassword);

Comment 12

[Biswatosh Chakraborty]

Yes, that is a nice solution. I just verified and it works fine. The output of
signtool with this solution and with the solution mentioned in Comment #9 are same
when password is given(right or wrong) or when not given at all.
certutil however does not call InitCrypto() and so it calls PK11_SetPasswordFunc(SECU_GetModulePassword) in it's main() itself. 
So, we have two alternative solutions to this bug and I am going to attach the cvs diff of the second solution(proposed by Nelson) for review.

Comment 13

[Biswatosh Chakraborty]

Attachment: An alternative solution.

This just repeats in the form of a cvs diff, of what is in Comment #11, a solution proposed by Nelson. Discussion on it is also there in Comment #12.

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #13)
> This just repeats in the form of a cvs diff, of what is in Comment #11, 

Except that comment 11 followed the NSS coding style and this patch doesn't.
:(  But with that one thing fixed, this patch would be OK.

Comment 15

[Biswatosh Chakraborty]

Attachment: same patch as in comment #13 except the code style

Oops! Sorry. Thanks Nelson for pointing that. Hope this one is OK.

Comment 16

[Neil Williams]

Biswatosh, the patch looks fine but there are a few bugzilla housekeeping detail to attend to:

1) make patch #258626 obsolete (you can edit it in the Details link)
2) change the bug status to "Accept"
3) request a formal review (from me)

After it's been formally reviewed again you can check it in.

Comment 17

[Biswatosh Chakraborty]

Neil,
Requested a formal review (by you) for the patch  #259298 ,given in Comment #15.

Comment 18

[Neil Williams]

Comment on attachment 259298 [details] [diff] [review]
same patch as in comment #13 except the code style

Great! Go ahead and check it in.

Comment 19

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 259298 [details] [diff] [review]
same patch as in comment #13 except the code style

       v a tab here (good)
>+	} else {
>+	    PK11_SetPasswordFunc(SECU_GetModulePassword);
>+       }
  ^^^^^^^  7 spaces

be sure to use 8 spaces, or (preferably) a tab, here.

Comment 20

[Biswatosh Chakraborty]

Corrected the patch according to the suggestion in Comment #19 and
commited to the HEAD.

Checking in util.c;
/cvsroot/mozilla/security/nss/cmd/signtool/util.c,v  <--  util.c
new revision: 1.23; previous revision: 1.22
done
 

Comment 21

[Biswatosh Chakraborty]

Attachment: For Super Review. This is the same patch which was commited to the HEAD in Comment #20

The patch (id: 259298) was modified in the indendation part only, based on the 
suggestion in Comment #19 and was committed to the HEAD. See Comment #20.
I intend to commit this to the NSS_3_11_BRANCH as well. Hence, the request for a super review.

Comment 22

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 261687 [details] [diff] [review]
For Super Review. This is the same patch which was commited to the HEAD in Comment #20

sr=nelson for branch

Comment 23

[Biswatosh Chakraborty]

Checking util.c to the NSS_3_11_BRANCH ...


Checking in util.c;
/cvsroot/mozilla/security/nss/cmd/signtool/util.c,v  <--  util.c
new revision: 1.22.28.1; previous revision: 1.22
done