245429 - ASN1 encoder asserts on CMMF templates

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Nelson Bolyard (seldom reads bugmail)]

When the ASN.1 encoder is called to encode a simple (?) CMMF message 
containing one or more certs, the encoder asserts.  Going past the first
assertion causes an essentially identical assertion to occur shortly 
thereafter. Turning off the assertions causes the encoder to output a 
message with invalid SEQUENCE lengths.  

The template in question is CMMFCertRepContentTemplate.
The function that attempts to encode with it is CMMF_EncodeCertRepContent.

Here are some of the relevant templates from file nss/lib/crmf/asn1cmn.c:

const SEC_ASN1Template CMMFSequenceOfCertsTemplate[] = {
    { SEC_ASN1_SEQUENCE_OF| SEC_ASN1_XTRN, 0,
                        SEC_ASN1_SUB(SEC_SignedCertificateTemplate)}
};

const SEC_ASN1Template CMMFCertRepContentTemplate[] = {
    { SEC_ASN1_SEQUENCE, 0, NULL, sizeof(CMMFCertRepContent)},
    { SEC_ASN1_CONSTRUCTED | SEC_ASN1_OPTIONAL | SEC_ASN1_CONTEXT_SPECIFIC | 1,
      offsetof(CMMFCertRepContent, caPubs), CMMFSequenceOfCertsTemplate },
    { SEC_ASN1_SEQUENCE_OF, offsetof(CMMFCertRepContent, response),
      CMMFCertResponseTemplate},
    { 0 }
};

The first assertion occurs in  function sec_asn1e_contents_length.  
It is trying to compute the length of the first component in the last 
SEQUENCE above, namely the constructed, optional, context-specific 1.
It has fetched the kind of the subtemplate, and is now asserting:

    /* Having any of these bits is not expected here...  */
    PORT_Assert ((underlying_kind & (SEC_ASN1_EXPLICIT | SEC_ASN1_OPTIONAL
                                     | SEC_ASN1_INLINE | SEC_ASN1_POINTER
                                     | SEC_ASN1_DYNAMIC | SEC_ASN1_MAY_STREAM
                                     | SEC_ASN1_SAVE | SEC_ASN1_SKIP)) == 0);

Later it asserts in function sec_asn1e_init_state_based_on_template,
in a very similar code path.

        /*
         * This is an implicit, non-universal (meaning, application-private
         * or context-specific) field.  This results in a "magic" tag but
         * encoding based on the underlying type.  We pushed a new state
         * that is based on the subtemplate (the underlying type), but
         * now we will sort of alias it to give it some of our properties
         * (tag, optional status, etc.).
         */

        under_kind = state->theTemplate->kind;
        if ((under_kind & SEC_ASN1_MAY_STREAM) && !ignore_stream) {
            may_stream = PR_TRUE;
        }
        under_kind &= ~SEC_ASN1_MAY_STREAM;
    } else {
        under_kind = encode_kind;
    }

    /*
     * Sanity check that there are no unwanted bits marked in under_kind.
     * These bits were either removed above (after we recorded them) or
     * they simply should not be found (signalling a bad/broken template).
     * XXX is this the right set of bits to test here? (i.e. need to add
     * or remove any?)
     */
    PORT_Assert ((under_kind & (SEC_ASN1_EXPLICIT | SEC_ASN1_OPTIONAL
                                | SEC_ASN1_SKIP | SEC_ASN1_INNER
                                | SEC_ASN1_DYNAMIC | SEC_ASN1_MAY_STREAM
                                | SEC_ASN1_INLINE | SEC_ASN1_POINTER)) == 0);


The encoder clearly doesn't expect that an implicit, non-pointer,
context-specific template will point to a subtemplate that has any of those
flags in them.  In this case, we have an implicit constructed context-specific
template pointing to a subtemplate that has the XTRN (a.k.a "DYNAMIC") flag.

Is that wrong?  invalid?  (and if so, why?)

Is some flag missing (such as EXPLICIT) that ought to be present in the outer
constructed context-specific template?

Does there need to be another level of indirection here?  

Taking a step back from the ASN.1 encoder issue itself, it seems to me that
this code is trying to go about this the wrong way.  Given that the cert(s)
to be sent out are already fully encoded, the templates should probably be
sending a sequence of ANY, rather than a sequence of
SEC_SignedCertificateTemplate.  

The CMMF code shouldn't be re-encoding the certs from their components 
(as it is now trying to do), but rather should be sending out the already 
encoded certs.  So, that will need to be fixed, and the fix may ultimately 
moot the question of why this particular set of templates doesn't work.  

But I want to know why this combination of templates fails, and whether 
these templates are invalid, or whether this is a bug in the encoder.  
If the encoder is buggy, I think it needs to be fixed, whether the CMMF
library is fixed or not.

We need a document that specifies what combinations of flags in a template
are not allowed, and what combinations of templates and subtemplates are 
not allowed, and WHY (e.g. would violate ASN.1, or just an implementation
limitation).  The existing document on our templates, at
http://www.mozilla.org/projects/security/pki/nss/tech-notes/tn1.html 
is very helpful, but stops short of documenting the disallowed and/or 
required combinations.

Julien, any light you can shed on this subject would be appreciated.

One more thought.  In XXXXXXXXXX there is a comment that asks:

        /* XXX Should we recurse here? */

I think the answer is yes.  Doing so eliminates the first assertion, but
not the second, and improves the lengths in the encoded output, but still
does not make them correct (they end up being about double the correct
values).

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

above I reported that when assertions are disabled, the output has invalid
SEQUENCE lengths.  That is the only problem, AFAIK.  The rest of the output
appears normal, and dumpasn1 has no trouble decoding, but the byte counts
in the first few SEQUENCES are very large seemingly random numbers.  

The string XXXXXXXXXXX in the above comment should be "lib/util/secasn1e.c".

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

I now have a LARGE patch to secasn1e.c that fixes both bug 244922 and bug 245429. 
However, it is difficult to review because it changes to many thing. 
So I have broken it up into a series of patches, each of which 
accomplishes a relatively small task (even if it touches many lines),
and thus is easy to review.  I shall attach those patches to this bug.
They all need to be reviewed (and checked in) in order.  

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch part 1 - global rename some variables

rename "explicit" to "isExplicit" because MSVC6 thinks explicit is a keyword.
rename "ignore_stream" to "disallowStreaming" because it doesn't merely ignore.

rename "ignoresubstream" to "disallowStreaming" for the same reason.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Patch part 2 - split big function into two

Function sec_asn1e_write_contents was two functions combined into one, 
separated by one huge if-then-else.  They do different things and take 
different arguments.  So, I split them into two separate functions:
sec_asn1e_write_contents and sec_asn1e_write_contents_from_buf 
the latter of which takes a buf pointer and length argument.  
The if statement that chooses between the two is now in the caller.  

It may be best to review this patch using bugzilla's side-by-size diff 
feature, especially if it can be persuaded to ignore whitespace diffs.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch part 3 - misc fixes.

Fix some casts.  Wrap some long lines.	Remove a bogus assert.
Factor a function call out of the arguments of another function call,
which is mostly to make that code more easily debugged.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Patch part 4 - handle dynamic subtemplates

This patch is really the fix for bug 245429.  It changes two places that 
handle implicit and explicit subtemplates.  The key points to this fix are:
a) recurse to handle implicit AND explicit subtemplates (formerly recursed only

   for explicit.
b) clear the DYNAMIC bit from the copy, since only GetSubtemplate needs it, 
   and that function always looks directly at the template.
c) After asserting that certain bits must be clear, clear them to ensure that 
   they will be clear when assertions are turned off. 
d) removes some old and now-pointless comments.

There is one typo in a comment in this patch.  Look for INNER< 
I fix it in the next patch.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch part 5 - handle streaming, separate no-header cases

This is the biggest of the patches.  It fixes bug 244922.  
The 4 preceeding patches are prerequisites.

This patch changes the arguments to static function sec_asn1e_contents_length.
It adds an argument that tells the function whether the caller will be 
definite or indefinite length encoding the result.  
If the caller is indefinite length encoding, then the only real purpose this
function serves is to determine if the contents are normal, or if they 
should be omitted for some reason, such as optional and not-present (zero 
length), or being a decoder-only template such as a SAVE, or if the header
should be ommitted because the contents is pre-encoded (an ANY).  

These different "header exception" conditions are now returned through an 
enumerated type.  Previously it was a boolean, which didn't provide enough
info to make proper choices.  

The code that forces a non-zero return length, when a zero would otherwise
be returned, is now used in ALL types encoded here, and not only in one 
default case.  However, it is used ONLY when the caller is indefinite 
length encoding, because in that case the actual length doesn't matter
except to know whether it is zero or not.  In the definite length encoding
cases, changing the length from zero to 1 always causes an error.  

all.sh passes with the above 5 patches in place.  crmftest (which is not
yet part of all.sh) gets farther than it did before.  OCSP testing remains
to be done.  I will work on that testing while these patches are being 
reviewed.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 150323 [details] [diff] [review]
patch part 1 - global rename some variables

Julien, please review all the bug comments above, and then please review this
patch.	Thanks.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 150324 [details] [diff] [review]
Patch part 2 - split big function into two

Julien, please review.	Thanks.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 150325 [details] [diff] [review]
patch part 3 - misc fixes.

Julien, please review.	I will wait on asking anyone for review of patch parts
4 and 5 until the first 3 small ones are reviewed.

Comment 11

[Wan-Teh Chang]

Comment on attachment 150323 [details] [diff] [review]
patch part 1 - global rename some variables

r=wtc.	There is one problem in this patch that
should be fixed.  The global find-and-replace in
this file changed an "explicit" in the comment
below to "isExplicit", which I think is wrong.

>-    PRBool explicit,		/* we are handling an explicit header */
>+    PRBool isExplicit,		/* we are handling an isExplicit header */

Comment 12

[Wan-Teh Chang]

Comment on attachment 150323 [details] [diff] [review]
patch part 1 - global rename some variables

One stylistic nit: it would look nicer to use
is_explicit and disallow_streaming instead to
follow the prevalent identifier naming convention
in this file.  (See the is_string and may_stream
members of the same structure.)

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 150323 [details] [diff] [review]
patch part 1 - global rename some variables

Wan-Teh, thanks for the review.  I will consider adding 
another patch below (patch part 7) to address any and all
stylistic changes suggested, after parts 1-6 are reviewed.

Comment 14

[Julien Pierre]

Nelson, I'm having trouble applying your patches :
dos2unix < /home/jp96085/p/245429a.txt | patch
Hmm...  Looks like a unified diff to me...
The text leading up to this was:
--------------------------
|--- secasn1e.c 2004/06/09 04:35:34     1.9
|+++ secasn1e.c 2004/06/09 04:38:49     1.10
--------------------------
Patching file secasn1e.c using Plan A...
Hunk #1 failed at 38.
Hunk #2 succeeded at 82.
Hunk #3 succeeded at 188.
Hunk #4 succeeded at 200.
Hunk #5 succeeded at 221.
Hunk #6 succeeded at 298.
Hunk #7 succeeded at 368.
Hunk #8 succeeded at 483.
Hunk #9 succeeded at 497.
Hunk #10 succeeded at 509.
Hunk #11 succeeded at 570.
Hunk #12 succeeded at 719.
Hunk #13 succeeded at 780.
Hunk #14 succeeded at 833.
1 out of 14 hunks failed--saving rejects to secasn1e.c.rej
done

[jp96085@variation]/home/jp96085/nss/tip/mozilla/security/nss/lib/util 101 %
dos2unix < /home/jp96085/p/245429b.txt | patch
Hmm...  Looks like a unified diff to me...
The text leading up to this was:
--------------------------
|--- secasn1e.c 2004/06/09 04:38:49     1.10
|+++ secasn1e.c 2004/06/09 04:41:02     1.11
--------------------------
Patching file secasn1e.c using Plan A...
Hunk #1 failed at 38.
Hunk #2 succeeded at 901.
Hunk #3 succeeded at 1215.
1 out of 3 hunks failed--saving rejects to secasn1e.c.rej
done

Could you regenerate patches that apply to the tip ? Or tell me what branch to
apply them to ? I wanted to look at the files side by side in my favorite
editor, Visual slick edit, which has a better diff function than cvs. In
particular I wanted to verify there is no actual runtime change in your patch 2.

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

Julien, I believe you'll find that the only "failed" hunks of patch are
hunks that change only the line that contains the RCS ID.  If that is so,
then your attempts to patch your files have all succeeded.    
These patches should patch OK against the trunk, except for the RCS IDs,
which aren't important.

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 150324 [details] [diff] [review]
Patch part 2 - split big function into two

Wan-Teh, please review

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 150325 [details] [diff] [review]
patch part 3 - misc fixes.

Wan-Teh, please review

Comment 18

[Wan-Teh Chang]

Comment on attachment 150325 [details] [diff] [review]
patch part 3 - misc fixes.

>@@ -1204,7 +1206,6 @@
>     sec_asn1e_state *state;
> 
>     if (cx->status == needBytes) {
>-	PORT_Assert (buf != NULL && len != 0);
> 	cx->status = keepGoing;
>     }

Why is this assertion bogus?

Comment 19

[Wan-Teh Chang]

Comment on attachment 150324 [details] [diff] [review]
Patch part 2 - split big function into two

> static void
>-sec_asn1e_write_contents (sec_asn1e_state *state,
>+sec_asn1e_write_contents_from_buf (sec_asn1e_state *state,
> 			  const char *buf, unsigned long len)
> {
>     PORT_Assert (state->place == duringContents);
>+    PORT_Assert (state->top->from_buf);
>+    PORT_Assert (state->may_stream && !state->disallowStreaming);

Can you explain the second new assertion?

Another question: are the following two always true?

    state->top == cx
    cx-> current == state

>+static void
>+sec_asn1e_write_contents (sec_asn1e_state *state)
>+{
>+    unsigned long len = 0;

It is not necessary to initialize 'len' to 0, right?  (I don't
object to that.  I just wanted to check my understanding.)

Comment 20

[Nelson Bolyard (seldom reads bugmail)]

In answer to comment 18, the code handles this case without problem.
Therefore, there is no need to assert that this case is not present.

In answer to comment 19, 
a) the second assertion is to detect a programming error, where the caller
of EncoderUpdate is attempting to encode data from a buffer but the current
state is not one for which that is an allowed behavior.  

I have considered writing a complete how-to guide for the encoder that 
would explain how it was (apparently) intended to be used.  Perhaps that
document would make it clearer.  Would you like me to write that document?

b) there is a linked list of states, which is treated like a stack.
Each state in the list points to the encoder context.  The encoder 
context points to one end of the list.

c) I think it is not necessary to initialize len.  This is defensive programming.

Comment 21

[Wan-Teh Chang]

Comment on attachment 150325 [details] [diff] [review]
patch part 3 - misc fixes.

r=wtc.	I don't know why the original casts were
wrong (no compiler warnings on Windows and Linux),
but the new casts are fine.

Comment 22

[Wan-Teh Chang]

Comment on attachment 150324 [details] [diff] [review]
Patch part 2 - split big function into two

r=wtc.

Comment 23

[Julien Pierre]

Nelson,

I think an encoder document would be useful.

Are the part 4 and part 5 patches ready to be reviewed ?

Comment 24

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 150326 [details] [diff] [review]
Patch part 4 - handle dynamic subtemplates

Julien and/or Wan-Teh, please review.

Here are additional comments, beyond those in comment 6 above:

There are two large functions in this file that strongly parallel each other
in structure and function.  They are sec_asn1e_contents_length and 
sec_asn1e_init_state_based_on_template.  Neither of them was coded to 
correctly handle a subtemplate that contained any of the following SEC_ASN1_
flags: EXPLICIT, OPTIONAL, INNER, INLINE, POINTER, DYNAMIC, SAVE.  
They both needed to be fixed.  

The fixes for the two are quite different.  For sec_asn1e_contents_length
the fix is tiny and elegant (IMO).  After selecting the subtemplate, it
simply recurses.  This correctly handles ALL the flags, because the 
length computation doesn't need to be modified by the parent template's 
state.

But for sec_asn1e_init_state_based_on_template, a similar approach of 
recursion causes the wrong encoding to be output.  This is because the 
child's encoding DOES need to be altered by the state of the parent 
template.  So, the patch attempts to combine the state and go on.  
I am not sure that this combination of state/flags is correct in all 
resonable cases.  I only know that it produces the correct results for
the test cases at hand, which come from the crmf/cmmf libraries.  
I believe the result is more correct than before in all case, but there
may exist cases (for which I do not have examples) in which they do not
produce correct results.   I felt it worthwhile to checkin this fix now,
and perhaps extend it when more test cases are available.  I invite your
comment on this approach.

There are some obvious typos in some of the comments.  I believe they 
are all fixed in the next patch (part 5).  Please don't r- this patch just
for those, but make a note and I will fix any before checkin.

Comment 25

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 150326 [details] [diff] [review]
Patch part 4 - handle dynamic subtemplates

Bob, please read all the above comments in this bug, and then review this
patch.

Comment 26

[Robert Relyea]

Comment on attachment 150326 [details] [diff] [review]
Patch part 4 - handle dynamic subtemplates

r=relyea

The tweaks to sec_asn1e_init_state_based_ontemplate are subtle. It's hard to
see if they are beneficial, detrimental, or neutral without a full review of
the function itself.

The rest of the changes are clearly correct.

Comment 27

[Nelson Bolyard (seldom reads bugmail)]

checked in patch 1
/cvsroot/mozilla/security/nss/lib/util/secasn1e.c,v  <--  secasn1e.c
new revision: 1.15; previous revision: 1.14

checked in patch 2
new revision: 1.16; previous revision: 1.15

checked in patch 3
new revision: 1.17; previous revision: 1.16

Comment 28

[Nelson Bolyard (seldom reads bugmail)]

Patch 4 checked in, after Bob's review.
/cvsroot/mozilla/security/nss/lib/util/secasn1e.c,v  <--  secasn1e.c
new revision: 1.18; previous revision: 1.17

Comment 29

[Nelson Bolyard (seldom reads bugmail)]

I am marking this bug resolved/FIXED.
The one remaining patch, patch part 5, is really a fix for bug 244922,
which presently remains open.

Comment 30

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 150326 [details] [diff] [review]
Patch part 4 - handle dynamic subtemplates

This patch (#4) was checked in after Bob's review.

Comment 31

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 150327 [details] [diff] [review]
patch part 5 - handle streaming, separate no-header cases

Julien, 

This is the bug I mentioned that tries to deal with the ambiguity of NULL 
pointers in optional templates while streaming.  Note that there have been 
several past attempts to address this problem, one of which created the 
SEC_ASN1_NO_STREAM flag.  

There were 5 patches for this bug.  The first 4 have been checked in.  
The fifth one requires real ASN.1 decoder expertise.  Please review this.

Comment 32

[Julien Pierre]

Nelson,

I'll try to review the 5th patch. But I have no expertise with the streaming
encoder case. I think we'll need some test cases in our QA test suite to make
sure things are correct here.

Comment 33

[Nelson Bolyard (seldom reads bugmail)]

As noted in comment 7 above, patch 5 is really a fix for bug 244922 .  
Please read comment 7 and bug 244922 for the best descriptions.  

Comment 34

[Alexei Volkov]

Tested patch #5 on linux and solaris x86. Crl generation tests pass now.
Noticed that patch didn't apply properly to the current tip. Additional
fixes are necessary in order to compile the tree.

Comment 35

[Julien Pierre]

Patch #5 is still not checked in, yet this bug is closed . Either the bug needs
to be reopened, or patch #5 should be attached to a different bug .

Comment 36

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #35)
> Patch #5 is still not checked in, yet this bug is closed . Either the bug needs
> to be reopened, or patch #5 should be attached to a different bug .

Please see comment 33 above