X509 crt verify SAN iPAddress

[gstrauss]

Description

X509 crt verify SAN iPAddress

Status

READY

Requires Backporting

NO

Additional comments

A fair bit of work went into #2906 with over 70 posts, but for some reason the PR was never completed. Related PRs:
#2906 Verify numerical ip SubjectAlternativeName properly
#4494 Backport 2.28: Verify numerical ip SubjectAlternativeName properly
#5082 subjectAltName IP:X.X.X.X address not recognized, CN verification fails
#6473 curl built with mbedtls fails on https://1.1.1.1/ (IP address in SubjectAltName)

Why did #2906 reinvent the wheel with parsing IPv4 and IPv6 addresses? I implemented this PR in under an hour using inet_pton() (and while the patches do backport cleanly to mbedtls 2.28.1, I understand the feature is unlikely to be backported)
Should I incorporate the ~110 lines of code from #2906 which re-implement IPv4 and IPv6 string parsing? Should that be done only when inet_pton() is not available? Can all of this be done more simply with a feature macro, e.g. #if defined(MBEDTLS_X509_CRT_VERIFY_SAN_IP) which is disabled by default, and if you enable it, you should have inet_pton()? Most modern systems have inet_pton() in libc, though there are some odd systems out there like Haiku which might have it in -lsocket or -lnetwork (I have not checked).

This PR defers checking MBEDTLS_X509_SAN_IP_ADDRESS. It flags if MBEDTLS_X509_SAN_IP_ADDRESS is seen while checking MBEDTLS_X509_SAN_DNS_NAME, and only if a dNSName match is not found (and MBEDTLS_X509_SAN_IP_ADDRESS is present) does this PR attempt to convert the string with inet_pton(), and then check for match with MBEDTLS_X509_SAN_IP_ADDRESS.

Todos

• Tests
• Documentation
• Changelog updated

[gstrauss]

Some builds pass.
Policy check fails because I have #if defined(MBEDTLS_X509_CRT_VERIFY_SAN_IP) || 1 /*(fabricated placeholder)*/
Windows build fails (as I expected) because of missing #include <arpa/inet.h> since Windows is obnoxious and has inet_pton() in <ws2tcpip.h>

[gstrauss]

Besides #includes and detection of inet_pton(), how does the rest of the PR look? Code good? Tests good?

While I could save a few lines of code by not flagging existence of MBEDTLS_X509_SAN_IP_ADDRESS in the loop which calls x509_crt_check_cn(), I think it a worthwhile addition to avoid work of converting string to binary IP if that work is guaranteed to be fruitless. Also, this PR differs from #2906 in that conversion of string to binary IP is deferred and then done once and only after dNSName does not match and iPAddress MBEDTLS_X509_SAN_IP_ADDRESS entries do exist. I think this approach is more performant for existing usage matching dNSName.

As for OpenBSD, the defines for AF_INET and AF_INET6 are in <sys/socket.h>, so that platform needs the pair of includes.

I am ok if this feature is only available on platforms using compilers supporting __has_include() (e.g. non-ancient gcc and clang) and defining IPv6 AF_INET6. Might we scope this PR to that and have a separate PR to expand availability more portably? I'll probably still need a platform abstraction like MBEDTLS_PLATFORM_INET_PTON_MACRO in order to use that as a prerequisite for the tests.

[gilles-peskine-arm]

Besides #includes and detection of inet_pton(), how does the rest of the PR look? Code good? Tests good?

Sorry, my review queue is about a dozen deep. I am not volunteering for a review of this PR at this time.

I am ok if this feature is only available on platforms using compilers supporting __has_include()

Consider the scenario: I am setting up an IoT application, and doing deployment testing on a PC. Everything works. But when I deploy to my embedded devices, it works on some and fails on others. This seems like a debugging nightmare.

I've kicked an internal discussion, and I personally need to think about this some more. My current thinking is that this would be ok for some platform-adjacent functionality, such as the ability to start an network connection. But for certificate validation, I would not expect this kind of platform adherence, especially when it's not about the standard library but about the compiler.

[gstrauss]

The reason I am working on this PR is the very real-world issue of #6473 "curl built with mbedtls fails on https://1.1.1.1/ (IP address in SubjectAltName)". This feature gap makes mbedtls unusable with some modern applications in certain situations.

For #6473, the patches here are likely to be backported to the openwrt mbedtls package (openwrt/packages#19677).

Consider the scenario: I am setting up an IoT application, and doing deployment testing on a PC. Everything works. But when I deploy to my embedded devices, it works on some and fails on others. This seems like a debugging nightmare.

Please compare that to the current situation where verification with SAN iPAddress fails on all platforms. While incomplete platform coverage for a feature is not ideal, it should still be obvious which one of the above two scenarios is better and which one is worse.

In any case, @gilles-peskine-arm since you do not have time to review this PR, please nominate someone else to review this PR so that this PR does not languish for 3 years as has happened with reviewer churn in PR #2906, a precursor to this PR. Please remove the label "needs-work" and allow the next reviewer to determine if the PR needs work, and to describe what that requested work is, so that I can address more detailed feedback.

---

I have added some additional commits to this PR. When inet_pton() is not easily available, I have written and added local implementations to convert IPv4 and IPv6 strings to binary addresses, without using multiplication and without using strtoul(). strtoul() supports a variety of string formats of numbers and I desired a stricter parse of IP-formatted strings.

I would prefer to have this PR scoped without the two extra commits I added to this PR. This would mean that the SAN iPAddress matching would be available on most, but not all mbedtls platforms, but should make the PR easier to review. If you remove the top two commits to this PR, tests pass (or are skipped) on all platforms in the mbedtls CI.

In a separate PR, the mbedtls team could provide more detailed guidance on how a platform abstraction might be structured for this. For example, I think that it is undesirable for libmbedx509 to grow a dependency on -lsocket -lnsl on Solaris, and so I have coded defined(__sunos) builds to use the local implementation rather than trying to make a mess to use the platform inet_pton(). Similarly, I coded Windows defined(_WIN32) builds to use my local implementation.

[gstrauss]

All tests pass, besides an unrelated ABI error about removing mbedtls_mpi_core_shift_r, which was not done here.

I have pushed an additional change to attempt to include <sys/socket.h> and <arpa/inet.h> if __has_include() is not present. Let's see if the CI like it on the test platforms.

[gilles-peskine-arm]

While incomplete platform coverage for a feature is not ideal, it should still be obvious which one of the above two scenarios is better and which one is worse.

Well, yes, that's obvious, but not the way you think. So no, it's actually not obvious. For a system-related feature, it's better to support the feature on some systems than on none. For a cryptographic feature, it's better to be consistent. Certificate verification is a cryptographic feature.

[gstrauss]

rebased to HEAD of development at request of @mprse

Only change is to fix one-line conflict introduced by 7224086. Fix was to merge in the removal of #include "mbedtls/legacy_or_psa.h" from tests/suites/test_suite_x509parse.function

[gstrauss]

rebased to HEAD of development at request of @mprse
@davidhorstmann-arm please re-approve.

[AndrzejKurek]

Merged as a part of #7436.