Skip to content

tls: move key and cert_type fields into ssl_primary_config#21667

Closed
MegaManSec wants to merge 8 commits into
curl:masterfrom
MegaManSec:n
Closed

tls: move key and cert_type fields into ssl_primary_config#21667
MegaManSec wants to merge 8 commits into
curl:masterfrom
MegaManSec:n

Conversation

@MegaManSec
Copy link
Copy Markdown
Contributor

@MegaManSec MegaManSec commented May 19, 2026

Description

cert_type, key, key_type, key_passwd and key_blob lived in ssl_config_data but not ssl_primary_config, so they were invisible to both match_ssl_primary_config (connection reuse) and the TLS session cache peer key and auth match. Only clientcert was compared; the rest of the client credential set was silently ignored.

Two concrete consequences:

  1. Connection reuse: handles sharing the same certificate but differing in key file, key type, cert type, or key blob could incorrectly reuse each other's connections.

  2. Session resumption bypass: a handle configured with the wrong key password would normally fail the TLS handshake — it cannot sign with the key. But if a correctly-configured handle had already cached a session, the misconfigured handle could resume that session without ever performing a handshake, bypassing key authentication entirely. key_passwd is now stored on the session cache peer and compared with Curl_timestrcmp at lookup, following the same pattern as SRP credentials.

Promote all five fields into ssl_primary_config so both the conn-reuse predicate and the session cache cover the complete client credential set. Also replaces the fixed :CCERT session cache marker with the actual client cert and key paths.

Adds unit3303 to verify conn-reuse matching distinguishes handles differing on these fields, and unit3304 to verify the session cache peer key correctly includes key path, key type and cert type but not key password.

cert_type, key, key_type, key_passwd and key_blob lived in
ssl_config_data but not in ssl_primary_config, so they were invisible
to match_ssl_primary_config() and to the TLS session cache peer key.

Two easy handles sharing a connection pool could reuse each other's
authenticated connections when they differed only on SSLKEY, SSLKEYTYPE,
KEYPASSWD, SSLCERTTYPE or SSLKEYBLOB. The second handle would silently
inherit the first handle's authenticated identity.

Promote all five fields into ssl_primary_config so the conn-reuse
predicate and session cache key cover the complete client credential
set. Also replace the fixed ":CCERT" session cache marker with the
actual clientcert path so sessions are not shared across different
client certificates.

Reported-By: Joshua Rogers (AISLE Research)
@github-actions github-actions Bot added the tests label May 19, 2026
@MegaManSec MegaManSec changed the title tls: fix incomplete mTLS config in conn reuse and session cache tls: move key and cert_type fields into ssl_primary_config May 19, 2026
@bagder bagder requested review from Copilot and icing May 19, 2026 11:24
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR moves client credential fields into ssl_primary_config so connection reuse and TLS session cache decisions can account for them consistently across TLS backends.

Changes:

  • Promotes cert/key type, key path, key password, and key blob fields into ssl_primary_config.
  • Updates TLS/SSH backend references to use the new field locations.
  • Adds unit3303 for connection config matching of several client credential fields.

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
lib/urldata.h Moves client credential fields into ssl_primary_config.
lib/vtls/vtls.c Adds credential fields to config matching, cloning, cleanup, and setup.
lib/vtls/vtls_scache.c Adds client cert/key path and key blob to TLS session cache peer key.
lib/vtls/openssl.c Updates OpenSSL credential field accesses.
lib/vtls/gtls.c Updates GnuTLS credential field accesses.
lib/vtls/mbedtls.c Updates mbedTLS credential field accesses.
lib/vtls/rustls.c Updates rustls key field accesses.
lib/vtls/schannel.c Updates Schannel credential field accesses.
lib/vtls/wolfssl.c Updates wolfSSL credential field accesses.
lib/vssh/libssh.c Updates SSH key password access.
lib/vssh/libssh2.c Updates SSH key password access.
lib/ldap.c Updates LDAP TLS cert type access.
tests/unit/unit3303.c Adds a unit test for connection config credential matching.
tests/unit/Makefile.inc Includes the new unit test in the unit test list.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread lib/vtls/vtls_scache.c Outdated
…n cache peer key

Two handles sharing the same cert/key paths but differing only in
cert_type, key_type or key_passwd would resolve to the same peer key
and could share a TLS session ticket. With session resumption the
server does not re-request the client certificate, so a handle with
the wrong password could reuse a ticket established by a handle that
authenticated correctly.

Add cert_type and key_type as plain discriminators in the peer key
string, and hash key_passwd via a new cf_ssl_peer_key_add_str_hash
helper (mirroring cf_ssl_peer_key_add_hash for blobs) so the secret
is never stored in plaintext in the cache key.
@MegaManSec MegaManSec force-pushed the n branch 2 times, most recently from 88d94ca to 52b7236 Compare May 19, 2026 11:34
Copy link
Copy Markdown
Contributor

@icing icing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not understand what this fixes. Talking about the password, for example. I'd be hesitant to put that into the session id, even as a hash. What is the benefit of matching this?

We are talking about a single user application where a connection/session might get reused after someone configured the wrong key password on a transfer? Why would anyone do this?

Extract the body of Curl_ssl_peer_key_make() into a new
Curl_ssl_peer_key_build() that takes ssl_primary_config and peer
descriptors directly, making the peer key logic unit-testable without
a full Curl_cfilter. Group the mTLS credential fields (clientcert, key,
key_blob, cert_type, key_type, key_passwd) into a static helper
cf_ssl_peer_key_add_mtls() to keep the main function within the
complexity budget.

unit3304 verifies that Curl_ssl_peer_key_build() returns distinct keys
when two configs differ only on key_passwd, key, key_type or cert_type.
Follow the same pattern as SRP credentials: do not embed key_passwd
in the peer key string at all. Instead store it on the
Curl_ssl_scache_peer struct and compare it at lookup time via
Curl_timestrcmp() in cf_ssl_scache_match_auth(). Also mark peers
with a key_passwd as non-exportable.

Update unit3304 to reflect that key_passwd does not affect the peer
key string.
cert_type and key_type are compared case-insensitively in
match_ssl_primary_config (via curl_strequal), so the session cache peer
key must also treat them case-insensitively.  Use Curl_raw_toupper when
building the :CT- and :KT- segments so "PEM" and "pem" map to the same
peer key.
@MegaManSec
Copy link
Copy Markdown
Contributor Author

@icing

I'd be hesitant to put that into the session id, even as a hash.

Fixed. Now we just do the same as the SRP credentials path, which isto store it as a separate field on Curl_ssl_scache_peer and compare it with Curl_timestrcmp at lookup time.

As for what this PR is actually addressing, it's about two different handles in the same multi-handle sharing a connection pool or TLS session cache, i.e.

  • Handle 1: CURLOPT_KEYPASSWD = "secret", performs a successful mTLS handshake, session cached.
  • Handle B: CURLOPT_KEYPASSWD = "", looks up the session cache, gets a hit (password not part of the key), resumes the TLS session of handle 1, despite not having access to the correct key password

@icing
Copy link
Copy Markdown
Contributor

icing commented May 19, 2026

@icing

I'd be hesitant to put that into the session id, even as a hash.

Fixed. Now we just do the same as the SRP credentials path, which isto store it as a separate field on Curl_ssl_scache_peer and compare it with Curl_timestrcmp at lookup time.

As for what this PR is actually addressing, it's about two different handles in the same multi-handle sharing a connection pool or TLS session cache, i.e.

  • Handle 1: CURLOPT_KEYPASSWD = "secret", performs a successful mTLS handshake, session cached.
  • Handle B: CURLOPT_KEYPASSWD = "", looks up the session cache, gets a hit (password not part of the key), resumes the TLS session of handle 1, despite not having access to the correct key password

So, this protects against a coding mistake in the application, which would fail the connect otherwise because of the wrong password. Hmm...

@MegaManSec
Copy link
Copy Markdown
Contributor Author

Is it a coding mistake in an application? Isn't this exactly the intended functionality of a multihandle?

e.g. a server-side application using libcurl to forward requests on behalf of different tenants, each configured with their own mTLS client cert/key on separate handles, all sharing one connection pool. Without this fix, tenant B's handle can resume tenant A's cached TLS session..

@MegaManSec
Copy link
Copy Markdown
Contributor Author

sorry, not the cert path itself, but rather the two handles using the same certificate but different private keys or key passwords would be mixed up.

Comment thread lib/vtls/vtls_scache.c Outdated
@icing
Copy link
Copy Markdown
Contributor

icing commented May 19, 2026

sorry, not the cert path itself, but rather the two handles using the same certificate but different private keys or key passwords would be mixed up.

Ah, ok. The key path and blob I understand. Thanks.

@bagder
Copy link
Copy Markdown
Member

bagder commented May 19, 2026

sorry, I caused a merge conflict just now when I merged another PR...

@MegaManSec MegaManSec force-pushed the n branch 2 times, most recently from 39de157 to 3406e8b Compare May 19, 2026 13:17
@MegaManSec
Copy link
Copy Markdown
Contributor Author

@bagder fixed, thanks

@icing thanks for looking. sometimes i press enter without thinking whether the person on the other side of the screen understands what i'm writing in the way i do. my bad

Comment thread tests/unit/Makefile.inc
Add tests/data/test3303 and tests/data/test3304 so the unit tests are
discovered by runtests.pl and included in the source tarball via
Makefile.am.
@bagder
Copy link
Copy Markdown
Member

bagder commented May 19, 2026

The torture memory leak is probably fixed with this:

diff --git a/tests/unit/unit3303.c b/tests/unit/unit3303.c
index e308085f8b..41bced542d 100644
--- a/tests/unit/unit3303.c
+++ b/tests/unit/unit3303.c
@@ -62,10 +62,12 @@ static CURLcode test_unit3303(const char *arg)
       goto unit_test_abort;
     }
 
     conn = curlx_calloc(1, sizeof(*conn));
     if(!conn || Curl_ssl_conn_config_init((struct Curl_easy *)curl, conn)) {
+      if(conn)
+        Curl_ssl_conn_config_cleanup(conn);
       curlx_free(conn);
       curl_easy_cleanup(curl);
       curl_global_cleanup();
       goto unit_test_abort;
     }

@bagder
Copy link
Copy Markdown
Member

bagder commented May 19, 2026

thanks!

vszakats added a commit that referenced this pull request May 19, 2026
- use `curlx_safefree()`.
- drop redundant blocks.

Follow-up to 7541ae5 #21667

Closes #21684
outcast36 pushed a commit to greearb/curl that referenced this pull request Jun 3, 2026
cert_type, key, key_type, key_passwd and key_blob lived in
ssl_config_data but not in ssl_primary_config, so they were invisible to
match_ssl_primary_config() and to the TLS session cache peer key.

Two easy handles sharing a connection pool could reuse each other's
authenticated connections when they differed only on SSLKEY, SSLKEYTYPE,
KEYPASSWD, SSLCERTTYPE or SSLKEYBLOB. The second handle would silently
inherit the first handle's authenticated identity.

Promote all five fields into ssl_primary_config so the conn-reuse
predicate and session cache key cover the complete client credential
set. Also replace the fixed ":CCERT" session cache marker with the
actual clientcert path so sessions are not shared across different
client certificates.

Verified by test 3303 and 3304

Reported-By: Joshua Rogers (AISLE Research)
Closes curl#21667
outcast36 pushed a commit to greearb/curl that referenced this pull request Jun 3, 2026
- use `curlx_safefree()`.
- drop redundant blocks.

Follow-up to 7541ae5 curl#21667

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

Labels

Development

Successfully merging this pull request may close these issues.

5 participants