zakird
commented
3 years ago We very regularly add code to expose additional data from zcrypto in order to support ZLint. If that's what would be best here, and it sounds like it might, we shouldn't shy away from exposing it.
On Fri, Aug 27, 2021 at 4:19 AM Rob @.***> wrote:
@.**** commented on this pull request.
In v3/lints/cabf_br/lint_subject_common_name_not_exactly_from_san_test.go https://github.com/zmap/zlint/pull/627#discussion_r697247887:
- {
- name: "Error - common name not in SAN.IPAddresses, IPv4",
- inputFile: "SANIPv4AddressNotMatchingCommonName.pem",
- expectedOutput: lint.Error,
- },
- {
- name: "Error - common name not in SAN.IPAddresses, IPv6",
- inputFile: "SANIPv6AddressNotMatchingCommonName.pem",
- expectedOutput: lint.Error,
- },
- {
- name: "NE - certificate issued before 21 August 2021",
- inputFile: "SANWithMissingCN.pem",
- expectedOutput: lint.NE,
- },
- }
The plot thickens. It turns out that a CN of "2001:db8::1:0:0:1" will be considered equal to a SAN IPAddress of "2001:db8:0:0:1:0:0:1" because the IP.String() function correctly abbreviates the latter. Whereas when both CN and SAN IPAddress are "2001:db8:0:0:1:0:0:1" then the lint returns an error as IP.String() on the SAN value returns "2001:db8::1:0:0:1"
It seems the issue is around when IP addresses are or are not parsed and that extends to generating the test data. For some reason the genTestCerts.go script won't create certificates containing properly formatted SAN IPv6 addresses and will always go for "2001:db8:0:0:1:0:0:1" over "2001:db8::1:0:0:1".
I think part of the issue here is that a lint will never have access to the raw SAN IPAddress information because it's presented in a zcrypto/x509.Certificate object which stores IP addresses as IP variables and the common name as a string. This allows for linting badly formatted common names but not SAN IPs. With that in mind I wonder if the best thing to do in this lint is to settle for checking if an IP in the common name is equivalent to one in the SAN even if the representation in the cert is different? Something like this:
if cnIP := net.ParseIP(cn); cnIP != nil { for _, ip := range c.IPAddresses { if cnIP.Equal(ip) { //== ip.String() { return &lint.LintResult{Status: lint.Pass} } } }
It is bringing along the assumption that all IPv6 addresses are properly formatted but that's where a lint to check that would come in. On the common name side it would at least be easy as we can check if net.ParseIP(commonName).String() == commonName but I still think we're going to struggle to deal with SAN IPv6 addresses in zlint alone unless we want to go digging through the Raw certificate data to find the SAN IPAddresses.
— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/zmap/zlint/pull/627#discussion_r697247887, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABREUCTR7U3EIGN3ISBS2DT65DCTANCNFSM5CUV533A . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.
dadrian
robplee
This is my solution to the issue I raised #625. I've gone with a straightforward fix by making the old lint ineffective from the 21st August 2021 and making a new lint which performs a case-sensitive string comparison between the common name and SAN dNSNames. I have also refactored the tests on the original lint to use the de facto standard table approach because it allowed me to largely copy and paste the new tests I wrote for the new lint across with just some minor tweaks.
Comments for reviewers: