Skip to content

Select the network interface in send() #2356

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

Merged
merged 1 commit into from
May 8, 2020
Merged

Conversation

guedou
Copy link
Member

@guedou guedou commented Dec 1, 2019

This PR fixes #2355.

@codecov
Copy link

codecov bot commented Dec 1, 2019

Codecov Report

Merging #2356 into master will increase coverage by 0.28%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2356      +/-   ##
==========================================
+ Coverage    87.5%   87.79%   +0.28%     
==========================================
  Files         243      243              
  Lines       50727    50728       +1     
==========================================
+ Hits        44390    44537     +147     
+ Misses       6337     6191     -146
Impacted Files Coverage Δ
scapy/sendrecv.py 85.48% <100%> (+0.02%) ⬆️
scapy/pton_ntop.py 89.04% <0%> (-8.22%) ⬇️
scapy/autorun.py 78.51% <0%> (-4.14%) ⬇️
scapy/layers/tls/record_sslv2.py 86.03% <0%> (-1.68%) ⬇️
scapy/layers/tls/record.py 89.59% <0%> (-1.16%) ⬇️
scapy/layers/tls/basefields.py 80.53% <0%> (-0.68%) ⬇️
scapy/automaton.py 87.22% <0%> (-0.4%) ⬇️
scapy/arch/windows/__init__.py 71.81% <0%> (-0.16%) ⬇️
scapy/layers/inet6.py 81.96% <0%> (+0.21%) ⬆️
scapy/layers/tls/handshake.py 86.37% <0%> (+0.28%) ⬆️
... and 6 more

@guedou
Copy link
Member Author

guedou commented Dec 5, 2019

Please don't merge yet. I have some doubts about this PR when multiple packets are sent.

@p-l-
Copy link
Member

p-l- commented Dec 10, 2019

I do not agree with this PR.

In my mind, send() is supposed to use Scapy's routing table (conf.iface) to send layer 3 packets, while sendp() sends layer 2 packets using conf.iface (or the value for parameter iface=).

Specifying an interface in send() is weird. For instance, what layer 2 address should Scapy use to send a packet outside the LAN?

An option would be to allow specifying an alternative routing table object (or a route).

What do you think?

@guedou
Copy link
Member Author

guedou commented Dec 10, 2019

The current implementation of sr1() behaves the same way as this PR: try to be smart and use the routing table instead of conf.iface.

@p-l-
Copy link
Member

p-l- commented Dec 11, 2019

OK I misunderstood your PR then, I'll have to read again. Anyway, having send() behavior consistent with sr() and sr1() is what we want.

@p-l-
Copy link
Member

p-l- commented Dec 11, 2019

But I really don't think that send() (or sr() and sr1()) should have an iface= argument.

@guedou
Copy link
Member Author

guedou commented Dec 11, 2019

The root issue might be related to this line https://github.com/secdev/scapy/blob/master/scapy/arch/linux.py#L478

@guedou
Copy link
Member Author

guedou commented Feb 10, 2020

Can this PR be merged?

@p-l-
Copy link
Member

p-l- commented Feb 11, 2020

As I said, I don't like the idea of send() having an iface= parameter. send() should rely on a routing table, not an interface (which, in turn, will provide an interface and a gateway). What is the meaning of providing an interface alone? We still need the routes to get the gateway IP address.

@guedou
Copy link
Member Author

guedou commented Feb 11, 2020 via email

@guedou
Copy link
Member Author

guedou commented Feb 21, 2020

This is indeed related to #2108 and #1975.

As explained several times in this thread, I really think that this PR should be merged.

Please merge or provide a solution to these issues.

@p-l- p-l- merged commit cb3ca9f into secdev:master May 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Interface routing issue with send on layer 3
3 participants