130703 - NSS 3.4 exported header issues

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Wan-Teh Chang]

I am reviewing the NSS 3.4 exported headers.  This bug will
be used to track the issues I found in my review.

Comment 1

[Wan-Teh Chang]

Attachment: Proposed patch

Comment 2

[Wan-Teh Chang]

Attachment: Proposed patch

I added a declaration for the exported function
CERT_ImportCAChainTrusted to cert.h.

Here is a summary of the changes in the patch.

1. I added some header #include's to fix the implicit
function declaration compiler warnings.

Ian, I found that dev3hack.c is not passing the second
argument to nssToken_DestroyCertList().  I pass PR_TRUE
as the second argument in my patch.  Is this correct?

2. Stan headers don't need to be exported in NSS 3.4.
These include nssasn1t.h, nssdevt.h, nsspki.h, and
nsspkit.h.

The only things certt.h needs from nsspkit.h are forward
declarations of struct NSSCertificateStr and struct
NSSTrustDomainStr.

3. In certt.h, the new fields of CERTCertificate and
CERTSignedCrl should occupy the positions of obsolete
or unused fields if possible, otherwise they should be
added to the end of the structures.

4. I added the include-once macros to some headers.

5. I added the missing function declarations for
SEC_PKCS5GetIV and CERT_ImportCAChainTrusted.

Comment 3

[Wan-Teh Chang]

Some of the issues I discovered break binary compatibility
with NSS 3.3.x, so they must be fixed in NSS 3.4.

Please review my patch.  Thanks.

Comment 4

[Robert Relyea]

Comment on attachment 74013 [details] [diff] [review]
Proposed patch

I like the bug fix in nssToken_DestroyCertList():).

To summarize for other reviewers, most of these changes are to 1) presever the
position of 3.3 elements in the data structures (some new field s replace some
old depricated fields where possible to preserve the size total size of the
structure).

stan headers are removed from public exports (majority of the changes).

Some exported functions didn't have prototypes in the public headers.

I saw one bug fix (NSSToken_DestroyCertList)

The changes look right to me. I've particularly reviewed the stuff in cert.h
and certt.h

Comment 5

[Wan-Teh Chang]

Comment on attachment 74013 [details] [diff] [review]
Proposed patch

Thanks for the code review and the nice summary, Bob :-)

I have a few questions.

1. The 'dbEntry' member of CERTCertificate and the 'dbEntry' and
'keep' members of CERTSignedCrl were removed in 3.4.  Can we be
confident that no clients of 3.3 are using those struct members?

2. The 'dbhandle' member of CERTSignedCrl is unused in 3.4.
Would you prefer that the new member 'slot' takes its position?

3. mozilla/security/nss/lib/jar/jarjart.c

>@@ -76,9 +76,8 @@
> 
>   ugly_list = (char **) data;
> 
>-  if (cert && cert->dbEntry)
>+  if (cert)
>     {
>-    /* name = cert->dbEntry->nickname; */
>     name = cert->nickname;

Please make sure that the check for cert->dbEntry is
unnecessary or redundant in 3.3.  I suspect it's
unnecessary because the only line that uses cert->dbEntry
was commented out.

In 3.3, is a cert with a non-NULL cert->dbEntry a
different kind of cert from a cert with a NULL
cert->dbEntry, or were we just trying to avoid a crash
in cert->dbEntry->nickname?

4. mozilla/security/nss/lib/pk11wrap/dev3hack.c

>@@ -186,7 +187,7 @@
>                                                        nss3slot->session,
>                                                        nss3slot->sessionLock,
>                                                        nss3slot->defRWSession);
>-    nssToken_DestroyCertList(token);
>+    nssToken_DestroyCertList(token, PR_TRUE);
>     return nssToken_LoadCerts(token);
> }
>

So you think that it is correct to pass PR_TRUE as the second
argument to nssToken_DestroyCertList?

Comment 6

[Ian McGreer]

The argument passed to nssToken_DestroyCertList should be PR_TRUE.  The patch is
correct.

Comment 7

[Robert Relyea]

Crls: We can be pretty confident about the non-use of keep, mostly because the
number of CRL aware clients are small, and the number of CRL exported interfaces
in NSS 3.3 was even smaller.

Certs: This one is less confident. I'm quite confident that any usages would be
rare. The DBEntry pointed to the actual low level database entry for the
certificate. I know PSM was using it in some cases. I'm pretty sure no NSS 3.3
app was using it. The field wasn't guarrentteed to be filled in for any given
cert (because not all certs came from the database). If we are paranoid we could
give up  the certificate size invariant and just make DBEntry a NULL pointer.

The PSM uses of the DBEntry was similiar to the one in libjar. It was a misuse
trying to play with nicknames. The PSM usage has been removed for a while now,
so it's reasonably safe. I think it was code copied from pre-HCL days. The
commented code in libjar was so it would compile when dbEntry became a void *.

bob

Comment 8

[Wan-Teh Chang]

Attachment: Final patch to certt.h

1. Keep CERTCertificate as is.	The new field 'nssCertificate'
takes the position of the deprecated field 'dbEntry'.  The
size of CERTCertificate is preserved.

2. Move the new fields of CERTSignedCrl to the end of the
structure.  Renamed the deprecated fields 'dbEntry' and 'keep'
to 'reserved1' and 'reserved2'.

Bob, please review this patch and verify it is what we agreed
on.

Comment 9

[Wan-Teh Chang]

Attachment: Diffs of certt.h against NSS 3.3.2

This patch shows exactly what changed in the CERTCertificate
and CERTSignedCrl structures between 3.3.2 and 3.4.

Comment 10

[Robert Relyea]

Comment on attachment 74381 [details] [diff] [review]
Final patch to certt.h

Yes, that looks correct.

bob

Comment 11

[Wan-Teh Chang]

I checked in attachment 74381 [details] [diff] [review].  Marked the bug fixed.