178895 - Quick decoder updates in lib/cryptohi, lib/pk11wrap, lib/softoken

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P3)

Description

[Robert Relyea]

The use of SEC_ASN1Decode and SECASN1DecodeItem should be examined in
lib/cryptohi, lib/pk11wrap, and lib/softoken to see if we can use the quick
decoder instead.

Comment 1

[Julien Pierre]

Attachment: Use QuickDER in cryptohi where possible

Comment 2

[Julien Pierre]

Attachment: Use QuickDER in softoken where possible

Comment 3

[Julien Pierre]

Attachment: Use QuickDER in pk11wrap where possible

Comment 4

[Julien Pierre]

I have completed the patches. The NSS QA passes.
However, I am not confident about all usages in these units, and these patches
could use reviews.

Comment 5

[Julien Pierre]

Taking bug.

Comment 6

[Julien Pierre]

Attachment: Use QuickDER in cryptohi where possible

Don't use mark & release

Comment 7

[Julien Pierre]

Performance enhancement. Reprioritizing to P3.

Comment 8

[Julien Pierre]

Attachment: cryptohi patch updated for 3.10, as checked in

Checking in seckey.c;
/cvsroot/mozilla/security/nss/lib/cryptohi/seckey.c,v  <--  seckey.c
new revision: 1.33; previous revision: 1.32

Comment 9

[Julien Pierre]

Attachment: pk11wrap patch updated for 3.10, as checked in

Checking in pk11sdr.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11sdr.c,v	<--  pk11sdr.c
new revision: 1.13; previous revision: 1.12
done

Comment 10

[Julien Pierre]

Attachment: softoken patch updated for 3.10, as checked in

Checking in keydb.c;
/cvsroot/mozilla/security/nss/lib/softoken/keydb.c,v  <--  keydb.c
new revision: 1.39; previous revision: 1.38
done

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

The patches shown above, already checked in, change the code so that it will
only accept DER encoding in places where it formerly accepted BER or DER.
I think this is potentially a mistake.

Although most implementations do and will tend to encode small things like
private keys in DER, the specs don't require it.  PKCS11, for example, 
explicitly says that private keys are BER encoded.  

So, this change potentially introduces an imcompatibility with some 
not-yet-known implementations.  When we run into one of those, it will be
difficult to diagnose, since it involves looking at the user's sensitive 
private key material to diagnose.  At this point in the life of NSS,
I'd prefer choices that minimize support burden, even if they have some
small performance cost.

I'm not vetoing this change, but I'd like to see more discussion.

Comment 12

[Julien Pierre]

Nelson, I had checked this in based on the superreview by Bob. I was unaware
that PKCS#11 allowed BER to be passed to a module. That would probably mean
parts of these changes should be backed out.

Comment 13

[Julien Pierre]

Comment on attachment 150054 [details] [diff] [review]
softoken patch updated for 3.10, as checked in

Bob, could you review these patches again with the issue of BER input and
PKCS#11 in mind ?

It seems according to the PKCS#11 spec, BER input is allowed in most places,
even for certs, so the patch should be backed out. That would create other
issues, since the limited ASN.1 decoder in softoken is DER only, not BER .

Before backing out this patch, I would like your opinion, rather than back it
out, ask review again, and perhaps recheck them in ... Also, perhaps only some
of them need to be removed (ie. the softoken one, but not the others).

Thanks.

Comment 14

[Robert Relyea]

OK so I'm splitting this up into to parts:
1) flag the parts of the patch in which there is absolutely no problem.
2) trying to make heads or tails of the standards.

For starters: PKCS #11 affect on the standards. PKCS #11 can at most restrict 
the existing standards more. If X509 certificates are allowed to be BER, then 
they are BER. I believe a strict reading of the spec (at least some form the 
the X509 spec) says that they are. I also believe the industry practice (and 
possibly modern codification of that practice) says you won't get very far if 
your certs aren't DER. The entire statement starting at "I believe" is pure 
conjecture, and it would be nice to verify or refute that with real quotes from 
the X509 spec.

That being said the following files have no issues:

softoken/keydb.c - This file only affects data as stored in the key3.db. This 
has always been DER. Only NSS ever writes it. That code is safe. The PKCS-8 
decoding in the softoken happens in pkcs11c.c.

cryptohi/seckey.c - This file is only cracking of FORTEZZA and DSA PQG 
parameters. If this isn't safe as DER, we need to rethink cracking certs 
altogether.

pk11wrap/pk11sdr.c - This case is netscape specific. We invented the DER data 
structure to hold this key. It's prefectly save.

 That leaves:
pk11wrap/pk11pbe.c (decoding pkcs #5 pbe's).
  and
pk11wrap/pk11pk12.c (decoding of private key info structures).

Neither of these structs are related to the PKCS #11 spec. (PKCS #11 takes 
encrypted private key info structures in the unwrap case, not decrypted 
structures).

In pk11pk12.c, the functions are operating on pkcs 8 data. The PKCS #8 spec 
says this data is BER encoded. For safety sake, we should back out the changes 
here. (BTW the user of the functions is Java, so normal NSS testing would not 
have caught any issues here).

The documentation which affects pk11pbe.c is pkcs #5 and pkcs #12. pkcs #5 
defines the ASN.1 syntax but does not say anything about the encoding. As far 
as pkcs #5 is concerned, you can encode PBE data in PER or XER if you want. 
pkcs #12, in general, encode things in BER. I couldn't where the spec specifies 
the encoding for the pkcs #5 data, but since BER is assumed throughout, it 
would be best to assume that BER is indeed used.

Summary: softoken/* cryptohi/* and pk11wrap/pk11sdr.c are definately safe.
pk11pbe.c and pk11pk12.c are not.

bob

Comment 15

[Julien Pierre]

Thank you very much for the extensive review, Bob !

I have backed out the pk11wrap/pk11pbe.c and pk11wrap/pk11pk12.c changes . We
certainly don't want to break anything related to Java, as we have many users of
JSS here.

Checking in pk11pbe.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11pbe.c,v  <--  pk11pbe.c
new revision: 1.9; previous revision: 1.8
done
Checking in pk11pk12.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11pk12.c,v  <--  pk11pk12.c
new revision: 1.10; previous revision: 1.9
done

Comment 16

[Robert Relyea]

Comment on attachment 150053 [details] [diff] [review]
pk11wrap patch updated for 3.10, as checked in

r+ for pk11sdr.c
r- for the other 2 based BER issues commented on in this bug.

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

IIRC (a big if), RFC 3280 requires the TBSCertificate portion of the cert 
(which is MOST of the cert) to be DER.  The rest may be BER.  

Comment 18

[Julien Pierre]

Section 4.1 of RFC2280 says :

4.1  Basic Certificate Fields

   The X.509 v3 certificate basic syntax is as follows.  For signature
   calculation, the data that is to be signed is encoded using the ASN.1
   distinguished encoding rules (DER) [X.690].  ASN.1 DER encoding is a
   tag, length, value encoding system for each element.

There are many different readings of this that can be made :

1) given the words "for signature calculation" this is another of those cases
where you are supposed to re-encode BER to DER to calculate the signature, and
RFC2280 doesn't actually say anything about the way the cert is actually
supposed to be encoded . Some other spec does, ie. ISO x.509 .
We would have to refer to that spec to know the real intent about the certs
encoding ...

2) the inside part is supposed to be DER encoded, the outside part isn't
required to be DER (ie.,  BER)

I think the first reading above is correct .