Closed Bug 210179 Opened 21 years ago Closed 20 years ago

pk12util produces grossly inflated pkcs 12 files

Categories

(NSS :: Libraries, defect, P2)

x86
Windows 2000
defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: nelson, Assigned: nelson)

Details

Attachments

(2 files)

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.
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
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.

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.
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)
Marking P3 for 3.10
Priority: -- → P3
Target Milestone: --- → 3.10
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)
Status: NEW → ASSIGNED
Whiteboard: awaiting review
Raising to P2 since the coding is done, and it's SUCH a big improvement.
Priority: P3 → P2
Attachment #150058 - Flags: superreview?(rrelyea0264)
Attachment #150058 - Flags: review?(wchang0222)
Attachment #150058 - Flags: review?(julien.pierre.bugs)
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+
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
Whiteboard: awaiting review
Attachment #150058 - Flags: review?(julien.pierre.bugs)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: