Closed
Bug 210179
Opened 21 years ago
Closed 20 years ago
pk12util produces grossly inflated pkcs 12 files
Categories
(NSS :: Libraries, defect, P2)
Tracking
(Not tracked)
RESOLVED
FIXED
3.10
People
(Reporter: nelson, Assigned: nelson)
Details
Attachments
(2 files)
4.15 KB,
application/octet-stream
|
Details | |
18.25 KB,
patch
|
rrelyea
:
superreview+
|
Details | Diff | Splinter Review |
I ran pk12util today to produce a .p12 file with a private key and cert chain. Then I looked at the .p12 file with dumpasn1. The bulk of the file is a large constructed OCTET STRING. Most of the definite length primitive substrings of that constructed OCTET STRING were just 1 byte each! This means that the output was fed to the encoder a byte at a time, and the final encoded output was about 3 times larger than it should have been, due to all the extra tag and length bytes of all those tiny primitive strings. Clearly, buffering encrypted data to the encoder is needed. I'd recommend buffering at least a kilobyte at a time, if possible. IMO, this should be fixed in the libraries, rather than in the utility program, if possible.
Assignee | ||
Comment 1•21 years ago
|
||
Assignee | ||
Comment 2•20 years ago
|
||
taking. I have implemented a patch for this. It GREATLY reduces the size of the pkcs12 files, and the encoder goes much faster, too. I need to test this patch with mozilla next, to make sure that mozilla can still output pkcs12 files with this patch in place. I think mozilla already has some kind of buffering, so I need to be sure they don't interact in some unexpected bad way. patch forthcoming.
Assignee: wchang0222 → MisterSSL
Assignee | ||
Comment 3•20 years ago
|
||
If you run dumpasn1 on the bad sample output above, you see something like this: 0 30 NDEF: SEQUENCE { 2 02 1: INTEGER 3 5 30 NDEF: SEQUENCE { 7 06 9: OBJECT IDENTIFIER data (1 2 840 113549 1 7 1) 18 A0 NDEF: [0] { 20 24 NDEF: OCTET STRING { 22 04 1: OCTET STRING : 30 25 04 1: OCTET STRING : 80 28 04 1: OCTET STRING : 30 31 04 1: OCTET STRING : 80 34 04 1: OCTET STRING : 06 37 04 1: OCTET STRING : 09 40 04 9: OCTET STRING : 2A 86 48 86 F7 0D 01 07 01 51 04 1: OCTET STRING : A0 54 04 1: OCTET STRING : 80 57 04 1: OCTET STRING : 24 60 04 1: OCTET STRING : 80 63 04 1: OCTET STRING : 04 66 04 1: OCTET STRING : 01 69 04 1: OCTET STRING : 30 72 04 1: OCTET STRING : 04 75 04 1: OCTET STRING : 03 78 04 3: OCTET STRING : 82 03 0A 83 04 1: OCTET STRING : 04 86 04 1: OCTET STRING : 01 89 04 1: OCTET STRING : 30 (cut off, hundreds more lines like the ones above). With my patch, you see this: [D:/nss_tip/mozilla/dist/WIN954.0_DBG.OBJ/bin] dumpasn1 /tmp/TestUser.p12-3 0 30 NDEF: SEQUENCE { 2 02 1: INTEGER 3 5 30 NDEF: SEQUENCE { 7 06 9: OBJECT IDENTIFIER data (1 2 840 113549 1 7 1) 18 A0 NDEF: [0] { 20 24 NDEF: OCTET STRING { 22 04 2341: OCTET STRING : 30 80 30 80 06 09 2A 86 48 86 F7 0D 01 07 01 A0 : 80 24 80 04 82 03 0E 30 82 03 0A 30 82 03 06 06 : 0B 2A 86 48 86 F7 0D 01 0C 0A 01 02 A0 82 02 AD : 30 82 02 A9 30 23 06 0A 2A 86 48 86 F7 0D 01 0C : 01 03 30 15 04 10 F4 AF FA 8E AA 04 6A B6 46 47 : 39 BC 2C 1C D0 4E 02 01 01 04 82 02 80 08 47 15 : 47 C7 2D C7 FD 91 6C 15 FD BD FC D7 12 C5 BD D5 : 81 A5 5E AB B2 93 C0 5C EB 3C 40 EB C4 DD 27 30 : [ Another 2213 bytes skipped ] : } : } : } 2373 30 53: SEQUENCE { 2375 30 33: SEQUENCE { 2377 30 9: SEQUENCE { 2379 06 5: OBJECT IDENTIFIER sha1 (1 3 14 3 2 26) 2386 05 0: NULL : } 2388 04 20: OCTET STRING : 52 E4 F9 93 D0 B2 A9 9B 63 4E 25 20 DC 23 D3 7F : 10 0A B3 6F : } 2410 04 16: OCTET STRING : 0B 80 F2 7E 85 7D 30 E8 04 49 9D 3C 49 F7 98 AD : } : } That's the whole file.
Assignee | ||
Comment 4•20 years ago
|
||
This patch was produced with diff -w, so it ignores whitespace changes. So, please avoid review comments on indentation, because the code actually looks better than it appears in this diff. This patch accomplishes the following: 1. Adds lots of comments, explaining the 5+ nested ASN.1 and PKCS7 enocders used in encoding a PKCS12 file. 2. renames numerous variables and functions, so that they more clearly indicate exactly what object they are or how they are used. No more are all contexts simply called "ecx". Now they have names that include prefixes such as "outer", "middle", and "inner". ASN.1 contexts have the letters A1 in their names, and PKCS7 context names include P7. Similarly functions are renamed to describe whether or not they are callbacks, and if so, what type of encoder calls them, and what they do. 3. Consolidates duplicated output functions into a single one for each type. 4. Finally, adds output buffering to the output callback function that is called by the ASN.1 encoders, and which feeds into the next PKCS7 encoder in the outward direction.
Assignee | ||
Comment 5•20 years ago
|
||
Comment on attachment 150058 [details] [diff] [review] patch v1 - from diff -w Julien, please read all the above comments in this bug, and then review this patch. Thanks.
Attachment #150058 -
Flags: review?(julien.pierre.bugs)
Assignee | ||
Comment 7•20 years ago
|
||
Comment on attachment 150058 [details] [diff] [review] patch v1 - from diff -w Wan-Teh, please review.
Attachment #150058 -
Flags: review?(julien.pierre.bugs) → review?(wchang0222)
Assignee | ||
Updated•20 years ago
|
Status: NEW → ASSIGNED
Whiteboard: awaiting review
Assignee | ||
Comment 8•20 years ago
|
||
Raising to P2 since the coding is done, and it's SUCH a big improvement.
Priority: P3 → P2
Updated•20 years ago
|
Attachment #150058 -
Flags: superreview?(rrelyea0264)
Attachment #150058 -
Flags: review?(wchang0222)
Attachment #150058 -
Flags: review?(julien.pierre.bugs)
Comment 9•20 years ago
|
||
Comment on attachment 150058 [details] [diff] [review] patch v1 - from diff -w sr=+ can be checked in without changes. Other comments: 1. Answer to NBB question in the code: integrityMech should be integrityKeyGenMech. PBE 's can generate cipher keys, cipher IV's and integrity check keys. integrityKeyGenMech is the mechanism to select the PBE to generate key key. hmacMech is the 'signing' mechanism that uses the generated key. p12exp->integrityInfo.pwdInfo.algorithm is an oid that specifies both values. 2. I'd feel more comfortable with some warnings in the code that p12enc->hmacCx and the buffer hmacCx point to the same physical context and care needs to employed when using them (don't try to use them both at the same time for instance). The current code is handling them correctly, but it requires a code review to know that the contexts are the same and not, say, clones of each other. (sr+ is not contigent on addressing these comments).
Attachment #150058 -
Flags: superreview?(rrelyea0264) → superreview+
Assignee | ||
Comment 10•20 years ago
|
||
Checking in lib/pkcs12/p12e.c; /cvsroot/mozilla/security/nss/lib/pkcs12/p12e.c,v <-- p12e.c new revision: 1.11; previous revision: 1.10 done
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•20 years ago
|
Whiteboard: awaiting review
Updated•20 years ago
|
Attachment #150058 -
Flags: review?(julien.pierre.bugs)
You need to log in
before you can comment on or make changes to this bug.
Description
•