303457 - extensions newly supported in libpkix must be marked supported

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P1)

Description

[Julien Pierre]

In NSS 3.12, we are going to add support for policy extensions . These will be
supported only in new verification APIs that replace CERT_VerifyCert .

The way we treat policy right now, since it is marked critical and it is not in
the OID table of supported extensions, the cert decoding fails completely.

On the NSS_LIBPKIX_BRANCH, we had to mark the following OIDs as supported in
secoid.c :
SEC_OID_X509_CERTIFICATE_POLICIES
SEC_OID_X509_POLICY_MAPPINGS
SEC_OID_X509_POLICY_CONSTRAINTS
SEC_OID_X509_INHIBIT_ANY (this is a new OID not previously defined)

We still want CERT_VerifyCert to consider those extensions unsupported and fails
if a cert contains them. But CERT_DecodeDERCertificate and PKIX_BuildChain (or
its wrapper) should not fail.

Thus, we need a way to mark these OIDs as "half-supported", ie. supported by
PKIX but not CERT_VerifyCert. This could be accomplished with extra flags.

I notice that SECSupportExtenTag is an enum, with values 0, 1 and 2 defined only
today.  If this is a public type, then we may not be able extend this type to
accomplish what's needed here (a bit flag would do). Do we have to have a 2nd
OID table ?

Comment 1

[Julien Pierre]

This may be a bigger problem than just for policy extensions.

libpkix might support other extensions that CERT_VerifyCert doesn't support.
In bug 321765, we are allowing the cert to be decoded regardless of unsupported extensions.

However, we need another bit to tell if the cert contains extensions not supported by libpkix. So we need a separate list of supported extensions for libpkix.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Because the use of libPKIX can be enabled and disabled in 3.12, 
the secoid table must adjust as libPKIX is enabled and disabled.

Note that there are already 2 or 3 separate tables of known OIDs in NSS.
Let's not add any more.  Let's try to get it down to no more than two.

Comment 3

[Julien Pierre]

At this point, all the aforementioned OIDs are marked as supported, except SEC_OID_X509_INHIBIT_ANY_POLICY .

This means that the old CERT_VerifyCert code will happily accept certs with policy extensions without doing the policy processing.

Bob, Alexei, I am also curious what the wrapper will do when NSS_ENABLE_PKIX_VERIFY is set.

Comment 4

[Julien Pierre]

I digged deeper into this.

Right now, we always decode all certs, regardless of any unsupported critical extensions the cert may have. If it does, we save this info into the cert structure bit options.bits.hasUnsupportedCriticalExt . This is due to the change I made a couple of years ago in bug 321765 . The legacy cert library (cert_VerifyCertChainOld) then checks this bit and will fail validation if it is set. Since we decided on tuesday to add another enum value to SECSupportExtenTag, I think we could easily add a second bit in the cert such as options.bits.hasUnsupportedCriticalExtLibpkix .

However, I don't see any code in libpkix that currently checks for this cert bit. Is there separate code in libpkix that checks for unsupported extensions ?  I did searches for SUPPORTED_CERT_EXTENSION in libpkix, but didn't find anything. I'm wondering if libpkix is currently allowing any certs with unknown critical extensions at this time. Alexei, can you please comment on this ?

Thanks.

Comment 5

[Alexei Volkov]

pkix_CheckCert is the function that in particular checks that libpkix satisfies all critical extension. Apparently, people who adopt lib pkix for nss were not aware of this flag. Libpkix uses its internal mechanism to track a support for a cert in term of extensions.

It works as follow: 

libpkix request a list of critical extension oids of the cert.

Then, lib pkix calls its cert checkers to validate the cert. A cert checker will remove an oid(s) of the extension that it knows about from the list of a cert critical extension oids.

At the end, pkix_CheckCert will throw "unknown critical extension " error if the list is not empty.

I'm sure with the options.bits.hasUnsupportedCriticalExtLibpkix we do not need the whole complexity. The function can be modified to immediately throw an error after checking the flag.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

The logic Alexei documented in comment 5 seems correct, even if complex.
It seems that libPKIX can determine when the cert contains unknown critical
extensions, without regard to the content of the OID table or the options.bits.hasUnsupportedCriticalExt flag bit.

Perhaps a path to solution is this:
1) change the OID table back to show the set of extensions understood by the 
old cert code, not by libPKIX, and 
2) only check the options.bits.hasUnsupportedCriticalExt flag bit in code 
paths that use the old cert code, but not in paths that use libPKIX.  
(I'm not sure that's feasible.  We'd need to look at all the places that check
that flag bit to see if they can know or infer whether libPKIX is being used.)

Comment 7

[Julien Pierre]

Thanks, Alexei. Bug 321765 was fixed after the libpkix work started, so the bit wasn't available for libpkix to check back then. I was afraid that libpkix relied on the decoder to check for unsupported extensions. I am glad this is not the case.

Nelson, I think your proposal is probably the way to go. The OID table only had to be changed because otherwise NSS (back in the 3.10 days when libpkix work was started) would not be able to decode the certs at all, thus libpkix could never validate them. But with the fix I made for bug 321765, this is no longer a problem. So, we can revert those OIDs to the old values. It will mean the OID table and functions that use it will only be relevant to the legacy cert library. And we don't have to add an enum value. There is only a single place in all of NSS that currently checks for the hasUnsupportedCriticalExt , and that's in  cert_VerifyCertChainOld , which is the legacy cert library. So there should be nothing else to change.

One thing I'm a little bit concerned with is which OIDs I have to revert the value for. I don't know if the list provided at the top of this bug is still accurate. There may be other OIDs that we changed the values for.

Comment 8

[Julien Pierre]

Attachment: Revert policy OIDs to unsupported status

This is safe to do since we determined that libpkix has its own logic for checking supported extensions which is independent of this table.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 313024 [details] [diff] [review]
Revert policy OIDs to unsupported status

I suspect it was the intent of this patch to exactly back out 
the checkin that was revision 1.37.  However, this patch does
not do that, exactly.  

For x509PolicyMappings, rather than reverting from 
SUPPORTED_CERT_EXTENSION to UNSUPPORTED_CERT_EXTENSION 
(as it was in rev 1.36), this patch changes it to a value it
never previously had, namely, FAKE_SUPPORTED_CERT_EXTENSION.

The reason that this extension OID was previously UNSUPPORTED
instead of FAKE_SUPPORTED is that RFC 3280 says this extension
"MUST be non-critical."  The value UNSUPPORTED enforces that.

If it was your intention to back out rev 1.37, and cause those
lines to revert to their state in 1.36, please submit another
patch that does that.  Otherwise, Please explain the rationale 
for going to FAKE_SUPPORTED here.

Comment 10

[Julien Pierre]

I thought FAKE_SUPPORTED_CERT_EXTENSION was meant to pretend to support extensions that we didn't really. I am fine with UNSUPPORTED as this follows RFC 3280. Just let me know if these are all the correct OIDs and I will check that change in.


 

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

FAKE_SUPPORTED_CERT_EXTENSION is intended for use ONLY with extension OIDs 
that RFC 3280 says MUST or MAY be critical.

The command 

  cvs diff -uw5 -r 1.37 -r 1.36 util/secoid.c

will give you exactly the patch you need. 

One last thought.  We _think_ this is going to give us the result we want,
but we're not entirely sure, and it could surprise us unpleasantly.
We should do some testing with certs that have these extensions. 
But I doubt that we're prepared to do so. :(
The 3.12 RC1 tag will probably be generated on Sunday or Monday, so let's 
be sure that we either have this patch thoroughly tested or backed out 
by then.

Comment 12

[Julien Pierre]

Attachment: Change policy mapping to unsupported

This patch is the output o fcvs diff -uw5 -r 1.37 -r 1.36 secoid.c 

I checked it in to the trunk.

Checking in secoid.c;
/cvsroot/mozilla/security/nss/lib/util/secoid.c,v  <--  secoid.c
new revision: 1.44; previous revision: 1.43
done