371685 - allow unsupported critical extensions in special builds

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P3)

Description

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v1

Right now, NSS strictly enforces RFC 3280's requirement that it dishonor 
certs with unknown critical extensions.  That's correct and appropriate for
the vast majority of NSS users, and is how we want general NSS releases to 
behave.  But there are some special users of NSS who want it to ignore 
unsupported critical extensions (specifically those defined in RFC 3280) 
in builds made specifically for them.  Hence this RFE.

This RFE proposes to be able to build a special version of NSS through 
conditional compilation.  The special version would have a special version
string identifying it as a Customized or Non-Standard build.  To build that
way, one would define the environment variable NSS_ALLOW_UNSUPPORTED_CRITICAL
before building NSS.  

Attached is a patch to implement this, for review.

Some things to review:
a) I add the new string to NSS_VERSION in nss.h.  This is different that the
approach used to add "(debug)".  To be honest, I don't understand why the 
debug string wasn't added in the same way as my new string.  If there is some
reason why adding this string to NSS_VERSION is unacceptable, please inform 
me.  It seems to work on Windows and solaris.

b) I added the string "(Customized build)" to NSS_VERSION.  I could have used
another string, such as "(Non-Standard build)".  Do you think that would be
better?  Any other suggestions?

I originally used the name NSS_ALLOW_UNSUPPORTED_CRITICAL_RFC3280_EXTENSIONS
in the ifdefs, but I discovered that the windows resource compiler truncates
any symbols defined by -D on the command line to 31 characters, and that
once so truncated, the ifdef was found to be false, because the truncated 
symbol didn't match the name in the ifdef.  So I shortened it to the present 
30 character string.

Comment 1

[Robert Relyea]

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

r+ rrelyea :(

Comment 2

[Wan-Teh Chang]

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

I hope your build system has a way to apply a local patch to
a standard NSS release.  Such customizations should ideally stay
in vendors' source trees.

You can move the definition of the FAKE_SUPPORTED_CERT_EXTENSION
macro from secoidt.h to secoid.c.

The reason " (debug)" is not defined in nss.h is that the version
string should consist of only the version number and an optional
"Beta" marker.  The ECC string sets a precedence of breaking this
rule -- it's much less work to add a string to NSS_VERSION than to
all the *.rc and *ver.c files.  We could move the definition of the
ECC string and " (debug)" to a private header so that they only need
to be defined once.

Comment 3

[Wan-Teh Chang]

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

I reviewed this patch again.

I verified that all the OIDs defined in RFC 3280 under the
id-ce and id-pe arcs are added by this patch and the values
of the OIDs are correct.

Nelson, RFC 3280 also defines two OIDs, anyPolicy and
anyExtendedKeyUsage, that are closely related to the
x509CertificatePolicies and x509ExtKeyUsage OIDs.  Do
we need to add anyPolicy and anyExtendedKeyUsage, too?

Some comments and suggested changes.

1. It would be nice to spell "CRL" consistently.  The
only exception to this rule in secoid.c is "x509CrlNumber".

2. You can combine these two lines:

  /* Standard x.509 v3 Certificate Extensions */
  /* Standard x.509 v3 CRL Extensions */

into one line:

   /* Standard x.509 v3 Certificate and CRL Extensions */

3. Fix the typo "RfC 3280" in secoid.c.  'f' should be capitalized.

4. I'm curious how you determined which OIDs should be "fake supported
cert extensions". 

5. I wonder if we can add a new flag for NSS_Initialize to enable
"fake supported cert extensions".  This way you won't need to produce
customized builds.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Committed on bypass branch.
coreconf/config.mk;     new: 1.17.28.2.10.1; previous: 1.17.28.2
nss/lib/nss/nss.h;      new: 1.46.2.15.2.1;  previous: 1.46.2.15
nss/lib/util/secoid.c;  new: 1.31.28.1.12.1; previous: 1.31.28.1
nss/lib/util/secoidt.h; new: 1.19.28.1.12.1; previous: 1.19.28.1

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v2 for trunk (updated for Camellia addition + WTC's comments)

This patch is essentially the same as the previous one, except that it 
applies cleanly to the trunk now that the Camellia patch has landed.
I also applied the changes Wan-Teh suggested.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Wan-Teh asked:

> RFC 3280 also defines two OIDs, anyPolicy and anyExtendedKeyUsage, that 
> are closely related to the x509CertificatePolicies and x509ExtKeyUsage OIDs.
> Do we need to add anyPolicy and anyExtendedKeyUsage, too?

Those OIDs do not identify extensions, but rather are special policy OIDs.
They would appear inside of a policy extension, but would not be a type of 
extension.  We will need them in NSS 3.12 with libPKIX, I believe, but not 
in 3.11.x, where we only want to be able to ignore certain extensions.

> I'm curious how you determined which OIDs should be "fake supported
> cert extensions". 

RFC 3280 defines extensions which MUST be critical, and which MAY be 
critical, and which MUST NOT be critical.  The set of "fake" extension OIDs
are the OIDS for extensions defined in RFC 3280 as MUST or MAY be critical,
that were previously unknown to NSS.  As a result of this change, NSS will
now at least recognize all the extension OIDs defined in RFC 3280 that 
could possibly be critical, even if NSS does support them per se'.
One immediate benefit of this change will be that NSS will be able to 
print the names of these types of extensions, where previously it could 
only print the OID in dotted decimal form.

One unusual thing about patch v2: it changes the "OIDTag" numbers for the 
recently-added Camellia algorithms.  This seems to be necessary in order
for these new OIDTags to have the same values in both 3.12 and 3.11.7.
I figure this is legitimate, since we have not yet released NSS 3.12.

Once 3.12 is released with the Camellia OIDTags, it will not be possible
to add more OIDTags to the 3.11 branch after that, without also defining
the camellia OIDTags on the 3.11 branch, to keep the OIDTag numbers in sync.
(There can't be any "holes" in the OIDTag space.)

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Wan-Teh wrote:

> I wonder if we can add a new flag for NSS_Initialize to enable
> "fake supported cert extensions".  This way you won't need to produce
> customized builds.

I would surely like to do that, but I haven't yet found a good way to do it.
The table named "oids" in secoid.c is const, so we can't alter it at run time.
If we could relax that restriction, we'd have more options.
  
The structure definition for entries in that table, and the values allowed 
for the "supportedExtension" member, are publicly defined, so I believe we
would break binary compatibility if we introduced new values.  

There are functions that return addresses of the entries in the table.  
SECOID_FindOIDByTag depends on the OIDTag being the index into the table.  
There is an assertion to enforce this in secoid_Init().  

One idea that I think might be workable would be to have two complete
copies of the oids table that are identical except for the value of the
"supportedExtension" member in the "fake" entries.  A global flag would
choose one of the two tables at run time.  

More ideas are welcome!

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 263561 [details] [diff] [review]
patch v2 for trunk (updated for Camellia addition + WTC's comments)

Bob, please review the change to the OIDTag numbers for Camellia ciphers, for 3.12

Comment 9

[Wan-Teh Chang]

Comment on attachment 263561 [details] [diff] [review]
patch v2 for trunk (updated for Camellia addition + WTC's comments)

r=wtc.  Some lines in the patch are not aligned right, for example,

> CONST_OID x509IssuerAltName[]         		= { ID_CE_OID, 18 };
> CONST_OID x509BasicConstraints[]      		= { ID_CE_OID, 19 };
>+CONST_OID x509CRLNumber[]                    	= { ID_CE_OID, 20 };
>+CONST_OID x509ReasonCode[]                   	= { ID_CE_OID, 21 };
>+CONST_OID x509HoldInstructionCode[]             = { ID_CE_OID, 23 };
>+CONST_OID x509InvalidDate[]                     = { ID_CE_OID, 24 };
>+CONST_OID x509DeltaCRLIndicator[]               = { ID_CE_OID, 27 };
>+CONST_OID x509IssuingDistributionPoint[]        = { ID_CE_OID, 28 };
>+CONST_OID x509CertIssuer[]                      = { ID_CE_OID, 29 };
> CONST_OID x509NameConstraints[]       		= { ID_CE_OID, 30 };
> CONST_OID x509CRLDistPoints[]         		= { ID_CE_OID, 31 };

and

>+#if defined(NSS_ALLOW_UNSUPPORTED_CRITICAL)
>+#define FAKE_SUPPORTED_CERT_EXTENSION   SUPPORTED_CERT_EXTENSION
>+#else
>+#define FAKE_SUPPORTED_CERT_EXTENSION UNSUPPORTED_CERT_EXTENSION
>+#endif

Perhaps those lines have tabs?

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Patch updated for 3.11 branch

This patch also incorporates two fixes for secoid.c from attachment 244522 [details] [diff] [review]
to bug 358785.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 263666 [details] [diff] [review]
Patch updated for 3.11 branch

Please review this update.

Comment 12

[Robert Relyea]

Comment on attachment 263561 [details] [diff] [review]
patch v2 for trunk (updated for Camellia addition + WTC's comments)

r+. We haven't shipped 3.12, even in beta, so the oid table renumber is fine.

Comment 13

[Wan-Teh Chang]

Comment on attachment 263666 [details] [diff] [review]
Patch updated for 3.11 branch

r=wtc.

Note: this patch is inside the FIPS crypto boundary (lib/util).

Comment 14

[Julien Pierre]

Nelson,

As far as how to switch this (mis)feature at runtime, I think you are on the right track when you mention having several copies of the table - as I suggested to you a few weeks ago.

But there are other possibilities. I'm not sure that we need to keep that table as const. I believe it is not directly exposed to anyone as it is static in secoid.c and not returned by any function. So, we could switch it to non-const, and just have NSS_Initialize poke into the table to allow the unsupported extensions. But certainly having 2 tables would work too.

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

Functions SECOID_FindOID, SECOID_FindOIDByTag, and SECOID_FindOIDByMechanism 
all return the addresses of entries from the table.  

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

The FIPS/crypto boundary comment reminds me that presently We link separate
copies of libutil.a into our various shared libs.  We have separate copies
of the OIDs table, the dynamic oids table, etc.  When an application 
registers a new dynamic OID, it only affects one copy, etc.  copies=Bloat. 

One the one hand, we can argue that this means that an application doesn't
change the behavior of softoken when it registers dynamic OIDs, which might
be good for FIPS purposes.  But the bad news is that every change to the 
code in lib/util affects code in the FIPS boundary, whether it changes the
softoken behavior or not.  It changes code in a source file, some parts of
which are used in the FIPS boundary.  

Making lib/util a shared library reduces the bloat, but probably only makes
the FIPS boundary issue even worse.  It's very unfortunate, IMO, that 
softoken has to have a DER decoder and has to parse certificates.  :(

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

On trunk:
coreconf/config.mk;     new revision: 1.19; previous revision: 1.18
nss/lib/util/secoid.c;  new revision: 1.35; previous revision: 1.34
nss/lib/util/secoidt.h; new revision: 1.22; previous revision: 1.21
nss/lib/nss/nss.h;      new revision: 1.51; previous revision: 1.50

Comment 18

[Nelson Bolyard (seldom reads bugmail)]

On 3.11 branch
coreconf/config.mk;     new revision: 1.17.28.3; previous revision: 1.17.28.2
nss/lib/util/secoid.c;  new revision: 1.31.28.2; previous revision: 1.31.28.1
nss/lib/util/secoidt.h; new revision: 1.19.28.2; previous revision: 1.19.28.1
nss/lib/nss/nss.h;      new revision: 1.46.2.17; previous revision: 1.46.2.16

Watching tinderboxes.