263691 - UMR in get_nss_trust

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Andrew Schultz]

when loading a secure site (including bugzilla), valgrind complains about an
uninitialized memory read in get_nss_trust:

Conditional jump or move depends on uninitialised value(s)
 get_nss_trust (ckhelper.c:520) 
 nssCryptokiTrust_GetAttributes (ckhelper.c:585) 
 nssTrust_Create (certificate.c:1042)
 nssTrustDomain_FindTrustForCertificate (trustdomain.c:1236)
 nssTrust_GetCERTCertTrustForCert (pki3hack.c:586) 
 fill_CERTCertificateFields (pki3hack.c:752) 
 stan_GetCERTCertificate (pki3hack.c:805) 
 STAN_GetCERTCertificate (pki3hack.c:829)

Comment 1

[Wan-Teh Chang]

It's not immediately obvious to me how
'caTrust' may be used uninitialized, but
it might be a good idea to initialize it
to a reasonable initial value.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

nssCryptokiTrust_GetAttributes calls  nssToken_GetCachedObjectAttributes to 
fill in values into the 4 CK_TRUST variables saTrust, caTrust, epTrust, csTrust;
and if that fails, nssCryptokiTrust_GetAttributes calls
nssCKObject_GetAttributes to fill in the values for those variables.  

It is possible for nssCKObject_GetAttributes to return success even if 
some of those variables are not filled in by the PKCS11 module, because their
attribute bytes are unrecognized.  That would leave one or more of the 
variables uninitialized.  

I would say the code should either

a) initialize all those variables with values that mean "no trust", or 

b) initialize all those variables with values that mean "invalid value",
and then test all the variables for that value after the call, to find
variables that were not filled in, and take some error action in that case.

However, another question is, how does the trust object end up pointing to 
a slot that doesn't recognize all the different trust attributes?

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

No way this is P1.  No crash.  Not even incorrect behavior documented.  

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Not platform dependent

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v1

Wan-Teh, please review.

Comment 6

[Wan-Teh Chang]

Comment on attachment 169492 [details] [diff] [review]
patch v1

r=wtc.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Thanks for the quick review!

/cvsroot/mozilla/security/nss/lib/dev/ckhelper.c,v  <--  ckhelper.c
new revision: 1.32; previous revision: 1.31
done