Closed Bug 178897 Opened 22 years ago Closed 20 years ago

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


(NSS :: Libraries, defect, P3)



(Not tracked)



(Reporter: rrelyea, Assigned: julien.pierre)


(Blocks 1 open bug)



(1 file)

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.
Blocks: 160805
Taking bug.
Assignee: wtc → jpierre
OS: Windows 2000 → All
Priority: -- → P1
Target Milestone: --- → 3.8
Target Milestone: 3.8 → 3.9
Performance enhancement. Reprioritizing to P3.
Priority: P1 → P3
Target Milestone: 3.9 → 3.10
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 .

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.
Attached patch proposed patchSplinter Review
- 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
Summary: Quick decoder upates in lib/pkcs7 lib/pkcs12 lib/smime lib/util → Quick decoder updates in lib/pkcs7 lib/pkcs12 lib/smime lib/util
Attachment #149981 - Flags: superreview?(MisterSSL)
Attachment #149981 - Flags: review?(bugz)
Hardware: PC → All
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.

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.
Attachment #149981 - Flags: review?(bugz) → review+
Attachment #149981 - Flags: superreview?(MisterSSL) → superreview?(bugz)
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
Checking in pkcs7/certread.c;
/cvsroot/mozilla/security/nss/lib/pkcs7/certread.c,v  <--  certread.c
new revision: 1.9; previous revision: 1.8
Checking in pkcs7/secmime.c;
/cvsroot/mozilla/security/nss/lib/pkcs7/secmime.c,v  <--  secmime.c
new revision: 1.4; previous revision: 1.3
Closed: 20 years ago
Resolution: --- → FIXED
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
Attachment #149981 - Flags: superreview?(bugz) → superreview+
You need to log in before you can comment on or make changes to this bug.