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 check to prevent applications from connecting to self #377

Open
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

ethouris
Copy link
Collaborator

@ethouris ethouris commented May 9, 2018

There's a problem when the Rendezvous connection mistakenly specifies a self address. In this case the connection facility gets confused by getting back packets that the agent himself has sent, taking these sent packets as if they came from the peer. This leads to misleading errors that result from interpreting packets that the agent shouldn't have received in the first place.

This fix causes error when srt_connect is called with target address that:

  • matches the adapter address, if the adapter IP was configured
  • matches any of the local machine IP addresses, if the socket was bound to INADDR_ANY

@ethouris ethouris changed the title Added check code for attempting rendezvous to self Added check to prevent applications from connecting to self May 17, 2018
CMakeLists.txt Outdated
@@ -643,6 +642,10 @@ macro(srt_make_application name)
set_target_properties(${name} PROPERTIES COMPILE_FLAGS "${CFLAGS_CXX_STANDARD} ${EXTRA_stransmit}" ${FORCE_RPATH})

target_link_libraries(${name} ${srt_link_library})

if (MICROSOFT)
target_link_libraries(${name} Iphlpapi)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Won't this add iphlpapi.dll as dependency for all apps, not just "testing" which the only place where it is currently needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This check is predicted to be done by all applications that make SRT connections, so this would be simply a complicated conditional slalom, to be withdrawn later anyway.

Copy link
Collaborator

Choose a reason for hiding this comment

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

No need for complicated slalom, just add a quick and dirty block, maybe a parameter to 'srt_make_application', that will be easy to remove later if and when we decide to add iphlpapi.dll dependancy to all apps.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This will be added to EVERY application, as long as explicitly requested. For Linux & Mac it will be always done (needs no extra features from the system). For Windows, it requires extra ENABLE_CONSELF_CHECK_WIN32 flag as it implies linking against extra Iphlpapi.lib.

@ethouris
Copy link
Collaborator Author

Current status: The current solution on Windows uses iphlpapi library which is deemed unreliable. For Windows there must be found some alternative method to list the IP addresses assigned to local NICs. Possible further steps:

  • Keep this PR in OnHold state until the solution for Windows is found
  • Remove the implementation on Windows here and make a new PR to provide the solution for this on Windows.

@ethouris ethouris added Priority: Low Type: Maintenance Work required to maintain or clean up the code labels Jan 21, 2019
@ethouris ethouris added this to the v.1.3.4 milestone May 23, 2019
@maxsharabayko maxsharabayko modified the milestones: v1.3.4, v1.4.0 Aug 14, 2019
ethouris pushed a commit to ethouris/srt that referenced this pull request Aug 14, 2019
@ethouris ethouris requested a review from rndi August 22, 2019 06:52
@ethouris
Copy link
Collaborator Author

ethouris commented Sep 3, 2019

Current status: A new flag has been added, which allows this prevention to be done in the applications also on Windows, as it requires an extra Iphlpapi.lib dependency. On Linux and BSD this is always done.

@ethouris ethouris added Status: Review Needed [apps] Area: Test applications related improvements [build] Area: Changes in build files and removed Status: Revision Needed labels Sep 3, 2019
@ethouris ethouris modified the milestones: v1.4.0, v1.4.1 Sep 12, 2019
@ethouris ethouris modified the milestones: v1.4.1, v1.4.2 Nov 6, 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[apps] Area: Test applications related improvements [build] Area: Changes in build files Priority: Low Type: Maintenance Work required to maintain or clean up the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants