178897 - Quick decoder updates in lib/pkcs7 lib/pkcs12 lib/smime lib/util

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P3)

Description

[Robert Relyea]

The use of SEC_ASN1Decode and SECASN1DecodeItem should be examined in lib/pkcs7
lib/pkcs12 lib/smime lib/util to see if we can use the quick decoder instead.

NOTE: some of these uses (particularly SEC_ASN1DecodeUpdate usages) need to use
the existing decoder. There are only a few, if any, changes needed in these
directorys, but there is a fair amount of investigation to make sure those
changes are correct.

Comment 1

[Julien Pierre]

Taking bug.

Comment 2

[Julien Pierre]

Performance enhancement. Reprioritizing to P3.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

The relevant messages in PKCS7 and SMIME are permitted to be BER, 
not required to be DER.

I wonder about PKCS12.

Comment 4

[Julien Pierre]

I just checked the spec from
ftp://ftp.rsasecurity.com/pub/pkcs/pkcs-12/pkcs-12v1.pdf .

From what I gathered, everything is BER-encoded, except for the certs inside the
CertBags, and CRLs inside the CRLBags, which are DER-encoded.

I'm curious what we actually do with CRLBags. I didn't know CRLs could be stored
in PKCS#12 files. I have never seen that case.

Comment 5

[Julien Pierre]

Attachment: proposed patch

- No changes for PKCS#12 - nothing is strictly DER, despite some erroneous
variable names.

- No changes for util . One was possible, but the structure was small and the
cost of the arena copy would have offset the benefit

- Minor changes in pkcs7 and smime for S/MIME capabilities and encryption key
preferences, which I believe are both DER

Comment 6

[Ian McGreer]

I'm not sure we should make these changes, given that the potential benefit is
very small.  Servers won't even use this code, and I doubt the client would see
much difference by more quickly decoding two small parts of the message.  There
could be some benefit in that the quick decoder is possibly less prone to the
kind of crash bugs we saw with the NISCC ASN.1 test suite, but again these
changes only affect minor parts of S/MIME messages (both of which are optional,
I believe), so it's probably not worth the change.

Comment 7

[Julien Pierre]

Ian,

The performance gain may be small, but I think it is still better to have
decoding code that's easy to debug if needed. Also, I think the change in
certread might lead to some decent performance improvement since the regular
decoder recurses every ASN.1 subcomponent which is very slow, even if it doesn't
need to as in this case for DER certs.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 149981 [details] [diff] [review]
proposed patch

I have not reviewed the code surrounding these patches (and cannot tell 
from the context diffs) whether the code ensures that the arena pointers
are always non-NULL before calling the decoder.  Since the QuickDER 
decoder requires an arena, we need to be sure of this.

My only other comment about this patch is that it takes two absurdly 
long lines of source and makes them even longer, without wrapping them.
Please wrap them.  They are these:

> 	    caps = NULL;
> 	    /* decode it */
>-	    if (SEC_ASN1DecodeItem(poolp, &caps, NSSSMIMECapabilitiesTemplate, profile) == SECSuccess &&
>+	    if (SEC_QuickDERDecodeItem(poolp, &caps, NSSSMIMECapabilitiesTemplate, profile) == SECSuccess &&
> 		    caps != NULL)


>     /* decode DERekp */
>-    if (SEC_ASN1DecodeItem(tmppoolp, &ekp, smime_encryptionkeypref_template, DERekp) != SECSuccess)
>+    if (SEC_QuickDERDecodeItem(tmppoolp, &ekp, smime_encryptionkeypref_template, DERekp) != SECSuccess)
> 	goto loser;
>

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 149981 [details] [diff] [review]
proposed patch

OK, I'm satisfed that poolp is never NULL.
SO, the only remaining issue is the very long unwrapped lines.
r+ with the priviso that those two long lines be wrapped.

Comment 10

[Julien Pierre]

checked in to the tip .

Checking in smime/smimeutil.c;
/cvsroot/mozilla/security/nss/lib/smime/smimeutil.c,v  <--  smimeutil.c
new revision: 1.15; previous revision: 1.14
done
Checking in pkcs7/certread.c;
/cvsroot/mozilla/security/nss/lib/pkcs7/certread.c,v  <--  certread.c
new revision: 1.9; previous revision: 1.8
done
Checking in pkcs7/secmime.c;
/cvsroot/mozilla/security/nss/lib/pkcs7/secmime.c,v  <--  secmime.c
new revision: 1.4; previous revision: 1.3
done

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Julien, 
It appears that you didn't wrap the long lines, as I requested in 
comments 8 and 9 as a condition of review.

Comment 12

[Julien Pierre]

Sorry, it was my intention to wrap them, but I have been caught doing too many
things at the same time today and forgot.

Checking in smimeutil.c;
/cvsroot/mozilla/security/nss/lib/smime/smimeutil.c,v  <--  smimeutil.c
new revision: 1.16; previous revision: 1.15