230951 - incorrect template renders some valid cert extensions invalid

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Nelson Bolyard (seldom reads bugmail)]

According to RFC 3280, page 32, the policyQualifiers sequence in a
PolicyInformation is OPTIONAL, but in NSS's template for this
CERT_PolicyInfoTemplate[] in nss/lib/certdb/polcyxtn.c
this component is not optional.  Consequently, policy extensions that
are properly constructed without this sequence are reported as having
invalid policy extensions.  

Patch is forthcoming.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v1

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

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

Julien, is there any reason to believe that SEQUENCE_OF is mutually exclusive
with OPTIONAL ?

Comment 3

[Julien Pierre]

No, there is no reason why that should not work.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Julien,  May I interpret your last comment as r=jpierre?

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

/cvsroot/mozilla/security/nss/lib/certdb/polcyxtn.c,v  <--  polcyxtn.c
new revision: 1.4; previous revision: 1.3

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Kin Blas reports:

When I tried linking to a debug build of the NSS trunk, the decode call would
crash instead of just returning NULL, because it was dereferencing
policyQualifiers without checking if it was NULL::

    while ( *policyQualifiers != NULL )

The following NSS patch to polcytxn.c fixes things for me:

Index: security/nss/lib/certdb/polcyxtn.c
===================================================================
RCS file: /cvsroot/mozilla/security/nss/lib/certdb/polcyxtn.c,v
retrieving revision 1.4
diff -u -r1.4 polcyxtn.c
--- security/nss/lib/certdb/polcyxtn.c    16 Jan 2004 05:36:08 -0000    1.4
+++ security/nss/lib/certdb/polcyxtn.c    23 Jan 2004 19:30:07 -0000
@@ -178,7 +178,7 @@
     policyInfo = *policyInfos;
     policyInfo->oid = SECOID_FindOIDTag(&policyInfo->policyID);
     policyQualifiers = policyInfo->policyQualifiers;
-    while ( *policyQualifiers != NULL ) {
+    while ( policyQualifiers && *policyQualifiers != NULL ) {
         policyQualifier = *policyQualifiers;
         policyQualifier->oid =
         SECOID_FindOIDTag(&policyQualifier->qualifierID);


I will also look for other places that might be affected.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

I found two places that had this same error.  Patch forthcoming.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch part 2 - v1

This is Kin's patch applied in two places.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 139750 [details] [diff] [review]
patch part 2 - v1

Kin, please review. :)

Comment 10

[kinmoz]

Comment on attachment 139750 [details] [diff] [review]
patch part 2 - v1

r=[e-mail]

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

/cvsroot/mozilla/security/nss/lib/certdb/polcyxtn.c,v  <--  polcyxtn.c
new revision: 1.4.2.1; previous revision: 1.4

Fixed (again)

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Also fixed on trunk.

/cvsroot/mozilla/security/nss/lib/certdb/polcyxtn.c,v  <--  polcyxtn.c
new revision: 1.5; previous revision: 1.4

Comment 13

[Wan-Teh Chang]

Comment on attachment 139750 [details] [diff] [review]
patch part 2 - v1

By searching for CERT_DecodeCertificatePoliciesExtension
in the NSS source tree, I found that the
secu_PrintPolicyInfo function in cmd/lib/secutil.c has
the same problem.

I guess we (the reviewers) need guidance on how to look
for similar bugs.  I am searching for
CERT_DecodeCertificatePoliciesExtension and checking
how its return value is used.  Is that the right way?

What about code outside NSS?  If they copied our code
they will crash when they upgrade to NSS 3.9.1.  Is
there a way to avoid that?

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

My patch to bug 132942 fixes this problem, which is why I didn't see the 
problem in secutil.c in my own search.  Perhaps we should take that patch 
for NSS 3.9.1 as I had originally intended.

To find all vulnerabile places, it suffices to search all code for the string
policyQualifiers 

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

To clarify: My patch named "patch part 1 = v1" to bug to bug 132942,
which I had originally intended for NSS 3.9.1, fixes the vulnerabilities
(there was more than 1) to absent optional policy extension pieces in
nss/cmd/lib/secutil.c.

That patch is awaiting review.

Comment 16

[Wan-Teh Chang]

Attachment: patch part 3 - v1

Fixes the same problem in cmd/lib/secutil.c.

Nelson, your patch (attachment 139547 [details] [diff] [review]) is huge.
Do we really need all of that to fix this bug?

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

That patch implements a substantian enhancement.  Clearly not all of it 
is necessary to fix this bug.  However, that patch does rewrite the code
that parses and displays policy extensions (the area that is having these
crashes), and I would suggest taking the entire portion of the patch that
accomplishes that.  

Comment 18

[Wan-Teh Chang]

Comment on attachment 139770 [details] [diff] [review]
patch part 3 - v1

Nelson, taking that portion of your patch is not
that simple because the patch moves the relevant
functions to the new file pppolicy.c.

Can we just take this patch and be done with this
bug?

Comment 19

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 139770 [details] [diff] [review]
patch part 3 - v1

done.  Checked in on 3.9 branch.  

/cvsroot/mozilla/security/nss/cmd/lib/secutil.c,v  <--	secutil.c
new revision: 1.60.2.1; previous revision: 1.60  

On the trunk, I plan to fix it with my other patch... :)

Comment 20

[Nelson Bolyard (seldom reads bugmail)]

Fixed, yet another time.