280121 - Can not encode CRL using classic ASN.1 encoder: unable to encode with the combination of SEC_ASN1_INLINE | SEC_ASN1_OPTIONAL template flags.

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Alexei Volkov]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.2) Gecko/20040803
Build Identifier: 


This regression was introduced by the fix for 143334.
The following code was added into security/nss/lib/cerdb/crl.c

+    { SEC_ASN1_INLINE | SEC_ASN1_OPTIONAL,
+	  offsetof(CERTCrl,nextUpdate), CERT_TimeChoiceTemplate },

which currently can not be handled by classic ASN.1 encoder/decoder.


Reproducible: Always

Comment 1

[Julien Pierre]

Attachment: allow encoding INLINE OPTIONAL if subtemplate is for a primitive type or a choice of primitive types

This fix allows encoding a TimeChoice for the nextUpdate field of the CRL.

Comment 2

[Wan-Teh Chang]

Should we enhance the classic ASN.1 decoder, too?

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 180038 [details] [diff] [review]
allow encoding INLINE OPTIONAL if subtemplate is for a primitive type or a choice of primitive types

>+static PRBool sec_asn1e_template_is_simple(const SEC_ASN1Template *theTemplate)

This comment is not my full review of the new code (because I
haven't reviewed it in depth yet), but I believe this new function
belongs in secasn1u.c, the file of utility functions shared by the 
encoder and decoder.  

And in reply to the previous comment, yes, I believe the encoder and
decoder should generally honor the same templates; that is, I 
believe that a template that is valid for one should generally be
valid for the other.  There are one or two exceptions to this policy
but I don't think this should become another exception.

Comment 4

[Julien Pierre]

In response to comment #2, I'm not against enhancing the classic ASN.1 decoder
to deal with this case as well, but I don't think this is something that needs
to go into 3.10 . I think it should be a separate RFE with no target. I won't
have time to do the decoder fix for 3.10, and it really isn't needed . The
reason is that the decoder used for CRLs is the QuickDER, which already deals
with this case, actually in a more general way. We have no templates today with
this INLINE OPTIONAL syntax that need to be decoded with the classic decoder at
this time.

Comment 5

[Julien Pierre]

I have filed bug 288649, the counterpart to this bug to add the functionality to
the classic ASN.1 decoder.

Comment 6

[Julien Pierre]

Sorry, that would be bug 289649 .

Comment 7

[Julien Pierre]

Attachment: move "is simple" test function to secasn1u.c

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 180140 [details] [diff] [review]
move "is simple" test function to secasn1u.c

r=nelson with the following provisos, suggestions, and observations:

Proviso (here and below):
>+	    if (PR_TRUE == SEC_ASN1IsTemplateSimple(subt)) {

Please change that to one of these:

 +	    if (PR_FALSE != SEC_ASN1IsTemplateSimple(subt)) {
 +	    if (SEC_ASN1IsTemplateSimple(subt)) {

>+	    } else {
>+		PORT_Assert(0); /* complex templates are not handled as
>+				   inline optional */

Perhaps you should return here, or take other appropriate error action.

>+		if (PR_TRUE == SEC_ASN1IsTemplateSimple(theTemplate)) {

Same issue here with == PR_TRUE, testing for equality with 1, rather than
for inequality with zero.

>+		} else {
>+		    PORT_Assert(0); /* complex templates not handled as inline optional */

In opt builds, take some error action here?
Also, please fold that line.  It's too wide.

>+		}
>+	    }
> 	}

>+PRBool SEC_ASN1IsTemplateSimple(const SEC_ASN1Template *theTemplate)
>+{
>+    if (!theTemplate) {
>+	return PR_TRUE; /* it doesn't get any simpler than NULL */
>+    }
>+    /* only templates made of one primitive type or a choice of primitive
>+       types are considered simple */
>+    if (! (theTemplate->kind & (~SEC_ASN1_TAGNUM_MASK))) {

I suspect that the test used here is more restrictive than necessary.
It disallows the presence of ANY flag bits.  And it disallows 
optional tagged primitives.  But it's not worse than what we have now, 
and it seems to satisfy the present requirements.  We can extend it
later if needed.

>+	return PR_TRUE; /* primitive type */
>+    }
>+    if (!theTemplate->kind & SEC_ASN1_CHOICE) {
>+	return PR_FALSE; /* no choice means not simple */
>+    }
>+    while (++theTemplate && theTemplate->kind) {
>+	if (theTemplate->kind & (~SEC_ASN1_TAGNUM_MASK)) {

This is the same test as used a few lines up.  I wonder if the 
definition of that test should be a macro, used in both places.

Comment 9

[Julien Pierre]

Attachment: changed tests and indentation - patched as checked in

I changed the tests and indentation as suggested.

Regarding the restriction on no other flags, I think it will be OK for now.

Regarding the duplication of the short test, I thought about it, but it is only
a one-line test, and the same kind of duplication of basic tests happens all
over the encoder source . I agree it should be changed to macros, but that
should be done globally.

Regarding setting an error in the optimized case, I don't know what to do in
the functions that are affected. Comments from Lisa indicate that she didn't
know either :-(. I think the risk in this case is low, because these assertions
will pop-up with any templates containing this syntax. If an application
developer creates such a template and ever runs his code in debug mode, he will
get the assertion unconditionally. One would have to write new templates and
never test debug bits at all for this error to go uncaught.

Checking in secasn1.h;
/cvsroot/mozilla/security/nss/lib/util/secasn1.h,v  <--  secasn1.h
new revision: 1.13; previous revision: 1.12
done
Checking in secasn1e.c;
/cvsroot/mozilla/security/nss/lib/util/secasn1e.c,v  <--  secasn1e.c
new revision: 1.19; previous revision: 1.18
done
Checking in secasn1u.c;
/cvsroot/mozilla/security/nss/lib/util/secasn1u.c,v  <--  secasn1u.c
new revision: 1.4; previous revision: 1.3
done

Comment 10

[Julien Pierre]

Attachment: test program

This is the source for a test program I wrote while developing the patch.
Correct output is no output. It could stand integration to our QA test suite.