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

RESOLVED FIXED in 3.10

Status

NSS
Libraries
P3
normal
RESOLVED FIXED
15 years ago
11 years ago

People

(Reporter: Robert Relyea, Assigned: Julien Pierre)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

3.87 KB, patch
Nelson Bolyard (seldom reads bugmail)
: review+
Ian McGreer
: superreview+
Details | Diff | Splinter Review
(Reporter)

Description

15 years ago
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.
(Reporter)

Updated

15 years ago
Blocks: 160805
(Assignee)

Comment 1

15 years ago
Taking bug.
Assignee: wtc → jpierre
OS: Windows 2000 → All
Priority: -- → P1
Target Milestone: --- → 3.8
(Assignee)

Updated

15 years ago
Target Milestone: 3.8 → 3.9
(Assignee)

Comment 2

15 years ago
Performance enhancement. Reprioritizing to P3.
Priority: P1 → P3
(Assignee)

Updated

14 years ago
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.
(Assignee)

Comment 4

14 years ago
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.
(Assignee)

Comment 5

14 years ago
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
(Assignee)

Updated

14 years ago
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
(Assignee)

Updated

14 years ago
Attachment #149981 - Flags: superreview?(MisterSSL)
Attachment #149981 - Flags: review?(bugz)
(Assignee)

Updated

14 years ago
Hardware: PC → All

Comment 6

14 years ago
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.
(Assignee)

Comment 7

14 years ago
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.
Attachment #149981 - Flags: review?(bugz) → review+
Attachment #149981 - Flags: superreview?(MisterSSL) → superreview?(bugz)
(Assignee)

Comment 10

14 years ago
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
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
Julien, 
It appears that you didn't wrap the long lines, as I requested in 
comments 8 and 9 as a condition of review.
(Assignee)

Comment 12

14 years ago
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

Updated

14 years ago
Attachment #149981 - Flags: superreview?(bugz) → superreview+
You need to log in before you can comment on or make changes to this bug.