436957 - Fix decoding in CERT_DecodeCRLDistributionPoints

Status: RESOLVED FIXED

Description

[Nelson Bolyard (seldom reads bugmail)]

In bug 321755 comment 10, in his review of the first patch for that bug, 
Julien observed that function CERT_DecodeCRLDistributionPoints and the 
template it uses (CRLDistributionPointTemplate) are flawed in the way 
they parse the CRLDP.  He wrote:

> The first entry in the sequence, for derDistPoint, is wrong in several ways :
> a) there should not be a SEC_ASN1_EXPLICIT nor SEC_ASN1_CONSTRUCTED
> b) we should not use SEC_AnyTemplate

> The corresponding code [...] is wrong . It tries to emulate choice by 
> looking at the DER.

(See <http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/certhigh/xcrldist.c&rev=1.6&mark=184-188#180>)

> Ugh! We need to use a choice in the template here.
> It appears that the same problem exists for the CRLDistributionPointStr
> structure, which was already there. They should both be fixed
> to use a CHOICE in the template for the distributionPointName component. 
> This is probably why the error was perpetuated . Sigh :(.

So, I propose to fix the existing flaw in CERT_DecodeCRLDistributionPoints 
before continuing to attempt to implement the RFE in bug 321755.

If I find that the code works that way because of some limitation of the 
existing NSS ASN.1 BER/DER decoders, we should at least know that here,
and have a bug on file about it.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

According to RFC 5280, the relevant ASN.1 definitions are:

   CRLDistributionPoints ::= SEQUENCE SIZE (1..MAX) OF DistributionPoint

   DistributionPoint ::= SEQUENCE {
        distributionPoint       [0]     DistributionPointName OPTIONAL,
        reasons                 [1]     ReasonFlags OPTIONAL,
        cRLIssuer               [2]     GeneralNames OPTIONAL }

   DistributionPointName ::= CHOICE {
        fullName                [0]     GeneralNames,
        nameRelativeToCRLIssuer [1]     RelativeDistinguishedName }

   ReasonFlags ::= BIT STRING {
        unused                  (0),
        keyCompromise           (1),
        cACompromise            (2),
        affiliationChanged      (3),
        superseded              (4),
        cessationOfOperation    (5),
        certificateHold         (6),
        privilegeWithdrawn      (7),
        aACompromise            (8) }

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

The definitions in comment 1 are part of an IMPLICITLY tagged module.

Here are more relevant definitions, also from an IMPLICITLY tagged module.

GeneralNames ::= SEQUENCE SIZE (1..MAX) OF GeneralName

GeneralName ::= CHOICE {
     otherName                 [0]  AnotherName,
     rfc822Name                [1]  IA5String,
     dNSName                   [2]  IA5String,
     x400Address               [3]  ORAddress,
     directoryName             [4]  Name,
     ediPartyName              [5]  EDIPartyName,
     uniformResourceIdentifier [6]  IA5String,
     iPAddress                 [7]  OCTET STRING,
     registeredID              [8]  OBJECT IDENTIFIER }


Observation:  
DistributionPoint.distributionPoint is an optional context-specific tagged
item that is itself a choice of context-specific tagged types.  If it is
not EXPLICITLY encoded, there will be a hopeless ambiguity for the decoder.
I think this is another case where an examination of X.680 and X.690 will
show that it must be explicitly tagged, even in an implicitly tagged module.
I'll examine those standards (again) for clarification. (I hate this stuff.)

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

OK, see the comment from David Kemp in bug 350200 comment 30.  
He wrote that the RFC authors were relying on the 1994 version of the 
X.680 and X.690 specs.

> However, the 1994 version of X.680 has a key difference from the 
> version you quote:
> 
>    28.6 The tagging construction specifies explicit tagging
>         if any of the following holds:
>          ...
>         c) the "Tag Type" alternative is used and the value of
>         "TagDefault" for the module is "IMPLICIT TAGS" or "AUTOMATIC
>         TAGS", but the type defined by "Type" is a choice type,
>         open type, or a "DummyReference" (see ... clause 8.3).
> 
> Notice that in 1994 there was no reference to an "untagged choice type". 

So, based on that, I think it is clear that the 
DistributionPoint.distributionPoint, defined as 
        distributionPoint       [0]     DistributionPointName OPTIONAL,
MUST be explicitly encoded because the referenced type is a choice type.

Julien also raised an issue about the template requiring CONSTRUCTED.
I think that comes down to whether the "base" type (the bottom-most 
underlying type) is constructed or not (e.g. a sequence or set).  
But this bears more investigation.  

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

X.690 (July 2002) section 8.14.2 and 8.14.3 says (paraphrased here) that
- explicit tags are always constructed, and
- implicit tags are constructed or not depending on the base encoding 
(the encoding of the tag that is being implicitly replaced).  

I think NSS's encoders and decoders do not require that SEC_ASN1_CONSTRUCTED
be specified together with SEC_ASN1_EXPLICIT, but I also don't think it hurts
to do so.  

So, It seems that the only remaining issue here is the use of the ANY type.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

I've been examining all the templates in NSS that use SEC_ASN1_EXPLICIT 
to see if they also specify SEC_ASN1_CONSTRUCTED.  All but one of them do. 
Out of 76 references to SEC_ASN1_EXPLICIT only 1 does not.  It is found in
SEC_PKCS12SecretAdditionalTemplate   in p12local.c line 1232 
I suspect it is flawed, and I suspect it is unused.  

So, I think I've established that the use of SEC_ASN1_EXPLICIT and SEC_ASN1_CONSTRUCTED in CRLDistributionPointTemplate is correct.  
The next step is to let the decoder do the decoding work that is now 
being done "by hand" inside CERT_DecodeCRLDistributionPoints.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

More thoughts on this.

1) IIRC, NSS doesn't support the combination of SEC_ASN1_OPTIONAL and 
SEC_ASN1_INLINE for constructed items that have their own separate 
subtemplates (e.g. subtemplates of sequences and sets).  IINM, 
SEC_ASN1_POINTER must be used instead of SEC_ASN1_INLINE, because the 
pointer value returned by SEC_ASN1_POINTER is the only reliable method to 
determine if the optional construct is present or not.  IIRC, this is not 
a problem for primitives subtemplates, including SEC_ASN1_ANY (which is 
treated like a primitive string type).

In the INLINE case, where the address of the struct holding the data to be encoded (or of the struct holding the data that was decoded) with the subtemplate is implicit and part of the caller's "dest", there is no way 
for inline templates to be omitted.  The address value used to determine 
if the item is to be omitted is always non-NULL.

(Maybe this is only a limitation of the ENcoder, and not of the DEcoder, 
but I think the decoder enforces this requirement also, to ensure that a 
template is good for both encoder and decoder.)  

2) I think it is not possible to use SEC_ASN1_SAVE in a CHOICE.  (Is it?)

So, I think the choices for decoding the DistributionPointName are:
a) leave it as a SEC_ASN1_ANY, and separately parse it (as is done now),
but use ASN1 templates and the ASN1 decoder to decode them, rather than
manually pulling apart the DER (as the code does now), or 

b) change the definition of struct CRLDistributionPointStr (a public struct)
to include a new pointer value, and change the template for the the first
member of CRLDistributionPointTemplate to use a SEC_ASN1_POINTER type.

The second of these two choices would be a problem with binary compatibility
(unless it can be shown that no code in the world could be using the existing
struct CRLDistributionPointStr because the decoder is badly broken).  
So I think the first of these choices is the obvious path to follow.  

I will work on a patch for the first choice.  

Julien, if you have any issue with that plan, please speak up very soon.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v0.1 (first draft)

Julien,  I'm not really asking you for a full review on this early patch, 
but I do want you to look at it to see the scope of what it changes. 
I suspect that it changes a lot less than you had expected it to. 
I really want your comments on the scope.  Is it far reaching enough?
Should I do more?

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

OBTW, while writing this patch, I found some bugs by code inspection and 
fixed them.  They are:

a) In CERT_EncodeCRLDistributionPoints, after calling 
		point->derCrlIssuer = cert_EncodeGeneralNames
		    (ourPool, point->crlIssuer);
the code was checking the input argument again, rather than the output 
value, for NULL, and it failed to set rv to SECFailure if it was NULL.

b) in CERT_DecodeCRLDistributionPoints, set rv to SECFailure in several 
error paths where it was previously being left as SECSuccess.  See the 
old code that was replaced by the generalName and default cases in the 
new switch.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

I just realized another flaw with the existing templates in xcrldist.c.
They're missing the end-of-template marker.  Let me illustrate that with
a pseudo-patch that corrects it:

 static const SEC_ASN1Template FullNameTemplate[] = {
     {SEC_ASN1_CONTEXT_SPECIFIC | SEC_ASN1_CONSTRUCTED | 0,
         offsetof (CRLDistributionPoint,derFullName),
-        CERT_GeneralNamesTemplate}
+        CERT_GeneralNamesTemplate},
+    { 0 }
 };
 
 static const SEC_ASN1Template RelativeNameTemplate[] = {
     {SEC_ASN1_CONTEXT_SPECIFIC | SEC_ASN1_CONSTRUCTED | 1,
         offsetof (CRLDistributionPoint,distPoint.relativeName),
-        CERT_RDNTemplate}
+        CERT_RDNTemplate},
+    { 0 }
 };

Now, I think this discovery constitutes proof that the existing code 
here is dead code, unused.  And that makes me wonder if we can get away
with making non-binary compatible changes, such as adding new members 
to the related structures, such as to struct CRLDistributionPointStr in lib/certdb/certt.h.  See
http://lxr.mozilla.org/security/source/security/nss/lib/certdb/certt.h#735

Wan-Teh, what do you think about that?

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Also 
  const SEC_ASN1Template CERTCRLDistributionPointsTemplate[] = {
-     {SEC_ASN1_SEQUENCE_OF, 0, CRLDistributionPointTemplate}
+     {SEC_ASN1_SEQUENCE_OF, 0, CRLDistributionPointTemplate},
+    { 0 }
 };

Comment 11

[Wan-Teh Chang]

I think so.  Isn't CRLDistributionPoint brand new code in NSS 3.12?

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

> Isn't CRLDistributionPoint brand new code in NSS 3.12?
No, xcrldist.c goes back to before NSS 3.0.  It and the associated 
struct CRLDistributionPointStr in cert.h were checked in to Mozilla cvs 
on 2000-03-31 with the very initial NSS 3.0 checkin.

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

I see that CERT_EncodeCRLDistributionPoints was added to nss.def for 3.5 
and   that CERT_DecodeCRLDistributionPoints was added to nss.def for 3.10.

NSS didn't use CERT_EncodeCRLDistributionPoints until certutil was enhanced
to be able to generate them in certs in 3.12.  However, I imagine that there
must have been some request for it to be exported back in 3.5, so I don't 
think we can assume it is unused outside of NSS.

CERT_DecodeCRLDistributionPoints has been used in 
CERT_FindCRLDistributionPoints since NSS 3.0 initial checkin, and is used 
by pp and certutil beginning in 3.10 to print out CRLDP extensions in certs.

Comment 14

[Julien Pierre]

Nelson,

Re: comment 9,

The end-of-template marker is only required for sequences and choices.

See http://www.mozilla.org/projects/security/pki/nss/tech-notes/tn1.html, decoder templates, kind .

The requirement does not apply to SEQUENCE_OF . So this is not proof that the templates are dead code.

Comment 15

[Julien Pierre]

To clarify my comment 14, the templates in comment 9 are correct as far as they don't need a NULL terminator, since they are not arrays. But they don't use SEQUENCE_OF - the templated quoted in comment 10 does, and also does not need termination.

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

Julien, If sequence and choice need a terminator, then so does set, surely.

Comment 17

[Julien Pierre]

Nelson,

You are right. SET would require a terminator too. It just so happens that SET is never used in all of NSS, nor in any PKCS standard. In DER one probably would always use a SEQUENCE rather than SET, since the main difference between SET and SEQUENCE is the order of components, but in DER even a SET has to be ordered, so they are really the same.

Comment 18

[Nelson Bolyard (seldom reads bugmail)]

> It just so happens that SET is never used in all of NSS,

Really?  A Distinguished Name (a.k.a. Directory Name), such as the issuer
name and subject names in a cert, is a sequence of sets.  Each of those
sets is a set of sequences.  So, I'm a little surprised that there are no 
sets in NSS, because I'd expect them to be necessary to encode and decode
certs' DNs.

Comment 19

[Julien Pierre]

Nelson,

SET is not the same as SET OF . SET OF, like SEQUENCE OF, does not require a template terminator.

Comment 20

[Julien Pierre]

Comment on attachment 323498 [details] [diff] [review]
patch v0.1 (first draft)

I see one main problem here which is in the decoding case for relativeDistinguishedName . You don't need the following call to QuickDER :

+		case relativeDistinguishedName:
+		    rv = SEC_QuickDERDecodeItem(arena, point, 
+			RelativeNameTemplate, &(point->derDistPoint));
+		    break;

You can just delete it and break. Otherwise you are decoding the same thing twice. With your new DistributionPointNameTemplate,  this field has already been decoded, so you have nothing left to do.

I wish you could eliminate FullNameTemplate and RelativeNameTemplate completely. They are only needed for the encoding right now. There should be a way to do that. I am not sure if it can be done without breaking the binary structure.

Comment 21

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v2, Delete duplicate decode.

Good catch, Julien.

Comment 22

[Nelson Bolyard (seldom reads bugmail)]

Checking in xcrldist.c; new revision: 1.7; previous revision: 1.6

Comment 23

[Alexei Volkov]

Looks like the checkin broke tinderbox tests. Reopening the bug...

Comment 24

[Alexei Volkov]

cert.sh: #431: Testing CRL Distribution Points Extension (1)  - FAILED
cert.sh ERROR: Testing CRL Distribution Points Extension failed 1

Comment 25

[Nelson Bolyard (seldom reads bugmail)]

Thanks, Alexei.  I backed that revision out. 

Comment 26

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v3 - part 1 of 2, compare to patch v2 (checked in)

Patch v2 was flawed.  It worked with CRLDPs of type GeneralName but not 
with those of type RDN.  This was concealed because NSS's cert display code
(common to certutil and pp) simply IGNORED all CRLDPs of type RDN.  

I also have a patch (to be attached shortly) that fixes our cert display code 
to properly display RDN CRLDPs, and also fixes the test script to not report 
false errors when the RDN CRLDPs are properly decoded.  I will attach it as
a separate patch so that the new patch for xcrldist.c will be comparable to
the old one.

Comment 27

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v3 part 2 (checked in, except for comments in secasn1t.h)

This patch fixes 3 issues.
1. It fixes a bug in the certext.txt test file (used by cert.sh) that caused
cert.sh to erroneously report failures when the certutil output was correct.

2. It fixes the cert pretty-printing code (used in certutil and pp) to 
properly and correctly format and display CRLDP points that are RDNs.
Previously, the pretty printing code completely ignored RDN points.

3. It corrects the erroneous documentation in secasn1t.h about the meanings
of the fields and flags in the ASN.1 en/decoder templates.  Specifically, 
it adds explanations about the fields that pertain to CHOICE templates, 
which was completely missing before, and it adds some documentation about 
the disallowed combinations of flags.

The patch to secasn1t.h can be reviewed separately from the rest.  The rest
of the patch does not depend on that part.  But the rest of the two part 
patch all goes together.  The fix to xcrldist.c cannot be checked in without
the fix to the certext.txt file.  The fixes to nss/cmd/lib/secutil.[ch] 
should also go in at the same time, IMO.

Comment 28

[Julien Pierre]

Comment on attachment 325894 [details] [diff] [review]
patch v3 part 2 (checked in, except for comments in secasn1t.h)

Some of the comments at the very end of secasn1t.h are not correct.

1) The combination of OPTIONAL INLINE is allowed for PRIMITIVE tags.


They don't have to be UNIVERSAL. At least for QuickDER that is the case. And it should be the case for the other decoder too (if not, that is a bug).

2) I am not sure, but I believe it is valid to explicitly encode a universal tag, though I'm not sure why you would want to do that. At least I don't think our decoders preclude it, though some standards might.

3) POINTER cannot be set together with any tag . You have to specify a subtemplate, or a dynamic function to provide one. 

4) SKIP is supported with optional if a tag is specified, at least by QuickDER. See the technote on the decoders.

5) your comments are unfinished :)

Comment 29

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 325894 [details] [diff] [review]
patch v3 part 2 (checked in, except for comments in secasn1t.h)

Julien, please review the rest of patch v3 part 2.  Your review comments 
apply only to the one part of that patch that was optional.  The necessary
part of patch v3 part 2 is the part that most urgently needs review.

The comments I made on restrictions were based on the assertions in the 
older BER/DER decoder and in the encoder, and I believe they accurately
describe those assertions.  However, Our en/decoders may be overly strict 
in some respects.  

I invite you to add more comments about the differences between quickder 
and the BER decoder.

Regarding pointer, you're right that all tags are disallowed.  The set 
of allowed flags depends on various conditions which I thought were too 
complicated to explain in the comments.  See 
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/util/secasn1d.c&rev=1.38#567

I don't intend to make the comments be 100% complete.  I intend to document 
the restrictions that were recently violated by senion NSS developers.

Comment 30

[Julien Pierre]

Nelson,

My comments were only on secasn1t.h because the rest of was attachment 325894 [details] [diff] [review] was OK.

Comment 31

[Nelson Bolyard (seldom reads bugmail)]

I interpret comment 30 to be a positive review for all of "patch v3 part 2"
except the part to secasn1t.h, and so I have committed all but that part.

lib/certhigh/xcrldist.c; new revision: 1.9; previous revision: 1.8
cmd/lib/secutil.c;       new revision: 1.90; previous revision: 1.89
cmd/lib/secutil.h;       new revision: 1.28; previous revision: 1.27
tests/cert/certext.txt;  new revision: 1.3; previous revision: 1.2

I will leave this bug open until the issues with secasn1t.h are resolved.

Comment 32

[Julien Pierre]

Comment on attachment 325894 [details] [diff] [review]
patch v3 part 2 (checked in, except for comments in secasn1t.h)

I thought I had already given an r- on this. The only thing wrong is the comments in secasn1t.h, as noted previously in comment 28. Nelson and I have been exchanging emails over this.

Comment 33

[Nelson Bolyard (seldom reads bugmail)]

This bug was fixed in NSS 3.12.1.
I have opened a new bug to track the issue about comments in secasn1t.h.
That is bug 566383.