211540 - regression: cert->emailAddr is never NULL

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Nelson Bolyard (seldom reads bugmail)]

The fix for bug 152986 introduced a regression in NSS.

Prior to that fix, if a cert had no email address, cert->emailAddr would be
NULL.  There are numerous places in NSS that test that pointer for NULL to 
see if the cert has an email address, when then take different courses of 
action depending on whether the pointer is NULL or not.  This undoubtedly
is causing undesirable behavior, such as creating empty attributes on some
cert objects.

The new function cert_GetCertificateEmailAddresses differs from the function 
it replaced when the cert contains no email address.  The old function returned
NULL.  The new function returns a pointer to an empty string.  This appears to
be wrong. 

I recommend that cert_GetCertificateEmailAddresses be changed to return NULL
rather than a pointer to an empty string, AND that all places that test
cert->emailAddr be changed to test for either a NULL pointer or an empty string,
e.g. 

-   if (cert->emailAddr)
+   if (cert->emailAddr && cert->emailAddr[0])

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

taking bug

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Attachment: return NULL instead of ptr to empty string.

This patch implements the first part of my propsed solution.  It will cause
the emailAddr pointer to be NULL instead of pointing to an empty string.

The second part of my proposed solution, adding testing for an empty string,
is suggested to detect any undesirable effects that have already taken place.
I will code a separate patch for that.

Comment 3

[Wan-Teh Chang]

Comment on attachment 126985 [details] [diff] [review]
return NULL instead of ptr to empty string.

>     /* now copy superstring to cert's arena */
>     finalLen = (pBuf - addrBuf) + 1;
>-    pBuf = PORT_ArenaAlloc(cert->arena, finalLen);
>-    if (pBuf) {
>-    	PORT_Memcpy(pBuf, addrBuf, finalLen);

It seems that the last byte we are copying from
(addrBuf[finalLen-1]) is never set to 0.  Am I
right?

Comment 4

[Wan-Teh Chang]

Comment on attachment 126985 [details] [diff] [review]
return NULL instead of ptr to empty string.

r=wtc.

Please ignore my previous comment.  addrBuf is
allocated with PORT_ArenaZAlloc, so all the bytes
are initialized to 0.  Sorry about my confusion.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Above patch checked in.

Comment 6

[Wan-Teh Chang]

Comment on attachment 126985 [details] [diff] [review]
return NULL instead of ptr to empty string.

I checked in this patch on the NSS_3_8_BRANCH (NSS 3.8.2)
and NSS_3_7_BRANCH (3.7.8) because this is a regression
introduced in NSS 3.7.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch part2

This patch supplements the previous patch (which is already checked in).
It detects empty emailAddr strings in certs and in the databases, and 
treats them the same as NULL emailAddr string pointers.  all.sh passes 
with this patch.

Comment 8

[Wan-Teh Chang]

Comment on attachment 132239 [details] [diff] [review]
patch part2

1. In cmd/lib/seccnames.c, function sec_CollectCertNamesAndTrust,
the new local variable 'addr' should be declared as 'char *', not
'char'.

2. I disagree with some of the changes.  I will talk to you about
them offline.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Subsequent to comment 8 above, cmd/lib/seccnames.c was removed from the tree,
so there is no need to change it further.

Wan-teh's other comments re patch part 2 above were:

a) Callers of CERT_GetFirtsEmailAddress and CERT_GetNextEmailAddress ought not
   to have to check for both a NULL pointer and a pointer to an empty string.
   The function should never return a pointer to an empty string.

b) Two essentially identical functions: 
      nsslowcert_FixupEmailAddr() in lib/softoken/lowcert.c and
      CERT_FixupEmailAddr         in lib/certdb/certdb.c
   take string pointers and return pointers to strdup'ed strings that 
   have been "fixed up" (read: downshifted).  These functions ought not
   to return NULL unless (a) they were passed NULL, or (b) strdup failed.

Since the time of those comments, code that was common to cmd/vfyutil and 
cmd/vfychain has been factored out and moved to cmd/lib.  The next patch
(part 2 v2) will address these points.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Patch part 2 - v2

I think this patch addresses the same issues as attachment 132239 [details] [diff] [review] above,
but also incorporates Wan-Teh's review feedback.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 135295 [details] [diff] [review]
Patch part 2 - v2

I'd be interested in seeing what impact, if any, this patch has on bug 224663

Comment 12

[Wan-Teh Chang]

Comment on attachment 135295 [details] [diff] [review]
Patch part 2 - v2

r=wtc.	This patch is good.  I have a question about dbck.c.
Since we are not using dbck.c right now, the question is low
priority.  I will ask Nelson the question offline.

Bob, could you review this patch, too?

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

/cvsroot/mozilla/security/nss/cmd/dbck/dbck.c,v  <--  dbck.c
new revision: 1.5; previous revision: 1.4

/cvsroot/mozilla/security/nss/cmd/signtool/util.c,v  <--  util.c
new revision: 1.16; previous revision: 1.15

/cvsroot/mozilla/security/nss/lib/certdb/certdb.c,v  <--  certdb.c
new revision: 1.56; previous revision: 1.55

/cvsroot/mozilla/security/nss/lib/certdb/stanpcertdb.c,v  <--  stanpcertdb.c
new revision: 1.60; previous revision: 1.59

/cvsroot/mozilla/security/nss/lib/pkcs7/p7decode.c,v  <--  p7decode.c
new revision: 1.14; previous revision: 1.13

/cvsroot/mozilla/security/nss/lib/pki/certificate.c,v  <--  certificate.c
new revision: 1.49; previous revision: 1.48

/cvsroot/mozilla/security/nss/lib/pki/pki3hack.c,v  <--  pki3hack.c
new revision: 1.79; previous revision: 1.78

/cvsroot/mozilla/security/nss/lib/smime/cmssiginfo.c,v  <--  cmssiginfo.c
new revision: 1.19; previous revision: 1.18

/cvsroot/mozilla/security/nss/lib/softoken/pkcs11u.c,v  <--  pkcs11u.c
new revision: 1.52; previous revision: 1.51

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

Also
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11cert.c,v  <--  pk11cert.c
new revision: 1.127; previous revision: 1.126

Comment 15

[timeless]

according to my uninformed reading, this resulted in bug 224663