From a49bb17dbb4ef2d8b1447aa3efbf4b14e6515696 Mon Sep 17 00:00:00 2001
From: George Joseph <gjoseph@sangoma.com>
Date: Fri, 15 Dec 2023 09:37:54 -0700
Subject: [PATCH] res_rtp_asterisk: Fix regression issues with DTLS client
check
* Since ICE candidates are used for the check and pjproject is
required to use ICE, res_rtp_asterisk was failing to compile
when pjproject wasn't available. The check is now wrapped
with an #ifdef HAVE_PJPROJECT.
* The rtp->ice_active_remote_candidates container was being
used to check the address on incoming packets but that
container doesn't contain peer reflexive candidates discovered
during negotiation. This was causing the check to fail
where it shouldn't. We now check against pjproject's
real_ice->rcand array which will contain those candidates.
* Also fixed a bug in ast_sockaddr_from_pj_sockaddr() where
we weren't zeroing out sin->sin_zero before returning. This
was causing ast_sockaddr_cmp() to always return false when
one of the inputs was converted from a pj_sockaddr, even
if both inputs had the same address and port.
Resolves: #500
Resolves: #503
Resolves: #505
---
include/asterisk/res_pjproject.h | 13 +++++++++++++
res/res_pjproject.c | 22 ++++++++++++++++++++++
res/res_rtp_asterisk.c | 22 ++++++++++------------
3 files changed, 45 insertions(+), 12 deletions(-)
diff --git a/include/asterisk/res_pjproject.h b/include/asterisk/res_pjproject.h
index eee8c4eee8..823f5cc545 100644
--- a/include/asterisk/res_pjproject.h
+++ b/include/asterisk/res_pjproject.h
@@ -115,4 +115,17 @@ int ast_sockaddr_to_pj_sockaddr(const struct ast_sockaddr *addr, pj_sockaddr *pj
*/
int ast_sockaddr_from_pj_sockaddr(struct ast_sockaddr *addr, const pj_sockaddr *pjaddr);
+/*!
+ * \brief Compare an ast_sockaddr to a pj_sockaddr
+ *
+ * \param addr pointer to ast_sockaddr structure
+ * \param pjaddr pointer to pj_sockaddr structure
+ *
+ * \retval -1 \a addr is lexicographically smaller than \a pjaddr
+ * \retval 0 \a addr is equal to \a pjaddr
+ * \retval 1 \a pjaddr is lexicographically smaller than \a addr
+*/
+int ast_sockaddr_pj_sockaddr_cmp(const struct ast_sockaddr *addr,
+ const pj_sockaddr *pjaddr);
+
#endif /* _RES_PJPROJECT_H */
diff --git a/res/res_pjproject.c b/res/res_pjproject.c
index 8eadaee82f..865f06278f 100644
--- a/res/res_pjproject.c
+++ b/res/res_pjproject.c
@@ -522,6 +522,7 @@ int ast_sockaddr_from_pj_sockaddr(struct ast_sockaddr *addr, const pj_sockaddr *
sin->sin_addr.s_addr = pjaddr->ipv4.sin_addr.s_addr;
#endif
sin->sin_port = pjaddr->ipv4.sin_port;
+ memset(sin->sin_zero, 0, sizeof(sin->sin_zero));
addr->len = sizeof(struct sockaddr_in);
} else if (pjaddr->addr.sa_family == pj_AF_INET6()) {
struct sockaddr_in6 *sin = (struct sockaddr_in6 *) &addr->ss;
@@ -538,6 +539,27 @@ int ast_sockaddr_from_pj_sockaddr(struct ast_sockaddr *addr, const pj_sockaddr *
return 0;
}
+int ast_sockaddr_pj_sockaddr_cmp(const struct ast_sockaddr *addr,
+ const pj_sockaddr *pjaddr)
+{
+ struct ast_sockaddr temp_pjaddr;
+ int rc = 0;
+
+ rc = ast_sockaddr_from_pj_sockaddr(&temp_pjaddr, pjaddr);
+ if (rc != 0) {
+ return -1;
+ }
+
+ rc = ast_sockaddr_cmp(addr, &temp_pjaddr);
+ if (DEBUG_ATLEAST(4)) {
+ char *a_str = ast_strdupa(ast_sockaddr_stringify(addr));
+ char *pj_str = ast_strdupa(ast_sockaddr_stringify(&temp_pjaddr));
+ ast_debug(4, "Comparing %s -> %s rc: %d\n", a_str, pj_str, rc);
+ }
+
+ return rc;
+}
+
#ifdef TEST_FRAMEWORK
static void fill_with_garbage(void *x, ssize_t len)
{
diff --git a/res/res_rtp_asterisk.c b/res/res_rtp_asterisk.c
index ddedc0f312..5da249d7de 100644
--- a/res/res_rtp_asterisk.c
+++ b/res/res_rtp_asterisk.c
@@ -3186,11 +3186,10 @@ static int __rtp_recvfrom(struct ast_rtp_instance *instance, void *buf, size_t s
* candidates list.
*/
+#ifdef HAVE_PJPROJECT
if (rtp->ice) {
int pass_src_check = 0;
- struct ao2_iterator i;
- struct ast_rtp_engine_ice_candidate *candidate;
- int cand_cnt = 0;
+ int ix = 0;
/*
* You'd think that this check would cause a "deadlock"
@@ -3211,20 +3210,18 @@ static int __rtp_recvfrom(struct ast_rtp_instance *instance, void *buf, size_t s
}
/*
- * If we got this far, then ice_active_remote_candidates
- * can't be NULL.
+ * If we got this far, then there have to be candidates.
+ * We have to use pjproject's rcands because they may have
+ * peer reflexive candidates that our ice_active_remote_candidates
+ * won't.
*/
- i = ao2_iterator_init(rtp->ice_active_remote_candidates, 0);
- while ((candidate = ao2_iterator_next(&i)) && (cand_cnt < PJ_ICE_MAX_CAND)) {
- res = ast_sockaddr_cmp_addr(&candidate->address, sa);
- ao2_ref(candidate, -1);
- if (res == 0) {
+ for (ix = 0; ix < rtp->ice->real_ice->rcand_cnt; ix++) {
+ pj_ice_sess_cand *rcand = &rtp->ice->real_ice->rcand[ix];
+ if (ast_sockaddr_pj_sockaddr_cmp(sa, &rcand->addr) == 0) {
pass_src_check = 1;
break;
}
- cand_cnt++;
}
- ao2_iterator_destroy(&i);
if (!pass_src_check) {
ast_log(LOG_WARNING, "%s: DTLS packet from %s dropped. Source not in ICE active candidate list.\n",
@@ -3233,6 +3230,7 @@ static int __rtp_recvfrom(struct ast_rtp_instance *instance, void *buf, size_t s
return 0;
}
}
+#endif
/*
* A race condition is prevented between dtls_perform_handshake()
--
GitLab