Closed
Bug 436957
Opened 13 years ago
Closed 11 years ago
Fix decoding in CERT_DecodeCRLDistributionPoints
Categories
(NSS :: Libraries, defect, P2)
Tracking
(Not tracked)
RESOLVED
FIXED
3.12.1
People
(Reporter: nelson, Assigned: nelson)
References
()
Details
Attachments
(2 files, 2 obsolete files)
8.89 KB,
patch
|
julien.pierre
:
review+
|
Details | Diff | Splinter Review |
6.68 KB,
patch
|
julien.pierre
:
review-
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•13 years ago
|
||
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) }
Assignee | ||
Comment 2•13 years ago
|
||
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.)
Assignee | ||
Comment 3•13 years ago
|
||
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.
Assignee | ||
Comment 4•13 years ago
|
||
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.
Assignee | ||
Comment 5•13 years ago
|
||
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.
Assignee | ||
Comment 6•13 years ago
|
||
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.
Assignee | ||
Comment 7•13 years ago
|
||
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?
Attachment #323498 -
Flags: review?(julien.pierre.boogz)
Assignee | ||
Comment 8•13 years ago
|
||
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.
Assignee | ||
Comment 9•13 years ago
|
||
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?
Priority: -- → P1
Assignee | ||
Comment 10•13 years ago
|
||
Also const SEC_ASN1Template CERTCRLDistributionPointsTemplate[] = { - {SEC_ASN1_SEQUENCE_OF, 0, CRLDistributionPointTemplate} + {SEC_ASN1_SEQUENCE_OF, 0, CRLDistributionPointTemplate}, + { 0 } };
Comment 11•13 years ago
|
||
I think so. Isn't CRLDistributionPoint brand new code in NSS 3.12?
Assignee | ||
Comment 12•13 years ago
|
||
> 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.
Assignee | ||
Comment 13•13 years ago
|
||
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•13 years ago
|
||
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•13 years ago
|
||
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.
Assignee | ||
Comment 16•13 years ago
|
||
Julien, If sequence and choice need a terminator, then so does set, surely.
Comment 17•13 years ago
|
||
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.
Assignee | ||
Comment 18•13 years ago
|
||
> 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•13 years ago
|
||
Nelson, SET is not the same as SET OF . SET OF, like SEQUENCE OF, does not require a template terminator.
Comment 20•13 years ago
|
||
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.
Attachment #323498 -
Flags: review?(julien.pierre.boogz) → review-
Assignee | ||
Comment 21•13 years ago
|
||
Good catch, Julien.
Attachment #323498 -
Attachment is obsolete: true
Assignee | ||
Updated•13 years ago
|
Attachment #325065 -
Flags: review?(julien.pierre.boogz)
Updated•13 years ago
|
Attachment #325065 -
Flags: review?(julien.pierre.boogz) → review+
Assignee | ||
Comment 22•13 years ago
|
||
Checking in xcrldist.c; new revision: 1.7; previous revision: 1.6
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Comment 23•13 years ago
|
||
Looks like the checkin broke tinderbox tests. Reopening the bug...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 24•13 years ago
|
||
cert.sh: #431: Testing CRL Distribution Points Extension (1) - FAILED cert.sh ERROR: Testing CRL Distribution Points Extension failed 1
Assignee | ||
Comment 25•13 years ago
|
||
Thanks, Alexei. I backed that revision out.
Assignee | ||
Comment 26•13 years ago
|
||
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.
Attachment #325065 -
Attachment is obsolete: true
Attachment #325890 -
Flags: review?(julien.pierre.boogz)
Assignee | ||
Comment 27•13 years ago
|
||
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.
Attachment #325894 -
Flags: review?(julien.pierre.boogz)
Assignee | ||
Updated•13 years ago
|
Attachment #325890 -
Attachment description: patch v3 - part 1 of several → patch v3 - part 1 of 2, compare to patch v2
Updated•13 years ago
|
Attachment #325890 -
Flags: review?(julien.pierre.boogz) → review+
Comment 28•13 years ago
|
||
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 :)
Attachment #325894 -
Flags: review?(julien.pierre.boogz) → review-
Assignee | ||
Comment 29•13 years ago
|
||
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.
Attachment #325894 -
Flags: review- → review?(julien.pierre.boogz)
Comment 30•13 years ago
|
||
Nelson, My comments were only on secasn1t.h because the rest of was attachment 325894 [details] [diff] [review] was OK.
Assignee | ||
Comment 31•13 years ago
|
||
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•13 years ago
|
||
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.
Attachment #325894 -
Flags: review?(julien.pierre.boogz) → review-
Assignee | ||
Updated•13 years ago
|
Attachment #325890 -
Attachment description: patch v3 - part 1 of 2, compare to patch v2 → patch v3 - part 1 of 2, compare to patch v2 (checked in)
Assignee | ||
Updated•13 years ago
|
Attachment #325894 -
Attachment description: patch v3 part 2 → patch v3 part 2 (checked in, except for comments in secasn1t.h)
Assignee | ||
Updated•12 years ago
|
Target Milestone: 3.12.1 → 3.12.5
Assignee | ||
Updated•11 years ago
|
Priority: P1 → P2
Target Milestone: 3.12.5 → ---
Assignee | ||
Comment 33•11 years ago
|
||
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.
Status: REOPENED → RESOLVED
Closed: 13 years ago → 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.12.1
You need to log in
before you can comment on or make changes to this bug.
Description
•