Skip to content

TLS ip address verification, extend test #19252

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.

Sign up for GitHub

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

Jump to bottom
Closed
icing wants to merge 5 commits into from
Closed

TLS ip address verification, extend test #19252

icing wants to merge 5 commits into from

Conversation

@icing
Copy link
Contributor

@icing icing commented Oct 27, 2025

Change the test certificate to carry a altname 'dns:127.0.0.1' which should not match in test_17_05_bad_ip_addr.

wolfSSL: since wolfSSL_check_domain_name() does not differentiate between DNS and IP names, use if only for DNS names. For IP addresses, get the peer certificate after the handshake and check that using wolfSSL_X509_check_ip_asc().

Unfortunately, this succeeds where it should not, as wolfSSL internally used the same check code for both cases. So, skip the test case until wolfSSL fixes that.

@icing
TLS ip address verification, extend test
9c6dc7e 
Change the test certificate to carry a altname 'dns:127.0.0.1' which
should *not* match in test_17_05_bad_ip_addr.

wolfSSL: since `wolfSSL_check_domain_name()` does not differentiate
between DNS and IP names, use if only for DNS names. For IP addresses,
get the peer certificate after the handshake and check that using
wolfSSL_X509_check_ip_asc().

Unfortunately, this succeeds where it should not, as wolfSSL internally
used the same check code for both cases. So, skip the test case until
wolfSSL fixes that.
@icing icing added the TLS label Oct 27, 2025
@github-actions github-actions bot added the tests label Oct 27, 2025
@icing icing mentioned this pull request Oct 27, 2025
Closed
@icing
Copy link
Contributor Author

icing commented Oct 27, 2025

Opened wolfSSL/wolfssl#9351 at the wolfSSL project.

@icing
make the very bad ip cert a separate test case, so we can see that
40b7c94 
wolfssl+mbedtls work on "normal" not-matching ip addresses.
@MegaManSec
Copy link
Contributor

MegaManSec commented Oct 27, 2025

Thanks for this! Any chance to add an ipv6 test too?

@icing
wolfssl: partially restore previous check_domain_name() code
7b7d1a9 
Add macos skips on wolfssl early data tests which are not reliable
@curl curl deleted a comment from testclutch Oct 27, 2025
@icing
Copy link
Contributor Author

icing commented Oct 27, 2025

Thanks for this! Any chance to add an ipv6 test too?

Not easily. We'd need a dynamic check if ipv6 is working on the platform.

Also: thinking some more about this test, it seems highly theoretical. If someone gets a CA to sign a cert with an ip address in a DNS alt name, they can probably trick other things in there as well. It's then a broken CA we would be dealing with, or?

@MegaManSec
Copy link
Contributor

MegaManSec commented Oct 27, 2025

We'd need a dynamic check if ipv6 is working on the platform.

Understood.

it seems highly theoretical

Yup, I agree,that's it is even untested from that other PR; it was just an idea I had about what this code could theoretically break.

By the way, I think there is another case in `lib/vquic/vquic-tls.c. Here's an ad-hoc patch which shows the issue:

diff --git a/lib/vquic/vquic-tls.c b/lib/vquic/vquic-tls.c
index fa89c0b80..3b6f2124d 100644
--- a/lib/vquic/vquic-tls.c
+++ b/lib/vquic/vquic-tls.c
@@ -178,12 +178,21 @@ CURLcode Curl_vquic_tls_verify_peer(struct curl_tls_ctx *ctx,
 #elif defined(USE_WOLFSSL)
   (void)data;
   if(conn_config->verifyhost) {
-    char *snihost = peer->sni ? peer->sni : peer->hostname;
-    WOLFSSL_X509* cert = wolfSSL_get_peer_certificate(ctx->wssl.ssl);
-    if(wolfSSL_X509_check_host(cert, snihost, strlen(snihost), 0, NULL)
-          == WOLFSSL_FAILURE) {
-      result = CURLE_PEER_FAILED_VERIFICATION;
+    const char *host = peer->sni ? peer->sni : peer->hostname;
+    WOLFSSL_X509 *cert = wolfSSL_get_peer_certificate(ctx->wssl.ssl);
+    int ok = 0;
+
+    if(host) {
+      if(Curl_host_is_ipnum(host)) {
+        ok = (wolfSSL_X509_check_ip_asc(cert, host, 0) == WOLFSSL_SUCCESS);
+      }
+      else {
+        ok = (wolfSSL_X509_check_host(cert, host, strlen(host), 0, NULL)
+              == WOLFSSL_SUCCESS);
+      }
     }
+    if(!ok)
+      result = CURLE_PEER_FAILED_VERIFICATION;
     wolfSSL_X509_free(cert);
   }
   if(!result)

@icing
Copy link
Contributor Author

icing commented Oct 27, 2025

@MegaManSec good catch.

@icing icing requested a review from bagder October 27, 2025 13:28
@bagder bagder closed this in 692c7f1 Oct 27, 2025
vszakats added a commit to vszakats/curl that referenced this pull request Nov 27, 2025
@vszakats
pytests: disable two H3 earlydata tests for all platforms (was: macOS)
387bf06 
Follow-up to 692c7f1 curl#19252
Follow-up to eefd03c curl#18703

Fixes curl#19724
@vszakats vszakats mentioned this pull request Nov 27, 2025
Closed
vszakats added a commit that referenced this pull request Nov 27, 2025
@vszakats
pytest: disable two H3 earlydata tests for all platforms (was: macOS)
0081c5b 
Follow-up to 692c7f1 #19252
Follow-up to eefd03c #18703

Ref: #19719
Ref: #19723

Fixes #19724
Closes #19730
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bagder bagder bagder approved these changes

Assignees

No one assigned

Labels

tests TLS

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@icing @MegaManSec @bagder