585842 - CERT_MakeCANickname returns static string in error case

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Matt Mueller]

Attachment: return NULL in error case.

CERT_MakeCANickname normally returns an allocated string, which the caller will free with PORT_Free.  If an error occurs, the loser block will cause it to return the static string "", which the caller would then try to free.

Comment 1

[Wan-Teh Chang]

It's clear that CERT_MakeCANickname should return
either PORT_Strdup("") or NULL on failure, but it's
not clear which one is more appropriate.  I don't
know if a nickname of "" is the same as no nickname.

I inspected the current callers of CERT_MakeCANickname
and believe that they all handle a NULL return value
properly, except that Mozilla passes the return value
to a %s printf format string, which in general cannot
handle a NULL input.  I think it is a bug for Mozilla
to pass a string to %s without null checking.

I also found some strange problems with CERT_MakeCANickname
itself.  This function hasn't changed much since NSS
was open sourced in year 2000.  I will describe the
problems tomorrow.  I will need Bob and Nelson's help
with these problems

Comment 2

[Matt Mueller]

It looks like the error cases of CERT_MakeCANickname are only memory allocation errors, so trying to PORT_strdup for the result doesn't seem useful.

Comment 3

[Wan-Teh Chang]

Attachment: return NULL in error case, by Matt Mueller (checked in: 3.12.8 and 3.13)

r=wtc.  (I added the missing semicolon to mattm's patch.)

I checked in the patch on the NSS trunk (NSS 3.13).

Checking in certdb.c;
/cvsroot/mozilla/security/nss/lib/certdb/certdb.c,v  <--  certdb.c
new revision: 1.105; previous revision: 1.104
done

Comment 4

[Wan-Teh Chang]

Attachment: Remove CERT_GetNickName, drop support for SEC_OID_NETSCAPE_NICKNAME (checked in)

In year 2000, in bug 45247, Nelson rewrote CERT_GetNickName.
The function essentially hasn't changed since then.

This MXR query shows CERT_GetNickName is not exported
in nss.def, and CERT_MakeCANickname is the only caller:
http://mxr.mozilla.org/security/search?string=CERT_GetNickName

Two bugs in CERT_MakeCANickname shows CERT_GetNickName
can be safely removed.

1. If CERT_GetNickName succeeds, its return value is
discarded by CERT_MakeCANickname:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/certdb/certdb.c&rev=1.104&mark=2034,2045-2046,2107,2109-2113#2033

2. If CERT_GetNickName succeeds, its return value was
allocated from cert->arena, but is freed with PORT_Free,
which may corrupt the heap.

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/certdb/genname.c&rev=1.38&mark=1774-1776,1790#1773

We can fix these bugs in CERT_MakeCANickname, but
I think we should just drop support for
SEC_OID_NETSCAPE_NICKNAME.  Bob, Nelson, what
do you think?

Note that this patch was generated ignoring whitespace,
because I had to change the indentation of many lines
in CERT_MakeCANickname.

Comment 5

[Wan-Teh Chang]

Attachment: Don't call CERT_IsCACert twice in CERT_ImportCerts

Save the return value in a local variable.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 465046 [details] [diff] [review]
Remove CERT_GetNickName, drop support for SEC_OID_NETSCAPE_NICKNAME (checked in)

SR += nelson

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 465055 [details] [diff] [review]
Don't call CERT_IsCACert twice in CERT_ImportCerts

r=nelson

Since you're trying to clean up this code, another cleanup you can do is 
to eliminate "freeNickname" entirely.  It is true whenever canickname is 
non-NULL, so it is redundant.  "if (freeNickname)" is equivalent to 
"if (canickname)".  But even better yet, PORT_Free always checks its 
argument for NULL, so you don't need to check before calling it.
Consequently, you can change:

-                if (PR_TRUE == freeNickname) {
-                    PORT_Free(canickname);
-                }
+                PORT_Free(canickname);

Comment 8

[Wan-Teh Chang]

Attachment: Don't call CERT_IsCACert twice in CERT_ImportCerts, v2 (checked in)

Nelson, I made your suggested change.  Thanks!
Checked in on the NSS trunk (NSS 3.13).

Checking in mozilla/security/nss/lib/certdb/certdb.c;
/cvsroot/mozilla/security/nss/lib/certdb/certdb.c,v  <--  certdb.c
new revision: 1.106; previous revision: 1.105
done

Comment 9

[Wan-Teh Chang]

Comment on attachment 465046 [details] [diff] [review]
Remove CERT_GetNickName, drop support for SEC_OID_NETSCAPE_NICKNAME (checked in)

Checked in on the NSS trunk (NSS 3.13).

Checking in certdb/cert.h;
/cvsroot/mozilla/security/nss/lib/certdb/cert.h,v  <--  cert.h
new revision: 1.81; previous revision: 1.80
done
Checking in certdb/certdb.c;
/cvsroot/mozilla/security/nss/lib/certdb/certdb.c,v  <--  certdb.c
new revision: 1.107; previous revision: 1.106
done
Checking in certdb/genname.c;
/cvsroot/mozilla/security/nss/lib/certdb/genname.c,v  <--  genname.c
new revision: 1.39; previous revision: 1.38
done
Checking in util/secoidt.h;
/cvsroot/mozilla/security/nss/lib/util/secoidt.h,v  <--  secoidt.h
new revision: 1.33; previous revision: 1.32
done

Comment 10

[Wan-Teh Chang]

Comment on attachment 464986 [details] [diff] [review]
return NULL in error case, by Matt Mueller (checked in: 3.12.8 and 3.13)

I backported this patch to the NSS_3_12_BRANCH (NSS 3.12.8).

Checking in certdb.c;
/cvsroot/mozilla/security/nss/lib/certdb/certdb.c,v  <--  certdb.c
new revision: 1.104.2.2; previous revision: 1.104.2.1
done

Comment 11

[Robert Relyea]

Comment on attachment 465046 [details] [diff] [review]
Remove CERT_GetNickName, drop support for SEC_OID_NETSCAPE_NICKNAME (checked in)

cancelling review since it already has r+ and is checked in. I only noticed white space issues.

Comment 12

[Wan-Teh Chang]

Bob: thanks.  The white space issues are not real because the
patch was generated with cvs diff -w to suppress indentation
changes.