Fix URL scheme parsing and TLS Server Name Indication (SNI) and its use in `apps/` by DDvO · Pull Request #27357 · openssl/openssl

DDvO · 2025-04-11T19:01:45Z

Add recommendation that TLS clients should use SSL_set_tlsext_host_name() for SNI
and that this should be done jointly with using {SSL,X509_VERIFY_PARAM}_{set1,add1}_host().
On this occasion fix nits and omissions in the respective .pod files. ~~Doc changes moved to #29150.~~

Fix OSSL_parse_url() going too far when scanning for '@' to detect the end of the userinfo component.,
going astray when an @ symbol occurs after the host component, for instance in the query component. meanwhile superseded by fix OSSL_parse_url userinfo scan to respect authority boundary #30319
Fix OSSL_parse_url() to correctly parse scheme (where only certain chars are allowed)
Make sure that app_http_tls_cb() calls SSL_set_tlsext_host_name() for setting the SNI only if a server name (rather than an IP address) is given.
Also fix the server name being set in the SNI: it must always be the server actually connecting to,
while for the CMP CLI app so far this is wrongly tied to the -tls_host option.
Add to OSSL_HTTP_REQ_CTX_nbio support for partial content-type string matching
Extend related doc, e.g., of SSL_set_tlsext_host_name() and OSSL_HTTP_REQ_CTX_set_expected().

Checklist

documentation is added or updated
tests are added or updated

Copilot

Copilot reviewed 6 out of 9 changed files in this pull request and generated 2 comments.

Files not reviewed (3)

doc/man1/openssl-cmp.pod.in: Language not supported
doc/man3/SSL_CTX_set_tlsext_servername_callback.pod: Language not supported
doc/man3/X509_VERIFY_PARAM_set_flags.pod: Language not supported

Comments suppressed due to low confidence (1)

apps/cmp.c:1520

According to the PR description, the '-tls_host' option should take precedence over the URL hostname; however, this call uses 'host' instead of checking if '-tls_host' is provided. Consider revisiting this logic to ensure the precedence is properly enforced.

if (!truststore_set_host_etc(trust_store, host))

DDvO · 2025-04-11T19:36:02Z

BTW, since doing the SNI and hostname/address checks is important for correct and secure TLS connections,
how about calling on client side SSL_set_tlsext_host_name() and X509_VERIFY_PARAM_set1_host() by default?

t8m · 2025-05-02T12:38:46Z

This needs a rebase due to conflicts.

DDvO · 2025-05-05T16:55:35Z

This needs a rebase due to conflicts.

Done.
Also squashed all fixup commits due to review comments.

openssl-machine · 2025-06-15T00:09:23Z

This PR is in a state where it requires action by @openssl/committers but the last update was 30 days ago

openssl-machine · 2025-07-17T00:09:29Z

This PR is in a state where it requires action by @openssl/committers but the last update was 30 days ago

vdukhovni

Please fix at least the "is IP address" test.

DDvO · 2026-05-21T14:55:54Z

@DDvO Can you please create the OSSL_parse_url() change backport PR for 4.0 and below?

I am fine with the changes but I am hesitant backporting all of them to stable branches.

No problem, done here: #31267

DDvO · 2026-05-21T14:56:56Z

When aligning this PR with a related one in a different project,
I got an interesting Copilot review comment pointing out a problem with the new host_is_ip_address() helper function:
it does not cope with IPv6 addresses enclosed in [ and ].

In retrospect, it would have been better if OSSL_*parse_url() returned the host component without these surrinding chars, which are needed only at URL level.
Yet this has been implemented and documented otherwise, so little chance to rectify this now.

So I had to push an extension of host_is_ip_address() here that glosses over those brackets if present.
and ask @t8m to reconfirm approval after this extra fix.

esyr

Generally looks good, the only blocker is UB.

esyr

LGTM now, thanks, the only nit is that the added test should probably rather use test_http_url_invalid().

…sext_host_name(); update format Add recommendation to use it for TLS clients, together with X509_VERIFY_PARAM_{set1,add1}_host()

…RIFY_PARAM_set1_host()

…f OSSL_HTTP_REQ_CTX_set_expected()

esyr

LGTM, thank you!

nhorman · 2026-06-02T15:44:32Z

    return bio;
+
+err:
+    BIO_free(sbio);


Is it safe to free the BIO here if the SSL object has it set above?

I'd say yes, it should be safe because

the BIO_set_ssl(sbio, ssl, BIO_CLOSE) call transfers the ownership of ssl to sbio, and

in the err branch, BIO_free(sbio) frees ssl along with sbio.

Both of which (as often, unfortunately) is not documented.

wait, I had this backwards (I had thought the call to BIO_set_ssl was actuall a call to SSL_set_bio). But, given that, if BIO_set_ssl fails/returns 0, then don't we leak the ssl we just created? I grant thats going to be an unlikely event (it can happen if ssl_new or BIO_up_ref fails).

It looks like thats actually a pretty consistent pattern in uses of BIO_set_ssl.

@nhorman can you then also approve?

sorry, I forgot to submit my last round of reviews. As noted above, I had the problem backwards. You're right in that BIO_free will free the SSL, but if BIO_set_ssl fails, we will leak the allocated SSL. not sure how big a deal that is (it seems to be a fairly common pattern), but it definately looks like a leak problem.

Now I see what you mean: what happens if the BIO_set_ssl() call fails?

I just had a closer look at BIO_set_ssl() and found that there is one case where ssl_ctrl() in ssl/bio_ssl.c returns 0 after transferring ownership (via bs->ssl = ssl), namely if BIO_up_ref(bio)) fails, which is very unlikely but possible.
Still, so far one cannot fully rely on the ownership transfer to happen if and only if BIO_set_ssl() succeeds,
so calling it, one either risks a mem leak (if ignoring its negative result) or a double free (if deallocating ssl on its failure).

To fully solve this, I now deallocate ssl in case BIO_set_ssl() fails
while adding a little bugfix commit that

makes sure that BIO_set_ssl() transfers ownership of ssl only on success, and

documents this

I guess you and Neil have just opened a can of worms: if I'm not mistaken, failing ssl_new() (due to failing OPENSSL_zalloc) after ssl_free would lead to BIO_SSL double-free, as b->ptr is not reset to NULL after OPENSSL_free call in ssl_free, and the latter would be called the second time in [1].

@esyr which b->ptr in which function context do you mean here?
Since I've changed ssl_ctrl() to set bs->ssl only at the end on success,
and as I call SSL_free(ssl) only on failure, I do not see that I introduced potential for double-free.
Which does not preclude that the wonderful code in bio_lib.c bears other potential for double-free,
but that's unrelated to this PR.

I think what @esyr is getting at is the following set of conditions:

BIO_set_ssl is called

In ssl_ctrl, we fall into the BIO_C_SET_SSL case

ssl_free is called on the passed bio b

ssl_free(b) is called

ssl_free calls BIO_get_data to get the data bio (bs)

SSL_free(bs->ssl) is called

OPENSSL_free(bs) is called. Critically the BIO data pointer remains pointed to bs (from (5)) which is now no longer valid

back in ssl_ctrl, ssl_new is called, which we assume fails on a malloc failure, meaning a new bs is not allocated

ssl_ctrl returns with an error code and the BIOS bs/data pointer still pointing to invalid memory (due to the free in (7)

your code above detects the error, frees the previously leaked SSL (so you've fixed that and jumps to err:

at the err label BIO_free is called

BIO_free calls ssl_free, which calls BIO_get_data to get bs again, which is non-null but invalid.

we try to call OPENSSL_free again on bs, which results in a double free.

If thats the case it seems like the best solution here is to call BIO_set_data(b, NULL) in ssl_free to set the freed pointer to NULL, and then check it at the top of ssl_free to not use it if it is NULL.

That all said, given the proliferation of the call sites for this, and the fact that no one currently checks the return code of the BIO_set_ssl call anywhere, this might be creeping out of scope for this PR. Maybe just let it be and open an issue to fix this separately along with all the call sites.

Yes, that what I was trying to tell. In general, I concur that having broken BIO_set_ssl() is out of scope for this PR. Created [1] to track this.

[1] #31388

…X509_VERIFY_PARAM_set1_host()

…ocument this

esyr · 2026-06-04T10:40:01Z

@@ -299,7 +299,6 @@ static long ssl_ctrl(BIO *b, int cmd, long num, void *ptr)
        }


I guess you and Neil have just opened a can of worms: if I'm not mistaken, failing ssl_new() (due to failing OPENSSL_zalloc) after ssl_free would lead to BIO_SSL double-free, as b->ptr is not reset to NULL after OPENSSL_free call in ssl_free, and the latter would be called the second time in [1].

[1] https://github.com/openssl/openssl/pull/27357/files#diff-22af71ddb18c32051f7e626ab7089fb69c056a419dd209f6c0c85a873e30c2f5R2983

esyr · 2026-06-04T19:50:34Z

            BIO_set_next(b, bio);
        }
        BIO_set_init(b, 1);
+        bs->ssl = ssl;


The change looks innocuous enough (famous last words), however it's slowly creeping out of scope of the PR.

@mattcaswell might also want to take a look at this.

openssl-machine · 2026-06-05T20:00:47Z

This pull request is ready to merge

DDvO · 2026-06-10T06:35:28Z

Merged - thanks @nhorman, @esyr, and @t8m for your approvals
and @InfoHunter, @mattcaswell, and @vdukhovni for your reviews!

DDvO requested a review from Copilot April 11, 2025 19:02

Copilot AI reviewed Apr 11, 2025

View reviewed changes

Comment thread apps/include/apps.h Outdated

Comment thread apps/cmp.c Outdated

DDvO force-pushed the SSL_set_tlsext_host_name-doc_and_use_fixes branch from 6b70f7a to 2013643 Compare April 11, 2025 19:15

InfoHunter reviewed Apr 16, 2025

View reviewed changes

Comment thread doc/man3/X509_VERIFY_PARAM_set_flags.pod Outdated

mattcaswell requested changes Apr 17, 2025

View reviewed changes

Comment thread doc/man3/SSL_CTX_set_tlsext_servername_callback.pod Outdated

Comment thread doc/man3/X509_VERIFY_PARAM_set_flags.pod Outdated

Comment thread apps/cmp.c

DDvO mentioned this pull request Apr 21, 2025

Fix URL parsing to handle missing ports and ISO 8601 timestamps in paths #26303

Closed

DDvO force-pushed the SSL_set_tlsext_host_name-doc_and_use_fixes branch from 574a9c5 to a8184c1 Compare April 21, 2025 09:13

mattcaswell previously approved these changes Apr 21, 2025

View reviewed changes

DDvO mentioned this pull request Apr 21, 2025

Fix SSL_{set1,add1}_host() handling of host name/IP address and related doc #27457

Closed

DDvO dismissed mattcaswell’s stale review via cdd9fb1 May 5, 2025 16:54

DDvO force-pushed the SSL_set_tlsext_host_name-doc_and_use_fixes branch from 0e1d7c0 to cdd9fb1 Compare May 5, 2025 16:54

DDvO force-pushed the SSL_set_tlsext_host_name-doc_and_use_fixes branch from cdd9fb1 to d25895d Compare May 5, 2025 18:42

DDvO mentioned this pull request May 7, 2025

Fix TLS hostname handling for SNI etc. siemens/gencmpclient#81

Merged

DDvO requested a review from mattcaswell May 15, 2025 07:43

mattcaswell requested changes Jun 16, 2025

View reviewed changes

Comment thread apps/include/apps.h Outdated

vdukhovni requested changes Jul 17, 2025

View reviewed changes

t8m requested a review from vdukhovni May 20, 2026 14:24

DDvO mentioned this pull request May 21, 2026

Fix OSSL_parse_url() scheme and host parsing backport of #27357 #31267

Closed

DDvO dismissed t8m’s stale review via b207775 May 21, 2026 14:56

DDvO force-pushed the SSL_set_tlsext_host_name-doc_and_use_fixes branch 3 times, most recently from 035d24c to 466239e Compare May 21, 2026 15:28

esyr requested changes May 22, 2026

View reviewed changes

DDvO force-pushed the SSL_set_tlsext_host_name-doc_and_use_fixes branch from 466239e to 6bb0219 Compare May 24, 2026 15:06

esyr requested changes May 24, 2026

View reviewed changes

Comment thread test/http_test.c Outdated

DDvO and others added 5 commits May 24, 2026 20:58

SSL_CTX_set_tlsext_servername_callback.pod: improve doc of SSL_set_tl…

cbfe299

…sext_host_name(); update format Add recommendation to use it for TLS clients, together with X509_VERIFY_PARAM_{set1,add1}_host()

http_lib.c: fix parsing 'scheme' part in OSSL_parse_url()

ab5d862

apps/cmp.c: fix use of SSL_set_tlsext_host_name() for SNI and X509_VE…

37c30ef

…RIFY_PARAM_set1_host()

OSSL_HTTP_REQ_CTX.pod: tweak description of 'expect_asn1' parameter o…

4025ad3

…f OSSL_HTTP_REQ_CTX_set_expected()

http_lib.c: add check that host_end is not past authority_end

b85b3de

esyr previously approved these changes May 24, 2026

View reviewed changes

nhorman reviewed Jun 2, 2026

View reviewed changes

DDvO added 2 commits June 4, 2026 06:47

fixup! apps/cmp.c: fix use of SSL_set_tlsext_host_name() for SNI and …

56bcdb2

…X509_VERIFY_PARAM_set1_host()

make sure BIO_set_ssl() transfers ownership of ssl only on success; d…

eab1708

…ocument this

esyr reviewed Jun 4, 2026

View reviewed changes

nhorman approved these changes Jun 4, 2026

View reviewed changes

esyr mentioned this pull request Jun 4, 2026

ssl_free() (or its callers) doesn't reset ptr to NULL, which may lead to double-free #31388

Open

esyr approved these changes Jun 4, 2026

View reviewed changes

t8m approved these changes Jun 9, 2026

View reviewed changes

DDvO mentioned this pull request Jun 27, 2026

OSSL_STORE, HTTP, apps/: fix URI scheme parsing and URI-related doc #27507

Closed

pag-crypto mentioned this pull request Aug 27, 2026

Leave BIO_f_ssl() consistent when replacing its SSL object fails #32552

Open

2 tasks

esyr mentioned this pull request Sep 9, 2026

[master] CHANGES.md, NEWS.md: update for 4.1.0-alpha1 #32758

Closed

		@@ -299,7 +299,6 @@ static long ssl_ctrl(BIO b, int cmd, long num, void ptr)
		}

Uh oh!

Uh oh!

Conversation

DDvO commented Apr 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

DDvO commented Apr 11, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

t8m commented May 2, 2025

Uh oh!

DDvO commented May 5, 2025

Uh oh!

openssl-machine commented Jun 15, 2025

Uh oh!

Uh oh!

openssl-machine commented Jul 17, 2025

Uh oh!

vdukhovni left a comment

Choose a reason for hiding this comment

Uh oh!

DDvO commented May 21, 2026

Uh oh!

DDvO commented May 21, 2026

Uh oh!

esyr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

esyr left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

esyr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DDvO Jun 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nhorman Jun 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

esyr Jun 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

DDvO commented Apr 11, 2025 •

edited

Loading

esyr left a comment •

edited

Loading

DDvO Jun 4, 2026 •

edited

Loading

nhorman Jun 4, 2026 •

edited

Loading

esyr Jun 4, 2026 •

edited

Loading