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

Erroneous handling of IP address SANs #3498

Closed
kFYatek opened this issue Jul 17, 2020 · 4 comments · Fixed by #3554
Closed

Erroneous handling of IP address SANs #3498

kFYatek opened this issue Jul 17, 2020 · 4 comments · Fixed by #3554
Assignees

Comments

@kFYatek
Copy link

kFYatek commented Jul 17, 2020

Description

  • Type: Bug
  • Priority: Minor

mbed TLS build:
Version: checked on 2.16.4 and git-8f4f9a8da, likely occurs on all versions that support SAN

Actual behavior
x509_crt_verify_name() treats all kinds of Subject Alternative Names equivalently, even in cases where it does not make sense. In particular, IP address SANs are compared byte-by-byte with whatever the value of cn is - and in the typical cases it is a hostname (specifically, in the case of normal mbedtls_ssl_context operation, it's the value configured via mbedtls_ssl_set_hostname()).

It is possible, for example, for a certificate to exist with an IP address SAN for 97.46.112.108 (a perfectly valid IP owned by Verizon in the US), which would erroneously match a hostname of a.pl (an actual Polish retailer's website). While unlikely, this is seems like a possibility for certificate spoofing.

This also means that it is very difficult to handle connecting to a server that actually uses an IP address SAN in its certificate, as there is no way to pass an IP address to mbed TLS when connecting.

Expected behavior
I would expect IP address SANs to be either ignored or compared in a more appropriate way (e.g. stringified first, perhaps?)

@mpg
Copy link
Contributor

mpg commented Jul 21, 2020

Thanks for your report! I agree that the way the library currently compares IPs to domain name is a bug, and that it should either ignore non-DNS SANs or handle them properly according to their types. I think there are two parts there:

  1. The comparison of names of different types as if they were the same, which is a bug with security implications as you mention.
  2. The fact that we don't handle IP SANs, which is a feature request - for this part, a PR was contributed some time ago, but unfortunately it hasn't been finalized and merged yet: https://github.com/ARMmbed/mbedtls/pull/2906/files

I'm currently investigating the first part, trying to clarify the implications, and coming up with a fix. Once that's done, we'll hopefully be in a better place to start supporting IP SANs.

@mpg
Copy link
Contributor

mpg commented Jul 24, 2020

I'm working on a fix, and writing a ChangeLog entry describing the issue and crediting you for finding and reporting it. How would you like to be credited: by your github handle, your legal name? do you want to include an affiliation? Thanks!

@kFYatek
Copy link
Author

kFYatek commented Jul 24, 2020

Hi, I don't really care about attribution that much. If you want to credit me, Github handle is OK.

@mpg
Copy link
Contributor

mpg commented Jul 24, 2020

Ok, thanks!

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

Successfully merging a pull request may close this issue.

2 participants