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

Added extraction of IP_PKTINFO when reading and setting it when sending back #456

Merged
merged 42 commits into from
Dec 23, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
42 commits
Select commit Hold shift + click to select a range
e825aa6
Added pktinfo retrieval and passing when any-bound socket.
Aug 20, 2018
6eea9f7
Fixed bug when setting data for sending. Added support for IPv6
Aug 21, 2018
5a1e508
Fixed usage of source address during handshake
Aug 21, 2018
95af2e3
Extra refactoring for improving call readiness
Aug 21, 2018
828b403
Some cosmetics
Aug 21, 2018
2ac3a99
Added recording the target address from HS packet if rendezvous
Aug 21, 2018
f57ca6f
Fixed build break on Mac. Centralized system headers. Cleaned up warn…
Sep 7, 2018
be0afb8
Merge branch 'upstream' into dev-pktinfo
Sep 7, 2018
76ad98a
Fixed build break on Windows. Added platform_sys as first header to e…
Sep 7, 2018
fec4909
Fixed new system includes for Mac
Sep 7, 2018
c1b4466
Upgraded to latest upstream
May 23, 2019
bcce38f
Fixed max space for CMSG buffer to handle runtime-only CMSG_SPACE on …
May 24, 2019
e2a1426
Fix: workaround for ref-referring to a static constant
May 24, 2019
bdd7ee2
Moved constant out of static const to avoid linker errors
May 24, 2019
ec349dc
Made pktinfo conditional and disabled on Windows
May 24, 2019
f37b21b
Made separate buffers for sending and receiving. They can be used by …
May 24, 2019
b8be09f
Added lacking initialization
Jun 19, 2019
58cd006
Upgraded from upstream
Jun 19, 2019
21e7740
Fixed conditional-syntax problem
Jun 19, 2019
af2870b
Merged with upstream
Sep 16, 2019
5d8dae1
Removed dead code
Sep 16, 2019
7d9eee0
Merge branch 'master' into dev-pktinfo
Sep 19, 2019
7f8a55b
Turning PKTINFO option default off. Fixed description
Sep 23, 2019
39f9565
Fixed unnecessary extra C++ includes in platform_sys
Oct 2, 2019
f1b2db8
Merge branch 'master' into dev-pktinfo
Oct 2, 2019
ba63b76
Added C++11 version of the against-value-to-function-cast error. Fixe…
Oct 22, 2019
b379292
Updated to latest upstream
Oct 22, 2019
c9cd6f6
Merge branch 'dev-fix-incompat-newcompiler' into dev-pktinfo
Oct 22, 2019
4742039
Fixed cmsg fit replacement structure to have last field last. This ca…
Oct 22, 2019
39c430a
Merge branch 'master' into dev-pktinfo
Oct 28, 2019
52f820d
Updated to latest upstream
Nov 5, 2019
f6ed44f
Post-review fixes
Nov 5, 2019
c25f2d7
Merge branch 'master' into dev-pktinfo
Nov 8, 2019
d44d96f
post-review fixes
Nov 12, 2019
cbae059
Merge branch 'master' into dev-pktinfo
Dec 5, 2019
3ac8c18
Merge branch 'master' into dev-pktinfo
Dec 9, 2019
660a126
Updated to latest upstream (untested)
Nov 30, 2022
b2c8e9a
Merge branch 'master' into dev-pktinfo
Dec 6, 2022
d5d84f3
Post-review fixes, phase 1
Dec 12, 2022
41ad2c0
Apply some suggestions from the doc code review, take 1
ethouris Dec 13, 2022
a087907
Applied some doc suggestions
ethouris Dec 19, 2022
a6f01ed
Fix for handling conditional-in-macro pp spec (Windows)
Dec 19, 2022
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
13 changes: 13 additions & 0 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,9 @@ if (ENABLE_DEBUG)
set(ENABLE_HEAVY_LOGGING_DEFAULT ON)
endif()

# Note that the IP_PKTINFO has a limited portability and may not work on some platforms
# (Windows, FreeBSD, ...).
set (ENABLE_PKTINFO_DEFAULT OFF)

set(ENABLE_STDCXX_SYNC_DEFAULT OFF)
set(ENABLE_MONOTONIC_CLOCK_DEFAULT OFF)
Expand Down Expand Up @@ -144,6 +147,7 @@ option(ENABLE_HEAVY_LOGGING "Should heavy debug logging be enabled" ${ENABLE_HEA
option(ENABLE_HAICRYPT_LOGGING "Should logging in haicrypt be enabled" 0)
option(ENABLE_SHARED "Should libsrt be built as a shared library" ON)
option(ENABLE_STATIC "Should libsrt be built as a static library" ON)
option(ENABLE_PKTINFO "Enable using IP_PKTINFO to allow the listener extracting the target IP address from incoming packets" ${ENABLE_PKTINFO_DEFAULT})
option(ENABLE_RELATIVE_LIBPATH "Should application contain relative library paths, like ../lib" OFF)
option(ENABLE_GETNAMEINFO "In-logs sockaddr-to-string should do rev-dns" OFF)
option(ENABLE_UNITTESTS "Enable unit tests" OFF)
Expand Down Expand Up @@ -711,6 +715,15 @@ if (ENABLE_GETNAMEINFO)
list(APPEND SRT_EXTRA_CFLAGS "-DENABLE_GETNAMEINFO=1")
endif()

if (ENABLE_PKTINFO)
if (WIN32 OR BSD)
message(FATAL_ERROR "PKTINFO is not implemented on Windows or *BSD.")
endif()

list(APPEND SRT_EXTRA_CFLAGS "-DSRT_ENABLE_PKTINFO=1")
endif()


# ENABLE_EXPERIMENTAL_BONDING is deprecated. Use ENABLE_BONDING. ENABLE_EXPERIMENTAL_BONDING is be removed in v1.6.0.
if (ENABLE_EXPERIMENTAL_BONDING)
message(DEPRECATION "ENABLE_EXPERIMENTAL_BONDING is deprecated. Please use ENABLE_BONDING instead.")
Expand Down
1 change: 1 addition & 0 deletions configure-data.tcl
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ set cmake_options {
enable-logging "Should logging be enabled (default: ON)"
enable-heavy-logging "Should heavy debug logging be enabled (default: OFF)"
enable-haicrypt-logging "Should logging in haicrypt be enabled (default: OFF)"
enable-pktinfo "Should pktinfo reading and using be enabled (POSIX only) (default: OFF)"
enable-shared "Should libsrt be built as a shared library (default: ON)"
enable-static "Should libsrt be built as a static library (default: ON)"
enable-relative-libpath "Should applications contain relative library paths, like ../lib (default: OFF)"
Expand Down
51 changes: 49 additions & 2 deletions docs/build/build-options.md
Original file line number Diff line number Diff line change
Expand Up @@ -39,13 +39,14 @@ Option details are given further below.
| [`ENABLE_HEAVY_LOGGING`](#enable_heavy_logging) | 1.3.0 | `BOOL` | OFF | Enables heavy logging instructions in the code that occur often and cover many detailed aspects of library behavior. Default: OFF in release mode. |
| [`ENABLE_INET_PTON`](#enable_inet_pton) | 1.3.2 | `BOOL` | ON | Enables usage of the `inet_pton` function used to resolve the network endpoint name into an IP address. |
| [`ENABLE_LOGGING`](#enable_logging) | 1.2.0 | `BOOL` | ON | Enables normal logging, including errors. |
| [`ENABLE_MONOTONIC_CLOCK`](#enable_monotonic_clock) | 1.4.0 | `BOOL` | ON* | Enforces the use of `clock_gettime` with a monotonic clock that is independent of the currently set time in the system. |
| [`ENABLE_MONOTONIC_CLOCK`](#enable_monotonic_clock) | 1.4.0 | `BOOL` | ON\* | Enforces the use of `clock_gettime` with a monotonic clock that is independent of the currently set time in the system. |
| [`ENABLE_PROFILE`](#enable_profile) | 1.2.0 | `BOOL` | OFF | Enables code instrumentation for profiling (only for GNU-compatible compilers). |
| [`ENABLE_RELATIVE_LIBPATH`](#enable_relative_libpath) | 1.3.2 | `BOOL` | OFF | Enables adding a relative path to a library for linking against a shared SRT library by reaching out to a sibling directory. |
| [`ENABLE_SHARED`](#enable_shared--enable_static) | 1.2.0 | `BOOL` | ON | Enables building SRT as a shared library. |
| [`ENABLE_SHOW_PROJECT_CONFIG`](#enable_show_project_config) | 1.5.0 | `BOOL` | OFF | When ON, the project configuration is displayed at the end of the CMake Configuration Step. |
| [`ENABLE_STATIC`](#enable_shared--enable_static) | 1.3.0 | `BOOL` | ON | Enables building SRT as a static library. |
| [`ENABLE_STDCXX_SYNC`](#enable_stdcxx_sync) | 1.4.2 | `BOOL` | ON* | Enables the standard C++11 `thread` and `chrono` libraries to be used by SRT instead of the `pthreads`. |
| [`ENABLE_STDCXX_SYNC`](#enable_stdcxx_sync) | 1.4.2 | `BOOL` | ON\* | Enables the standard C++11 `thread` and `chrono` libraries to be used by SRT instead of the `pthreads`. |
| [`ENABLE_PKTINFO`](#enable_pktinfo) | 1.5.2 | `BOOL` | OFF\* | Enables using `IP_PKTINFO` to allow the listener extracting the target IP address from incoming packets |
| [`ENABLE_TESTING`](#enable_testing) | 1.3.0 | `BOOL` | OFF | Enables compiling of developer testing applications (`srt-test-live`, etc.). |
| [`ENABLE_THREAD_CHECK`](#enable_thread_check) | 1.3.0 | `BOOL` | OFF | Enables `#include <threadcheck.h>`, which implements `THREAD_*` macros" to support better thread debugging. |
| [`ENABLE_UNITTESTS`](#enable_unittests) | 1.3.2 | `BOOL` | OFF | Enables building unit tests. |
Expand Down Expand Up @@ -421,6 +422,52 @@ to use either of those (`ENABLE_STDCXX_SYNC` excludes `ENABLE_MONOTONIC_CLOCK`).
When ON, this option enables the standard C++ `thread` and `chrono` libraries
(available since C++11) to be used by SRT instead of the `pthreads` libraries.

#### ENABLE_PKTINFO
**`--enable-pktinfo`** (default: OFF)

This allows the use of the `IP_PKTINFO` control message to extract the true target IP
address from the incoming UDP packets to a listener bound to "any" address. The "any"
address is defined in IPv4 as 0.0.0.0 (`INADDR_ANY`) and in IPv6 as :: (`in6addr_any`).
Applications usually implement it by clearing the `sockaddr*` structure and only setting
the port number. This true address can then be used to override the source IP address
when sending packets to the peer. This solves the problem where routing rules
in an agent's host send a packet using a different source IP address than the target
IP address in the UDP packet from the peer. The peer will reject such a packet as a
suspected man-in-the-middle (MITM) attempt, leading to a connection failure.

This problem has been observed where an agent's host has at least
two IP addresses that share the same broadcast prefix, and it is being contacted
by a peer using an address other than the first one. For example:

The host has set the following local IP addresses:

(1) 192.168.10.5 - routing to 192.168.10.1
(2) 10.10.5.10 - routing to 10.10.5.1
(3) 10.10.5.20 - routing to 10.10.5.1

For all of them the netmask is 255.255.255.0, which means that the second
and third IP addresses share the same broadcast prefix (10.10.5.0).
The problem occurs when an agent running on this host is contacted
by a peer using the address 10.10.5.20.

In such a case the source address set in the UDP packet being sent will
always be the first of these addresses (10.10.5.10), which will be different
from the one from which the packet is actually being sent (10.10.5.20).
The peer will then reject such a packet because its source address is different.

This problem occurs only with listener sockets bound to "any" address - when it
is bound to a specific IP address, this will be always set as source address
for outgoing packets.

By enabling this feature SRT mitigates the problem by first reading the real
target IP address from the incoming handshake packet, and then forcing this
specific address to be set in the source address field of every UDP packet sent
from this socket. This behavior is also consistent with TCP.

Note that this feature is available only on certain platforms. Notably the BSD
systems are known to not provide this feature and the current implementation
doesn't support this feature on Windows systems.
Comment on lines +467 to +469
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Note that this feature is available only on certain platforms. Notably the BSD
systems are known to not provide this feature and the current implementation
doesn't support this feature on Windows systems.
Note that this feature is not supported on BSD or Windows systems.

Copy link
Collaborator Author

@ethouris ethouris Dec 13, 2022

Choose a reason for hiding this comment

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

By treating these systems differently in the description I wanted to highlight the fact that in BSD systems this feature really isn't implemented and there's no known way to implement it, while in case of Windows it is possible, but the API used currently for sending and receiving UDP packets does not allow the use of CMSG messages. Such an API exists, so for Windows this would only require changing the API functions used for sending/receiving (use WSASendMsg instead of WSASendTo etc.).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Note that this feature is available only on certain platforms. Notably the BSD
systems are known to not provide this feature and the current implementation
doesn't support this feature on Windows systems.
Note that this feature is not supported on BSD systems. On Windows systems it is possible, but the API used currently for sending and receiving UDP packets does not allow the use of CMSG messages. Such an API exists, so for Windows this would only require changing the API functions used for sending/receiving (use `WSASendMsg` instead of `WSASendTo` etc.).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well, I wouldn't go into such details - maybe this way:

Note that this feature is not supported on BSD systems (not possible due to lacking IP_PKTINFO feature). On Windows it is theoretically possible, but currently not implemented.



#### ENABLE_TESTING
**`--enable-testing`** (default: OFF)
Expand Down
110 changes: 103 additions & 7 deletions srtcore/channel.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,24 @@ static int set_cloexec(int fd, int set)

srt::CChannel::CChannel()
: m_iSocket(INVALID_SOCKET)
#ifdef SRT_ENABLE_PKTINFO
, m_bBindMasked(true)
#endif
{
#ifdef SRT_ENABLE_PKTINFO
// Do the check for ancillary data buffer size, kinda assertion
static const size_t CMSG_MAX_SPACE = sizeof (CMSGNodeAlike);

if (CMSG_MAX_SPACE < CMSG_SPACE(sizeof(in_pktinfo))
|| CMSG_MAX_SPACE < CMSG_SPACE(sizeof(in6_pktinfo)))
{
LOGC(kmlog.Fatal, log << "Size of CMSG_MAX_SPACE="
<< CMSG_MAX_SPACE << " too short for cmsg "
<< CMSG_SPACE(sizeof(in_pktinfo)) << ", "
<< CMSG_SPACE(sizeof(in6_pktinfo)) << " - PLEASE FIX");
throw CUDTException(MJ_SETUP, MN_NONE, 0);
}
#endif
}

srt::CChannel::~CChannel() {}
Expand Down Expand Up @@ -207,6 +224,9 @@ void srt::CChannel::open(const sockaddr_any& addr)
throw CUDTException(MJ_SETUP, MN_NORES, NET_ERROR);

m_BindAddr = addr;
#ifdef SRT_ENABLE_PKTINFO
m_bBindMasked = m_BindAddr.isany();
#endif
LOGC(kmlog.Debug, log << "CHANNEL: Bound to local address: " << m_BindAddr.str());

setUDPSockOpt();
Expand Down Expand Up @@ -247,6 +267,12 @@ void srt::CChannel::open(int family)
}
m_BindAddr = sockaddr_any(res->ai_addr, (sockaddr_any::len_t)res->ai_addrlen);

#ifdef SRT_ENABLE_PKTINFO
// We know that this is intentionally bound now to "any",
// so the requester-destination address must be remembered and passed.
m_bBindMasked = true;
#endif

::freeaddrinfo(res);

HLOGC(kmlog.Debug, log << "CHANNEL: Bound to local address: " << m_BindAddr.str());
Expand Down Expand Up @@ -472,6 +498,19 @@ void srt::CChannel::setUDPSockOpt()
if (0 != ::setsockopt(m_iSocket, SOL_SOCKET, SO_RCVTIMEO, (char*)&tv, sizeof(timeval)))
throw CUDTException(MJ_SETUP, MN_NORES, NET_ERROR);
#endif

#ifdef SRT_ENABLE_PKTINFO
if (m_bBindMasked)
{
HLOGP(kmlog.Debug, "Socket bound to ANY - setting PKTINFO for address retrieval");
const int on = 1, off SRT_ATR_UNUSED = 0;
::setsockopt(m_iSocket, IPPROTO_IP, IP_PKTINFO, (char*)&on, sizeof(on));
::setsockopt(m_iSocket, IPPROTO_IPV6, IPV6_RECVPKTINFO, &on, sizeof(on));

// XXX Unknown why this has to be off. RETEST.
//::setsockopt(m_iSocket, IPPROTO_IPV6, IPV6_V6ONLY, &off, sizeof(off));
Comment on lines +510 to +511
Copy link
Collaborator

Choose a reason for hiding this comment

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

?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I found this line in the old implementation that was there years ago. Unsure what this was about. Left for a case when someone finds a problem on IPv6 using it.

}
#endif
}

void srt::CChannel::close() const
Expand Down Expand Up @@ -610,11 +649,19 @@ void srt::CChannel::getPeerAddr(sockaddr_any& w_addr) const
w_addr.len = namelen;
}

int srt::CChannel::sendto(const sockaddr_any& addr, CPacket& packet) const
int srt::CChannel::sendto(const sockaddr_any& addr, CPacket& packet, const sockaddr_any& source_addr SRT_ATR_UNUSED) const
{
HLOGC(kslog.Debug,
log << "CChannel::sendto: SENDING NOW DST=" << addr.str() << " target=@" << packet.m_iID
<< " size=" << packet.getLength() << " pkt.ts=" << packet.m_iTimeStamp << " " << packet.Info());
#if ENABLE_HEAVY_LOGGING
ostringstream dsrc;
#ifdef SRT_ENABLE_PKTINFO
dsrc << " sourceIP=" << (m_bBindMasked && !source_addr.isany() ? source_addr.str() : "default");
#endif

LOGC(kslog.Debug,
log << "CChannel::sendto: SENDING NOW DST=" << addr.str() << " target=@" << packet.m_iID
<< " size=" << packet.getLength() << " pkt.ts=" << packet.m_iTimeStamp
<< dsrc.str() << " " << packet.Info());
#endif

#ifdef SRT_TEST_FAKE_LOSS

Expand Down Expand Up @@ -683,8 +730,28 @@ int srt::CChannel::sendto(const sockaddr_any& addr, CPacket& packet) const
mh.msg_namelen = addr.size();
mh.msg_iov = (iovec*)packet.m_PacketVector;
mh.msg_iovlen = 2;
mh.msg_control = NULL;
mh.msg_controllen = 0;
bool have_set_src = false;

#ifdef SRT_ENABLE_PKTINFO
if (m_bBindMasked && !source_addr.isany())
{
if (!setSourceAddress(mh, source_addr))
{
LOGC(kslog.Error, log << "CChannel::setSourceAddress: source address invalid family #" << source_addr.family() << ", NOT setting.");
}
else
{
HLOGC(kslog.Debug, log << "CChannel::setSourceAddress: setting as " << source_addr.str());
have_set_src = true;
}
}
#endif

if (!have_set_src)
{
mh.msg_control = NULL;
mh.msg_controllen = 0;
}
mh.msg_flags = 0;

const int res = ::sendmsg(m_iSocket, &mh, 0);
Expand Down Expand Up @@ -725,15 +792,33 @@ srt::EReadStatus srt::CChannel::recvfrom(sockaddr_any& w_addr, CPacket& w_packet
}

#ifndef _WIN32
msghdr mh; // will not be used on failure

if (select_ret > 0)
{
msghdr mh;
mh.msg_name = (w_addr.get());
mh.msg_namelen = w_addr.size();
mh.msg_iov = (w_packet.m_PacketVector);
mh.msg_iovlen = 2;

// Default
mh.msg_control = NULL;
mh.msg_controllen = 0;

#ifdef SRT_ENABLE_PKTINFO
// Without m_bBindMasked, we don't need ancillary data - the source
// address will always be the bound address.
if (m_bBindMasked)
{
// Extract the destination IP address from the ancillary
// data. This might be interesting for the connection to
// know to which address the packet should be sent back during
// the handshake and then addressed when sending during connection.
mh.msg_control = (m_acCmsgRecvBuffer);
mh.msg_controllen = sizeof m_acCmsgRecvBuffer;
}
#endif

mh.msg_flags = 0;

recv_size = ::recvmsg(m_iSocket, (&mh), 0);
Expand Down Expand Up @@ -779,6 +864,17 @@ srt::EReadStatus srt::CChannel::recvfrom(sockaddr_any& w_addr, CPacket& w_packet
goto Return_error;
}

#ifdef SRT_ENABLE_PKTINFO
if (m_bBindMasked)
{
// Extract the address. Set it explicitly; if this returns address that isany(),
// it will simply set this on the packet so that it behaves as if nothing was
// extracted (it will "fail the old way").
w_packet.m_DestAddr = getTargetAddress(mh);
HLOGC(krlog.Debug, log << CONID() << "(sys)recvmsg: ANY BOUND, retrieved DEST ADDR: " << w_packet.m_DestAddr.str());
}
#endif

#else
// XXX REFACTORING NEEDED!
// This procedure uses the WSARecvFrom function that just reads
Expand Down
Loading