583308 - NSS ASN.1 encoder fails to handle EXPLICIT POINTER template

Status: NEW

(NSS :: Libraries, defect, P5)

Description

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Patch v1 for NSS Trunk

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. :(

Comment 1

[Hanno Boeck]

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 2

[Wan-Teh Chang]

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.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Hanno's test program

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.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Hanno's test program, with fix

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.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

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

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 7

[Hanno Boeck]

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.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Hanno's test program, v3, with fix

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.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Hanno's test program, v4, with alternate fixes

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.

Comment 10

[Wan-Teh Chang]

(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.

Comment 11

[Wan-Teh Chang]

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.

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

> 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. :)

Comment 13

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.