fix dangling local raw_chain in GnuTLS ssl_update_certs_info

[uwezkhan]

Description

ssl_update_certs_info() in the GnuTLS backend stores the local certificate's raw chain in ssock->info_pool, then calls pj_pool_reset() on that same pool before storing the peer's chain. Any handshake where the peer presents a certificate leaves local_cert_info.raw_chain.cert_raw pointing at released pool memory. The peer chain is carved from the same address, so pj_ssl_sock_get_info() returns the peer's leaf certificate as the local raw chain.

Before: the pool was reset once per branch, so the second reset invalidated the chain stored by the first, and pj_ssl_cert_info was zeroed only when gnutls_x509_crt_init() had produced no certificate. Where init succeeded but both imports failed, the identity fields kept the previous handshake's values next to a released raw chain.

After: info_pool is reset once at the top of the update, the two chains are allocated back to back, and each half tracks whether it completed. A half either fully refreshes its pj_ssl_cert_info or zeroes the whole struct, so subject, issuer, serial, SAN and raw chain always describe the same certificate. The reset sits in this function because it is the only place the GnuTLS backend allocates from info_pool, and callers only see the finished pj_ssl_cert_info.

Tradeoff: a failed certificate import now reports an empty pj_ssl_cert_info for that end rather than the previous update's values. Reporting nothing is preferable to reporting fields that no longer agree with each other.

Motivation and Context

raw_chain is public API on pj_ssl_cert_info. An application that reads its own chain from local_cert_info (to log, pin or fingerprint it) gets bytes chosen by the remote peer, through a pointer into memory the pool has already handed out again. The OpenSSL backend only keeps the peer chain in info_pool, so it is not affected.

How Has This Been Tested?

Configured with --with-gnutls (GnuTLS 3.8.13, macOS arm64) and ran a full make; no new warnings. Rebuilt clean against OpenSSL 3.6.4 as well.

Reproducer: mutual TLS over loopback between two pj_ssl_sock instances using two different certificates, then pj_ssl_sock_get_info() on both ends.

• Before: on both ends local_cert_info->raw_chain.cert_raw and remote_cert_info->raw_chain.cert_raw are the same address, and entry 0 of the local chain is the peer's DER.
• After: the two arrays are distinct and entry 0 of the local chain matches the local certificate.

Added a check to ssl_sock_test in pjlib-test. It runs on the client and on the accepted socket in the existing "client cert required and provided" echo case, and requires the local raw chain to hold the DER of the certificate that end presents, in storage of its own. The client now has its own certificate (clicert.pem, issued by the existing test CA, with cliprivkey.pem), so the content comparison distinguishes a dangling local chain from a correct one no matter what address the pool returns. Backends that do not populate the local raw chain, such as OpenSSL, leave cnt zero and are skipped. The Apple backends take the private key from the Keychain, where only the server key is stored, so there the client keeps reusing the server certificate and the storage comparison does the work.

Verification with GnuTLS: against the unpatched ssl_sock_gtls.c both ends report invalid local raw chain (PJ_EBUG) and the test fails in about 6 seconds; with the storage comparison disabled so that only the content comparison can fire, both ends still report it. With the patch, ssl_sock_test, ssl_sock_stress_test and activesock_test pass, and a full pjlib-test --ci-mode run is 23/23. ssl_sock_test and ssl_sock_stress_test also pass with OpenSSL 3.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

• My code follows the CODING STYLE of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• All new and existing tests passed.

[nanangizz]

Thanks, the analysis is correct: the second pj_pool_reset() released the local raw chain, and the peer chain was then carved from the same memory. Resetting once and clearing both raw_chain fields also covers the branches that skip a certificate, so no stale pointer into the reset pool is left.

Verified with GnuTLS 3.8.3 on Linux: ssl_sock_test and activesock_test pass, and with the previous ssl_sock_gtls.c the new check fails with local/remote raw chain overlap (PJ_EBUG).

[sauwming]

This check only fails if the allocator returns the exact same address for both chains. It never checks that the local chain actually holds the local certificate. With PJ_POOL_DEBUG or a malloc-backed pool policy (e.g. under ASan), or if the remote chain lands at a different offset after the reset, the local cert_raw can still dangle while differing from the remote one, and the test passes on the unfixed code.

Could it compare the content instead, e.g. check that lci->raw_chain.cert_raw[0] matches the DER of the configured local certificate (or at least that it differs from rci->raw_chain.cert_raw[0])?

[uwezkhan]

Added, it now compares lci->raw_chain.cert_raw[0] against cacert.der, which is already in pjlib/build/ and is byte-identical to the cacert.pem the test configures.

Worth flagging though: in echo_test the client reuses the server's certificate, so the content comparison can't catch this particular bug on its own. I checked that rather than assumed it, by disabling the array comparison and running the strengthened check against the old ssl_sock_gtls.c: it passes. The dangling local chain points at the peer entry, and the peer DER is the same bytes, so the content matches. The array comparison is what fires there, on both ends.

So both checks are in. The content one catches a chain overwritten with something else, and it would catch the dangle on any setup where the two ends present different certificates. Your allocator point still stands for the overlap half, and making the test independent of the pool handing back the same address needs a second certificate for the client. Say the word and I'll add one.

[sauwming]

Yes, please add a second certificate for the client. As you found, with the same cert on both ends the content check can't tell a dangling local chain from a correct one, and the overlap check depends on the pool returning the same address. With different certs on each side, the content check catches the bug no matter how the allocator behaves.

[uwezkhan]

Added. clicert.pem plus cliprivkey.pem and clicert.der, a leaf issued by the existing cacert.pem CA with its own 2048-bit key, encrypted PKCS#8 with the same privkeypass and the same PBES2 parameters as privkey.pem, CN=client.pjsip.lab, EKU clientAuth, expiring inside the CA's window. The client loads it in the client_provide_cert case and check_raw_chain() takes the expected DER from the test state, so each end compares against the certificate it actually presents.

Before: both ends presented cacert.pem, so the content comparison matched a dangling local chain and only the storage comparison could fire, which is the allocator dependency you flagged. After: the content comparison is decisive. I disabled the storage comparison and ran the test against the unpatched ssl_sock_gtls.c: both the client and the accepted socket report invalid local raw chain on content alone.

One backend keeps the old arrangement. The Apple backends take the private key from the Keychain and only the server key is in privkey.p12, so TEST_CLI_OWN_CERT is 0 for PJ_SSL_SOCK_IMP_APPLE and PJ_SSL_SOCK_IMP_DARWIN, the client there keeps reusing the server certificate, and the storage comparison does the work as before. Tradeoff is three new files in pjlib/build/ and a test CA that now signs something, against a check that no longer depends on the pool returning the same address.

[sauwming]

The check only runs in ssl_on_connect_complete() (the client side). The bug affects both ends, but ssl_on_accept_complete() never calls pj_ssl_sock_get_info() + check_raw_chain(), so a regression on the server side wouldn't be caught. Please add the same check there.

[uwezkhan]

Done, it runs on the accepted socket now as well.

One deviation from the surrounding error paths: it reports through parent_st->err and returns PJ_TRUE rather than closing newsock. Closing from on_accept_complete2 crashes, because on_handshake_complete() reads ssock->parent->param.grp_lock right after the callback returns and the close has already released that socket's pool. I hit it as a segfault in on_handshake_complete while testing the new check against the old backend.

That's independent of this PR, the existing goto on_return paths in the same callback would do it too, so I left it alone and just kept the new check off that path. echo_test already propagates state_serv.err, so the failure surfaces the same way, and against the unpatched ssl_sock_gtls.c both ends now report invalid local raw chain and the test fails cleanly with PJ_EBUG instead of crashing.

[sauwming]

With the raw chains now cleared at the top of the function, there's a new mismatch on this path. If gnutls_x509_crt_init() succeeds but both imports fail (e.g. on renegotiation), we jump here with cert != NULL, so local_cert_info is not zeroed. Subject, issuer, serial and SAN are kept from the previous handshake, but raw_chain is now empty. Before this PR, the stale raw chain at least matched the stale identity fields.

peer_out has the same issue for remote_cert_info. Suggest clearing the whole struct on any failure, not only when cert is NULL.

[uwezkhan]

Right, that mismatch was real. Changed it to track whether each half completed and zero the whole pj_ssl_cert_info otherwise, at both us_out and peer_out.

Before: the struct was zeroed only when cert was NULL, so the init-succeeded-but-both-imports-failed path kept stale identity fields next to an empty raw chain. After: each half either fully refreshes its struct or fully clears it. That also made the two raw_chain bzeros at the top redundant, so they're gone and the single pj_pool_reset() is all that's left there.

Tradeoff is the same one noted in the PR description, just wider: a failed import now reports empty identity fields too, instead of the previous handshake's values.

[uwezkhan]

Pushed as 6ef4d03, replies in the three threads.

Verification, GnuTLS 3.8.13 on macOS arm64: ssl_sock_test, ssl_sock_stress_test and activesock_test all pass, no new warnings. Against the unpatched ssl_sock_gtls.c the test fails with invalid local raw chain (PJ_EBUG) from both the client and the accepted socket. Rebuilt clean with OpenSSL 3.6.4 as well, where ssl_sock_test passes and the check is a no-op because that backend only populates the peer chain.

[sauwming]

Thanks for the updates. Please also update the PR description to match 6ef4d03. It still says that only the raw_chain fields are cleared and that a failed import only loses the raw chain. Now the whole pj_ssl_cert_info (subject, issuer, SAN, raw chain) is zeroed when an import fails. The description becomes the squash-merge message, so it should describe the final behaviour.

[sauwming]

Returning PJ_TRUE here without starting a read leaves the server socket idle, and that doesn't fail cleanly everywhere:

• perf_test() also uses ssl_on_accept_complete(), but its wait loop is while (clients_num) and it only checks state_serv.err afterwards. The server never echoes, so no client finishes and the test hangs until the CI runner times out.
• In echo_test(), nothing closes the accepted socket. Its cleanup doesn't close state_serv.accepted_ssock (unlike client_non_ssl() and large_msg_test()), so the socket and its pool leak, and the ioqueue is destroyed with the key still registered.

Suggestion: record the error in parent_st->err but don't return early. Fall through to pj_ssl_sock_start_read2() so the connection runs to completion and the error is reported at the end. Also close state_serv.accepted_ssock in echo_test()'s cleanup.

[uwezkhan]

Done, both parts.

Before: the callback set parent_st->err and returned PJ_TRUE without starting a read, so perf_test() sat in while (clients_num) until the runner timed out, and echo_test() left the accepted socket and its pool behind. After: it records the error, resets status, and falls through to pj_ssl_sock_start_read2() and the send loop, so the connection runs to completion and the error surfaces at the end.

For the cleanup I had to pick between closing state_serv.accepted_ssock unconditionally, as client_non_ssl() and large_msg_test() do, and guarding it. Unconditional is a double close here: in echo_test() the accepted socket closes itself from ssl_on_data_read() on EOF, and without a grp_lock pj_ssl_sock_close() runs ssl_on_destroy() synchronously and releases ssock->pool, which is where ssock lives, so the is_closing guard never gets a chance to run. The accept callback now stores the accepted socket's own test state in the parent, and echo_test() closes on !err && !done, matching the guard already used for ssock_cli. All three of the socket's self-close paths use exactly that condition.

[uwezkhan]

Pushed as 748cf61, replies in the two threads.

PR description updated to match the final behavior: it now says the whole pj_ssl_cert_info is zeroed when a half fails to refresh, and the tradeoff section covers the identity fields rather than just the raw chain.

Verification, GnuTLS 3.8.13 on macOS arm64: ssl_sock_test, ssl_sock_stress_test and activesock_test pass, and a full pjlib-test --ci-mode run is 23/23 with no new warnings. Against the unpatched ssl_sock_gtls.c the test fails in about 6 seconds with invalid local raw chain (PJ_EBUG) from both the client and the accepted socket, and it still fails with the storage comparison disabled, so the content comparison carries it on its own now. Rebuilt clean with OpenSSL 3.6.4 as well, where ssl_sock_test and ssl_sock_stress_test pass and the check is a no-op because that backend only populates the peer chain.