354003 - Permit dynamic OIDs to override built-in OIDs

Status: ASSIGNED

(NSS :: Libraries, enhancement, P2)

Description

[Nelson Bolyard (seldom reads bugmail)]

Today, NSS has a built-in constant table of OIDs, and has a second "dynamic" 
table of OIDs to which an application can add its own OID definitions, 
extending the set of OIDs that NSS recognizes.  The appliation can extend 
the set of known OIDs, but an appliation cannot override the built_in set.

If an appliation wishes to change the state of a known OID, from 
UNSUPPORTED_CERT_EXTENSION to SUPPORTED_CERT_EXTENSION, the application
cannot override the built-in OID definition adding a new definition to 
the dynamic table.  The application can add the entry to the dynamic table
but it will never be found, because NSS always searches the built-in table
first, and does not search the dynamic table when it finds a match in the
built-in constant table.

So, I propose to change the behavior of SECOID_FindOID to search the 
dynamic table first, thereby allowing dynamic entries to override the 
built-in constant ones.  

This would take a lot of pressure OFF of NSS to implement certain changes
of dubious security.

Patch forthcoming.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v1, search dynamic table first

I think this is all that's needed.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

I'm looking to see if there's consensus for this RFE.

Comment 3

[Wan-Teh Chang]

Comment on attachment 239844 [details] [diff] [review]
patch v1, search dynamic table first

Bob and Julien are more qualified than I am to
review changes to certificate handling code.  The
patch is correct, but it's the implications of the
patch that need to be reviewed.

Comment 4

[Robert Relyea]

This seems a good idea.

I was a little worried that you could redefine certain oids to mean something completely different (like changing SEC_OID_SHA1 to mean some other hash algorithm), but added oids is already a trusted operation, so I don't see a problem.

bob

Comment 5

[Robert Relyea]

Comment on attachment 239844 [details] [diff] [review]
patch v1, search dynamic table first

NOTE: This change will effect parsing of raw oids presented to us. It does not effect any built-in use of the oid tag.

I think this is OK (and probably prefered).

There should be a negligible performance penalty for looking up a known oid if there are not dynamic oids defined
(An extra PORT_SetError call, which we could remove).

The only fields we can change by adding new dynamic oids are 1) the cert extension value [the desired affect of the change] 2) the PKCS #11 mechanism that maps this oid [described above] 3) The description of the oid value. 4) the OID tag [the old OID tag will still work and map to the old definition].

bob

Comment 6

[Julien Pierre]

I'm not opposed to doing this, but I think we need to further review the usage of the OID table and what code it applies to before we make this type of change, to make sure we don't create more problems down the road.

There are inadequacies with the existing OID table system.

Some parts of the existing NSS code support certain OIDs, but others don't. For example, the AuthorityKeyID extension is supported in certificates, but not in CRLs. The current system is a global table - which is the union of the static and dynamic OID table.

We also already know that the current static OID table is not appropriate for libpkix, because it will have a different set of supported features. In NSS 3.12, we will have two implementations of PKIX - the current legacy CERT_VerifyCert, which won't support policy OIDs or other OIDs, and the new APIs will support some of them.

I think this shows that one OID table is currently inadequate, and will be even more so in 3.12 . My original proposal for 3.12 was going to be to have 2 separate OID tables, one for the legacy code and one for libpkix.

It seems that this is another instance where we need an alternate version of the OID table. Perhaps we can devise a better solution.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Julien, does your comment 6 constitute r+ ? or r- ? or something else?

Comment 8

[Julien Pierre]

Nelson, at this time, it constitutes neither. I'm not ready to give review on the patch before the issues I pointed out in comment 6 are further examined.

Comment 9

[Alexei Volkov]

Comment on attachment 239844 [details] [diff] [review]
patch v1, search dynamic table first

patch is incomplete in a sense that it still does not allow to add a an OID into a dynamic table if that OID already exists in static table.

Function SECOID_AddEntry
fails to overwrite an existing OID :

1629     */
1630     ret = SECOID_FindOIDTag(&src->oid);
1631     if (ret != SEC_OID_UNKNOWN) {...}

Comment 10

[Wan-Teh Chang]

Comment on attachment 239844 [details] [diff] [review]
patch v1, search dynamic table first

This patch modifies a file that may be used by the softoken.
Please think twice before fixing this bug in NSS 3.11.4.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

remove target milestone, since the target was missed.

Comment 12

[Sean Leonard]

Has this been worked on and completed? Just curious.