Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add lint to detect Domain Labels that violate LDH-Label syntax #646

Closed
wants to merge 3 commits into from
Closed

Add lint to detect Domain Labels that violate LDH-Label syntax #646

wants to merge 3 commits into from

Conversation

CBonnell
Copy link
Contributor

While there are several lints that checks for the syntactic correctness of domain labels, there appears to be no comprehensive check to ensure that all labels of a FQDN are LDH Labels.

While some of the checks in this lint are duplicative of other lints (label too long, etc.), I think it's best to have a single lint that comprehensively checks for all issues related to LDH Labels/preferred name syntax.

As a future step, the now-redundant lints could be removed.

Comment on lines 29 to 32
Description: "The FQDN contained in the dNSName MUST be composed entirely of LDH Labels.",
Citation: "BR 7.1.4.2.1",
Source: lint.CABFBaselineRequirements,
EffectiveDate: util.CABEffectiveDate,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this correct?

That is:

My suggestion would be this should be an RFC 5280 lint, and the description updated. I'm definitely supportive of this lint, though.

Copy link
Contributor Author

@CBonnell CBonnell Oct 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. The main concern with implementing this as an RFC lint is that 5280 allows for wildcards, the specification of which is beyond the scope of that document. We need to allow for Wildcard Domain Names (as defined in the BRs), but a strict interpretation of "preferred name syntax" will yield false positives with asterisk-only labels.
  2. I modified the Description from the original BR text introduced in BR 1.8.0 to make it less verbose. If the convention is that we copy verbatim from the source text, I can pull in the full quote.
  3. Yes, I know. I wrote it 😃
  4. This raises a good point. The simplest solution is to make the EffectiveDate the date BR v1.8.0 became effective, at the risk of missing some historical mis-issuance. We could make a more historically accurate check by reducing the severity (or suppressing altogether) of findings in those certificates issued under SC12, but that introduces a lot of implementation complexity (and therefore potential for bugs) that is of no benefit for issuance moving forward. Given this, I'm leaning towards changing the effective date to the effective date of v1.8.0.

Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are many integration test failures caused by redacted pre-certs (e.g., a SAN dNSName of "?.example.com") that were issued several years ago. This will be another source of "false" positives if the EffectiveDate is not amended to BR v1.8.0. Given this, I think the cleanest solution is to modify the EffectiveDate to avoid the morass of exceptions that have been historically allowed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I'm not sure I understand the point about wildcards. You handle those here as well? Are you concerned about situations like a*b.example.com? RFC6125-bis is specifically addressing that as part of the updates. I don't anticipate any issues adding this as an RFC lint, but happy to hear from @zakird @cpu or @dadrian if there are.
  2. I think eliding is fine, but perhaps indicate where that is (e.g. '...'). Mostly, the goal is to provide reference to the text, and that's why I raised it - it wasn't exactly obvious to me where this requirement was derived from.
  3. Right :) My point was that it was new language, but tied with the old date.

As to 'redacted' precerts, they were considered misissued by some root programs, so it doesn't seem unreasonable to keep the effective date as when it was? That is, they're true positives, but in the past, and so I'm not sure the issues/concerns there?

Obviously, I'm generally going to argue for the extreme end of the spectrum, and be open to be talked down from that ;) I'm obviously a vocal believer that this was always an RFC requirement, so I'd be perfectly happy with the lint always being set. I'd be curious how the other maintainers feel about situations here where some root programs allowed/ignored the error due to potential CA confusion.

Possible options:

  • LDH lint with the RFC effective dates (which goes to 2459)
  • LDH lint with the RFC effective date, but moved to a LDH+U lint for the period between SC12/BR 1.6.2 and the SC12 sunset date (2019-05-01)
  • LDH+U lint for the period of SC12 to sunset (2019-05-01) + LDH lint for 2019-05-01 and later
  • As to the wildcard issue, one way would be for the regex to be LDH+W everywhere.
    • BRs since 1.0 have required in the "left most position", and so while 1.8.0 used better terminology, it seems there was consensus that was an existing clarification, and lint_dnsname_wildcard_only_in_left_label already catches this.

I don't think the effective date of 1.8.0 sounds right, if only because the years of discussion (and the explicit text) seem to address this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Yes, I handle the wildcard syntax, as defined in the BRs, in this lint in the cabf_br package. Unless you happen to know of a RFC that constrains wildcard syntax in the same manner as the BRs that we can cite in this lint, it is not appropriate to add this lint to the RFC lints. I don't believe RFC 6125-bis is an appropriate document for this purpose, as it is merely states best practices for client-side certificate processing and doesn't constrain certificate profiles. Even if it did, it would be very strange to cite RFC 6125-bis in this lint as RFC 6125-bis hasn't been published yet as an RFC despite your wanting to make the effective date of this lint January 1999 (the publication date of RFC 2459). Given all this, I believe the cabf_br lint suite is the best location for this lint.
  2. I agree with you that quoting verbatim the relevant text has value; the user can then do a copy/paste to search in the relevant document to find a fuller description of the requirement. I updated the Description of this lint in the latest commit.

As to 'redacted' precerts, they were considered misissued by some root programs, so it doesn't seem unreasonable to keep the effective date as when it was? That is, they're true positives, but in the past, and so I'm not sure the issues/concerns there?

My understanding is that the only public notification by any Root Program that redacted pre-certificates are mis-issuances was a singular post on the CT Policy mailing list in May 2016. Given that this was the policy decision of a single Root Program, it would be historically inaccurate to classify this prohibition as a BR prohibition shared across Root Programs.

But more generally, I'm not sure of the value of faithfully replicating historical policy changes that are no longer in force as new lints. I think a bit of historical inaccuracy (in terms of false positives or negatives) is fine as long as it decreases the amount of code/maintenance burden for the project. My opinion is that we should optimize for correctness of the current requirements while allowing for some historical inaccuracy. Otherwise, the authoring and maintenance of lints becomes increasingly onerous. Like @sleevi, I would be very interested to hear the maintainers' opinions on the best direction here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@CBonnell To your point about the RFC prohibiting it, namely

if a specification does not allow for that character to be used as a wildcard, then the RFC lint is being overly liberal in allowing it.

I tried to address in #646 (comment) the relevant text from the RFC, namely:

Finally, the semantics of subject alternative names that include wildcard characters (e.g., as a placeholder for a set of names) are not addressed by this specification. Applications with specific requirements MAY use such names, but they must define the semantics.

Similarly, I don’t think your solution is good: redacted precerts are not a valid thing, and have never been. This really is solely about wildcards here, and in the context of a DNSName lint, then beyond RFC 3280/5280, you have RFC 2818 and 6125 making it clear they’re permitted and refining the syntax.

As to the meta-goals for ZLint, to date it has been focused on what is being actively maintained. This is why, for example, there are discussions of removing the ETSI lints absent more demonstrable maintenance. However, that largely seems irrelevant here, because it seems the crux of the argument is “the RFC forbids wildcards”, but it doesn’t.

Copy link
Contributor Author

@CBonnell CBonnell Nov 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you have RFC 2818 and 6125 making it clear they’re permitted and refining the syntax

RFC 6125 makes it clear that the asterisk ("*") can appear within a label and does not have to be the sole character, despite this being disallowed by CABF rules. Additionally, the ABNF for "preferred name syntax" in RFC 1034 makes it clear that asterisks are not allowed: https://datatracker.ietf.org/doc/html/rfc1034#section-3.5. So I disagree that allowing asterisks in a generic RFC-level lint is an appropriate solution.

As to the meta-goals for ZLint, to date it has been focused on what is being actively maintained.

There has been a desire to add support for linting other certificate types (#504) or even other document types (#458). Adding TLS-flavoring to RFC lints that are general across certificate types will complicate the implementation of such lints.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What part of the MAY here mentioned directly in RFC 5280 is problematic here?

I’m not sure how/why you’re arguing that the following language prohibits wildcards:

Applications with specific requirements MAY use such names, but they must define the semantics.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applications with specific requirements MAY use such names, but they must define the semantics.

This is exactly why I classified this lint as a CABF lint. The specification in question (CABF BRs) defines the semantics of wildcards. None of the RFCs define the semantics of wildcards that match that of the CABF BRs.

Copy link
Contributor

@sleevi sleevi Nov 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, in the discussion, a few things are getting lost here, so let me try to restate:

Wildcards

  1. What characters does RFC 5280 allow within the dNSName?
    • Unambiguously, we know any characters in the "preferred name syntax" (a-z, A-Z, 0-9, -) are allowed.
    • We also can see that the RFC permits wildcards, but defers to other RFCs to define the syntax and semantics of where those wildcards can be placed.
  2. In the context of TLS using dNSNames, as RFC 6125, Appendix B captures, several RFCs have defined a variety of syntaxes, which also supports the "Wildcard is allowed"

This lint is not a lint for "left-most label" wildcard syntax. We already have a lint for this. As noted by the name, this is merely a lint for LDH / "preferred name syntax", including the syntactical rules from preferred name syntax (e.g. prohibitions on - as a leading/trailing character).

This lint can fully be an RFC lint if nonLDHCharacterRegex is updated to permit * as a character, given that RFC 5280 permits such characters. This would also obviate the need to util.RemovePrependedWildcard - because the regex would be satisfied by such labels.

Existing lints for wildcard labels, within the BR section, will handle the BR compliance. There's no need to try to lint for that in this lint - and ample reason not to (conflating multiple requirements)

Underscores

The question I raised in #646 (comment) was related to the Effective Date of the lint. I believe we agree that the "preferred name syntax" is an existing RFC requirement since the first RFC requirement. Preferred name syntax prohibits underscores.

The CABF, briefly, had an exception to the RFC (SC12 adoption to sunset date). The question was whether RFC lints should be selectively enforced based on CABF requirements, for historic purposes, and it seems there's agreement that's of limited value. That is, underscores were prohibited, and presently are prohibited, so we don't need to worry about disabling the RFC lint because the CABF said it was OK.

If we apply this logic to nameConstraints, it would equally be possible to imagine there is a "name constraints MUST be critical" RFC lint, which treats it as an error, and require the CA to selectively ignore such lints (e.g. because the BRs permit), along with a BR lint "name constraints MAY be critical" (which could be sunset once the BRs remove that exception, which is doable within a few months)

Question Marks

You raised a concern with making the LDH lint effective since the RFC start, as it would flag pre-certificates using redaction as misissued. However, this is correct as well - there was no support for redaction in CTv1 (which ZLint supports), and while there were several drafts for redaction in CTv2 using different approaches, they were just that - drafts. At issue here is that while SCTs are versioned, at the time, the precertificates stored in logs weren't unambiguously so, and so the signed data used for a RFC6962-bis-draft-14 log (i.e. Symantec's logs) could be replayed against a CTv1 log (i.e. every other log) and be treated as a signed certificate from the CA.

However, as with underscores, ZLint doesn't necessarily need to concern itself with that now-quite-old historic accident, which was equally misissuance at the time anyways. So this is moot.

Changes Requested

To recap, I'm requesting you:

  1. Change this from a BR lint to an RFC lint
  2. Change the effective date to the RFC date
  3. Change the regex to allow *
    • Which means you can safely remove the util.RemovePrependedWildcard without breaking any certs that would be accepted today.
    • Will ensure you don't cause errors on certs that are BR-errors not RFC-errors

Copy link
Contributor

@sleevi sleevi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just reflecting in GitHub don’t think we should land this as a BR lint, and it should be an RFC lint. I’m trying to keep an open mind in light of new information/perspective, but in the present form, think it should be changed.

@vanbroup
Copy link
Contributor

Should this lint also check for any reserved LDH Labels that are not P-Labels?

In relation to:
https://bugzilla.mozilla.org/show_bug.cgi?id=1740493

@CBonnell
Copy link
Contributor Author

Should this lint also check for any reserved LDH Labels that are not P-Labels?

In relation to: https://bugzilla.mozilla.org/show_bug.cgi?id=1740493

Hi Paul, that lint was implemented as part of #635.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants