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. :(
Created attachment 34169 [details]
Sample Program to reproduce problem
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
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
Object Identifier (8)
1 2 840 113549 3 7 (DES-EDE3-CBC)
Octet String (8)
da 34 04 9e 82 2b ba 1e
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
derdump: error -8183: security library: improperly formatted DER-encoded
End of Contents
Assigned the bug to Ian.
Created attachment 43749 [details] [diff] [review]
Smaller program to reproduce the crash.
derdump of the generated DER (Note that the  field should not appear since
the corresponding optional entry was NULL in the data structure)
(299)javi@javi:~> derdump < der.bin
derdump: error -8183: security library: improperly formatted DER-encoded message
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.
Created attachment 44043 [details] [diff] [review]
Patch that fixes ASN1 regression
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?
Created attachment 44249 [details] [diff] [review]
Updated patch which uses the parent's streaming property instead of optional property.
Created attachment 44263 [details] [diff] [review]
Last patch had a bug.
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
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.
OK, it fixes what we know about. let's go with it. Leave a log comment in the
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
Yes, please check it into NSS_3_3_BRANCH as well.
Marking fixed for now, but we may have to re-open this when the S/MIME libraries
get used much more often by PSM.
re-opening, because my patch broke pk12util
Created attachment 44482 [details] [diff] [review]
A new patch that uses info in template for disabling sub-template streaming.
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.
Created attachment 44636 [details] [diff] [review]
My last sample test program, so it doesn't get lost.
QA tests passed. Marking fixed.
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.