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.
Performance enhancement. Reprioritizing to P3.
The relevant messages in PKCS7 and SMIME are permitted to be BER, not required to be DER. I wonder about PKCS12.
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.
Created attachment 149981 [details] [diff] [review] 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
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.
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 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 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.
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
Julien, It appears that you didn't wrap the long lines, as I requested in comments 8 and 9 as a condition of review.
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