371160 - bogus PKCS12_KEY_USAGE in secoid table

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P3)

Description

[Nelson Bolyard (seldom reads bugmail)]

I was studying the list of OIDs in lib/util/secoid.c, looking to see 
which extension OIDs defined in RFC 3280 were missing from that table.

Along the way I discovered that there is one OID that is duplicated.
It appears in the table twice.  There are two OIDTAGs for it.

One of those OID entries says it is an invalid extension.
The other entry says it is a supported extension!  
In fact, it is a supported extension.  

This raises a question about PL_HashTableAdd.  When you call it to add a 
string to the table that is already in the tablw, what does it do?
Does it just go ahead and add it?  
Does it return the address of the previous entry for that string?
Does it replace the previous entry with a new one?

And, after you've added a duplicate to the table, when you search for that
strin value with PL_HashTableLookup, which entry will it return?
The first one added?  
The second one added?
A randomly chosen matching entry?

I have written a patch to solve this, but I am not sure it is right until
I know the answers to the above questions.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Oh, I meant to add,  The duplicated OID entries/tags are:

SEC_OID_X509_KEY_USAGE
SEC_OID_PKCS12_KEY_USAGE

In secoid.c, these map to the byte arrays named
x509KeyUsage and pkcs12KeyUsageAttr, respectively.
The values of those arrays don't look the same, but are.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

I looked at the source to PL_HashTableAdd.  If I'm reading it correctly,
when adding a new entry, if both the "key" and "value" identically match 
the value already in the table, then it returns the address of the entry
already in the table.  But if the values don't match, then it replaces
the old entry with the new one.  

I think this means that the entry for SEC_OID_PKCS12_KEY_USAGE always 
replaces the entry for SEC_OID_X509_KEY_USAGE.  Since the entry for 
SEC_OID_PKCS12_KEY_USAGE says that the OID is an INVALID cert extension OID,
I think this means that we never actually process Key Usage extensions in
certs. If that is true, this is a bigger problem than I had first thought.
I'll have to rewrite my patch if this is true.

I guess I'll have to write a little test program and play with it a bit 
to see if/how NSS is handling key usage extensions in certs presently.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Seems the duplicate pair I found by code inspection is not the only one!
PL_HashTableAdd thinks there are also these pairs (actually triplets!):
-
    SEC_OID_ANSIX9_DSA_SIGNATURE_WITH_SHA1_DIGEST = 125,
    SEC_OID_BOGUS_DSA_SIGNATURE_WITH_SHA1_DIGEST = 126,
    SEC_OID_SDN702_DSA_SIGNATURE = 189,
-
    SEC_OID_AES_128_ECB         = 183,
    SEC_OID_AES_192_ECB         = 185,
    SEC_OID_AES_256_ECB         = 187,
-
    SEC_OID_AES_128_CBC         = 184,
    SEC_OID_AES_192_CBC         = 186,
    SEC_OID_AES_256_CBC         = 188,
-
    SEC_OID_AES_128_KEY_WRAP    = 197,
    SEC_OID_AES_192_KEY_WRAP    = 198,
    SEC_OID_AES_256_KEY_WRAP    = 199,
-
Something is WAY wrong here.  

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

OK, please ignore comment 3 above.  Those were collisions on the mechanism
hash table, not on the OID hash table.  There are many entries in the OID
table with duplicate mechanism numbers, and to be honest, I'm not sure that
the mechanism hash table is of any value.  But the collisions in that hash
table are harmless.

So, there is only one collision in the OID table, which is the collision of 
SEC_OID_X509_KEY_USAGE and SEC_OID_PKCS12_KEY_USAGE.  The latter one replaces
the former in the table, so I have to make both entries identical (except for
their tags) and then I have to alias SEC_OID_X509_KEY_USAGE to be equal to 
SEC_OID_PKCS12_KEY_USAGE.  The alias must always choose the higher value of
the pair of duplicates, not the lower value.

I'll have a patch soon.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

In secoid.c, we see this line of code:

CONST_OID pkcs12KeyUsageAttr[] = { 2, 5, 29, 15 };

PKCS12 does not define a keyUsage attribute, and neither does PKCS9.
RFC 3280 defines a keyUsage extension with the OID ( 2 5 29 15 )
and it appears that whoever wrote the above line of code was trying to 
encode the OID ( 2 5 29 15 ) but encoded it incorrectly.  
The correct encoding is { 0x55, 29, 15 }
and that happens to be the value of another OID in the OID table, 
one in the array named x509KeyUsage.
The current value in array pkcs12KeyUsageAttr doesn't match any known OID.

If I correct the faulty encoding of pkcs12KeyUsageAttr, I create a 
duplicate entry in the table, which can be fixed but potentially 
reduces binary compatibility.  

So, I've decided I'm just going to leave pkcs12KeyUsageAttr alone, and not 
correct it.  It appears to me to have been a mistake from the beginning. 
It's been wrong for about 8 years now, and won't hurt to leave it that way.

Resolving: WONTFIX.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Reopening.  I thought of a simpler fix that won't introduce any binary 
compatiblity problems.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

taking.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v1 - stop using bogus encoding, but leave it in the table

This patch redefines SEC_OID_PKCS12_KEY_USAGE  to be a synonym of SEC_OID_X509_KEY_USAGE.  It leaves the old bogus encoding in the OID table
for binary compatibility.  Newly recompiled programs will get the correct
enoding.

Comment 9

[Robert Relyea]

Comment on attachment 256428 [details] [diff] [review]
patch v1 - stop using bogus encoding, but leave it in the table

r+ rrelyea

Looks like a reasonable solution to me

bob

Comment 10

[Wan-Teh Chang]

Comment on attachment 256428 [details] [diff] [review]
patch v1 - stop using bogus encoding, but leave it in the table

Perhaps pkcs12KeyUsageAttr should also be renamed
bogusKeyUsage or bogusX509KeyUsage?

Comment 11

[Wan-Teh Chang]

Comment on attachment 256428 [details] [diff] [review]
patch v1 - stop using bogus encoding, but leave it in the table

We should also review the use of SEC_OID_PKCS12_KEY_USAGE
in p12tmpl.c.  That case has been essentially
dead code.  This patch will turn that case into
a case for SEC_OID_X509_KEY_USAGE.  We should
either remove that case or use SEC_OID_X509_KEY_USAGE
for that case explicitly.

Perhaps for maximum backward compatibility we
should define SEC_OID_PKCS12_KEY_USAGE as
SEC_OID_BOGUS_KEY_USAGE instead.  It may be
risky to enable what used to be dead code.

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

If we define SEC_OID_PKCS12_KEY_USAGE as SEC_OID_BOGUS_KEY_USAGE, 
we end up with a table that is identical to the one we have today.
In effect, all such a change would do is establish SEC_OID_BOGUS_KEY_USAGE
as a synonym of the existing SEC_OID_PKCS12_KEY_USAGE symbol and its value.
I see no value in establishing such a new synonym for an existing symbol, 
if it's not going to change anything.

It was my intent to resurrect the dead code in sec_pkcs12_choose_attr_type()
as a side effect of this change. Obviously, a much simpler way to do that 
(if that were the only goal) is simply to change that function to use 
SEC_OID_X509_KEY_USAGE as its case label.  I thought that fixing the
symbol SEC_OID_PKCS12_KEY_USAGE might have a similarly beneficial effect on 
other (third party) software.  If you think that is undesirable, I'm willing
to simply write a one-line patch for sec_pkcs12_choose_attr_type() as an 
alternative to patch v1 (attachment 256428 [details] [diff] [review]).

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Attachment: fix sec_pkcs12_choose_attr_type instead

alternative to patch v1.

Comment 14

[Wan-Teh Chang]

Comment on attachment 257701 [details] [diff] [review]
fix sec_pkcs12_choose_attr_type instead

The reason I suggest not using SEC_OID_PKCS12_KEY_USAGE in p12tmpl.c
is that NSS code should serve as sample code.  (I assume there is no
such thing as PKCS #12 Key Usage.)  Unfortunately I don't know PKCS #12
well enough to review this patch -- I don't know if we should instead
remove the SEC_OID_PKCS12_KEY_USAGE case.

I didn't object to your first patch.  I merely pointed out that your
first patch will activate dead code, and that is a risk.  Since p12tmpl.c
is probably the only user of SEC_OID_PKCS12_KEY_USAGE, as long as you've
reviewed the code in p12tmpl.c, it should be fine.

Comment 15

[Wan-Teh Chang]

Comment on attachment 257701 [details] [diff] [review]
fix sec_pkcs12_choose_attr_type instead

r=wtc.  (This patch is independent of the first patch.)

Nelson, I felt bad about declining your review request, so
I studied the attributes for a PKCS #12 "SafeBag".  I concluded
that this patch is safe and correct, because PKCS #12 allows
us to have any OID in that switch statement, and SEC_BitStringTemplate
is the correct template for SEC_OID_X509_KEY_USAGE.

However, I also verified that this case is dead code.  Neither
PKCS #12 nor PKCS #9 specifies the X.509 key usage as a
PKCS #12 attribute.  Our PKCS #12 encoding and decoding code
doesn't use X.509 key usage as a PKCS #12 attribute.  So this
case can also be safely removed, and I recommend we remove it
to avoid future confusion.

To verify our PKCS #12 code doesn't use this case:
1. In p12e.c, search for "sec_PKCS12AddAttributeToBag" and verify
that we only use this function with SEC_OID_PKCS9_FRIENDLY_NAME
and SEC_OID_PKCS9_LOCAL_KEY_ID.
2. In p12d.c, search for "sec_pkcs12_decoder_set_attribute_value"
and "sec_pkcs12_get_attribute_value", and verify that we only use
these two functions with SEC_OID_PKCS9_FRIENDLY_NAME and
SEC_OID_PKCS9_LOCAL_KEY_ID.
3. In p12d.c, examine the source code of "sec_pkcs12_set_nickname"
and verify that it only sets the SEC_OID_PKCS9_FRIENDLY_NAME
attribute.

I determined which functions to look at by searching for "->attrValue",
which indicates code that actually uses the 'attrValue' field of
the sec_PKCS12Attribute structure.

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

Wan-teh,  
IIRC, at one time, another vendor's implementation of PKCS#12 put these 
attributes into "pfx" files.  We encountered them, and our original 
PKCS#12 developer attempted to make the code decode them correctly.  
So, it seems to me the primary question is not whether they are in the 
PKCS#12 standard, but is whether we see them in the wild or not.  

But as you previously noted, because of this error in the OID table, 
this has been dead code for a long time, and therefore is obbiously 
not necessary.  So I agree that we could just drop it from that switch.

I'll write another patch.

Comment 17

[Wan-Teh Chang]

By code inspection I also verified that NSS neither encodes
nor uses the decoded "PKCS #12 Key Usage" attribute value.
So that case in the switch statement still won't be used
when we reactivate it.  We might as well just let the default
case handle it.

Comment 18

[Nelson Bolyard (seldom reads bugmail)]

On trunk:
Checking in secoid.c;  new revision: 1.36; previous revision: 1.35
Checking in secoidt.h; new revision: 1.23; previous revision: 1.22

On branch:
Checking in secoid.c;  new revision: 1.31.28.3; previous revision: 1.31.28.2
Checking in secoidt.h; new revision: 1.19.28.3; previous revision: 1.19.28.2

Watching tinderbox