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

support juice_bind_stun to reflect stun requests from unbound client. #248

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions include/juice/juice.h
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,16 @@ typedef void (*juice_cb_gathering_done_t)(juice_agent_t *agent, void *user_ptr);
typedef void (*juice_cb_recv_t)(juice_agent_t *agent, const char *data, size_t size,
void *user_ptr);

typedef struct juice_mux_incoming {
Copy link
Owner

Choose a reason for hiding this comment

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

Since this does not actually represent incoming connections in the end (for instance in the case of closed connections), I think it should be renamed for clarity. For instance juice_mux_binding_request.

const char *local_ufrag;
const char *remote_ufrag;

const char *address;
uint16_t port;
} juice_mux_incoming_t;

typedef void (*juice_cb_mux_incoming_t)(const juice_mux_incoming_t *info, void *user_ptr);

typedef struct juice_turn_server {
const char *host;
const char *username;
Expand Down Expand Up @@ -118,6 +128,8 @@ JUICE_EXPORT int juice_get_selected_addresses(juice_agent_t *agent, char *local,
char *remote, size_t remote_size);
JUICE_EXPORT int juice_set_local_ice_attributes(juice_agent_t *agent, const char *ufrag, const char *pwd);
JUICE_EXPORT const char *juice_state_to_string(juice_state_t state);
JUICE_EXPORT int juice_mux_listen(const char *bind_address, int local_port, juice_cb_mux_incoming_t cb, void *user_ptr);
JUICE_EXPORT int juice_mux_stop_listen(const char *bind_address, int local_port);

// ICE server

Expand Down
58 changes: 57 additions & 1 deletion src/conn.c
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ static void release_registry(conn_mode_entry_t *entry) {

// registry must be locked

if (registry->agents_count == 0) {
if (registry->agents_count == 0 && registry->cb_mux_incoming == NULL) {
JLOG_DEBUG("No connection left, destroying connections registry");
mutex_unlock(&registry->mutex);

Expand Down Expand Up @@ -247,3 +247,59 @@ int conn_get_addrs(juice_agent_t *agent, addr_record_t *records, size_t size) {

return get_mode_entry(agent)->get_addrs_func(agent, records, size);
}

int juice_mux_stop_listen(const char *bind_address, int local_port) {
Copy link
Owner

Choose a reason for hiding this comment

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

If juice_mux_stop_listen() now has the same bind_address and local_port parameters as juice_mux_listen(), why not simply call juice_mux_listen(bind_address, local_port, NULL) to stop listening? It would also permit multiple ports in the future, while being less clumsy that those two useless parameters.

(void)bind_address;
(void)local_port;

conn_mode_entry_t *entry = &mode_entries[JUICE_CONCURRENCY_MODE_MUX];

mutex_lock(&entry->mutex);

conn_registry_t *registry = entry->registry;
if (!registry) {
mutex_unlock(&entry->mutex);
return -1;
}

mutex_lock(&registry->mutex);

registry->cb_mux_incoming = NULL;
registry->mux_incoming_user_ptr = NULL;
conn_mux_interrupt_registry(registry);

release_registry(entry);

mutex_unlock(&entry->mutex);

return 0;
}

int juice_mux_listen(const char *bind_address, int local_port, juice_cb_mux_incoming_t cb, void *user_ptr)
{
conn_mode_entry_t *entry = &mode_entries[JUICE_CONCURRENCY_MODE_MUX];

udp_socket_config_t config;
config.bind_address = bind_address;
config.port_begin = config.port_end = local_port;

mutex_lock(&entry->mutex);

if (entry->registry) {
mutex_unlock(&entry->mutex);
JLOG_DEBUG("juice_mux_listen needs to be called before establishing any mux connection.");
return -1;
xicilion marked this conversation as resolved.
Show resolved Hide resolved
}

conn_registry_t *registry = acquire_registry(entry, &config);
mutex_unlock(&entry->mutex);

if (!registry)
return -2;

registry->cb_mux_incoming = cb;
registry->mux_incoming_user_ptr = user_ptr;
mutex_unlock(&registry->mutex);

return 0;
}
2 changes: 2 additions & 0 deletions src/conn.h
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ typedef struct conn_registry {
juice_agent_t **agents;
int agents_size;
int agents_count;
juice_cb_mux_incoming_t cb_mux_incoming;
void *mux_incoming_user_ptr;
Comment on lines +33 to +34
Copy link
Owner

Choose a reason for hiding this comment

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

Since they are specific to the mux implementation, they should be moved to struct registry_impl in conn_mux.c instead, and you can then drop the mux_ prefix in the names.

} conn_registry_t;

int conn_create(juice_agent_t *agent, udp_socket_config_t *config);
Expand Down
41 changes: 33 additions & 8 deletions src/conn_mux.c
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,8 @@ int conn_mux_prepare(conn_registry_t *registry, struct pollfd *pfd, timestamp_t
}

int count = registry->agents_count;
if (registry->cb_mux_incoming)
++count;
mutex_unlock(&registry->mutex);
return count;
}
Expand Down Expand Up @@ -295,6 +297,25 @@ static juice_agent_t *lookup_agent(conn_registry_t *registry, char *buf, size_t
}
}

if (registry->cb_mux_incoming) {
JLOG_DEBUG("Found STUN request with unknown ICE ufrag");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found an issue here.

When connecting to the server using a browser and the server closes the peer connection, the browser will continue to send STUN messages, and the incoming event will be triggered again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to the RFC document:

[7.1.2](https://www.rfc-editor.org/rfc/rfc8445.html#section-7.1.2).  USE-CANDIDATE

   The controlling agent MUST include the USE-CANDIDATE attribute in
   order to nominate a candidate pair ([Section 8.1.1](https://www.rfc-editor.org/rfc/rfc8445.html#section-8.1.1)).  The controlled
   agent MUST NOT include the USE-CANDIDATE attribute in a Binding
   request.

An agent must not include the USE-CANDIDATE attribute when sending a binding request. I tested this in practice, the binding request message received has use_candidate=0. After the connection is closed, the browser continues to send stun messages with use_candidate=1.

I will proceed to test further based on this strategy.

Copy link
Contributor Author

@xicilion xicilion Jul 31, 2024

Choose a reason for hiding this comment

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

2024-07-31 23:04:44.369 VERB  [387904] [rtc::impl::IceTransport::LogCallback@372] juice: conn.c:125: 0 connection left
2024-07-31 23:04:44.369 VERB  [387904] [rtc::impl::IceTransport::LogCallback@372] juice: agent.c:192: Destroyed agent
2024-07-31 23:04:44.369 VERB  [387845] [rtc::impl::IceTransport::LogCallback@372] juice: conn_mux.c:449: Leaving poll
2024-07-31 23:04:44.369 VERB  [387845] [rtc::impl::IceTransport::LogCallback@372] juice: conn_mux.c:408: Receiving datagram
2024-07-31 23:04:44.369 VERB  [387845] [rtc::impl::IceTransport::LogCallback@372] juice: conn_mux.c:417: No more datagrams to receive
2024-07-31 23:04:44.369 VERB  [387845] [rtc::impl::IceTransport::LogCallback@372] juice: conn_mux.c:447: Entering poll for 60000 ms
2024-07-31 23:04:44.369 VERB  [387845] [rtc::impl::IceTransport::LogCallback@372] juice: conn_mux.c:449: Leaving poll
2024-07-31 23:04:44.369 VERB  [387845] [rtc::impl::IceTransport::LogCallback@372] juice: conn_mux.c:408: Receiving datagram
2024-07-31 23:04:44.369 VERB  [387845] [rtc::impl::IceTransport::LogCallback@372] juice: conn_mux.c:363: Demultiplexing incoming datagram from 127.0.0.1:54664
2024-07-31 23:04:44.369 VERB  [387845] [rtc::impl::IceTransport::LogCallback@372] juice: conn_mux.c:255: Looking up agent from address
2024-07-31 23:04:44.369 VERB  [387845] [rtc::impl::IceTransport::LogCallback@372] juice: stun.c:485: Not a STUN message: magic number invalid
2024-07-31 23:04:44.369 INFO  [387845] [rtc::impl::IceTransport::LogCallback@372] juice: conn_mux.c:266: Got non-STUN message from unknown source address
2024-07-31 23:04:44.369 VERB  [387845] [rtc::impl::IceTransport::LogCallback@372] juice: conn_mux.c:368: Agent not found for incoming datagram, dropping
2024-07-31 23:04:44.369 VERB  [387845] [rtc::impl::IceTransport::LogCallback@372] juice: conn_mux.c:408: Receiving datagram
2024-07-31 23:04:44.369 VERB  [387845] [rtc::impl::IceTransport::LogCallback@372] juice: conn_mux.c:417: No more datagrams to receive
2024-07-31 23:04:44.369 VERB  [387845] [rtc::impl::IceTransport::LogCallback@372] juice: conn_mux.c:447: Entering poll for 60000 ms
2024-07-31 23:04:44.553 VERB  [387845] [rtc::impl::IceTransport::LogCallback@372] juice: conn_mux.c:449: Leaving poll
2024-07-31 23:04:44.553 VERB  [387845] [rtc::impl::IceTransport::LogCallback@372] juice: conn_mux.c:408: Receiving datagram
2024-07-31 23:04:44.553 VERB  [387845] [rtc::impl::IceTransport::LogCallback@372] juice: conn_mux.c:363: Demultiplexing incoming datagram from 127.0.0.1:54664
2024-07-31 23:04:44.553 VERB  [387845] [rtc::impl::IceTransport::LogCallback@372] juice: conn_mux.c:255: Looking up agent from address
2024-07-31 23:04:44.553 VERB  [387845] [rtc::impl::IceTransport::LogCallback@372] juice: conn_mux.c:270: Looking up agent from STUN message content
2024-07-31 23:04:44.553 VERB  [387845] [rtc::impl::IceTransport::LogCallback@372] juice: stun.c:528: Reading STUN message, class=0x0, method=0x1
2024-07-31 23:04:44.553 VERB  [387845] [rtc::impl::IceTransport::LogCallback@372] juice: stun.c:622: Reading attribute 0x6, length=67
2024-07-31 23:04:44.553 VERB  [387845] [rtc::impl::IceTransport::LogCallback@372] juice: stun.c:700: Reading username
2024-07-31 23:04:44.553 VERB  [387845] [rtc::impl::IceTransport::LogCallback@372] juice: stun.c:707: Got username: libp2p+webrtc+v1/5fhhLR3XJZHejuox:libp2p+webrtc+v1/5fhhLR3XJZHejuox
2024-07-31 23:04:44.553 VERB  [387845] [rtc::impl::IceTransport::LogCallback@372] juice: stun.c:622: Reading attribute 0xC057, length=4
2024-07-31 23:04:44.553 VERB  [387845] [rtc::impl::IceTransport::LogCallback@372] juice: stun.c:1014: Ignoring unknown optional STUN attribute type 0xC057
2024-07-31 23:04:44.553 VERB  [387845] [rtc::impl::IceTransport::LogCallback@372] juice: stun.c:622: Reading attribute 0x802A, length=8
2024-07-31 23:04:44.553 VERB  [387845] [rtc::impl::IceTransport::LogCallback@372] juice: stun.c:906: Found ICE controlling attribute
2024-07-31 23:04:44.553 VERB  [387845] [rtc::impl::IceTransport::LogCallback@372] juice: stun.c:622: Reading attribute 0x25, length=0
2024-07-31 23:04:44.553 VERB  [387845] [rtc::impl::IceTransport::LogCallback@372] juice: stun.c:901: Found use candidate flag
2024-07-31 23:04:44.553 VERB  [387845] [rtc::impl::IceTransport::LogCallback@372] juice: stun.c:622: Reading attribute 0x24, length=4
2024-07-31 23:04:44.553 VERB  [387845] [rtc::impl::IceTransport::LogCallback@372] juice: stun.c:891: Reading priority
2024-07-31 23:04:44.553 VERB  [387845] [rtc::impl::IceTransport::LogCallback@372] juice: stun.c:897: Got priority: 1845501695
2024-07-31 23:04:44.553 VERB  [387845] [rtc::impl::IceTransport::LogCallback@372] juice: stun.c:622: Reading attribute 0x8, length=20
2024-07-31 23:04:44.553 VERB  [387845] [rtc::impl::IceTransport::LogCallback@372] juice: stun.c:711: Reading message integrity
2024-07-31 23:04:44.553 VERB  [387845] [rtc::impl::IceTransport::LogCallback@372] juice: stun.c:622: Reading attribute 0x8028, length=4
2024-07-31 23:04:44.553 VERB  [387845] [rtc::impl::IceTransport::LogCallback@372] juice: stun.c:729: Reading fingerprint
2024-07-31 23:04:44.553 VERB  [387845] [rtc::impl::IceTransport::LogCallback@372] juice: stun.c:745: STUN fingerprint check succeeded
2024-07-31 23:04:44.553 VERB  [387845] [rtc::impl::IceTransport::LogCallback@372] juice: stun.c:545: Finished reading STUN attributes
2024-07-31 23:04:44.553 VERB  [387845] [rtc::impl::IceTransport::LogCallback@372] juice: conn_mux.c:368: Agent not found for incoming datagram, dropping

===========================================================================

2024-07-31 23:04:49.570 VERB  [387845] [rtc::impl::IceTransport::LogCallback@372] juice: conn_mux.c:408: Receiving datagram
2024-07-31 23:04:49.570 VERB  [387845] [rtc::impl::IceTransport::LogCallback@372] juice: conn_mux.c:363: Demultiplexing incoming datagram from 127.0.0.1:54664
2024-07-31 23:04:49.570 VERB  [387845] [rtc::impl::IceTransport::LogCallback@372] juice: conn_mux.c:255: Looking up agent from address
2024-07-31 23:04:49.570 VERB  [387845] [rtc::impl::IceTransport::LogCallback@372] juice: conn_mux.c:270: Looking up agent from STUN message content
2024-07-31 23:04:49.570 VERB  [387845] [rtc::impl::IceTransport::LogCallback@372] juice: stun.c:528: Reading STUN message, class=0x0, method=0x1
2024-07-31 23:04:49.570 VERB  [387845] [rtc::impl::IceTransport::LogCallback@372] juice: stun.c:622: Reading attribute 0x6, length=67
2024-07-31 23:04:49.570 VERB  [387845] [rtc::impl::IceTransport::LogCallback@372] juice: stun.c:700: Reading username
2024-07-31 23:04:49.570 VERB  [387845] [rtc::impl::IceTransport::LogCallback@372] juice: stun.c:707: Got username: libp2p+webrtc+v1/5fhhLR3XJZHejuox:libp2p+webrtc+v1/5fhhLR3XJZHejuox
2024-07-31 23:04:49.570 VERB  [387845] [rtc::impl::IceTransport::LogCallback@372] juice: stun.c:622: Reading attribute 0xC057, length=4
2024-07-31 23:04:49.570 VERB  [387845] [rtc::impl::IceTransport::LogCallback@372] juice: stun.c:1014: Ignoring unknown optional STUN attribute type 0xC057
2024-07-31 23:04:49.570 VERB  [387845] [rtc::impl::IceTransport::LogCallback@372] juice: stun.c:622: Reading attribute 0x802A, length=8
2024-07-31 23:04:49.570 VERB  [387845] [rtc::impl::IceTransport::LogCallback@372] juice: stun.c:906: Found ICE controlling attribute
2024-07-31 23:04:49.570 VERB  [387845] [rtc::impl::IceTransport::LogCallback@372] juice: stun.c:622: Reading attribute 0x24, length=4
2024-07-31 23:04:49.570 VERB  [387845] [rtc::impl::IceTransport::LogCallback@372] juice: stun.c:891: Reading priority
2024-07-31 23:04:49.570 VERB  [387845] [rtc::impl::IceTransport::LogCallback@372] juice: stun.c:897: Got priority: 1845501695
2024-07-31 23:04:49.570 VERB  [387845] [rtc::impl::IceTransport::LogCallback@372] juice: stun.c:622: Reading attribute 0x8, length=20
2024-07-31 23:04:49.570 VERB  [387845] [rtc::impl::IceTransport::LogCallback@372] juice: stun.c:711: Reading message integrity
2024-07-31 23:04:49.570 VERB  [387845] [rtc::impl::IceTransport::LogCallback@372] juice: stun.c:622: Reading attribute 0x8028, length=4
2024-07-31 23:04:49.570 VERB  [387845] [rtc::impl::IceTransport::LogCallback@372] juice: stun.c:729: Reading fingerprint
2024-07-31 23:04:49.570 VERB  [387845] [rtc::impl::IceTransport::LogCallback@372] juice: stun.c:745: STUN fingerprint check succeeded
2024-07-31 23:04:49.570 VERB  [387845] [rtc::impl::IceTransport::LogCallback@372] juice: stun.c:545: Finished reading STUN attributes
2024-07-31 23:04:49.570 VERB  [387845] [rtc::impl::IceTransport::LogCallback@372] juice: conn_mux.c:307: Found STUN request with unknown ICE ufrag
2024-07-31 23:04:49.570 VERB  [387845] [rtc::impl::IceTransport::LogCallback@372] juice: conn_mux.c:368: Agent not found for incoming datagram, dropping
2024-07-31 23:04:49.570 VERB  [387845] [rtc::impl::IceTransport::LogCallback@372] juice: conn_mux.c:408: Receiving datagram
2024-07-31 23:04:49.570 VERB  [387845] [rtc::impl::IceTransport::LogCallback@372] juice: conn_mux.c:417: No more datagrams to receive
2024-07-31 23:04:49.570 VERB  [387845] [rtc::impl::IceTransport::LogCallback@372] juice: conn_mux.c:447: Entering poll for 60000 ms

After successfully intercepting dozens of messages, the browser still stubbornly sent a binding request message with USE-CANDIDATE set to 0.

Copy link
Contributor Author

@xicilion xicilion Jul 31, 2024

Choose a reason for hiding this comment

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

I examined numerous attributes in the STUN message, but did not identify any additional characteristics that could be utilized to detect the binding request after the connection is terminated.

@paullouisageneau need your help, do you have any suggestions?

Copy link
Owner

Choose a reason for hiding this comment

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

An agent must not include the USE-CANDIDATE attribute when sending a binding request.

I think you misunderstood what USE-CANDIDATE means. The RFC says "The controlled agent MUST NOT include the USE-CANDIDATE attribute in a Binding request". Only the controlling agent includes it to nominate a candidate pair (i.e. to tell the controlled agent which pair to use), otherwise it's not present.

I examined numerous attributes in the STUN message, but did not identify any additional characteristics that could be utilized to detect the binding request after the connection is terminated.

To my knowledge there is no way to know the connection state from STUN attributes, since the ICE layer is unaware of upper layers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I have already canceled this commit. I cached the information of the just closed RTCPeerConnection at the business layer for a while, and blocked the same request from being accepted.

char host[ADDR_MAX_NUMERICHOST_LEN];
if (getnameinfo((const struct sockaddr *)&src->addr, src->len, host, ADDR_MAX_NUMERICHOST_LEN, NULL, 0, NI_NUMERICHOST)) {
JLOG_ERROR("getnameinfo failed, errno=%d", sockerrno);
return NULL;
}

juice_mux_incoming_t incoming_info;

incoming_info.local_ufrag = local_ufrag;
incoming_info.remote_ufrag = separator + 1;
incoming_info.address = host;
incoming_info.port = addr_get_port((struct sockaddr *)src);

registry->cb_mux_incoming(&incoming_info, registry->mux_incoming_user_ptr);

return NULL;
}
} else {
if (!STUN_IS_RESPONSE(msg.msg_class)) {
JLOG_INFO("Got unexpected STUN message from unknown source address");
Expand Down Expand Up @@ -479,14 +500,7 @@ void conn_mux_unlock(juice_agent_t *agent) {
mutex_unlock(&registry->mutex);
}

int conn_mux_interrupt(juice_agent_t *agent) {
conn_impl_t *conn_impl = agent->conn_impl;
conn_registry_t *registry = conn_impl->registry;

mutex_lock(&registry->mutex);
conn_impl->next_timestamp = current_timestamp();
mutex_unlock(&registry->mutex);

int conn_mux_interrupt_registry(conn_registry_t *registry) {
JLOG_VERBOSE("Interrupting connections thread");

registry_impl_t *registry_impl = registry->impl;
Expand All @@ -502,6 +516,17 @@ int conn_mux_interrupt(juice_agent_t *agent) {
return 0;
}

int conn_mux_interrupt(juice_agent_t *agent) {
conn_impl_t *conn_impl = agent->conn_impl;
conn_registry_t *registry = conn_impl->registry;

mutex_lock(&registry->mutex);
conn_impl->next_timestamp = current_timestamp();
mutex_unlock(&registry->mutex);

return conn_mux_interrupt_registry(registry);
}

int conn_mux_send(juice_agent_t *agent, const addr_record_t *dst, const char *data, size_t size,
int ds) {
conn_impl_t *conn_impl = agent->conn_impl;
Expand Down
1 change: 1 addition & 0 deletions src/conn_mux.h
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ int conn_mux_init(juice_agent_t *agent, conn_registry_t *registry, udp_socket_co
void conn_mux_cleanup(juice_agent_t *agent);
void conn_mux_lock(juice_agent_t *agent);
void conn_mux_unlock(juice_agent_t *agent);
int conn_mux_interrupt_registry(conn_registry_t *registry);
int conn_mux_interrupt(juice_agent_t *agent);
int conn_mux_send(juice_agent_t *agent, const addr_record_t *dst, const char *data, size_t size,
int ds);
Expand Down
Loading