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

Conversation

ethouris
Copy link
Collaborator

@ethouris ethouris commented Sep 5, 2018

This fixes the problem that is caused by sending back packets in response to a caller's connection request with the IP address that happens to not be the address from which the caller's packets have come. This made the caller confused as it received the response from different IP address than to which it has sent, and recognize this as an attack attempt.

This case happens when you have more than one local IP address that refers to the same network, and your listener socket is bound to "any" wildcard (INADDR_ANY for IPv4 and in6addr_any for IPv6). In this case the source address of the packet being sent through such a socket might be different than the destination address to which a packet was sent to this socket.

The source address is selected by first extracting the network broadcast address from the destination IP address, then the source IP address is chosen through the routing table, as assigned to this network broadcast address. So if there is more than one IP address matching this network broadcast address, only one of them will be selected as the source address and this won't necessarily be the right one.

The assignment of the target IP address to a particular connection session is the SRT logics about which the system has no idea. This fixes then the problem by reading the target IP address from the incoming UDP packets, and then this address is passed down the path in the CPacket object. When the accepted socket is created, this address is recorded in the socket for permanent use, and then it will be passed to the channel when sending.

This whole procedure will not be used in case when the socket is bound to an address explicitly. In this case the source address will always be the address to which the socket was bound, at least as long as the target address matches the network broadcast address of this bound address - but in SRT, as it works on bidirectional UDP channels, this will never be the case because a socket bound to a specific IP address will be only able to receive packets from the IP address that matches the network broadcast address of its bound address (or at least the default routing rules will never reroute other UDP packets to this socket).

Fixes #431

Update 11.08.22

Portability (FreeBSD, Windows, ...) might be an issue here. E.g. see this StackOverflow discussion.

@ethouris
Copy link
Collaborator Author

This PR requires to be tested for whether it adds any performance penalty.

@ethouris ethouris added this to the v.1.3.4 milestone May 24, 2019
@ethouris ethouris added this to Needs review in Development via automation May 24, 2019
configure-data.tcl Outdated Show resolved Hide resolved
@ethouris ethouris removed this from Needs review in Development Dec 12, 2019
@maxsharabayko maxsharabayko modified the milestones: v1.5.0, v1.5.1 Jul 27, 2020
@mbakholdina mbakholdina modified the milestones: v1.5.1, Backlog May 26, 2021
@maxsharabayko maxsharabayko modified the milestones: Backlog, v1.5.2 Dec 12, 2022
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated
@@ -144,6 +148,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 target IP address extraction on listner" ${ENABLE_PKTINFO_DEFAULT})
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
option(ENABLE_PKTINFO "Enable target IP address extraction on listner" ${ENABLE_PKTINFO_DEFAULT})
option(ENABLE_PKTINFO "Enable using IP_PKTINFO to allow the listener extracting the target IP address from incoming packets" ${ENABLE_PKTINFO_DEFAULT})

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe just use OFF here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We may decide in the future to turn it to ON on all supporting platforms, and this way it will be only OFF on platforms that do not support it.

Note also that this IS supported on Windows, just the procedure of sending and receiving UDP packets is using a function that doesn't allow the use of CMSG. To enable that it would have to be reimplemented.

srtcore/channel.cpp Outdated Show resolved Hide resolved
srtcore/channel.cpp Outdated Show resolved Hide resolved
Comment on lines +510 to +511
// XXX Unknown why this has to be off. RETEST.
//::setsockopt(m_iSocket, IPPROTO_IPV6, IPV6_V6ONLY, &off, sizeof(off));
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.

srtcore/queue.h Outdated Show resolved Hide resolved
Comment on lines +470 to +472
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.
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.

Copy link
Collaborator

@stevomatthews stevomatthews left a comment

Choose a reason for hiding this comment

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

Some edits. Please review for technical accuracy.

Co-authored-by: stevomatthews <smatthews@haivision.com>
@maxsharabayko
Copy link
Collaborator

@ethouris Note AppVeyor build failures.

Copy link
Collaborator

@stevomatthews stevomatthews left a comment

Choose a reason for hiding this comment

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

More edits in response to comments.

ethouris and others added 2 commits December 19, 2022 17:10
@maxsharabayko maxsharabayko merged commit 98b1b00 into Haivision:master Dec 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: High Type: Enhancement Indicates new feature requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Connection failed when using virtual IP address
4 participants