Regression in ASN1 Decoder



17 years ago
16 years ago


(Reporter: Javier Delgadillo, Assigned: Ian McGreer)



Firefox Tracking Flags

(Not tracked)



(7 attachments)



17 years ago
In working on the crypto object, I've come across what I believe is a regression
in the NSS ASN1 encoder.

This only shows up when trying to encode the PKIArchiveOptions Control (which is
what CRMF uses to wrap the private key for escrow.)

I've written a small test program that reproduces the bad ASN1.  I will attach
that program shortly.

Key Escrow will not work in PSM 2.0 until we fix this bug.  :(

Comment 1

17 years ago
Created attachment 34169 [details]
Sample Program to reproduce problem

Comment 2

17 years ago
This is the ASN1 template in question:

The problem manifests itself when the encoder gets down to the valueHint member
of the structure.  In the structure passed in to the encoder, that SECItem has a
data field with a NULL pointer, put the encoder decides to try and encode
something anyway.

I wrote the NSS code originally, and the besides some macros nelsonb added to
get dll's working on Win32, the templates haven't changed.  While debugging, the
templates seemed correct, it was the encoder's interpretation of the OCTET
string that seemed to be in question.

I'll need help from someone with more intimiate knowledge of the encoder to help
me track this one down.

Here is a sample run of the program:

The DER generate for the PKI Archive Options control with encrypted value is
Will not try to Decode the DER...

At this point, the program Assert fails and core dumps trying to decode the ASN1
it just produced.

Here is the output from derdump on the generated ASN1:

(297)javi@javi:~> derdump < pkiArch-bin.der 
C-[0]  (515)
   C-Sequence  (511)
      C-[1]  (20)
         Object Identifier  (8)
            1 2 840 113549 3 7 (DES-EDE3-CBC)
         Octet String  (8)
            da 34 04 9e 82 2b ba 1e 
      [2]  (129)
         00 2d 8e b0 d5 cc 90 2e 4e 6d a8 60 61 38 98 9a 2e 3b b6 56 ac 
         83 01 38 f9 c8 4b a3 7d 1a e4 6f 80 07 e9 4e 2a ae 32 7a 5f 0a 
         a6 e9 32 dc da a8 ec 51 71 e4 47 a7 69 33 18 47 47 f2 ba 7b 5c 
         51 ac 3b 1a e3 8b fc 80 5f b8 f2 b8 5f 18 58 fd f3 8f e8 8f b4 
         05 4f cc 25 38 5a 44 94 ef f1 35 61 31 63 fc 94 45 8c a5 79 47 
         7d a1 ac 1b ba a7 86 21 c4 3b f0 f9 20 e6 3f 3f 17 33 b0 e8 85 
         db 3f e6 
      [4]  (1)
      [2]  (1)
derdump: error -8183: security library: improperly formatted DER-encoded
      End of Contents

Comment 3

16 years ago
Assigned the bug to Ian.
Assignee: wtc → mcgreer
Priority: -- → P1
Target Milestone: --- → 3.3


16 years ago
Target Milestone: 3.3 → 3.4


16 years ago
Blocks: 83130


16 years ago
No longer blocks: 83130


16 years ago
Blocks: 92502

Comment 4

16 years ago
Created attachment 43749 [details] [diff] [review]
Smaller program to reproduce the crash.

Comment 5

16 years ago
derdump of the generated DER (Note that the [4] field should not appear since
the corresponding optional entry was NULL in the data structure)

(299)javi@javi:~> derdump < der.bin
C-Sequence  (3)
   [4]  (1)
Boolean  (31)
derdump: error -8183: security library: improperly formatted DER-encoded message

Comment 6

16 years ago
If I revert secasn1e.c, then my latest sample program works.  I'm adding chrisk
to the cc list since he was the last one to touch the file.

Comment 7

16 years ago
Created attachment 44043 [details] [diff] [review]
Patch that fixes ASN1 regression

Comment 8

16 years ago
The problem was in calculating the length of an item that could be streaming and
was optional.  As part of the ASN.1 engine, a sub-template can specify how to
interpret the data, but when the parent was optional, the function to calculate
the length always assumed a length of zero with a type that could stream was a
place holder for streaming data.

I've added code to pass the parent's optional flag down and use it when
calculating the length of the content.  With this patch you will not be able
stream in the contents of an optional field. passes as well as Key Escrow with Mozilla with this patch.

Who wants to review?


Comment 9

16 years ago
Created attachment 44249 [details] [diff] [review]
Updated patch which uses the parent's streaming property instead of optional property.

Comment 10

16 years ago
Created attachment 44263 [details] [diff] [review]
Last patch had a bug.

Comment 11

16 years ago
It looks ok from your description. My only question is what happens during the
actual encoding? what is it doing currently when it hits an optional parameter
in streaming and non-streaming modes. r=relyea

Comment 12

16 years ago
I don't know of any test cases to run when streaming is used.  Christian said
that not all cases would be tested by  I'm guessing there needs to be a
test case added to that does operations on big files to force streaming
to be used when testing the smime functionality.

I just haven't had the time to do that.

With the patch above, Key Escrow,, and my small sample program all
generate correct DER which NSS can successfully read in.

Comment 13

16 years ago
OK, it fixes what we know about. let's go with it. Leave a log comment in the
log, though;).

Comment 14

16 years ago
Will check this into trunk and NSS_3_2_BRANCH to get this for PSM.  Should I go
ahead and check into NSS_3_3_BRANCH

Comment 15

16 years ago
Yes, please check it into NSS_3_3_BRANCH as well.


16 years ago
Target Milestone: 3.4 → 3.2.2

Comment 16

16 years ago
Marking fixed for now, but we may have to re-open this when the S/MIME libraries
get used much more often by PSM.
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 17

16 years ago
re-opening, because my patch broke pk12util
Resolution: FIXED → ---

Comment 18

16 years ago
Created attachment 44482 [details] [diff] [review]
A new patch that uses info in template for disabling sub-template streaming.

Comment 19

16 years ago
My latest patch uses info in the template to decide on whether to ignore the
info about streaming when calculating the length.  This will require I change
the CRMF templates to get key escrow working again, but at least none of the
older templates will break.

Comment 20

16 years ago
Created attachment 44636 [details] [diff] [review]
My last sample test program, so it doesn't get lost.

Comment 21

16 years ago
QA tests passed.  Marking fixed.
Last Resolved: 16 years ago16 years ago
Resolution: --- → FIXED

Comment 22

16 years ago
The fix is not in 3.2.2 or 3.3 but is in 3.3.1 and
on the 3.2 and 3.3 branches.  Since we are not planning
to make any new 3.2.x releases, I am setting the target
milestone to 3.3.1.
Target Milestone: 3.2.2 → 3.3.1
You need to log in before you can comment on or make changes to this bug.