search

baresip / re

Generic library for real-time communications with async IO support
BSD 3-Clause "New" or "Revised" License
132 stars 81 forks source link

Broken LibreSSL tls_conn_change_cert implementation #1135

Open sreimers opened 4 months ago

sreimers commented 4 months ago

Looks like there are some broken LibreSSL implementations https://github.com/baresip/re/pull/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"

https://github.com/baresip/re/blob/b41f503085af62e1d7f1d12b1226ee361b7e02cc/src/tls/openssl/tls_tcp.c#L331-L333

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.

https://github.com/baresip/re/blob/b41f503085af62e1d7f1d12b1226ee361b7e02cc/src/sip/transp.c#L827-L843

We should keep all SSL implementations very generic.

@cHuberCoffee

sreimers commented 4 months ago

Maybe SSL_set_SSL_CTX can be used here too, will try after https://github.com/baresip/re/pull/1136 is merged.

cHuberCoffee commented 4 months ago

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 commented 4 months ago

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

https://github.com/baresip/re/pull/1136

cHuberCoffee commented 4 months ago

"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 commented 3 months ago

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