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

CABF SMIME BR 7.1.2.3.e - KeyUsages #757

Merged
merged 13 commits into from
Nov 5, 2023

Conversation

robplee
Copy link
Contributor

@robplee robplee commented Oct 26, 2023

Addressing #754 with separate lints to check presence and criticality of the KU extension as well as covering the allowed KUs for RSA, EC and Edwards curve public keys.

One shortcoming is that for the Edwards curve keys I've had to leave a TODO in place as I don't think zcrypto has support for curve 448 in certificates (correct me if I'm wrong but I couldn't see it when I looked). That being said, it doesn't stop the checks working adequately for keys defined on curve 25519 so I don't think it should be a blocker for merging this PR.

Copy link
Member

@christopher-henderson christopher-henderson left a comment

Choose a reason for hiding this comment

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

Thank you @robplee!

At very first glance I thought ...that's a bit much for one PR... but as it turns out 7.1.2.3.e is quite a lot for just one subsection 😆 .

So thank you for tackling this hefty chunk.


...as I don't think zcrypto has support for curve 448 in certificates (correct me if I'm wrong but I couldn't see it when I looked)

It's fine if it doesn't. This is what the integration test corpus is for (either we would likely see issues or there simply are no such certs in the wild making it somewhat irrelevant).


There was a lot of a sophisticated use of bit flags that I believe you may have been endeavoring for in order to satisfy the clause ...Other bit positions SHALL NOT be set.

However, I think it would be easier to write that clause as its own lint and to let the other lints otherwise ignore that nuance.


Edit: I'm not convinced of my own implementation here.

I offer the following, although I am somewhat unsure of its semantics. My rationale is...

If 7.1.2.3.e ever referred to the KU as being a SHALL or MAY in then it is allowed. All others are not.

/*
 * ZLint Copyright 2023 Regents of the University of Michigan
 *
 * Licensed under the Apache License, Version 2.0 (the "License"); you may not
 * use this file except in compliance with the License. You may obtain a copy
 * of the License at http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
 * implied. See the License for the specific language governing
 * permissions and limitations under the License.
 */

package cabf_smime_br

import (
	"github.com/zmap/zcrypto/x509"
	"github.com/zmap/zlint/v3/lint"
	"github.com/zmap/zlint/v3/util"
)

func init() {
	lint.RegisterLint(&lint.Lint{
		Name:          "e_superfluous_key_usages",
		Description:   "...Other bit positions SHALL NOT be set",
		Citation:      "7.1.2.3.e",
		Source:        lint.CABFSMIMEBaselineRequirements,
		EffectiveDate: util.CABF_SMIME_BRs_1_0_0_Date,
		Lint:          NewSuperfluousKeyUsages,
	})
}

type superfluousKeyUsages struct{}

func NewSuperfluousKeyUsages() lint.LintInterface {
	return &superfluousKeyUsages{}
}

func (l *superfluousKeyUsages) CheckApplies(c *x509.Certificate) bool {
	return util.IsSubscriberCert(c) && util.IsSMIMEBRCertificate(c) && util.IsExtInCert(c, util.KeyUsageOID)
}

func (l *superfluousKeyUsages) Execute(c *x509.Certificate) *lint.LintResult {
	allowed := x509.KeyUsageKeyEncipherment | x509.KeyUsageDigitalSignature | x509.KeyUsageContentCommitment | x509.KeyUsageDataEncipherment
	if c.KeyUsage^allowed != 0 {
		return &lint.LintResult{Status: lint.Error}
	}
	return &lint.LintResult{Status: lint.Pass}
}

@cardonator
Copy link
Contributor

Can you guys help me understand why I seem to be having an issue testing this and some other changes regarding SMIME lints out?

I have a PEM with Email Protection EKU and I can't get any SMIME lints to trigger when running zlint directly. -lint-sources-list doesn't show SMIME and the lints aren't ever added to the registry that I can tell. Is there some other pieces missing that we need for running the CLI or as a library?

@christopher-henderson
Copy link
Member

@cardonator do you have a particular branch I can take a look at?

@cardonator
Copy link
Contributor

cardonator commented Oct 30, 2023

I am working on this branch. master...cardonator:zlint:check_aia_internal_names

I will email you a pem I'm testing with, I haven't generated a cert to test with yet. I was trying to test this by building zlint and then running ./zlint cert.pem. None of the SMIME lints show as running in the response.

@christopher-henderson
Copy link
Member

@cardonator

Ah, so the cert that you sent me is fine, and it indeed passes the "is this an email cert" check, however there seems to be a regression with regard to cert-to-source-document dispatch that broke smime from the CLI.

$ ./zlint -list-lints-source
    Apple
    CABF_BR
    CABF_BR
    CABF_EV
    Community
    ETSI_ESI
    Mozilla
    RFC3279
    RFC5280
    RFC5280
    RFC5480
    RFC5891
    RFC8813

Listing CABF twice with no SMIME version is incorrect and smime lints are not showing up in the registry on master. I've fired up #759 for me to go fix this.

In the meantime, you are fine to continue writing-and-testing lints as this is an issue in the CLI.

@cardonator
Copy link
Contributor

Thanks @christopher-henderson and sorry to hijack this thread. I will continue on my branch.

@christopher-henderson christopher-henderson merged commit 8923170 into zmap:master Nov 5, 2023
4 checks passed
}

case dualUsage:
mask := 0x1FF ^ (x509.KeyUsageDigitalSignature | x509.KeyUsageContentCommitment | x509.KeyUsageKeyAgreement | x509.KeyUsageEncipherOnly | x509.KeyUsageDecipherOnly)
Copy link

Choose a reason for hiding this comment

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

Requirement: "For signing only, bit positions SHALL be set for digitalSignature and MAY be set for nonRepudiation.
For key management only, bit positions SHALL be set for keyAgreement and MAY be set for encipherOnly or decipherOnly. For dual use, bit positions SHALL be set for digitalSignature and keyAgreement and MAY be set for nonRepudiation and for encipherOnly or decipherOnly (only if keyAgreement is set)."

My understanding is that OR is either encipher or decipher, but not both. I think the current code permits the combination of both.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think the text is written in a way that makes your interpretation the only acceptable one. Generally speaking "or" means a&&!b, !a&&b, a&&b. I don't think the BRs support the narrower interpretation you're putting on them

Copy link

Choose a reason for hiding this comment

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

Thanks, I agree it's ambiguous - I'll raise this topic in the working group for clarification.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any usecase for doing so however?

The purpose of encipherOnly and decipherOnly is to limit the usage of the key. The absense of both, allows for both options to be used. As such, adding both, would be redundant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm a bit surprised that it's not written unambiguously anywhere that a cert can't have both EncipherOnly and DecipherOnly KUs. While writing my comment I checked RFC5280 and that doesn't actually say that it's not allowable to have both at the same time which I'm a little surprised by...

}

default:
return &lint.LintResult{Status: lint.NA}
Copy link

Choose a reason for hiding this comment

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

So does this mean that not having DigSig or KeyAgreement is permitted? I think the requirement is to have at least one of these two values?

Copy link
Contributor Author

@robplee robplee Jan 3, 2024

Choose a reason for hiding this comment

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

There isn't text in the SMIME BRs for the requirement you're suggesting exists. The text says that for <category> then <KU/s> SHALL be present. I was very careful when writing this lint to see if there was any text that states that a cert with an ECDSA/RSA KU must be a "signing only" "key management only" or "dual use" certificate and such a requirement doesn't exist. The EdDSA KU text is more tightly specified but the ECDSA/RSA text doesn't include the requirement you're claiming

Copy link

Choose a reason for hiding this comment

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

It seems implied to me that an RSA/ECDSA SMIME BR certificate either must comply with signing, key agreement or dual use (and nothing else) - but it's true that's not explicit. I'll also raise this in the WG.

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.

5 participants