178894 - Quick decoder updates for lib/certdb and lib/certhigh

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P3)

Description

[Robert Relyea]

The use of SEC_ASN1Decode and SECASN1DecodeItem should be examined in lib/certdb
and lib/certhigh to see if we can use the quick decoder instead.

Comment 1

[Julien Pierre]

Attachment: use QuickDER where possible in lib/certdb

Comment 2

[Julien Pierre]

Attachment: use QuickDER where possible in lib/certhigh

Comment 3

[Julien Pierre]

I just created two patches for these libraries. In several places, I have to
make a copy of the input data into the arena in order to ensure that the
resulting decoded structures point to memory that's not going to be deallocated
(ie. stack or temporary heap).

There are only two places left in the CRL code where I couldn't use QuickDER
because the API didn't take an arena. I have tested the changes using the
regular QA suite and everything passed.

Comment 4

[Julien Pierre]

Attachment: use QuickDER where possible in lib/certhigh

correct patch

Comment 5

[Julien Pierre]

Taking bug. Feel free to review th epatches.

Comment 6

[Julien Pierre]

Really taking bug. Patch reviews still needed.

Comment 7

[Julien Pierre]

Nelson pointed out potential problems in the usage of QuickDER in certv3 to look
up subject key ID. This is because of an incorrectly designed API. See bug
183612 comment 19 (http://bugzilla.mozilla.org/show_bug.cgi?id=183612#c19) for
more information.

I actually ran into a crash with certutil -A which was caused by it in my tree.
So I'm removing that usage from the patch and generate a new one. 

Comment 8

[Julien Pierre]

Attachment: updated patch

Don't use QuickDER to look up subject key ID due to memory allocation issues

Comment 9

[Julien Pierre]

Attachment: use QuickDER where possible in lib/certdb

Don't use mark & release

Comment 10

[Julien Pierre]

Attachment: use QuickDER where possible in lib/certhigh

Don't use mark & release

Comment 11

[Julien Pierre]

Comment on attachment 116371 [details] [diff] [review]
use QuickDER where possible in lib/certdb

There is something wrong in one of the files in this patch. It causes "certutil
-d . -h all -L" to assert under the MS debugger on NT. See bug 199390 .

Comment 12

[Julien Pierre]

Performance enhancement. Reprioritizing to P3.

Comment 13

[Julien Pierre]

Bob, are you sure you meant to add a review+ for this patch ? It is known to
cause problems in the test suite, which is why it has not been checked in yet
and this bug is still opened. I hoped to get to this in 3.9 but I'm not sure now
given close deadlines.

Comment 14

[Robert Relyea]

I didn't notice anything wrong in the review, but if it doesn't pass the test
suite, we definately shouldn't check it in. I'm just trying to clean up my
review log.

bob

Comment 15

[Julien Pierre]

Attachment: updated optimizations for lib/certdb and lib/certhigh

all.sh passes with these .

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 182838 [details] [diff] [review]
updated optimizations for lib/certdb and lib/certhigh

Several problems with this patch.
1. There are several functions that previously accepted NULL arena
   pointers that will now fail with NULL arena pointers, AND will
   leak allocated memory when they do.	
2. Several functions changed here have always leaked memory in 
   their error paths, (when arena was NULL), and now they leak more
   than before.  

I'm not sure whether it's OK for functions to now fail where they
previously succeeded (e.g. with NULL arenas), but I'm sure it's 
NOT ok for functions to leak if they fail.  

I think every function that fails when arena is NULL should detect
NULL arena explicitly at the begining, and return with an error 
code set before allocating anything that might leak.  In public
functions, it's not enough just to PORT_Assert(arena), because
(as we recently learned) some of our customers always test with
our optimized builds (even in their own debug builds).	So 
assertions need to be followed by tests that deal with their failures.

3. There appears to be one new leak that occurs in a success case.
That occurs in CERT_GetNickName:



>@@ -1566,21 +1582,21 @@
> 	    SECOID_FindOIDTag(&current->name.OthName.oid) == 
> 	      SEC_OID_NETSCAPE_NICKNAME) {
> 	    found = 1;
> 	    break;
> 	}
> 	current = CERT_GetNextGeneralName(current);
>     } while (current != names);
>     if (!found)
>     	goto loser;
> 
>-    rv = SEC_ASN1DecodeItem(arena, &nick, SEC_IA5StringTemplate, 
>+    rv = SEC_QuickDERDecodeItem(arena, &nick, SEC_IA5StringTemplate, 
> 			    &current->name.OthName.name);
>     if (rv != SECSuccess) {
> 	goto loser;
>     }
> 
>     /* make a null terminated string out of nick, with room enough at
>     ** the end to add on a number of up to 21 digits in length, (a signed
>     ** 64-bit number in decimal) plus a space and a "#". 
>     */
>     nickname = (char*)PORT_ZAlloc(nick.len + 24);
>@@ -1600,21 +1616,21 @@
> 		goto loser;
> 	}
> 	count++;
> 	sprintf(nickname, "%s #%d", basename, count);
>     }
> 
>     /* success */
>     if (nicknameArena) {
> 	returnName =  PORT_ArenaStrdup(nicknameArena, nickname);
>     } else {
>-	returnName = nickname;
>+	returnName = PORT_Strdup(nickname);
> 	nickname = NULL;
>     }
> loser:
>     if (arena != NULL) 
> 	PORT_FreeArena(arena, PR_FALSE);
>     if (nickname)
> 	PORT_Free(nickname);

The above new PORT_Strdup call causes nickname to be leaked in
that path.  nickname is allocated, (see the PORT_Zalloc call in
the "hunk" just above), and so the Strdup is unnecessary. 
I just wouldn't make this Strdup change, but it would also suffice 
to remove the nickname = NULL line just below the new Strdup call.

Comment 17

[Robert Relyea]

Comment on attachment 182838 [details] [diff] [review]
updated optimizations for lib/certdb and lib/certhigh

Nelson already rejected this patch, removing review request

Comment 18

[Julien Pierre]

Nelson,

Responding to comment #16 :

1. Since my patch applied to a number of functions, it would have been very helpful in your review if you could have specified exactly which functions have these problems, rather than just "several". I have had to review all of them in order to make sense of your comment.

Most of the functions actually require arenas to operate properly - otherwise there would be major problems such as leaks. Thus, I assumed they were already called with non-NULL arenas. I had done this analysis when I wrote the patch, but now I have done it all over again, and the result is below. Most of the changes I will make that result are simply to check for a non-NULL arena upon function entry. I will rename all the "arena" argument to "reqArena" in the prototype in the .c and .h files to make it clear when arenas are required.

- CERT_KeyFromDERCert is a private function. It is only called from 2 places, both with an arena. It will not leak if called with a NULL arena - it will just fail. So, no issue.

- CERT_KeyFromDERCrl is a public function . Internally, it is always called with a non-NULL arena.
Before this patch, it would allocated the returned derCrl either in the main heap if the passed-in arena was NULL, or in the arena if it wasn't - this is the behavior of SEC_ASN1DecodeItem.
With this patch, it does the same, by using SECITEM_CopyItem.
However, there is a bug in my patch where the second call to SEC_QuickDERDecodeItem uses the passed-in arena ("arena") rather than the temporary arena ("myArena") and thus the function will fail if a NULL arena is passed in. I will provide a fix for this. 
The function only allocated the memory upon decoder success, and the temporary arena is always freed. Thus, there is no leak introduced by my patch in this function.

- CERT_GeneralName is a public function. It didn't enforce a non-NULL arena upon entry. There is one previous caller which also didn't check if the arena it passed was non-NULL, CERT_DecodeAuthInfoAccessExtension .
Thus, the patch will make the function fail if arena argument is non-NULL. 

This function will leak in this case because it calls SECITEM_ArenaDupItem to make a temporary copy of the encoded name in the arena. Unfortunately, SECITEM_ArenaDupItem doesn't fail if the arena pointer is NULL ; it uses the heap instead.

To fix the leak, the function could be made to fail if arena is NULL before calling SECITEM_ArenaDupItem, since it will fail in SEC_QuickDERDecodeItem anyway without an arena. I think this is the best solution, because we can't really support this function with a NULL arena. There are pre-existing leaks in SEC_ASN1DecodeItem when called with a NULL arena because CERTGeneralName is a complex structure that should not have its individual fields allocated on the heap (eg. the array of rdns within CERTName) ; otherwise it would be extremely tedious to free. And we don't have any code anywhere to do that .

Thus, I propose that simply checking for a non-NULL arena upon function entry is the fix for this function.

- cert_DecodeNameConstraint has the same issue where it allocates something with SECITEM_ArenaDupItem which may end up on the heap instead of an arena . It is a private function which only has one caller, cert_DecodeNameConstraintSubTree , which is in turned called only by cert_DecodeNameConstraints (plural).
It returns a CERTNameConstraint structure which contains a CERTGeneralName, created by invoking CERT_DecodeGeneralName . We have already determined that CERT_DecodeGeneralName requires a non-NULL arena. Thus cert_DecodeNAmeConstraint also requires an arena.
A check should be added for it upon function entry.

- cert_DecodeNameConstraints (plural) has the same issue where it allocates something with SECITEM_ArenaDupItem which may end up on the heap instead of an arena. It is an internal function, called by a public function which does not check for an arena, CERT_DecodeNameConstraintsExtension , and by a private function, CERT_CompareNameSpace . The later is always called with an arena. 

cert_DecodeNameConstraints calls cert_DecodeNameConstraintSubTree, which calls cert_DecodeNameConstraint, which requires an arena. Thus, cert_DecodeNameConstraints should also require an arena.
A check should be added upon entry of this function .

- CERT_DecodeAltNameExtension is a public function which doesn't check for a NULL arena upon entry. It calls cert_decodeGeneralNames , which calls CERT_DecodeGeneralName, which requires an arena . Thus this function should check for a non-NULL arena upon entry.

- CERT_DecodeAuthInfoAccessExtension calls CERT_DecodeGeneralName, thus it should check for a non-NULL arena upon entry .

- ocsp_FinishDecodingSingleResponses previously called PORT_ArenaZAlloc which requires an arena. This call will fail (or even crash) if called with a NULL arena. It precedes the SECITEM_ArenaDupItem added by my patch, so I don't think there is a leak introduced by my patch, since there would be a crash or at least prior failure if a NULL arena was given. This function should be changed to check for a NULL arena upon entry.

2. Regarding pre-existing leaks on error paths with NULL arenas, please let me know if there is anything I haven't covered in the review above.

3. Regarding my change to CERT_GetNickName, you are right, that PORT_Strdup doesn't belong. I didn't notice the nickname = NULL; statement that prevents the freeing of the string upon exit .

A new patch will be forthcoming.

Comment 19

[Julien Pierre]

Attachment: Update

Comment 20

[Julien Pierre]

In comment 18, the third listed function should be "CERT_DecodeGeneralName", not CERT_GeneralName . This is important since this function is referenced in many places afterwards in my comments as having a requirement for a non-NULL arena.

Comment 21

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 232232 [details] [diff] [review]
 Update

I'm sorry that I didn't name the functions in my previous comment.  If I should do that again in some future review, please don't hesitate to ask me to add that info, and please do it right away!

This patch looks good.  We *may* have some complaints about binary compatibility (failing where it previously succeeded but leaked).  I think we'll just have to cross that bridge when we come to it.
r=nelson

Comment 22

[Julien Pierre]

Thanks for the review, Nelson . I checked in this patch to the tip.

Checking in certdb/cert.h;
/cvsroot/mozilla/security/nss/lib/certdb/cert.h,v  <--  cert.h
new revision: 1.54; previous revision: 1.53
done
Checking in certdb/certdb.c;
/cvsroot/mozilla/security/nss/lib/certdb/certdb.c,v  <--  certdb.c
new revision: 1.78; previous revision: 1.77
done
Checking in certdb/crl.c;
/cvsroot/mozilla/security/nss/lib/certdb/crl.c,v  <--  crl.c
new revision: 1.53; previous revision: 1.52
done
Checking in certdb/genname.c;
/cvsroot/mozilla/security/nss/lib/certdb/genname.c,v  <--  genname.c
new revision: 1.31; previous revision: 1.30
done
Checking in certdb/genname.h;
/cvsroot/mozilla/security/nss/lib/certdb/genname.h,v  <--  genname.h
new revision: 1.10; previous revision: 1.9
done
Checking in certdb/xconst.c;
/cvsroot/mozilla/security/nss/lib/certdb/xconst.c,v  <--  xconst.c
new revision: 1.9; previous revision: 1.8
done
Checking in certhigh/ocsp.c;
/cvsroot/mozilla/security/nss/lib/certhigh/ocsp.c,v  <--  ocsp.c
new revision: 1.27; previous revision: 1.26
done