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

Add the option to check the source address of ClientHello message on DTLS-SRTP #4261

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
10 changes: 10 additions & 0 deletions pjmedia/include/pjmedia/config.h
Original file line number Diff line number Diff line change
Expand Up @@ -1100,6 +1100,16 @@
# define PJMEDIA_SRTP_DTLS_OSSL_CIPHERS "DEFAULT"
#endif

/**
* Enabled this to check the source address of ClientHello message coming
* from a valid address. See PJ_ICE_SESS_CHECK_SRC_ADDR when ICE is used.
*
* Default value: 0
*/
#ifndef PJMEDIA_SRTP_DTLS_CHECK_HELLO_ADDR
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need a compile-time setting? and why is the default 0 if it may cause a security issue?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might add a slight delay in starting the handshake (wait for the transport address and RTP source address to be available) and possible issues when using aggressive nomination.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After reading the pdf, I believe the address check should be made mandatory for ICE, and the address should match one of the verified (valid?) candidates obtained during ICE check, so there should be no delay because ICE check has been done.

For no-ICE, then you can use the compile-time setting, because the best we can do is compare it with the SDP address, which may not be accurate.

# define PJMEDIA_SRTP_DTLS_CHECK_HELLO_ADDR 0
#endif


/**
* Maximum number of SRTP cryptos.
Expand Down
57 changes: 57 additions & 0 deletions pjmedia/src/pjmedia/transport_srtp_dtls.c
Original file line number Diff line number Diff line change
Expand Up @@ -1178,6 +1178,19 @@ static pj_status_t get_rem_addrs(dtls_srtp *ds,
return PJ_SUCCESS;
}

static pj_bool_t is_valid_src_addr(dtls_srtp *ds, unsigned idx,
pj_sockaddr *src_addr)
{
pj_sockaddr *rem_addr;

if (idx == RTP_CHANNEL) {
rem_addr = &ds->rem_addr;
} else {
rem_addr = &ds->rem_rtcp;
}
return (pj_sockaddr_cmp(rem_addr, src_addr) == 0);
}

/* Check if an incoming packet is a DTLS packet (rfc5764 section 5.1.2) */
#define IS_DTLS_PKT(pkt, pkt_len) (*(char*)pkt > 19 && *(char*)pkt < 64)

Expand Down Expand Up @@ -1361,6 +1374,50 @@ static pj_status_t dtls_on_recv(pjmedia_transport *tp, unsigned idx,
(ds->setup == DTLS_SETUP_ACTPASS || ds->setup == DTLS_SETUP_PASSIVE))
{
pj_status_t status;

#if defined(PJMEDIA_SRTP_DTLS_CHECK_HELLO_ADDR) && \
PJMEDIA_SRTP_DTLS_CHECK_HELLO_ADDR==1

/* Check the souce address with the specified remote address from
* the SDP. When ICE is used, the source address checking will be
* done in ICE session.
*/
if (!ds->use_ice) {
pjmedia_transport_info info;
pj_sockaddr src_addr;
pj_bool_t src_addr_avail = PJ_TRUE;
Copy link
Member

@sauwming sauwming Feb 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

source address must be available, because it's where the packet comes from.


pjmedia_transport_get_info(ds->srtp->member_tp, &info);
if (idx == RTP_CHANNEL) {
if (!pj_sockaddr_has_addr(&info.src_rtp_name)) {
src_addr_avail = PJ_FALSE;
} else {
pj_sockaddr_cp(&src_addr, &info.src_rtp_name);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is RTP address, not SDP address.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is RTP address, not SDP address.

Yes, it is to get the message's source address.
The SDP address/check address is here:

static pj_bool_t is_valid_src_addr(dtls_srtp *ds, unsigned idx,
pj_sockaddr *src_addr)
{
pj_sockaddr *rem_addr;
if (idx == RTP_CHANNEL) {
rem_addr = &ds->rem_addr;
} else {
rem_addr = &ds->rem_rtcp;
}
return (pj_sockaddr_cmp(rem_addr, src_addr) == 0);

}
} else {
if (!pj_sockaddr_has_addr(&info.src_rtcp_name)) {
src_addr_avail = PJ_FALSE;
} else {
pj_sockaddr_cp(&src_addr, &info.src_rtcp_name);
}
}

if (!src_addr_avail || !is_valid_src_addr(ds, idx, &src_addr)) {
char psrc_addr[PJ_INET6_ADDRSTRLEN] = "Unknown";

if (src_addr_avail) {
pj_sockaddr_print(&src_addr, psrc_addr,
sizeof(psrc_addr), 3);
}
PJ_LOG(2, (ds->base.name, "DTLS-SRTP %s ignoring %lu bytes, "
"from unrecognized src addr [%s]", CHANNEL_TO_STRING(idx),
(unsigned long)size, psrc_addr));

DTLS_UNLOCK(ds);
return PJ_SUCCESS;
}
}
#endif
ds->setup = DTLS_SETUP_PASSIVE;
status = ssl_handshake_channel(ds, idx);
if (status != PJ_SUCCESS) {
Expand Down
10 changes: 10 additions & 0 deletions pjnath/include/pjnath/config.h
Original file line number Diff line number Diff line change
Expand Up @@ -394,6 +394,16 @@
# define PJ_ICE_NOMINATED_CHECK_DELAY (4*PJ_STUN_RTO_VALUE)
#endif

/**
* Specify whether to check the source address of the incoming messages.
* The source address will be compared to the remote candidate which has
* a completed connectivity check or received a connectivity check.
*
* Defalut: 1 (yes)
*/
#ifndef PJ_ICE_SESS_CHECK_SRC_ADDR
# define PJ_ICE_SESS_CHECK_SRC_ADDR 1
#endif

/**
* Minimum interval value to be used for sending STUN keep-alive on the ICE
Expand Down
22 changes: 22 additions & 0 deletions pjnath/include/pjnath/ice_session.h
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,12 @@ typedef struct pj_ice_sess_comp
*/
pj_stun_session *stun_sess;

/**
* The remote candidate checked address. This is expected address that
* the remote going to use.
*/
pj_sockaddr rcand_check_addr;

} pj_ice_sess_comp;


Expand Down Expand Up @@ -317,6 +323,13 @@ struct pj_ice_sess_cand
*/
pj_sockaddr rel_addr;

/**
* Indicate that remote connectivity check has been received or the check
* has been successful for this candidate. It is applicable for
* remote candidate only.
*
*/
pj_bool_t checked;
sauwming marked this conversation as resolved.
Show resolved Hide resolved
};


Expand Down Expand Up @@ -672,6 +685,15 @@ typedef struct pj_ice_sess_options
*/
pj_ice_sess_trickle trickle;

/**
* Specify whether to check the source address of the incoming messages.
* The source address will be compared to the remote candidate which has
* a completed connectivity check or received a connectivity check.
*
* Default value is PJ_ICE_SESS_CHECK_SRC_ADDR.
*/
pj_bool_t check_src_addr;

} pj_ice_sess_options;


Expand Down
48 changes: 48 additions & 0 deletions pjnath/src/pjnath/ice_session.c
Original file line number Diff line number Diff line change
Expand Up @@ -325,6 +325,7 @@ PJ_DEF(void) pj_ice_sess_options_default(pj_ice_sess_options *opt)
opt->controlled_agent_want_nom_timeout =
ICE_CONTROLLED_AGENT_WAIT_NOMINATION_TIMEOUT;
opt->trickle = PJ_ICE_SESS_TRICKLE_DISABLED;
opt->check_src_addr = PJ_ICE_SESS_CHECK_SRC_ADDR;
}

/*
Expand Down Expand Up @@ -2883,6 +2884,8 @@ static void on_stun_request_complete(pj_stun_session *stun_sess,
&ice->clist, check),
(check->nominated ? " (nominated)" : " (not nominated)")));

check->rcand->checked = PJ_TRUE;

/* Get the STUN XOR-MAPPED-ADDRESS attribute. */
xaddr = (pj_stun_xor_mapped_addr_attr*)
pj_stun_msg_find_attr(response, PJ_STUN_ATTR_XOR_MAPPED_ADDR,0);
Expand Down Expand Up @@ -3141,6 +3144,13 @@ static pj_status_t on_stun_rx_request(pj_stun_session *sess,
uc_attr = (pj_stun_use_candidate_attr*)
pj_stun_msg_find_attr(msg, PJ_STUN_ATTR_USE_CANDIDATE, 0);

if (uc_attr != NULL && ice->is_complete) {
LOG4((ice->obj_name,
"Ignored incoming check after ICE nego has been completed!, "
"no need to send response"));
pj_grp_lock_release(ice->grp_lock);
return PJ_SUCCESS;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part seems rather OOT and change the ICE flow/algo? Usually this kind of changes is specified (including the reason) in the PR desc.


/* Get ICE-CONTROLLING or ICE-CONTROLLED */
role_attr = (pj_stun_uint64_attr*)
Expand Down Expand Up @@ -3447,6 +3457,8 @@ static void handle_incoming_check(pj_ice_sess *ice,
*/
c->nominated = ((rcheck->use_candidate) || c->nominated);

rcand->checked = PJ_TRUE;

if (c->state == PJ_ICE_SESS_CHECK_STATE_FROZEN ||
c->state == PJ_ICE_SESS_CHECK_STATE_WAITING)
{
Expand Down Expand Up @@ -3734,6 +3746,42 @@ PJ_DEF(pj_status_t) pj_ice_sess_on_rx_pkt(pj_ice_sess *ice,

PJ_RACE_ME(5);

if (ice->opt.check_src_addr) {
pj_bool_t check_addr = PJ_TRUE;
pj_sockaddr *raddr = &comp->rcand_check_addr;
char psrc_addr[PJ_INET6_ADDRSTRLEN] = {0};

if (pj_sockaddr_has_addr(src_addr)) {
pj_sockaddr_print(src_addr, psrc_addr, sizeof(psrc_addr), 3);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Always printing address does not look very optimized, print only when needed, e.g: inside the most inner log printing block?


if (!pj_sockaddr_has_addr(raddr)) {
for (i = 0; i < ice->rcand_cnt; ++i) {
if (ice->rcand[i].comp_id == comp_id &&
ice->rcand[i].checked &&
pj_sockaddr_cmp(src_addr, &ice->rcand[i].addr) == 0)
{
pj_sockaddr_cp(raddr, src_addr);
PJ_LOG(4, (ice->obj_name, "Using [%s] as valid address "
"for component [%d]",
psrc_addr, comp_id));

check_addr = PJ_FALSE;
break;
}
}
}
if (check_addr &&
(!pj_sockaddr_has_addr(raddr) ||
pj_sockaddr_cmp(src_addr, raddr) != 0))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The check can be optimized to use only pj_sockaddr_cmp() I guess?

{
PJ_LOG(4, (ice->obj_name, "Ignoring incoming message for "
"component [%d] because source addr [%s] unrecognized",
comp_id, psrc_addr));
return PJ_SUCCESS;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once rcand_check_addr is set, it cannot be changed, right? Is there possibility that it may settle with a wrong source address? Note that from the code pj_ice_sess_send_data(), it seems to allow sending data while ICE nego is on progress (using valid check with highest prio), and the valid check may change while ICE nego is not completed.

}

(*ice->cb.on_rx_data)(ice, comp_id, transport_id, pkt, pkt_size,
src_addr, src_addr_len);
status = PJ_SUCCESS;
Expand Down
Loading