Re: Ring
Hi Bastien,
On Fri, Sep 29, 2023 at 09:12:57PM +0000, Bastien Roucariès wrote:
> Hi,
>
> I tried to fix CVE-2021-32686 by using patch from upstream.
>
> I think the problem is hard to solve:
> - patch does not apply cleanly and backport will be difficult (moreover it is hard to test this kind of race condition)
I somewhat disagree with your assessment here. I agree that the patch
does not apply cleanly, but the backport seems fairly straightforward.
You did not mention the source you used for the upstream patch and it is
not linked in the security tracker, so I had to go and do some digging.
I was able to find the commit [0] in savoirfairelinux/pjproject (which
is the project which contains the forked pjsip library which is used by
jami, formerly called ring).
The older pjsip located in that project lacks ssl_sock_imp_common.c but
has the other two files. Most of the remainder of the patch applied
(with a small amount of fuzz) and what didn't apply was fairly
straightforward to backport.
I have attached to this email what I think is a reasonable attempt at
backporting upstream's patch of the forked/modified pjsip on top of
version 20190215.1.f152c98~ds1-1+deb10u2 of ring. Note that I have not
run any build or done any sort of testing on this.
Could you review the patch and, if you agree that it seems like a
reasonable backport, then build and test the package?
> - ring use a heavy patched PJSIP. A solution will be to use the repackaged dfsg pjsip from asterisk (debian dir) and try if ring patches apply
>
> However the second solution will take time for something that is DOS by NULL pointer deference....
>
I agree that this second idea is likely not a good one. We have no way
to feasibly determine if upstream's forked/modified pjsip is entirely
compatible with the pjsip in the Debian archive. The risk would be too
great to switch out an underlying library implementation in this
fashion.
> Maybe a dsa-ignore will be better for this issue
>
Perhaps first consider the patch that I backported and only if that
doesn't solve the issue satisfactorily, then reconsider marking the CVE
as ignored.
All of that said, it is interesting to me that fairly recently (at the
end of August) the ring package in buster was updated to fix 23 CVEs,
but this particular CVE was left open. Perhaps it would be worthwhile to
find out from Thorsten (who prepared the most recent update) why that
decision was made.
Regards,
-Roberto
[0] https://github.com/savoirfairelinux/pjproject/commit/d5f95aa066f878b0aef6a64e60b61e8626e664cd
--
Roberto C. Sánchez ◈ Freexian SARL
https://www.freexian.com
diff -ur ring-20190215.1.f152c98~ds1.orig/daemon/contrib/tarballs-unpacked/pjproject-2.8.tar.gz/pjproject-2.8/pjlib/src/pj/ssl_sock_ossl.c ring-20190215.1.f152c98~ds1/daemon/contrib/tarballs-unpacked/pjproject-2.8.tar.gz/pjproject-2.8/pjlib/src/pj/ssl_sock_ossl.c
--- ring-20190215.1.f152c98~ds1.orig/daemon/contrib/tarballs-unpacked/pjproject-2.8.tar.gz/pjproject-2.8/pjlib/src/pj/ssl_sock_ossl.c 2018-09-04 23:52:06.000000000 -0400
+++ ring-20190215.1.f152c98~ds1/daemon/contrib/tarballs-unpacked/pjproject-2.8.tar.gz/pjproject-2.8/pjlib/src/pj/ssl_sock_ossl.c 2023-10-06 15:11:16.166154584 -0400
@@ -451,7 +451,8 @@
ERROR_LOG("STATUS_FROM_SSL_ERR", err);
}
- ssock->last_err = err;
+ if (ssock)
+ ssock->last_err = err;
return GET_STATUS_FROM_SSL_ERR(err);
}
@@ -468,7 +469,8 @@
/* Dig for more from OpenSSL error queue */
SSLLogErrors(action, ret, err, len);
- ssock->last_err = ssl_err;
+ if (ssock)
+ ssock->last_err = ssl_err;
return GET_STATUS_FROM_SSL_ERR(ssl_err);
}
@@ -656,6 +658,13 @@
/* Create OpenSSL application data index for SSL socket */
sslsock_idx = SSL_get_ex_new_index(0, "SSL socket", NULL, NULL, NULL);
+ if (sslsock_idx == -1) {
+ status = STATUS_FROM_SSL_ERR2("Init", NULL, -1, ERR_get_error(), 0);
+ PJ_LOG(1,(THIS_FILE,
+ "Fatal error: failed to get application data index for "
+ "SSL socket"));
+ return status;
+ }
return status;
}
@@ -683,21 +692,36 @@
}
-/* SSL password callback. */
+/* SSL certificate verification result callback.
+ * Note that this callback seems to be always called from library worker
+ * thread, e.g: active socket on_read_complete callback, which should have
+ * already been equipped with race condition avoidance mechanism (should not
+ * be destroyed while callback is being invoked).
+ */
static int verify_cb(int preverify_ok, X509_STORE_CTX *x509_ctx)
{
- pj_ssl_sock_t *ssock;
- SSL *ossl_ssl;
+ pj_ssl_sock_t *ssock = NULL;
+ SSL *ossl_ssl = NULL;
int err;
/* Get SSL instance */
ossl_ssl = X509_STORE_CTX_get_ex_data(x509_ctx,
SSL_get_ex_data_X509_STORE_CTX_idx());
- pj_assert(ossl_ssl);
+ if (!ossl_ssl) {
+ PJ_LOG(1,(THIS_FILE,
+ "SSL verification callback failed to get SSL instance"));
+ goto on_return;
+ }
/* Get SSL socket instance */
ssock = SSL_get_ex_data(ossl_ssl, sslsock_idx);
- pj_assert(ssock);
+ if (!ssock) {
+ /* SSL socket may have been destroyed */
+ PJ_LOG(1,(THIS_FILE,
+ "SSL verification callback failed to get SSL socket "
+ "instance (sslsock_idx=%d).", sslsock_idx));
+ goto on_return;
+ }
/* Store verification status */
err = X509_STORE_CTX_get_error(x509_ctx);
@@ -775,6 +799,7 @@
if (PJ_FALSE == ssock->param.verify_peer)
preverify_ok = 1;
+on_return:
return preverify_ok;
}
@@ -1188,6 +1213,9 @@
{
/* Destroy SSL instance */
if (ssock->ossl_ssl) {
+ /* Detach from SSL instance */
+ SSL_set_ex_data(ossock->ossl_ssl, sslsock_idx, NULL);
+
/**
* Avoid calling SSL_shutdown() if handshake wasn't completed.
* OpenSSL 1.0.2f complains if SSL_shutdown() is called during an
diff -ur ring-20190215.1.f152c98~ds1.orig/daemon/contrib/tarballs-unpacked/pjproject-2.8.tar.gz/pjproject-2.8/pjsip/src/pjsip/sip_transport_tls.c ring-20190215.1.f152c98~ds1/daemon/contrib/tarballs-unpacked/pjproject-2.8.tar.gz/pjproject-2.8/pjsip/src/pjsip/sip_transport_tls.c
--- ring-20190215.1.f152c98~ds1.orig/daemon/contrib/tarballs-unpacked/pjproject-2.8.tar.gz/pjproject-2.8/pjsip/src/pjsip/sip_transport_tls.c 2018-09-04 23:52:06.000000000 -0400
+++ ring-20190215.1.f152c98~ds1/daemon/contrib/tarballs-unpacked/pjproject-2.8.tar.gz/pjproject-2.8/pjsip/src/pjsip/sip_transport_tls.c 2023-10-06 15:15:50.478539417 -0400
@@ -1322,11 +1322,31 @@
PJ_UNUSED_ARG(src_addr_len);
listener = (struct tls_listener*) pj_ssl_sock_get_user_data(ssock);
+ if (!listener) {
+ /* Listener already destroyed, e.g: after TCP accept but before SSL
+ * handshake is completed.
+ */
+ if (new_ssock && accept_status == PJ_SUCCESS) {
+ /* Close the SSL socket if the accept op is successful */
+ PJ_LOG(4,(THIS_FILE,
+ "Incoming TLS connection from %s (sock=%d) is discarded "
+ "because listener is already destroyed",
+ pj_sockaddr_print(src_addr, addr, sizeof(addr), 3),
+ new_ssock));
+
+ pj_ssl_sock_close(new_ssock);
+ }
+
+ return PJ_FALSE;
+ }
PJ_ASSERT_RETURN(new_ssock, PJ_TRUE);
- if (!listener->is_registered)
+ if (!listener->is_registered) {
+ pj_ssl_sock_close(new_ssock);
+
return PJ_FALSE;
+ }
PJ_LOG(4,(listener->factory.obj_name,
"TLS listener %s: got incoming TLS connection "
@@ -1357,8 +1377,11 @@
status = tls_create( listener, NULL, new_ssock, PJ_TRUE,
&ssl_info.local_addr, &tmp_src_addr, NULL, &tls);
- if (status != PJ_SUCCESS)
+ if (status != PJ_SUCCESS) {
+ pj_ssl_sock_close(new_ssock);
+
return PJ_TRUE;
+ }
/* Set the "pending" SSL socket user data */
pj_ssl_sock_set_user_data(new_ssock, tls);
Reply to:
- Follow-Ups:
- Re: Ring
- From: Bastien Roucariès <rouca@debian.org>