Skip to content
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.

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

Broken LibreSSL tls_conn_change_cert implementation #1135

Open
sreimers opened this issue Jun 2, 2024 · 5 comments
Open

Broken LibreSSL tls_conn_change_cert implementation #1135

sreimers opened this issue Jun 2, 2024 · 5 comments
Milestone

Comments

@sreimers
Copy link
Member

sreimers commented Jun 2, 2024

Looks like there are some broken LibreSSL implementations #1127.

Let's start with test_tls_cli_conn_change_cert and tls_conn_change_cert, this does not work within libressl since SSL_certs_clear is not supported (There is usually a good reason why LibreSSL drops some APIs, mostly for security maintenance reasons).

tlstest: TEST_STRCMP: /home/sreimers/projekte/baresip/baresip-linux/re/test/tls.c:539: failed
expected string: (21 bytes)
"Mr Retest Client Cert"
actual string: (9 bytes)
"127.0.0.1"

#if !defined(LIBRESSL_VERSION_NUMBER)
SSL_certs_clear(tc->ssl);
#endif

After studying the usage, I wonder why the ssl object is changed and not the ssl ctx before (or a new one used)? Since within tls_start_tcp a new ssl object is created from ctx. This way SSL_certs_clear can be avoided, I think.

re/src/sip/transp.c

Lines 827 to 843 in b41f503

err = tls_start_tcp(&conn->sc, transp->tls, conn->tc, 0);
if (err)
goto out;
hash = get_hash_of_fromhdr(mb);
ccert = list_ledata(
list_head(hash_list(transp->ht_ccert, hash)));
if (ccert) {
char *f;
err = pl_strdup(&f, &ccert->file);
if (err)
goto out;
err = tls_conn_change_cert(conn->sc, f);
mem_deref(f);
if (err)
goto out;

We should keep all SSL implementations very generic.

@cHuberCoffee

@sreimers sreimers changed the title Broken libreSSL tls_conn_change_cert implementation Broken LibreSSL tls_conn_change_cert implementation Jun 2, 2024
@sreimers
Copy link
Member Author

sreimers commented Jun 3, 2024

Maybe SSL_set_SSL_CTX can be used here too, will try after #1136 is merged.

@cHuberCoffee
Copy link
Contributor

So the allocation of the tls struct containing an SSL_CTX is done by uag_transp_add.
Basically we only have one SSL_CTX where all SSL objects are inherited. This includes the loaded certificate which we need to replace. The SSL_certs_clear is for sure not a good implementation for doing so. The SSL_set_SSL_CTX would mean a new context for each certificate configured on user agent level.

It would be possible to create a new context for each such a configuration and saving these contexts as hash_list instead of the certificate path. Then using the SSL_set_SSL_CTX. For OpenSSL we have to find the same functionality. At least in the documentation i found nothing.

@sreimers
Copy link
Member Author

sreimers commented Jun 4, 2024

SSL_set_SSL_CTX is supported and recommended by OpenSSL but not very documented:

#1136

@cHuberCoffee
Copy link
Contributor

"not very documented" points it out.
But you are right, following down the implementation in OpenSSL for the function does look a lot like its exactly what we want to archive here.

@cspiel1 cspiel1 added this to the v3.14.0 milestone Jun 19, 2024
@cspiel1
Copy link
Collaborator

cspiel1 commented Jun 19, 2024

Is v3.14.0 okay for this issue? Today we planned to release v3.13.0.

@sreimers sreimers modified the milestones: v3.14.0, v3.15.0 Jul 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants