220016 - certutil and PP print bogus cert fingerprints

Status: RESOLVED FIXED

(NSS :: Tools, defect, P1)

Description

[Nelson Bolyard (seldom reads bugmail)]

All recent versions of certutil and PP print the same exact MD5 and SHA1
fingerprint values for ALL certificates, regardless of their content.

I believe this bug is a regression that was introduced by the fix for bug 45303,
which was checked in in the summer of 2000.  Likely all versions of NSS 3.x
beginning with 3.1 or 3.2 have had this bug.  

The fix is tiny.  Patch to follow.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Marking P1 for 3.9.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v1

This patch keeps the output format the same as it was in NSS 3.8, but fixes
the computed fingerprint values.  

But to be honest, I think the fingerprint should be displayed after the
signature, not before.	If we aren't required to preserve the format of the
output exactly,
(and I think we are not) then I would rather move the Fingerprints down.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v2

This alternative patch causes the fingerprints to appear immediately after
the signature, not before it.  I invite review comments on either patch.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Before the changes for bug 45303, certutil printed the fingerprints last,
after the cert and the trust flags, and pp printed neither fingerprints nor
trust flags.  With my patch v2, both certutil and pp print the fingerprints
immediately after the signature, and certutil additionally prints the trust
flags.  

I will checkin patch v2 shortly.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Checked in patch v2.  Marking fixed.  
Some question remains about whether NSS fingerprints will now match OpenSSL
fingerprints.  If they do not, I will reopen this bug.

Comment 6

[Wan-Teh Chang]

Comment on attachment 131925 [details] [diff] [review]
patch v2

>@@ -2386,6 +2385,7 @@
>     SECU_PrintAsHex(out, &sd->signature, "Signature", level+1);
> loser:
>     PORT_FreeArena(arena, PR_FALSE);
>+    SECU_PrintFingerprints(out, der, "Fingerprint", level+1);
>     return rv;

Would it be more correct to print the fingerprints before the "loser"
label?

Comment 7

[Wan-Teh Chang]

Comment on attachment 131924 [details] [diff] [review]
patch v1

>@@ -2377,6 +2376,7 @@
> 
>     SECU_Indent(out, level); fprintf(out, "%s:\n", m);
>     rv = (*inner)(out, &sd->data, "Data", level+1);
>+    SECU_PrintFingerprints(out, der, "Fingerprint", level+1);
>     if (rv) 
> 	goto loser;

Similar comment: I think it would be better to move
SECU_PrintFingerprints() after "goto loser;".

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

The print functions should attempt to print all they can.  
They should NOT "bail out" on the first error, but rather go on and 
attempt to print whatever they can.
In this case, even if the attempt to parse the signed portion of the
certificate fails, it is still possible to compute and display the 
fingerprint of the der cert, and that's what these patches do.