Merge sndrcv() and sniff() implementations for Unix & Windows systems #770

p-l- · 2017-08-22T18:08:40Z

This avoids duplicated code and brings to Windows enhancements that exist for Unix (sniff() on multiple interfaces, parallel sndrcv()).

It also brings a huge code clean-up on both functions and may have a (limited but positive) impact on performances (some tests have been moved outside of sniff loops). The code is (IMO) much more readable and maintainable.

It changes the behavior of sniff(): it is now possible to provide lists for offline and opened_socket parameters (that was only possible for iface). It is also possible, for those three parameters, to provide dict objects mapping an element (interface or file name, or opened socket) to a label.

Also, fixes #780.

p-l- · 2017-08-22T18:49:27Z

@gpotter2 could you have a look at this one? If it works, I'll continue this work with sniff(). Also, the Windows tests seem broken (they last timeout after one hour...), any idea what could be wrong there?

gpotter2 · 2017-08-22T18:51:07Z

Traceback (most recent call last):
  File "C:\Python27\lib\runpy.py", line 174, in _run_module_as_main
    "__main__", fname, loader, pkg_name)
  File "C:\Python27\lib\runpy.py", line 72, in _run_code
    exec code in run_globals
  File "Z:\Coding\github\scapy\scapy\__init__.py", line 93, in <module>
    interact()
  File "scapy\main.py", line 365, in interact
    init_session(session_name, mydict)
  File "scapy\main.py", line 191, in init_session
    scapy_builtins = {k: v for k, v in six.iteritems(importlib.import_module(".all", "scapy").__dict__) if _validate_local(k)}
  File "C:\Python27\lib\importlib\__init__.py", line 37, in import_module
    __import__(name)
  File "scapy\all.py", line 16, in <module>
    from scapy.arch import *
  File "scapy\arch\__init__.py", line 76, in <module>
    from scapy.arch.windows.compatibility import *
  File "scapy\arch\windows\compatibility.py", line 99, in <module>
    sendrecv.sniff = sniff
NameError: name 'sendrecv' is not defined
Appuyez sur une touche pour continuer...

p-l- · 2017-08-22T18:52:54Z

@gpotter2 I was talking of the other builds, this one if failing for a good reason indeed!

gpotter2 · 2017-08-22T18:54:55Z

@p-l- Oh right, I'm checking that...

gpotter2 · 2017-08-22T19:06:12Z

Tests are stuck on

Test basic save_session, load_session and update_session

Checking why...

gpotter2 · 2017-08-23T01:04:07Z

scapy/sendrecv.py

-        pid=1
+        rdpipe, wrpipe = os.pipe()
+        rdpipe = os.fdopen(rdpipe)
+        wrpipe = os.fdopen(wrpipe,"w")


For multiprocessing, you may use
multiprocessing.Pipe(False) (uni-directional) to get Connection objects.
You can select those or, on windows, use the poll function to replace the current pipes

Thanks a lot!

p-l- · 2017-08-23T12:18:12Z

@gpotter2 I've finally used threads rather than processes, it's much easier and it's better for this kind of operations. Can you by any chance have a look at why it fails under Windows (the error log is not helping me...)? Just getting this code, running sr(IP(dst="8.8.8.8")/ICMP()) or anything similar and reporting the error(s) here would be very helpful.

codecov-io · 2017-08-23T12:54:10Z

Codecov Report

Merging #770 into master will increase coverage by 0.12%.
The diff coverage is 68.09%.

@@            Coverage Diff             @@
##           master     #770      +/-   ##
==========================================
+ Coverage   80.29%   80.41%   +0.12%     
==========================================
  Files         138      137       -1     
  Lines       34088    33896     -192     
==========================================
- Hits        27370    27257     -113     
+ Misses       6718     6639      -79

Impacted Files	Coverage Δ
scapy/arch/__init__.py	`88% <ø> (+0.72%)`	⬆️
scapy/plist.py	`43.05% <0%> (ø)`	⬆️
scapy/config.py	`83.75% <0%> (-0.48%)`	⬇️
scapy/layers/l2.py	`82.08% <0%> (ø)`	⬆️
scapy/arch/bpf/supersocket.py	`75.78% <100%> (-0.11%)`	⬇️
scapy/utils6.py	`87.72% <100%> (+0.03%)`	⬆️
scapy/arch/windows/__init__.py	`73.82% <58.82%> (-0.81%)`	⬇️
scapy/sendrecv.py	`52.87% <67.78%> (+5.55%)`	⬆️
scapy/arch/pcapdnet.py	`64.78% <90.47%> (+2.2%)`	⬆️
scapy/consts.py	`66.07% <0%> (-1.79%)`	⬇️
... and 4 more

gpotter2 · 2017-08-23T13:11:16Z

It seems that you should restore the appveyor file as it was before :/

Everything seems to be working fine :) great job !

p-l- · 2017-08-23T13:25:41Z

@gpotter2 I have removed the commit, and it still fails without any message...

Could you also try with sr1(Ether(...)...) rather than sr()?

gpotter2 · 2017-08-23T15:27:04Z

Current appveyor tests are very glitchy :/

p-l- · 2017-08-23T15:36:03Z

On my setup (nothing to do with Windows : Python 2.7.13 on ArchLinux), the tests fail, but pass when I remove the Main.py tests (hence this test commit).

gpotter2 · 2017-08-23T15:43:43Z

Those tests will be removed anyway with the new IPython console implementation, so if they break anything it's fine to remove them.

p-l- · 2017-08-23T22:16:07Z

@gpotter2 unfortunately that's something else. I guess I'll have to get a Windows VM with Python + Scapy... Can you confirm that this code seems to work on your setup? Thanks!

p-l- · 2017-08-24T06:07:07Z

@gpotter2 I've checked #773, but for this one the tests won't timeout, they fail quickly :-/

gpotter2 · 2017-08-24T06:39:40Z

I'm going to try to debug this.
Edit: For now, it seems that Python is crashing (litterally: python is crashing: no stacktrace/error) on the reall networking tests. Definatly related to this PR.

gpotter2 · 2017-08-24T09:10:35Z

scapy/arch/pcapdnet.py

-                del(self.ins)
-            if hasattr(self, "outs"):
-                del(self.outs)
+            if self.closed:


?! Dont you mean not self.closed ? Same above

gpotter2 · 2017-08-24T09:11:45Z

scapy/sendrecv.py

+                    elif conf.use_pcap:
+                        r = pks.nonblock_recv()
+                    elif not isinstance(pks, StreamSocket) and (
+                            FREEBSD or DARWIN or OPENBSD or WINDOWS


The or WINDOWS will never be triggered here...

Totally. This is probably not the reason why the tests fail, but still. Thanks!

p-l- · 2017-08-24T09:30:46Z

@gpotter2 changes from your remarks have been applied.

gpotter2 · 2017-08-24T10:15:52Z

I think I have found the bug.

Winpcap cannot support multi-threaded different actions on the same socket objects: it's not possible to recieve and send packets at the same time on the same socket (pcap_open object).

>>> a = conf.L2socket()
>>> def _test(a):
...     while True:
...         a.recv()
...
>>> Thread(target=_test, args=(a,)).start()
>>> send(IP(), socket=a)
Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "scapy\sendrecv.py", line 266, in send
    realtime=realtime, return_packets=return_packets)
  File "scapy\sendrecv.py", line 240, in __gen_send
    s.send(p)
  File "scapy\arch\pcapdnet.py", line 277, in send
    return self.ins.send(sx)
  File "scapy\arch\pcapdnet.py", line 175, in send
    pcap_sendpacket(self.pcap, x, len(x))
WindowsError: exception: access violation reading 0x0000000000000170

Several options:

we can't contact Npcap devs to ask them if it would be possible to get this working, but it might be a windows limitation
use different sockets to send and to recieve
?

p-l- · 2017-08-24T10:56:02Z

What is a exactly in your code? Can you report the output of a, a.ins, a.outs and a.ins is a.outs?

gpotter2 · 2017-08-24T11:20:02Z

Sorry, updated my code for a.

>>> a
<scapy.arch.pcapdnet.L2pcapSocket object at 0x0000000005EF5BA8>
>>> a.ins
<scapy.arch.pcapdnet._PcapWrapper_pypcap instance at 0x0000000005F01A08>
>>> a.outs
Traceback (most recent call last):
  File "<console>", line 1, in <module>
AttributeError: 'L2pcapSocket' object has no attribute 'outs'

I tried with your branch and upstream/master. Same result

guedou · 2017-08-25T11:23:57Z

scapy/sendrecv.py

@@ -44,12 +47,41 @@ class debug:
 ####################


+def _sndrcv_snd(pks, timeout, inter, verbose, tobesent, all_stimuli, stopevent):


Could you add a doctring here ?

guedou

Looks good to me. I would really that you move the _select() and _get_pkt() definitions in a dedicated function.

guedou · 2017-08-25T11:25:04Z

scapy/sendrecv.py

+    except KeyboardInterrupt:
+        pass
+    except:
+        log_runtime.exception("--- Error sending packets")


This will be nicer with:

msg = "--- Error sending packets" log_runtime.exception(msg) log_runtime.info(msg)

Totally. I've used a single call, to .info() with exc_info instead.

guedou · 2017-08-25T11:27:38Z

scapy/sendrecv.py

-
+        autostop = 0
+
+    if WINDOWS:


Could we move this piece of code in a dedicated function, for example in scapy/arch ? This will make sndrcv() smaller and easier to read.

_get_pkt() has specific implementations:

under windows (this one)

(but also) when using BPF

(or) when using PCAP under Darwin, FreeBSD or OpenBSD, and the socket is not a SuperSocket instance.

I don't think it would make the code easier to read to move only this implementation elsewhere. Plus the last specific implementation relies on the socket type.

The important point was to move the tests outside the loops.

guedou · 2017-08-25T11:28:53Z

scapy/sendrecv.py

@@ -213,20 +200,20 @@ def sndrcv(pks, pkt, timeout = None, inter = 0, verbose=None, chainCC=0, retry=0
        if len(tobesent) == 0:
            break
        retry -= 1
-        
+
    if conf.debug_match:
        debug.sent=plist.PacketList(remain[:],"Sent")


While you are at it s/,/, /

guedou · 2017-08-25T11:29:52Z

scapy/sendrecv.py

+                 is displayed.
+                 Ex: prn = lambda x: x.summary()
+filter:        BPF filter
+lfilter:       python function applied to each packet to determine if further


s/python/Python/g as well as below

guedou · 2017-08-25T11:34:32Z

scapy/sendrecv.py

    lst = []
    if timeout is not None:
        stoptime = time.time()+timeout
    remain = None
+    read_allowed_exceptions = ()
+    if conf.use_bpf:


Same as before, could we move this piece of code in a dedicated function ?

See my answer to your previous comment. Those functions use a local variable of sniff(). We could add a second parameter, but defining an auxiliary function works well here.

p-l- · 2017-08-25T12:21:52Z

Thanks @guedou for your review, I've updated this PR and answered your comments. @gpotter2 FYI the code has been modified since you've approved it.

gpotter2

Looks good !

guedou · 2017-08-28T08:55:53Z

OK, I understand. I don't like function definitions inside functions. When possible, I prefer to define them outside. Here, it might be to cumbersome to define get_pkt() outside sndrcv()

p-l- · 2017-08-28T10:37:14Z

@guedou the purpose of using such functions is to prevent tests from being checked inside the loop. However, to decide which function we want to use, we need information we can only have inside the function. That's why these functions are defined inside the function (but outside the loop).

guedou · 2017-08-28T11:31:34Z

@p-l- I understood the first time. I agree, that in this case, it is not possible to do what I asked for.

guedou · 2017-08-28T11:39:06Z

Good to be merged. @gpotter2 do you want to have another look ?

gpotter2 · 2017-08-28T11:46:09Z

scapy/arch/windows/compatibility.py

-
-import scapy.sendrecv
-sendrecv.sniff = sniff
+from scapy.config import conf

 # If wpcap.dll is not available
 if not (conf.use_winpcapy or conf.use_pcap or conf.use_dnet):


You should be able to delete compatibility.py now, just add what's left instead of the dirty import in arch/__init__.py

gpotter2 · 2017-08-28T11:52:59Z

scapy/sendrecv.py

-    c = 0
+    for arg in ['opened_socket', 'offline', 'iface']:
+        if arg in kargs:
+            del kargs[arg]


I guess we should print a warning

gpotter2 · 2017-08-28T11:56:22Z

scapy/sendrecv.py

+                                 for s, label in iteritems(opened_socket))
+        else:
+            sniff_sockets.update((s, "socket%d" % i)
+                                 for i, s in enumerate(opened_socket))


Would prefer

if isinstance(opened_socket, dict): sniff_sockets.update((s, label) for s, label in iteritems(opened_socket)) elif isinstance(opened_socket, list): sniff_sockets.update((s, "socket%d" % i) for i, s in enumerate(opened_socket)) else: opened_socket = [opened_socket]

I think you have an error in your else: block, but the idea is good indeed!

This removes duplicated code and should make bridge_and_sniff() function work under Windows.

p-l- · 2017-08-28T13:48:25Z

@gpotter2 @guedou should be ok now

gpotter2 · 2017-08-28T14:20:59Z

Could you leave the import to disable_sendrecv, if wpcap.dll is disabled ?

p-l- · 2017-08-28T16:55:06Z

@gpotter2 done. I think the NotAvailableSocket is enough. I also think it's OK to raise a RuntimeException even in interactive mode.

guedou · 2017-08-29T17:58:18Z

I am fine with this PR. Could you merge it for me ? I am currently away form a decent keyboard.

p-l- · 2017-08-29T20:27:08Z

Thanks @guedou @gpotter2 for this one!

p-l- force-pushed the windows-convergence branch from b07efa8 to 41b06f2 Compare August 22, 2017 18:54

gpotter2 reviewed Aug 23, 2017

View reviewed changes

p-l- force-pushed the windows-convergence branch 2 times, most recently from 8fa13ee to 87374a8 Compare August 23, 2017 11:40

p-l- force-pushed the windows-convergence branch from 1f82413 to dfaf10f Compare August 23, 2017 13:17

p-l- force-pushed the windows-convergence branch from dfaf10f to fdb6cfc Compare August 23, 2017 15:24

p-l- force-pushed the windows-convergence branch from 0188ec2 to 952bbf2 Compare August 24, 2017 05:26

gpotter2 reviewed Aug 24, 2017

View reviewed changes

p-l- force-pushed the windows-convergence branch from 952bbf2 to 53ecfb7 Compare August 24, 2017 09:16

gpotter2 mentioned this pull request Aug 24, 2017

Npcap: Access violation while multi-threading nmap/nmap#982

Closed

p-l- force-pushed the windows-convergence branch from 8ddab19 to d852e77 Compare August 25, 2017 10:48

guedou reviewed Aug 25, 2017

View reviewed changes

guedou requested changes Aug 25, 2017

View reviewed changes

p-l- force-pushed the windows-convergence branch from d852e77 to 8d69d48 Compare August 25, 2017 12:07

This was referenced Aug 25, 2017

Child died unexpectedly. Packets may have not been sent #780

Closed

TUN/TAP Interfaces, improve & test bridge_and_sniff() #782

Merged

gpotter2 approved these changes Aug 27, 2017

View reviewed changes

guedou approved these changes Aug 28, 2017

View reviewed changes

gpotter2 reviewed Aug 28, 2017

View reviewed changes

p-l- force-pushed the windows-convergence branch 2 times, most recently from 0439a92 to e8ef4a6 Compare August 28, 2017 13:05

p-l- added 3 commits August 28, 2017 15:21

sniff(): merge Windows & Unix implementations

e99fb53

sendrecv: clean-up

a80424e

Implement bridge_and_sniff() using sniff()

d5e3fc2

This removes duplicated code and should make bridge_and_sniff() function work under Windows.

p-l- force-pushed the windows-convergence branch from e8ef4a6 to a9c489d Compare August 28, 2017 13:21

Windows: remove compatibility submodule

47bd4e2

p-l- force-pushed the windows-convergence branch from a9c489d to 47bd4e2 Compare August 28, 2017 16:49

gpotter2 approved these changes Aug 28, 2017

View reviewed changes

p-l- merged commit ea91eba into secdev:master Aug 29, 2017

p-l- deleted the windows-convergence branch August 29, 2017 20:26

		@@ -44,12 +47,41 @@ class debug:
		####################


		def _sndrcv_snd(pks, timeout, inter, verbose, tobesent, all_stimuli, stopevent):

Merge sndrcv() and sniff() implementations for Unix & Windows systems #770

Merge sndrcv() and sniff() implementations for Unix & Windows systems #770

Conversation

p-l- commented Aug 22, 2017 • edited

p-l- commented Aug 22, 2017

gpotter2 commented Aug 22, 2017

p-l- commented Aug 22, 2017

gpotter2 commented Aug 22, 2017

gpotter2 commented Aug 22, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p-l- commented Aug 23, 2017

codecov-io commented Aug 23, 2017 • edited

Codecov Report

gpotter2 commented Aug 23, 2017 • edited

p-l- commented Aug 23, 2017 • edited

gpotter2 commented Aug 23, 2017 • edited

p-l- commented Aug 23, 2017

gpotter2 commented Aug 23, 2017

p-l- commented Aug 23, 2017

p-l- commented Aug 24, 2017

gpotter2 commented Aug 24, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p-l- commented Aug 24, 2017

gpotter2 commented Aug 24, 2017 • edited

p-l- commented Aug 24, 2017

gpotter2 commented Aug 24, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guedou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p-l- Aug 25, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p-l- Aug 25, 2017 • edited

Choose a reason for hiding this comment

p-l- commented Aug 25, 2017

gpotter2 left a comment

Choose a reason for hiding this comment

guedou commented Aug 28, 2017 via email

p-l- commented Aug 28, 2017

guedou commented Aug 28, 2017

guedou commented Aug 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p-l- commented Aug 28, 2017

gpotter2 commented Aug 28, 2017

p-l- commented Aug 28, 2017

guedou commented Aug 29, 2017 via email

p-l- commented Aug 29, 2017

p-l- commented Aug 22, 2017 •

edited

gpotter2 commented Aug 22, 2017 •

edited

codecov-io commented Aug 23, 2017 •

edited

gpotter2 commented Aug 23, 2017 •

edited

p-l- commented Aug 23, 2017 •

edited

gpotter2 commented Aug 23, 2017 •

edited

gpotter2 commented Aug 24, 2017 •

edited

gpotter2 commented Aug 24, 2017 •

edited

gpotter2 commented Aug 24, 2017 •

edited

p-l- Aug 25, 2017 •

edited

p-l- Aug 25, 2017 •

edited