fixeria has submitted this change. ( https://gerrit.osmocom.org/c/osmo-pcap/+/42849?usp=email ) Change subject: tls: fix broken certificate hostname verification ...................................................................... tls: fix broken certificate hostname verification verify_cert_cb() retrieved the gnutls session pointer and passed it to gnutls_certificate_verify_peers3() as the expected hostname. But the session pointer is set to the osmo_tls_session struct (it is needed by cert_callback()), not a hostname string. Hostname matching was therefore performed against raw struct bytes, rendering verification meaningless and potentially reading out of bounds, even when "tls verify-cert" was enabled. Store the configured hostname in struct osmo_tls_session and have verify_cert_cb() read it from there. Also drop the stray gnutls_certificate_verify_peers3() call in the client setup: it ran before any handshake (so there were no peer certificates yet) and its result was ignored; the real verification happens via the registered callback during the handshake. Change-Id: If64950a698bfcfbf556a37ef1be3e68abc124384 AI-Assisted: yes (Claude) --- M include/osmo-pcap/osmo_tls.h M src/osmo_tls.c 2 files changed, 18 insertions(+), 4 deletions(-) Approvals: pespin: Looks good to me, but someone else must approve fixeria: Looks good to me, approved Jenkins Builder: Verified laforge: Looks good to me, but someone else must approve diff --git a/include/osmo-pcap/osmo_tls.h b/include/osmo-pcap/osmo_tls.h index 716604e..6afbe71 100644 --- a/include/osmo-pcap/osmo_tls.h +++ b/include/osmo-pcap/osmo_tls.h @@ -38,6 +38,9 @@ bool need_resend; gnutls_session_t session; + /* expected peer hostname for certificate verification (may be NULL) */ + const char *tls_hostname; + /* any credentials */ bool anon_alloc; gnutls_anon_client_credentials_t anon_cred; diff --git a/src/osmo_tls.c b/src/osmo_tls.c index f06f50e..d3ed45e 100644 --- a/src/osmo_tls.c +++ b/src/osmo_tls.c @@ -139,11 +139,15 @@ static int verify_cert_cb(gnutls_session_t session) { + const struct osmo_tls_session *sess; const char *hostname; unsigned int status; int ret; - hostname = gnutls_session_get_ptr(session); + /* The session ptr is the osmo_tls_session (see gnutls_session_set_ptr()); + * the expected hostname is stored inside it. */ + sess = gnutls_session_get_ptr(session); + hostname = sess ? sess->tls_hostname : NULL; ret = gnutls_certificate_verify_peers3(session, hostname, &status); if (ret != 0) @@ -448,7 +452,6 @@ { struct osmo_tls_session *sess = &client->tls_session; struct osmo_wqueue *wq = &client->wqueue; - unsigned int status; int rc; gnutls_global_set_log_level(client->tls_log_level); @@ -518,14 +521,19 @@ gnutls_certificate_set_retrieve_function2(sess->cert_cred, cert_callback); /* set the hostname if we have one */ - if (client->tls_hostname) + if (client->tls_hostname) { gnutls_server_name_set(sess->session, GNUTLS_NAME_DNS, client->tls_hostname, strlen(client->tls_hostname)); + /* Remember it so verify_cert_cb() can match it against the peer + * certificate during the handshake. */ + sess->tls_hostname = talloc_strdup(client, client->tls_hostname); + } /* do the verification */ if (client->tls_verify) { + /* The actual verification happens during the handshake via the + * registered callback; there are no peer certificates yet here. */ gnutls_certificate_set_verify_function(sess->cert_cred, verify_cert_cb); - gnutls_certificate_verify_peers3(sess->session, client->tls_hostname, &status); } else LOGP(DTLS, LOGL_NOTICE, "Not going to validate certs as configured\n"); @@ -545,6 +553,9 @@ gnutls_deinit(session->session); + talloc_free((void *)session->tls_hostname); + session->tls_hostname = NULL; + release_keys(session); if (session->anon_alloc) -- To view, visit https://gerrit.osmocom.org/c/osmo-pcap/+/42849?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email Gerrit-MessageType: merged Gerrit-Project: osmo-pcap Gerrit-Branch: master Gerrit-Change-Id: If64950a698bfcfbf556a37ef1be3e68abc124384 Gerrit-Change-Number: 42849 Gerrit-PatchSet: 4 Gerrit-Owner: fixeria <vyanitskiy@sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <vyanitskiy@sysmocom.de> Gerrit-Reviewer: laforge <laforge@osmocom.org> Gerrit-Reviewer: pespin <pespin@sysmocom.de>