210179 - pk12util produces grossly inflated pkcs 12 files

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Attachment: PKCS12 file with zillions of tiny strings

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v1 - from diff -w

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 5

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Marking P3 for 3.10

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 150058 [details] [diff] [review]
patch v1 - from diff -w 

Wan-Teh, please review.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Raising to P2 since the coding is done, and it's SUCH a big improvement.

Comment 9

[Robert Relyea]

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

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

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