pk12util produces grossly inflated pkcs 12 files

RESOLVED FIXED in 3.10

Status

NSS
Libraries
P2
normal
RESOLVED FIXED
15 years ago
14 years ago

People

(Reporter: Nelson Bolyard (seldom reads bugmail), Assigned: Nelson Bolyard (seldom reads bugmail))

Tracking

3.10
x86
Windows 2000

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

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

15 years ago
Created attachment 126168 [details]
PKCS12 file with zillions of tiny strings
(Assignee)

Comment 2

14 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

14 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

14 years ago
Created attachment 150058 [details] [diff] [review]
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.
(Assignee)

Comment 5

14 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 6

14 years ago
Marking P3 for 3.10
Priority: -- → P3
Target Milestone: --- → 3.10
(Assignee)

Comment 7

14 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

14 years ago
Status: NEW → ASSIGNED
Whiteboard: awaiting review
(Assignee)

Comment 8

14 years ago
Raising to P2 since the coding is done, and it's SUCH a big improvement.
Priority: P3 → P2

Updated

14 years ago
Attachment #150058 - Flags: superreview?(rrelyea0264)
Attachment #150058 - Flags: review?(wchang0222)
Attachment #150058 - Flags: review?(julien.pierre.bugs)

Comment 9

14 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

14 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
Last Resolved: 14 years ago
Resolution: --- → FIXED
(Assignee)

Updated

14 years ago
Whiteboard: awaiting review

Updated

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