Add the option to check the source address of ClientHello message on DTLS-SRTP #4261
Conversation
| * | ||
| * Default value: 0 | ||
| */ | ||
| #ifndef PJMEDIA_SRTP_DTLS_CHECK_HELLO_ADDR |
There was a problem hiding this comment.
Why do we need a compile-time setting? and why is the default 0 if it may cause a security issue?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
When using this option, it is recommended to use regular nomination for ICE. When enabling STUN with aggressive nomination might result in a different candidate pair type being used between endpoints and preventing the handshake. |
This is the case when using ICE with STUN (aggressive nomination):
|
|
For trickle ICE, the candidates may be added rapidly while app is allowed to send/recv data before the ICE nego is completed. So if the proposed feature is on, perhaps it should indeed check the source address against all remote candidates. This may be applicable for aggressive mode too. |
- The check will always be performed when ICE is use - Check the RTP/RTPC address against the candidate when ICE is use
|
So now, does it work for aggressive & trickle modes? |
Yes, now working with aggressive and trickle |
|
For ICE: If no ICE is used: |
It looks like "verified" is specific to the recommendation, not really ICE things? |
Correct. ICE itself doesn't use the term "verified", and the document explains it in the media consent verification phase, which means that that the other party is the peer they claim to be. So this way, security can be achieved.
Yes, this is fine too. So, it means that |
Yes. Note that in ICE, the status belongs to pair-check instead of candidate, a remote candidate may be paired (& checked) against multiple local candidates. If DTLS just needs to list remote candidates that has been checked, it can be done but needs to be clarified in the docs. |
|
There's an issue when using aggressive nomination/trickle: Controlled side: Check 1 connectivity check was received late, so the controlling side will use Check 3 instead and fail the Check 1. |
|
It looks like it's caused by pjproject/pjnath/src/pjnath/ice_session.c Lines 1734 to 1741 in f986ad8 pjproject/pjnath/include/pjnath/config.h Lines 352 to 360 in f986ad8 I don't quite understand the reasoning why we enable this setting by default though, especially about the statement |
|
After setting |
|
Yes, we need to check against all verified remote candidates. According to my understanding (let me know if this is not correct), a verified remote candidate is a remote candidate that has successfully sent an ICE check to us. |
|
I assume |
|
From the docs:
I think we can interpret it as: So I agree with @sauwming that a verified remote candidate is a remote candidate that has successfully sent an ICE check to us, with additional note, it uses the correct user & pass (I think our ICE/STUN implementation has checked the user/pass before responding, need to verify though). IIRC ICE doesn't maintain a list of them. Furthermore, if ICE is active, what if we put the source check in ICE itself, e.g: in pj_ice_sess_on_rx_pkt(), and as usual, it is better configurable. |
- Remove the enum candidate APIs - Move the source checking for ICE - Use the remote candidate which has received STUN request or has completed the connectivity check as a valid source - Don't send respond to STUN request with USE-CANDIDATE when the ICE is completed.
pjnath/src/pjnath/ice_session.c
Outdated
| } | ||
| PJ_LOG(2, (ice->obj_name, "Ignoring incoming message for " | ||
| "component [%d], from src addr [%s]", | ||
| comp_id, psrc_addr)); |
There was a problem hiding this comment.
- Specify the reason of ignoring, e.g: s/"component [%d], from src addr [%s]"/"component %d because source address %s is unrecognized"? Note that
pj_sockaddr_print(..,3)will also print bracket for IPv6. - Won't log level 2 be too verbose for low level API, perhaps 4?
pjnath/src/pjnath/ice_session.c
Outdated
| "no need to send response")); | ||
| pj_grp_lock_release(ice->grp_lock); | ||
| return PJ_SUCCESS; | ||
| } |
There was a problem hiding this comment.
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.
pjnath/include/pjnath/ice_session.h
Outdated
| * | ||
| * Default value is PJ_ICE_SESS_CHECK_SRC_ADDR. | ||
| */ | ||
| unsigned check_src_addr; |
| dtls_srtp_ice_cand ice_rem_cand[NUM_CHANNEL]; | ||
|
|
| typedef struct dtls_srtp_ice_cand | ||
| { | ||
| unsigned cand_cnt; | ||
| pj_sockaddr cand_addr[PJ_ICE_MAX_CAND]; | ||
| } dtls_srtp_ice_cand; | ||
|
|
| * has been successful for this candidate. | ||
| * | ||
| */ | ||
| pj_bool_t checked; |
There was a problem hiding this comment.
Need to specify that this flag is only used by remote candidate?
| @@ -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; | |||
| pj_bool_t check_hello_addr = PJ_FALSE; | |||
There was a problem hiding this comment.
Is this still needed if the check is already done in ICE?
And if we don't use ICE, should we compare it against the SDP?
I don't mind if we don't, but we need to document it somewhere
pjnath/src/pjnath/ice_session.c
Outdated
| @@ -3734,6 +3746,29 @@ 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) { | |||
| for (i = 0; i < ice->rcand_cnt; ++i) { | |||
There was a problem hiding this comment.
Won't this slow down ICE tp considerably, if we have to iterate through all remote candidates for every packet?
- store the valid address to avoid checking with all of remote candidate - add comment regarding the check source address in SRTP-DTLS is for non-ICE use
|
There were ICE errors before the last commit (perhaps due to algo change?) and it seems to still happen. |
| if (check_addr && | ||
| (!pj_sockaddr_has_addr(raddr) || | ||
| pj_sockaddr_cmp(src_addr, raddr) != 0)) | ||
| { | ||
| PJ_LOG(4, (ice->obj_name, "Ignoring incoming message for " | ||
| "component [%d] because source addr [%s] unrecognized", | ||
| comp_id, psrc_addr)); | ||
| return PJ_SUCCESS; | ||
| } |
There was a problem hiding this comment.
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.
| if (!pj_sockaddr_has_addr(&info.src_rtp_name)) { | ||
| src_addr_avail = PJ_FALSE; | ||
| } else { | ||
| pj_sockaddr_cp(&src_addr, &info.src_rtp_name); |
There was a problem hiding this comment.
This is RTP address, not SDP address.
There was a problem hiding this comment.
This is RTP address, not SDP address.
Yes, it is to get the message's source address.
The SDP address/check address is here:
pjproject/pjmedia/src/pjmedia/transport_srtp_dtls.c
Lines 1181 to 1191 in dcf9c65
- Revert the ICE behavior change since it cause the pjnath-test to fail - Settle the valid remote address after the ICE is completed
| if (!ds->use_ice) { | ||
| pjmedia_transport_info info; | ||
| pj_sockaddr src_addr; | ||
| pj_bool_t src_addr_avail = PJ_TRUE; |
There was a problem hiding this comment.
source address must be available, because it's where the packet comes from.
| /* Before ICE completed, it is still allowed to receive | ||
| * data from other candidates. | ||
| */ | ||
| if (ice->is_complete) { |
There was a problem hiding this comment.
Looks better, but adding some delay (e.g: ~1s, compile-time configurable) before fixing it to an address perhaps even better as there can be race conditions when ICE has just been completed. Note that before it is fixed, the comparison still needs to be done against all "verified" remote candidate, as already done now.
|
|
||
| if (pj_sockaddr_has_addr(src_addr)) { | ||
| pj_sockaddr_print(src_addr, psrc_addr, sizeof(psrc_addr), 3); | ||
| } |
There was a problem hiding this comment.
Always printing address does not look very optimized, print only when needed, e.g: inside the most inner log printing block?
| } | ||
| if (check_addr && | ||
| (!pj_sockaddr_has_addr(raddr) || | ||
| pj_sockaddr_cmp(src_addr, raddr) != 0)) |
There was a problem hiding this comment.
The check can be optimized to use only pj_sockaddr_cmp() I guess?
There is a possible issue with
DTLS“ClientHello” Race Conditions leading to DDoS attack.This is done by sending a malicious DTLS ClientHello message from any
IP address to the expected port, potentially causing a “network race condition” if the malicious
messages is processed before the legitimate one.
For ICE use, this patch will add
pj_ice_sess_options::check_src_addrandPJ_ICE_SESS_CHECK_SRC_ADDRto allow ICE sessionto check the source against the 'verified' remote candidate. i.e.: remote candidates which has
a completed connectivity check or received a connectivity check.
For non-ICE use, this patch will add the option (
PJMEDIA_SRTP_DTLS_CHECK_HELLO_ADDR) to enable checking the source address of the "ClientHello" message is coming from the source specified in the SDP.