Closed Bug 263948 Opened 20 years ago Closed 16 years ago

make signtool work with shared libraries

Categories

(NSS :: Tools, defect, P2)

3.9.2
defect

Tracking

(Not tracked)

RESOLVED DUPLICATE of bug 438876

People

(Reporter: julien.pierre, Assigned: alvolkov.bgs)

Details

Attachments

(1 file, 2 obsolete files)

signtool currently links statically with some NSS libraries, and uses internal
functions such as SECMOD_GetDefaultModuleListLock .

It should be made to work with shared libraries, and the offending internal
functions should not be used.
Priority: -- → P2
Target Milestone: --- → 3.10
The description above says "uses internal functions such as
SECMOD_GetDefaultModuleListLock", but IINM, SECMOD_GetDefaultModuleListLock is
part of the solution, not the problem.

I believe signtool currently is using SECMOD_NewListLock().  Bob Relyea wrote:

> signtool is getting the default modules list, but using it's own lock to 
> 'protect' it. This is obviously wrong (it's probably only safe because 
> signtool is single threaded.  The correct code should call 
> SECMOD_GetDefaultModuleListLock() ! 
This is not only an internal function, but the wrong one to use, as previously
discussed via e-mail .
FYI, after commenting out USE_STATIC_LIBS in manifest.mn, I got the following
link errors, which are all due to the use of internal functions :

ild: (undefined symbol) MD5_End -- referenced in the text segment of
SunOS5.9_DBG.OBJ/sign.o
ild: (undefined symbol) SHA1_NewContext -- referenced in the text segment of
SunOS5.9_DBG.OBJ/sign.o
ild: (undefined symbol) MD5_Update -- referenced in the text segment of
SunOS5.9_DBG.OBJ/sign.o
ild: (undefined symbol) SEC_PKCS7EncodeItem -- referenced in the text segment of
SunOS5.9_DBG.OBJ/certgen.o
ild: (undefined symbol) SHA1_Update -- referenced in the text segment of
SunOS5.9_DBG.OBJ/sign.o
ild: (undefined symbol) SHA1_DestroyContext -- referenced in the text segment of
SunOS5.9_DBG.OBJ/sign.o
ild: (undefined symbol) SHA1_Begin -- referenced in the text segment of
SunOS5.9_DBG.OBJ/sign.o
ild: (undefined symbol) MD5_NewContext -- referenced in the text segment of
SunOS5.9_DBG.OBJ/sign.o
ild: (undefined symbol) DER_TimeChoiceDayToAscii -- referenced in the text
segment of SunOS5.9_DBG.OBJ/list.o
ild: (undefined symbol) MD5_DestroyContext -- referenced in the text segment of
SunOS5.9_DBG.OBJ/sign.o
ild: (undefined symbol) MD5_Begin -- referenced in the text segment of
SunOS5.9_DBG.OBJ/sign.o
ild: (undefined symbol) SHA1_End -- referenced in the text segment of
SunOS5.9_DBG.OBJ/sign.o
ild: (undefined symbol) PK11_FindObjectForCert -- referenced in the text segment
of SunOS5.9_DBG.OBJ/sign.o
ild: (undefined symbol) CERT_DecodeDERCertificate -- referenced in the text
segment of SunOS5.9_DBG.OBJ/certgen.o
gmake: *** [SunOS5.9_DBG.OBJ/signtool] Error 5
Assignee: wtchang → alexei.volkov.bugs
QA Contact: bishakhabanerjee → jason.m.reid
Target Milestone: 3.10 → 3.11
QA Contact: jason.m.reid → tools
Target Milestone: 3.11 → 3.11.1
Attached patch combined fix (obsolete) — Splinter Review
Patch includes changes to make signtool use static libraries. Also fixes couple ref leaks in list.c file. 

Function  timeChoiceDayToAscii is created to take care of time conversion.
Could have been easer if we just make DER_TimeChoiceDayToAscii a part of public API, but if not, we could just use timeChoiceDayToAscii.
Attachment #168272 - Attachment is obsolete: true
Attachment #241392 - Flags: superreview?(nelson)
Attachment #241392 - Flags: review?(julien.pierre.bugs)
Comment on attachment 241392 [details] [diff] [review]
combined fix

I'm going to review this patch in pieces as separate comments, 
and then give an overall review to the whole patch when done.

In comment 4, Alexei wrote:

> Could have been easer if we just make DER_TimeChoiceDayToAscii 
> a part of public API,

So, why don't you do that?  I have no objection.  Has anyone else objected? 

If you're going to reimplement it in signtool source files, put the 
implementation in list.c, in the same source file that calls the function, 
rather than putting it into a separate source file util.c.

Comments by source file:

>Index: certgen.c

>-    newcert = CERT_DecodeDERCertificate(derCert, PR_TRUE, NULL);
>+    newcert = CERT_DecodeCertFromPackage((char *)derCert->data, derCert->len);

I don't understand why this change is desired.  It occurs in a function
that is called for only one purpose, to "install" a DER certificate that 
has just been created.  The new function is able to decode a certificate
in just about any format, including DER, PEM, PKCS#7, and others.  But 
since the caller only ever has a DER cert, I don't see what problem this
change solves.  Please explain it.

>Index: list.c

I'm going to have to review the change to this file separately.

>Index: manifest.mn

Looks good.

>Index: sign.c

>-    PK11_FindObjectForCert (cert, /*wincx*/ NULL, &slot);
>+    privk = PK11_FindKeyByAnyCert (cert, NULL); 

Looks good.

>@@ -838,65 +827,77 @@
> static int	
> calculate_MD5_range (FILE *fp, long r1, long r2, JAR_Digest *dig)

IINM, the BIG change to this function is exactly equivalent to adding 
two calls to PK11_HashBuf instead.  I'd much rather see this code call 
PK11_HashBuf twice than duplicate its code twice.  Also, since this 
function computes both MD5 and SHA1 hashes, it shouldn't be named 
calculate_MD5_range, so please give it a better name.

>Index: util.c

>-    if ((moduleLock = SECMOD_NewListLock()) == NULL) {
>+    if ((moduleLock = SECMOD_GetDefaultModuleListLock()) == NULL) {
> 	/* this is the wrong text */

Please get rid of that comment.  With this patch, It is the right text.
Comment on attachment 241392 [details] [diff] [review]
combined fix

Please address the review comments given above in a new patch, and then I will review that.
Attachment #241392 - Flags: superreview?(nelson) → superreview-
(In reply to comment #5)
> (From update of attachment 241392 [details] [diff] [review] [edit])
> 
> > Could have been easer if we just make DER_TimeChoiceDayToAscii 
> > a part of public API,
> 
> So, why don't you do that?  I have no objection.  Has anyone else objected? 

Since there are no objections, adding DER_TimeChoiceDayToAscii to public nss.def

> 
> >Index: certgen.c
> 
> >-    newcert = CERT_DecodeDERCertificate(derCert, PR_TRUE, NULL);
> >+    newcert = CERT_DecodeCertFromPackage((char *)derCert->data, derCert->len);
> 
> I don't understand why this change is desired.  It occurs in a function
> that is called for only one purpose, to "install" a DER certificate that 
> has just been created.  The new function is able to decode a certificate
> in just about any format, including DER, PEM, PKCS#7, and others.  But 
> since the caller only ever has a DER cert, I don't see what problem this
> change solves.  Please explain it.

The replaced function is not public and was replace with CERT_DecodeCertFromPackage which is a part of API.

> 
> >Index: list.c
> 
> I'm going to have to review the change to this file separately.
> 
Any comments regarding changes in list.c?


@@
> > static int	
> > calculate_MD5_range (FILE *fp, long r1, long r2, JAR_Digest *dig)
> 

> IINM, the BIG change to this function is exactly equivalent to adding 
> two calls to PK11_HashBuf instead.  I'd much rather see this code call 
> PK11_HashBuf twice than duplicate its code twice.  Also, since this 
> function computes both MD5 and SHA1 hashes, it shouldn't be named 
> calculate_MD5_range, so please give it a better name.

Absolutely agree with comment. Changing the function to use PK11_HashBuf.

This patch also swaps two lines in nss.def that were not in alphabetic order:

893 SECKEY_ECParamsToKeySize;
894 SECKEY_ECParamsToBasePointOrderLen;
Attachment #241392 - Attachment is obsolete: true
Attachment #248313 - Flags: review?(nelson)
Attachment #241392 - Flags: review?(bugzilla)
Comment on attachment 248313 [details] [diff] [review]
accumulates review suggestions 

1. regarding this change:
>-    newcert = CERT_DecodeDERCertificate(derCert, PR_TRUE, NULL);
>+    newcert = CERT_DecodeCertFromPackage((char *)derCert->data, derCert->len);

if you #include "nssrenam.h", you will find that calls to the function 
CERT_DecodeDERCertificate magically compile and work, I think.  
Please try that, instead of changing to CERT_DecodeCertFromPackage.  

There is also another, third, way to import a DER cert, using 
CERT_NewTempCertificate, which also requires nssrenam.h, IIRC.  

I will send some email to mozilla-nssdev about this issue.

2. There are some problems in the function:
> cert_trav_callback(CERTCertificate *cert, SECItem *k, void *data)
and since you're replacing nearly the entire body of the function,
(by unindenting it), it might be good to fix them at this time.  
But I won't hold review of this patch just for this.

3. Please change this code:

>+    if (rv != SECSuccess) {
>+	PR_fprintf(errorFD, "%s: unable to calculate hash: %s(%d)\n", PROGRAM_NAME,
>+                   SECU_Strerror(PORT_GetError()), PORT_GetError());

to only call PORT_GetError once, and save its value in a variable,
then pass that variable twice, rather than calling PORT_GetError twice.
Perhaps we should officially recognize CERT_DecodeDERCertificate as
a supported function and get rid of nssrenam.h.
In file cmd/signtool/certgen.c, function install_cert tries to decode a
certificate into a DER cert, and then "import" that cert into a PKCS#11
module as a new "token" object.  To do that, it calls two functions:
   CERT_DecodeDERCertificate
   PK11_ImportCertForKey
It also returns the address of the CERTCertificate struct returned by
CERT_DecodeDERCertificate.

Bob Relyea pointed out that NSS has two public functions whose purpose
is to start with a DER cert and import it into a token as a new token
object, in a single API call.  They are
  PK11_ImportDerCert
  PK11_ImportDERCertForKey
The former requires the caller to specify a slot.  The latter determines
the slot from the caller-supplied key argument.

If function install_cert was not required to return a CERTCertificate*, 
then we could change that function to just call PK11_ImportDERCertForKey.
Comment on attachment 248313 [details] [diff] [review]
accumulates review suggestions 

Please see the two suggested changes in comment 8.
Attachment #248313 - Flags: review?(nelson) → review-
Target Milestone: 3.11.1 → 3.12
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: