Last Comment Bug 80416 - Regression in ASN1 Decoder
: Regression in ASN1 Decoder
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: 3.2.1
: x86 Linux
: P1 critical (vote)
: 3.3.1
Assigned To: Ian McGreer
: Sonja Mirtitsch
Depends on:
Blocks: 92502
  Show dependency treegraph
Reported: 2001-05-11 18:58 PDT by Javier Delgadillo
Modified: 2001-11-08 12:09 PST (History)
6 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

Sample Program to reproduce problem (5.08 KB, text/plain)
2001-05-11 19:00 PDT, Javier Delgadillo
no flags Details
Smaller program to reproduce the crash. (1.30 KB, patch)
2001-07-26 17:51 PDT, Javier Delgadillo
no flags Details | Diff | Splinter Review
Patch that fixes ASN1 regression (3.58 KB, patch)
2001-07-30 17:09 PDT, Javier Delgadillo
no flags Details | Diff | Splinter Review
Updated patch which uses the parent's streaming property instead of optional property. (3.58 KB, patch)
2001-08-01 10:20 PDT, Javier Delgadillo
no flags Details | Diff | Splinter Review
Last patch had a bug. (3.64 KB, patch)
2001-08-01 11:02 PDT, Javier Delgadillo
no flags Details | Diff | Splinter Review
A new patch that uses info in template for disabling sub-template streaming. (8.01 KB, patch)
2001-08-02 17:38 PDT, Javier Delgadillo
no flags Details | Diff | Splinter Review
My last sample test program, so it doesn't get lost. (1.52 KB, patch)
2001-08-03 15:56 PDT, Javier Delgadillo
no flags Details | Diff | Splinter Review

Description Javier Delgadillo 2001-05-11 18:58:55 PDT
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 Javier Delgadillo 2001-05-11 19:00:01 PDT
Created attachment 34169 [details]
Sample Program to reproduce problem
Comment 2 Javier Delgadillo 2001-05-11 19:06:06 PDT
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 Wan-Teh Chang 2001-06-06 09:18:47 PDT
Assigned the bug to Ian.
Comment 4 Javier Delgadillo 2001-07-26 17:51:44 PDT
Created attachment 43749 [details] [diff] [review]
Smaller program to reproduce the crash.
Comment 5 Javier Delgadillo 2001-07-26 17:56:40 PDT
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 Javier Delgadillo 2001-07-27 17:51:42 PDT
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 Javier Delgadillo 2001-07-30 17:09:56 PDT
Created attachment 44043 [details] [diff] [review]
Patch that fixes ASN1 regression
Comment 8 Javier Delgadillo 2001-07-30 17:16:54 PDT
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 Javier Delgadillo 2001-08-01 10:20:19 PDT
Created attachment 44249 [details] [diff] [review]
Updated patch which uses the parent's streaming property instead of optional property.
Comment 10 Javier Delgadillo 2001-08-01 11:02:30 PDT
Created attachment 44263 [details] [diff] [review]
Last patch had a bug.
Comment 11 Robert Relyea 2001-08-01 14:59:24 PDT
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 Javier Delgadillo 2001-08-01 15:15:24 PDT
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 Robert Relyea 2001-08-01 15:23:41 PDT
OK, it fixes what we know about. let's go with it. Leave a log comment in the
log, though;).
Comment 14 Javier Delgadillo 2001-08-01 15:39:59 PDT
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 Wan-Teh Chang 2001-08-01 15:44:59 PDT
Yes, please check it into NSS_3_3_BRANCH as well.
Comment 16 Javier Delgadillo 2001-08-01 17:44:59 PDT
Marking fixed for now, but we may have to re-open this when the S/MIME libraries
get used much more often by PSM.
Comment 17 Javier Delgadillo 2001-08-02 17:06:35 PDT
re-opening, because my patch broke pk12util
Comment 18 Javier Delgadillo 2001-08-02 17:38:04 PDT
Created attachment 44482 [details] [diff] [review]
A new patch that uses info in template for disabling sub-template streaming.
Comment 19 Javier Delgadillo 2001-08-02 17:40:01 PDT
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 Javier Delgadillo 2001-08-03 15:56:04 PDT
Created attachment 44636 [details] [diff] [review]
My last sample test program, so it doesn't get lost.
Comment 21 Javier Delgadillo 2001-08-03 15:56:51 PDT
QA tests passed.  Marking fixed.
Comment 22 Wan-Teh Chang 2001-11-08 12:09:59 PST
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.

Note You need to log in before you can comment on or make changes to this bug.