Closed Bug 280121 Opened 20 years ago Closed 20 years ago

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


(NSS :: Libraries, defect, P1)



(Not tracked)



(Reporter: alvolkov.bgs, Assigned: julien.pierre)




(2 files, 2 obsolete files)

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

+	  offsetof(CERTCrl,nextUpdate), CERT_TimeChoiceTemplate },

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

Reproducible: Always
Summary: Can not encode CRL using classic ASN.1 encoder: unable to encode with the combination of SEC_ASN1_INLINE | SEC_ASN1_OPTIONAL template flags. → Can not encode CRL using classic ASN.1 encoder: unable to encode with the combination of SEC_ASN1_INLINE | SEC_ASN1_OPTIONAL template flags.
Ever confirmed: true
Blocks: 265033
Assignee: wtchang → julien.pierre.bugs
Priority: -- → P1
Target Milestone: --- → 3.10
Blocks: 265003
No longer blocks: 265033
This fix allows encoding a TimeChoice for the nextUpdate field of the CRL.
Attachment #180038 - Flags: review?(nelson)
Should we enhance the classic ASN.1 decoder, too?
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.
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.
I have filed bug 288649, the counterpart to this bug to add the functionality to
the classic ASN.1 decoder.
Sorry, that would be bug 289649 .
Attachment #180038 - Attachment is obsolete: true
Attachment #180140 - Flags: review?(nelson)
Attachment #180038 - Flags: review?(nelson)
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.
Attachment #180140 - Flags: review?(nelson) → review+
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
Checking in secasn1e.c;
/cvsroot/mozilla/security/nss/lib/util/secasn1e.c,v  <--  secasn1e.c
new revision: 1.19; previous revision: 1.18
Checking in secasn1u.c;
/cvsroot/mozilla/security/nss/lib/util/secasn1u.c,v  <--  secasn1u.c
new revision: 1.4; previous revision: 1.3
Attachment #180140 - Attachment is obsolete: true
Closed: 20 years ago
Resolution: --- → FIXED
Attached patch test programSplinter Review
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.
You need to log in before you can comment on or make changes to this bug.