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
Revisiting e_key_usage_and_extended_key_usage_inconsistent
lint and RFC interpretation.
#553
Comments
👋 @cardonator Interested in your thoughts on this case too |
Thanks for pinging me on this @cpu. I want to review this question/use case more thoroughly with some other internal folks and I'll get back with some better thoughts on this subject. |
I can say that after we merged this I I ran a certificate that immediately triggered this error. Granted, it was a certificate that was generated by an internal CA in order to secure a DB API, so it wasn't exactly representative. But it did make me wonder.
I'll take this on to at least smoke check what was going on with these. |
The examples given here are represented in many (10^8 order of magnitude, it looks like) of the certificates used in the WebPKI, so at least w.r.t. a certificate that has |
We would need to ask @ZsofiaTomicsko about this.
|
My issue is interpreting the "or" in the RFC as a "xor" still seems to be the most logical, and at the moment the only possible solution, since the author is using the "and/or" expression too. For example:
Because of this I feel like "or" and "and/or" must be interpreted differently, and I can't think of anything else other than "or" meaning "xor" and "and/or" meaning the traditional "or". Technically in that special case you mentioned:
the digitalSignature and the keyEncipherment aren't seperated by an "or", but a comma, so maybe it could be replaced by an "and/or" instead, but this interpretation seems a bit forced to me at the moment. For me personally the more unclear question was whether this list was meant to be a recommendation only, or if these are the only valid combinations. Cristopher described this problem really well in #497 :
In my comment (#497 (comment) point 3) I started leaning towards marking this as a warning instead too, and I still stand by that opinion. Please tell me what you think. |
Thanks @ZsofiaTomicsko, I think your instinct here was good. Apologies we didn't catch this sooner :-)
I started to pull on this thread and I'm wondering if there's also something going on with our integration tests..... Running: make integration SHELL=(which bash) INT_FLAGS="-lintSummary -fingerprintSummary -lintFilter='e_key_usage_and_extended_key_usage_inconsistent'" I get the summary:
But there are ~566,923 certificate fingerprints with "errs: 1" in the output from I'm out of time to dig before work but will pick this up later. Either I've made a PEBKAC error or something is up... |
I don’t think treating it as xor is correct, especially when looking at the RFCs that use this (e.g. https://tools.ietf.org/html/rfc5246#section-7.4.2 ). Specifically, this is a permission grant for ciphersuites, and “any of these bits are acceptable for permission grants” is how it should be read. To the question of warning/error however, I do stand by the Error classification, as both the intent then and now (as reflected in https://tools.ietf.org/html/rfc8813 ), was that only the key usages specified are considered consistent, and any key usages not specified are inconsistent. This was, and is, important for considering cross-protocol issues. I think the semantic of or vs and/or here might be traceable to an editing mistake/change, rather than a semantic intent. |
@sleevi That makes sense to me, and so
I would personally feel better about making a call on this if we find more clarity on the integration test diffs. If it's ~140 certs and we can spot check some to verify that their KU/EKUs are definitely inconsistent an error result seems fine. If it's >100k then I think we're either kicking off a very large incident for a bunch of CAs or have made an error. In general I'm nervous about false positives from key lint sources resulting in CAs turning off pre-issuance linting or ignoring large swaths of lints. |
Yup. I think I misread on the original issue regarding the discussion of xor/exclusive or. It’s an or of an exclusive set, but not an exclusive or 😅
Totally agreed! |
RFC5480 specifies "the format of the subjectPublicKeyInfo field in X.509 certificates [RFC5280] that use Elliptic Curve Cryptography (ECC)" (see https://tools.ietf.org/html/rfc5480#section-1). I think it's therefore reasonable to assume that RFC5480 is intended to be consistent with (the intent of) RFC5280. https://tools.ietf.org/html/rfc5480#section-3 says:
I think the intended meaning of that "or" is that any combination of digitalSignature/keyEncipherment/keyAgreement is permitted, except that keyEncipherment and keyAgreement should not appear together (because at most one of these two bits could possibly ever be useful in any given certificate). keyAgreement enables (non-ephemeral) cipher suites that use an ECC leaf certificate. This is relevant: https://security.stackexchange.com/questions/24106/which-key-usages-are-required-by-each-key-exchange-method |
I just filed an erratum against RFC5280: https://www.rfc-editor.org/errata/eid6414 |
@robstradling Your interpretation here makes sense to me and I'm supportive of changing the lint logic to match. Thanks for filing the errata! |
Discussion of the erratum on the PKIX list: https://mailarchive.ietf.org/arch/msg/pkix/rLa6v4kOlD8BQaJBwnmwX4oxsrE/ |
I think it's reasonable to assume that any combination of the values is acceptable, but it's hard to understand why the clearly carefully thought out distinction of the other descriptions in the same section are explicitly written with "and/or" notations where this one is not, unless it was intended to be that way. Interpreting 4.2.1.12 using the same logic as section 3 makes sense to me, but wouldn't that also mean "any combination"? I think the errata is both more accurate and harder to comprehend than it is now 😄 |
@robstradling (I don't have an account on the PKIX list so I guess I'll throw my two cents onto the floor of this thread) For however painful it might be for the person who updates the document, perhaps 5280 should explicitly enumerate the valid combinations rather than attempting a plain English description of it? I'm sure that future implementers would deeply appreciate the oven-ready combinations that they could almost copy-paste into their code rather than going through the same pain we're going through right now (and it would likely greatly reduce re-creating these mistakes). |
I think I figured out what's going on here, and it's a silly bug of my own creation: #555 I will fix this independently of this discussion. Unfortunately I think it hid the true impact of this lint, making it seem like only 140 integration test certificates were linted to an error level by this lint. In reality it's 566,924 new error findings. |
With this in mind I'm pretty strongly in favour of reverting the lint as it exists now and re-working any bug fixes in a follow-up PR. |
I completely agree with this and assume it will be a pretty major problem if not reverted for the 3.1.0 release. |
The `e_key_usage_and_extended_key_usage_inconsistent` lint's interpretation of RFC 5280 is under question (see zmap#553). We also had an integration test bug that resulted in massively under-estimating it's impact on our integration test corpus (see zmap#555). Let's remove this lint while we sort out the correct logic.
I'll push a revert PR + a fix to the integration tests after dinner. |
The `e_key_usage_and_extended_key_usage_inconsistent` lint's interpretation of RFC 5280 is under question (see zmap#553). We also had an integration test bug that resulted in massively under-estimating it's impact on our integration test corpus (see zmap#555). Let's remove this lint while we sort out the correct logic.
The `e_key_usage_and_extended_key_usage_inconsistent` lint's interpretation of RFC 5280 is under question (see zmap#553). We also had an integration test bug that resulted in massively under-estimating it's impact on our integration test corpus (see zmap#555). Let's remove this lint while we sort out the correct logic.
The `e_key_usage_and_extended_key_usage_inconsistent` lint's interpretation of RFC 5280 is under question (see #553). We also had an integration test bug that resulted in massively under-estimating it's impact on our integration test corpus (see #555). Let's remove this lint while we sort out the correct logic.
I agree with reverting the lint too, and I apologize for misunderstanding the RFC in the past. Thank you very much for the clarification! |
@ZsofiaTomicsko No apology needed! Your logic was sound, but the text in the RFC was misleading. Thank you for your contributions! |
@ZsofiaTomicsko Echoing @robstradling - definitely no need to apologize :-) The RFC text is unclear, it was missed in review, and our integration tests fell down on the job. We caught it before the release, no harm done! Thanks again. |
The
e_key_usage_and_extended_key_usage_inconsistent
lint introduced in #497 (and refined in #528) had a lot of discussion about the right interpretation of RFC 5280. I'm sorry that I missed getting a chance to review that PR and weigh in at the time, but I wanted to re-open the discussion with one concrete case in mind. I also think we should have a larger discussion about the conclusion to use anError
finding + the update to our integration test data.EKU DigitalSignature|KeyEncipherment
Consider a certificate that has
ExtKeyUsage
==ExtKeyUsageServerAuth
andKeyUsage
==KeyUsageDigitalSignature|KeyUsageKeyEncipherment
. In this case the certificate will take the server cert arm of theExecute
function's EKU loop:On L52 the cert's
KeyUsage
is used as a lookup in theserverAuth
map, and if it isn't found, alint.Error
status result is returned.The
serverAuth
map is defined as:The combined
KeyUsageDigitalSignature|KeyUsageKeyEncipherment
is not present inserverAuth
, and so an error result is returned.RFC 5280 interpretation
RFC 5280 4.2.1.12 says:
The crux of whether this is or isn't a false positive seems to come down to the interpretation of this text, and the "or" in particular. It's not crystal clear to me whether the
keyEncipherment|keyAgreement
combination of these usages is consistent withid-kp-serverAuth
. My sense is that this combination should be allowed -- I don't see how this combined EKU and the KU are inconsistent at all in the case of a certificate with an RSA public key.Error Result finding + Integration test data
Given the uncertainty about how to interpret RFC 5280's language here, and the fact that this lint introduced 140 new Error level findings across our integration test corpus, I think we should re-evaluate whether we should be using a warning or info result instead. It doesn't look like there was any analysis done on these 140 certificates at the time the lint was merged. Were they all true positives? Unambiguously?
@sleevi @zakird @christopher-henderson @ZsofiaTomicsko @RufusJWB What do you folks think? Is this more clear cut than I think? I'd love to figure out whether we need to take any action with this lint or not ahead of cutting the v3.1.0 tag if possible since that release will include this lint.
The text was updated successfully, but these errors were encountered: