When an ASN.1 template (e.g. sequence) includes an OPTIONAL component, which points to a subtemplate that has the MAY_STREAM attribute, and the structure of source data for the optional component has zero length (indicating that the optional component is NOT to be encoded), then the encoder incorrectly encodes the optional component, as if it was present, with length of 1. This causes the entire rest of the encoded output to be trashed, and undecodable. A concrete example of this is seen in the template CRMFOptionalValidityTemplate in nss/lib/crmf/crmftmpl.c, which includes two optional components, each of which refers to SEC_UTCTimeTemplate in secasn1d.c, which contains the SEC_ASN1_MAY_STREAM attribute. When the crmftest program attempts to encode a validity period that has a notBefore, but no notAfter date, the encoded output is garbage beginning with the incorrectly encoded notAfter component.

This problem appears to be a design defect in the original encoder. Several developers have attempted to fix it at various times in the past. In rev 1.2 of secasn1e.c, chrisk fixed one place where it occurred. In rev 1.3 and again in rev 1.4 of secasn1e.c, Javi worked on another aspect of this same problem. The problem seems to be rooted in an ambiguity of the meaning of a zero-length SECItem. It seems to mean EITHER omitted optional component, OR component whose value is supplied via one of the various methods of "streaming" input to the encoded (as is used with indefinite length encoding). Function sec_asn1e_contents_length does not have access to enough information to determine which of those two meanings of a zero-length item is correct when it encounters that. Rev 1.2 overrides one of the values returned by sec_asn1e_contents_length after one of the calls to that function, but there are 6 calls to it. Revs 1.3 and 1.4 attempted to solve it by passing in an additional argument to sec_asn1e_contents_length, providing a hint on how to interpret it, but the hint is evidently incorrect in some cases. Before this problem can be finally resolved, we must first understand exactly when the zero-length item should be interpreted as a place holder for streaming input. If we were designing it today, I might suggest using a particular value of the secitem's "type" field to make it unambiguous, but doing that now would make it incompatible with existing uses of the encoder. There are already NUMEROUS flags and variables that attempt to control the use of streaming in the encoder, and methods to manipulate them. Among them are: SEC_ASN1_MAY_STREAM SEC_ASN1_NO_STREAM - flags in templates may_stream ignore_stream - flag variables in the stacked encoder states streaming from_buf - flag variables in the encoder context SEC_ASN1EncoderClearTakeFromBuf SEC_ASN1EncoderSetTakeFromBuf - methods to change the from_buf flag SEC_ASN1EncoderClearStreaming SEC_ASN1EncoderSetStreaming - methods to change the streaming flag SEC_ASN1EncoderUpdate (buf and len arguments) - method to provide streaming input data. I hope to be able to simplify the code after figuring this all out, and also to write some how-to documentation for the encoder.

bugzilla bug 80416 sheds some light on how the ASN.1 encoder got into its present state.

See "patch part 5" for bug http://bugzilla.mozilla.org/show_bug.cgi?id=245429 for the fix for this problem. That patch depends on the other 4 patches that preceed it in that bug report. This bug is blocked waiting for the patches for bug 245429+ to be reviewed.

*** Bug 250234 has been marked as a duplicate of this bug. ***

*** Bug 292896 has been marked as a duplicate of this bug. ***

Maybe in 3.11. :(

This is that same patch upon which this bug has been waiting for about a year. I just moved it to this bug, since the bug to which it was originally attached has been resolved as fixed, and someone insisted on removing the review request from that patch because the bug was resolved. I believe the status of this patch is that Alexei is preparing some significant regression testing to ensure that, unlike its predecessors, this patch will not cause any new problems for the other users of this encoder.

Alexei, have you reviewed the NSS templates and found out if this patch broke any of them ?

I also fixed a comment (ECPLICIT->EXPLICIT);).

This is the differences between my patch and nelsons after I merged nelson's with Julien's 1.8 change.

Note: the change from hdr_decoder to hdr_option in my 'diff' was correcting my guess at what Julien's patch to the correct value.

Comment on attachment 197634 [details] [diff] [review] Don't set indefinte encoding until we actually output the header, merge patch with Julien's rev 1.8 checking This patch is very large, and IMO requires explanation before I can do a meaningful review. I'll talk to Nelson on wednesday since he wrote the original patch.

Comment on attachment 187163 [details] [diff] [review] patch (formerly attached to bug 245429) cancelling review request

Comment on attachment 197634 [details] [diff] [review] Don't set indefinte encoding until we actually output the header, merge patch with Julien's rev 1.8 checking Bob, the differences between your patch and mine are ENORMOUS. I see many diffs not shown in your "essentially the differences" attachment. I can't approve your new patch on the basis of that smaller attachment, and it will take me DAYS to review all the diffs between the two full patches. :( Please don't combine this crucial logic fix with an enormous whitespace change. Please make a patch that, when compared to mine with buzilla's patch diffing tool, shows only the logic changes. Otherwise it will take me days to review this new patch. Sorry. :( Plan to fix whitespace in a subsequent patch. Thanks.

So this patch is for the top of the tree. Your patch is for 3.10. Between the two julien checked in revision 1.19 If you visually compare them, you will see I did not change the white space in my patch with respect to the tip My patch is your patched advanded to revision 1.19 plus my diffs. You can reproduce my base patch yourself, if you don't trust me, by applying your patch to the tip. If you diff the results, you should get the diff I attached. I know that the line numbers as a result of 1.19 makes automatic direct comparisons of the two patches impossible. That is why I produced the essential diffs, to aid in your review (and prevent you from having to take days to review the code:). The essential diff was produced by taking the original application of your patch to the trunk, which I saved, against the changed code with is now working. I can produce a patch with just my diffs, which would target 3.10, but such a patch would be useless for the tip unless we back out 1.19. I hope that explanation helps. bob

I'm making this patch to help me review Bob's patch above. I took the trunk, applied my old original patch to it, fixed the one line that I found that hadn't patched cleanly (although patch didn't report it) and then by hand applied the "essentially the differences" patch. Now I'll see how similar this is to Bob's patch attachment 197634 [details] [diff] [review]

Comment on attachment 197634 [details] [diff] [review] Don't set indefinte encoding until we actually output the header, merge patch with Julien's rev 1.8 checking After diffing with my newer patch, I'm convinced that Bob's new patch has those "essential differences". r=nelson

Bugzilla Bug 244922 ASN.1 encoder outputs trash for optional may-stream subtemplate r=nelson (original patch by nelson, modifications by me). checked into the tip. I'll need one more review before I can check it into 3.10.2. bob

Bob, please attach your final (checked-in) patch to this bug, so it can be easily diffed against the other incarnations of that patch. Thanks.

I regenerated this patch using diff -u and made Nelson's patch the "before" code and Bob's patch the "after" code. I find the patch easier to understand this way. Also note that Nelson's patch did not modify the new code that Julien added; this patch shows that (*noheaderp = PR_TRUE;).

Comment on attachment 197634 [details] [diff] [review] Don't set indefinte encoding until we actually output the header, merge patch with Julien's rev 1.8 checking Nelson, Bob, After staring at this patch for a while, I have a preliminary understanding of the patch. But I still don't fully understand it, and I have some questions, so I can't give it a sr+ yet. Here are my questions. 1. The hdr_placeholder enumeration constant is not used. Is it necessary? How will it be used? typedef enum { ... hdr_placeholder = 4 /* place holder for from_buf content */ } sec_asn1e_hdr_encoding; 2. sec_asn1e_contents_length used to pass disallowStreaming=PR_FALSE to recursive calls to itself. Now it passes its disallowStreaming argument, which may be modified in the function, to the recursive calls. So Javi's comment at the beginning of this function is no longer correct: /* * ... * So we add the disallowStreaming flag which is passed in when * writing the contents, but for all recursive calls to * sec_asn1e_contents_length, we pass PR_FALSE, because this * function correctly calculates the length for children templates * from that point on. ... * ... */ This comment needs to be updated to reflect the new code. 3. In sec_asn1e_contents_length, we have: static unsigned long sec_asn1e_contents_length (const SEC_ASN1Template *theTemplate, void *src, PRBool disallowStreaming, PRBool insideIndefinite, sec_asn1e_hdr_encoding *pHdrException) { unsigned long encode_kind, underlying_kind; PRBool isExplicit, optional, universal, may_stream; ... may_stream = (encode_kind & SEC_ASN1_MAY_STREAM) ? PR_TRUE : PR_FALSE; encode_kind &= ~SEC_ASN1_MAY_STREAM; ... if (encode_kind & SEC_ASN1_NO_STREAM) { disallowStreaming = PR_TRUE; } encode_kind &= ~SEC_ASN1_NO_STREAM; Notice the similar yet different ways we use to set may_stream and disallowStreaming, and that may_stream is a local variable whereas disallowStreaming is a function argument. It is also unusual to modify a function argument's value like this. Contrast the above code with the related sec_asn1e_init_state_based_on_template function: static sec_asn1e_state * sec_asn1e_init_state_based_on_template (sec_asn1e_state *state) { PRBool isExplicit, is_string, may_stream, optional, universal; PRBool disallowStreaming; ... may_stream = (encode_kind & SEC_ASN1_MAY_STREAM) ? PR_TRUE : PR_FALSE; encode_kind &= ~SEC_ASN1_MAY_STREAM; disallowStreaming = (encode_kind & SEC_ASN1_NO_STREAM) ? PR_TRUE : PR_FALSE; encode_kind &= ~SEC_ASN1_NO_STREAM; Should disallowStreaming be a local variable in sec_asn1e_contents_length, and should it be set like this? disallowStreaming = (encode_kind & SEC_ASN1_NO_STREAM) ? PR_TRUE : PR_FALSE; 4. I have two questions about the following block of code: @@ -735,16 +756,23 @@ default: len = ((SECItem *)src)->len; - if (may_stream && len == 0 && !disallowStreaming) - len = 1; /* if we're streaming, we may have a secitem w/len 0 as placeholder */ break; + } /* end switch */ + +#ifndef WHAT_PROBLEM_DOES_THIS_SOLVE + /* if we're streaming, we may have a secitem w/len 0 as placeholder */ + if (!len && insideIndefinite && may_stream && !disallowStreaming) { + len = 1; } - } +#endif + } /* end else */ Why is the code moved from the default case to the outside of the switch statement? Why the "WHAT_PROBLEM_DOES_THIS_SOLVE" macro/comment? This is the only place in the function where you use the new insideIndefinite argument, so you must know what this code is for. Otherwise, why add the new insideIndefinite argument? 5. Two nits in this block of code: /* The !isString test below is apparently intended to ensure that all ** constructed types receive indefinite length encoding. */ indefinite = (PRBool) (state->top->streaming && state->may_stream && (state->top->from_buf || !state->is_string)); "isString" should be "is_string". The "indefinite = (PRBool)" line should be indented one more space. 6.

Comment on attachment 197634 [details] [diff] [review] Don't set indefinte encoding until we actually output the header, merge patch with Julien's rev 1.8 checking One more question: why does the Exception in the variable name hdrException mean? sec_asn1e_hdr_encoding hdrException = hdr_normal;

Comment on attachment 197634 [details] [diff] [review] Don't set indefinte encoding until we actually output the header, merge patch with Julien's rev 1.8 checking r=wtc. I didn't see any problem with the code. I'd still like to see the comments updated to describe the new code.

The fix is in the NSS tip (NSS 3.11) and NSS_3_10_BRANCH (3.10.2).