Open Bug 583308 Opened 11 years ago Updated 9 years ago
.1 encoder fails to handle EXPLICIT POINTER template
Whan an ASN.1 template has the attributes OPTIONAL, EXPLICIT and POINTER, and the corresponding struct's pointer for the OPTIONAL value is NULL, indicating that the value is to be omitted from the encoding, the encoder crashes on an assertion failure. The problem is that the function sec_asn1e_init_state_based_on_template fails to clear the state->isExplicit bit before returning in this path. The attached patch should fix this crash, however, the results may or may not be properly encoded. I haven't tested this. Hanno, Please test it and let us know if this fixes it, and if the result is correctly encoded. It MAY produce a result bearing the explicit tag for the omitted AlgID. :(
Attachment #461611 - Flags: review?(wtc)
Nelson, the patch does not fix the issue. Also the problem does not appear when the struct's pointer for the OPTIONAL value is NULL. I just tried and when I have a template with a single explicid|optional value and set the struct pointer to NULL, I get a valid empty asn1 struct back (both with and without your patch).
Comment on attachment 461611 [details] [diff] [review] Patch v1 for NSS Trunk Nelson, thanks a lot for your help with Hannos' encoding problem. Based on my investigation, I believe this bug is INVALID or WONTFIX. I found that the SEC_ASN1_POINTER | SEC_ASN1_EXPLICIT combination is the cause of the problem. One evidence that our ASN.1 encoder doesn't support this combination is that there are no ASN.1 templates in NSS that use this combination. I also found that NSS uses "PointerTo" templates to deal with an optional field with an explicit tag. I posted a reply in the NSS newsgroup with more details. Marked the patch review- based on Hanno's testing and my findings.
Attachment #461611 - Flags: review?(wtc) → review-
This is Hanno's test code, turned into a real program, (has a main function) with #includes added (it builds) and memory allocation corrected. It reproduces the crash.
Here's Hanno's test program with two versions of the encoder templates, one that works and one that does not. It can be conditionally compiled to use either set, to reproduce the problem, or reproduce the solution.
Attachment #461824 - Attachment is obsolete: true
Wan-Teh wrote the following comments about this issue in a posting to mozilla.dev.tech.crypto: I saw the problem that Hanno reported, that the ASN.1 encoder calls SEC_ASN1GetSubTemplate one time too many. The first SEC_ASN1GetSubTemplate call is caused by the SEC_ASN1_OPTIONAL flag. The second SEC_ASN1GetSubTemplate call, which causes the assertion failure, is caused by the SEC_ASN1_EXPLICIT flag. Using this MXR query for SEC_ASN1_POINTER, you can see that no ASN.1 templates use SEC_ASN1_POINTER with SEC_ASN1_EXPLICIT: http://mxr.mozilla.org/security/search?string=SEC_ASN1_POINTER So it's not surprising the ASN.1 encoder may have problems with the SEC_ASN1_POINTER | SEC_ASN1_EXPLICIT combination. So one solution is to not use a pointer field in the structure. But I understand you want the field to be a pointer because it is optional. Then I found that NSS deals with such an "optional field with an explicit tag" by using a "PointerTo" template. Here is an example in mozilla/security/nss/lib/certhigh/ocsp.c: http://mxr.mozilla.org/security/source/security/nss/lib/certhigh/ocsp.c#1188
Above, Wan-Teh wrote "The first SEC_ASN1GetSubTemplate call is caused by the SEC_ASN1_OPTIONAL flag." I think he meant "by the SEC_ASN1_POINTER flag." because in subsequent sentences, he refers to the combination of POINTER and EXPLICIT. Wan-Teh, please correct that if I am mistaken. For YEARS, I have wondered why NSS has those "PointerTo" templates. Now Wan-Teh has figured out the answer to that mystery. It's because the ASN.1 encoder cannot handle the combination of POINTER and some other flag (presumably EXPLICIT) in the same template, so the early NSS developers moved POINTER its own separate child template, separate from the other flag. I'm guessing that all (or most) of those existing "PointerTo" templates are referenced by EXPLICIT templates somewhere. But I am a bit mystified about this. POINTER and OPTIONAL logically go together. It's the NULL/non-NULL value of the pointer that determines whether the OPTIONAL value will be omitted or encoded. Putting OPTIONAL in the parent template and POINTER in the child is certainly counter-intuitive. Also, I'd think you'd want to evaluation the OPTIONAL decision before encoding the EXPLICIT construct, so again, it doesn't make sense to me that EXPLICIT would be in the parent and POINTER in the child. So I'm going to experiment with some things, including: a) have a NULL pointer in the test program, and see how it behaves. b) reverse the parent and child flags, Have the parent be the optional pointer, and the child be the EXPICIT, and see if that works. In any case, I believe we should make one of two changes, either: a) change all the ASN.1 encoders and decoders to ASSERT that no template contains both POINTER and EXPLICIT, along with a comment explaining why, or else b) change the ASN.1 encoder to make that combination work (it already works in the decoders, I believe). The templates should work consistently for encoders and decoders, except for those few flags that are DOCUMENTED to be ONLY for one or only for the other. Since POINTER and EXPLICIT flags are valid in both encoders and decoders, then IMO combinations of them should be valid (or invalid) in BOTH encoders and decoders alike. I can see a FLAG being valid in one or the other and not both, but for flags that are valid in both, I cannot see having combinations that are valid only in one and not the other.
Summary: NSS ASN.1 encoder mishandles omitted OPTIONAL EXPLICIT POINTER → NSS ASN.1 encoder fails to handle EXPLICIT POINTER template
Nelson, I can confirm that it works in the decoders, see my pss verification patch attached to bug #158750 that uses this. Doing your suggestion a) is probably not an option as it will break currently working code.
This version tests both null and non-NULL optional field pointers. Well, I tried playing with reversing the POINTER and EXPLICIT and that fails because EXPLICIT must go with the CONTEXT_SPECIFIC flag. So even if seems unintuitive to me, I'll accept that that's how it works, POINTER templates are children of EXPLICIT, but OPTIONAL remains in the parent, not with the POINTER. That's how it is in the ENcoder today.
Attachment #461826 - Attachment is obsolete: true
After more experimentation and code reading, I found a combination that enables OPTIONAL and POINTER to be used together in the parent, and EXPLICIT to be used in the child. This is FAR more intuitively correct in my mind. The attached source program can be compiled in several different ways, two of the three work. I'd really like to fix the encoder so that all three work, but doubt I have time to do that.
Attachment #461830 - Attachment is obsolete: true
(In reply to comment #6) > Above, Wan-Teh wrote "The first SEC_ASN1GetSubTemplate call is caused by the > SEC_ASN1_OPTIONAL flag." I think he meant "by the SEC_ASN1_POINTER flag." > because in subsequent sentences, he refers to the combination of POINTER and > EXPLICIT. > > Wan-Teh, please correct that if I am mistaken. Nelson, you're right. Sorry about the confusion caused by my typo.
Nelson, do you still want to fix this bug? I am fine with marking this WONTFIX because I am nervous about changing the ASN.1 encoder when the only documentation of it is Julien's technical note and the existing templates.
> Nelson, do you still want to fix this bug? I'd LIKE to fix it. I believe I could fix it. But in all honesty, I'm unlikely to find time for it this year. :-( Maybe I should start playing the lottery. :)
You need to log in before you can comment on or make changes to this bug.