283765 - uninitialized memory read on NSSUsage structure

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Julien Pierre]

The dbx check access feature found that there is code in Stan that reads an
uninitialized NSSUsage structure. It needs to be initialized from the caller in
pk11wrap. Patch forthcoming.

Comment 1

[Julien Pierre]

Attachment: initialize NSSUsage structure

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 175624 [details] [diff] [review]
initialize NSSUsage structure

NSSUsage is one of the new(er) Stan types.  It is a multi-word struct that
takes the place of a value that was a single int before stan.  There are 
numerous problems with it, one of which is the cause of this bug.  Here
are some of them:

1. NONE of the new stan functions that take pointers to NSSUsage declare
their arguments to be pointers to const, even though they make no changes
to the NSSUsage structs.  This prevents the callers of those functions 
from having static/global const values, which is what they should be.  
Today, each function that passes an NSSUsage must create/initialize a 
non-const copy before passing it.  That's inefficient. 

The members of an NSSUsage include two PRBools and an enum, which could
have been packed all into a PRUint32 (e.g. as bit-fields).  That would 
have eliminated all issues with const and with UMRs and would have been
considerably more efficient.

2. One of the members of NSSUsage is a PRBool named anyUsage.  If non-zero,
it means "ignore all other members of this NSSUsage, and match any value".
This value should always be checked first in functions that use the values
of NSSUsages.  If it is non-zero, the other values should not be loaded.

It *should* be valid to leave the other members of NSSUsage uninitialized
when anyUsage is true.	But the function that experienced the UMR in this 
case fetched every other member of the NSSUsage first, and checked anyYsage
last.  If it had checked the value of anyUsage first, this UMR would not have
occurred.  It never actually checks the values that it loaded first, but
loading them triggers the UMRs.  Whatta waste.

A very large percentage of the memset calls done by NSS (directly or 
indirectly) are for 16 bytes or less of memory.  We want to reduce the 
number of such memset calls, not increase them.  So, I object to this
patch's use of memset to eliminate this pointless UMR.

Memset-free alternatives to eliminate this UMR include:

a) fix nss3certificate_matchUsage to not check/load the other members
of NSSUsage until after it has checked anyUsage and found it to to be 
false.	(In general, change it to not load members of argument structs
until they're needed, for efficiency.)

b) declare nss3certificate_matchUsage(), 
nssCertificateArray_FindBestCertificate(), and nssDecodedCertStr.matchUsage()
to take pointers to CONST NSSUsage, then change PK11_FindCertFromNickname()
as follows:

-    NSSUsage usage;
+    static const NSSUsage usage = {PR_TRUE };

-    usage.anyUsage = PR_TRUE; 

c) change PK11_FindCertFromNickname() as follows:

-    NSSUsage usage;
+    static NSSUsage usage = {PR_TRUE };

-    usage.anyUsage = PR_TRUE; 

d) (least efficient) change PK11_FindCertFromNickname() as follows:
-    NSSUsage usage;
+    static NSSUsage usage;

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Attachment: alternative a

This patch implements alternative a described above.  It is untested.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch alternatives a + b

This patch implaments alternatives a and b.  It builds without any new
warnings about const, but it is otherwise untested.  I prefer this one.
Julien, please review.	If you like alternatives a and/or b, and if they
solve the UMR, please feel free to commit either one.

Comment 5

[Julien Pierre]

Comment on attachment 175673 [details] [diff] [review]
alternative a

this patch is OK, but I like the other one better

Comment 6

[Julien Pierre]

Comment on attachment 175677 [details] [diff] [review]
patch alternatives a + b

I have checked in this patch. I like the proper use of const - wish NSS did it
consistently .

Checking in pki3hack.c;
/cvsroot/mozilla/security/nss/lib/pki/pki3hack.c,v  <--  pki3hack.c
new revision: 1.86; previous revision: 1.85
done
Checking in pkibase.c;
/cvsroot/mozilla/security/nss/lib/pki/pkibase.c,v  <--	pkibase.c
new revision: 1.25; previous revision: 1.24
done
Checking in pkim.h;
/cvsroot/mozilla/security/nss/lib/pki/pkim.h,v	<--  pkim.h
new revision: 1.26; previous revision: 1.25
done
Checking in pkitm.h;
/cvsroot/mozilla/security/nss/lib/pki/pkitm.h,v  <--  pkitm.h
new revision: 1.13; previous revision: 1.12
done

Comment 7

[Julien Pierre]

I forgot to mention that I verified it solved UMR with dbx . This was happening
with certutil -V . In dbx you can do "check -all" to enable the access checking
feature, which reports UMRs .