80416 - Regression in ASN1 Decoder

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Javier Delgadillo]

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]

Attachment: Sample Program to reproduce problem

Comment 2

[Javier Delgadillo]

This is the ASN1 template in question:

http://lxr.mozilla.org/mozilla/source/security/nss/lib/crmf/crmftmpl.c#243

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
oIICAzCCAf+hFAYIKoZIhvcNAwcECNo0BJ6CK7oegoGBAC2OsNXMkC5ObahgYTiY
mi47tlasgwE4+chLo30a5G+AB+lOKq4yel8Kpuky3Nqo7FFx5EenaTMYR0fyuntc
Uaw7GuOL/IBfuPK4XxhY/fOP6I+0BU/MJThaRJTv8TVhMWP8lEWMpXlHfaGsG7qn
hiHEO/D5IOY/PxczsOiF2z/mhAEDggFhAN1GR1G3eLU0AzH975T2mXpMeCWw15lz
VDBw9uWu0vcltlTKmxvP72F0QN64bgr/0l6EQWYCp+MOmrghiomKAQNec3NJAKTq
hrt8pe4deTBF4sODctvxpkwGrmpmFIM0nkAJ4nfg7AbHAfA07hBemKeJFR8xFdF+
sJv/BhZIlyzQqjdoAvng4aMMGdBGQnJvw7xmy0jc3xr4CZDsw78ZSW2/fHufl9o7
H65YyOAD8B7R27fzgBiBTx8q3URfz2rRkAmIFFS0LwRGObySeUFLecwwdjqLM1yd
yIUaZmjk/Tz9lQrIr3m93kntwsGwwTdwltA91jZORXmNKSFAkJHpnt3wGlKkJ3BR
vLcKuFAkAh+JIuUljzIxjBBcR6TOpazFQCMZ5/BTQLfH5htjFjC5FcgYZWc7/NtK
S2jfu3WyY01fjQQWw0M4y+PEEf+eETpbmuYiBfOya3t115f0Fjz9qN4=
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)
         03 
      [2]  (1)
         61 
derdump: error -8183: security library: improperly formatted DER-encoded
message.
      End of Contents

Comment 3

[Wan-Teh Chang]

Assigned the bug to Ian.

Comment 4

[Javier Delgadillo]

Attachment: Smaller program to reproduce the crash.

Comment 5

[Javier Delgadillo]

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)
      02 
Boolean  (31)
derdump: error -8183: security library: improperly formatted DER-encoded message

Comment 6

[Javier Delgadillo]

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]

Attachment: Patch that fixes ASN1 regression

Comment 8

[Javier Delgadillo]

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.

smime.sh passes as well as Key Escrow with Mozilla with this patch.

Who wants to review?

Comment 9

[Javier Delgadillo]

Attachment: Updated patch which uses the parent's streaming property instead of optional property.

Comment 10

[Javier Delgadillo]

Attachment: Last patch had a bug.

Comment 11

[Robert Relyea]

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]

I don't know of any test cases to run when streaming is used.  Christian said
that not all cases would be tested by smime.sh  I'm guessing there needs to be a
test case added to smime.sh 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, smime.sh, and my small sample program all
generate correct DER which NSS can successfully read in.

Comment 13

[Robert Relyea]

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

Comment 14

[Javier Delgadillo]

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]

Yes, please check it into NSS_3_3_BRANCH as well.

Comment 16

[Javier Delgadillo]

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]

re-opening, because my patch broke pk12util

Comment 18

[Javier Delgadillo]

Attachment: A new patch that uses info in template for disabling sub-template streaming.

Comment 19

[Javier Delgadillo]

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]

Attachment: My last sample test program, so it doesn't get lost.

Comment 21

[Javier Delgadillo]

QA tests passed.  Marking fixed.

Comment 22

[Wan-Teh Chang]

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.