Closed jaylogue closed 2 years ago
This pull request introduces 5 alerts when merging 7ee936218709e8b54f93f1be7414865590e13395 into 7f33e06596b05d6997fbe48480196644d58e0a5c - view on LGTM.com
new alerts:
This pull request introduces 4 alerts when merging cb2272d80c6198c3a19a0c324a4825bd30ea2323 into 7f33e06596b05d6997fbe48480196644d58e0a5c - view on LGTM.com
new alerts:
Ooh, nice. Thank you very much for implementing this. I'll try this out when I get a little time, maybe on Saturday.
@jaylogue Have you tried running in on a Windows instance of Wireshark ? I have copied all required files (looking at the cp command) in the extcap of my Wireshark install (C:\Program Files\Wireshark\extcap) but am not able to see the capture device after refresh.
With the exact same setup, running 'python ./sniff_receiver.py -s COM8 -e -a -l' shows me advertisement packets.
EDIT : I sort of found the issue. Apparently on Windows, an additional bat file is needed to call the python script (see https://www.wireshark.org/docs/wsdg_html_chunked/ChCaptureExtcap.html#ChCaptureExtcapWindowsShell). Maybe you could add the bat script to your PR ?
Once I added the bat file and start capturing, I get some errors related to pipes. Maybe there is still some code that is not 'Windows-friendly' inside the modules ?
Error by extcap pipe: Traceback (most recent call last):
File "C:\Program Files\Wireshark\extcap\sniffle_extcap.py", line 66, in main
self.capture()
File "C:\Program Files\Wireshark\extcap\sniffle_extcap.py", line 322, in capture
pcapWriter = PcapBleWriter(self.args.fifo)
File "C:\Program Files\Wireshark\extcap\pcap.py", line 46, in __init__
self.output = open(output,'wb')
OSError: [Errno 22] Invalid argument: '\\\\.\\pipe\\wireshark_extcap_sniffle_20211004172103'
I haven't tried on other platforms yet. I wanted to see if the general thrust of this code was acceptable before investing more time. I will investigate the Windows issue and update the PR accordingly.
The general approach of this code looks good to me, and it doesn't look like much of an additional effort to maintain feature parity with the rest of Sniffle going forward. I still haven't gotten around to trying this out, but hopefully I'll find some time this week.
@sultanqasim Regarding code maintenance, I do note that there is a certain amount of duplication between the code in sniffle_extcap.py and sniff_receiver.py, particularly in the areas of argument parsing and capture setup. One possible way to avoid this would be to extend sniffle_extcap.py to incorporate the functionality sniff_receiver.py, such that a single script could be used both in the context of Wireshark and from the command line. In the latter case, I envision the command line interface would be identical to sniff_receiver.py, to maintain compatibility.
As I was writing sniffle_extcap.py I noted that this would be pretty easy to implement. However I wanted to some feedback on the core Wireshark functionality before attempting this.
(Attn @nono313 ) I have updated the PR with support for Windows and better access to internal logging.
This pull request introduces 7 alerts when merging ae550af573da56c457f98d8430ddabebe2d5d5a3 into 7f33e06596b05d6997fbe48480196644d58e0a5c - view on LGTM.com
new alerts:
Thanks @jaylogue. I've just tested it. It works fine, I am able to see both of my LE Coded PHY advertising devices.
I do however have some issues where if I stop the scanning, Wireshark crashs without any error message.
I had seen that too, but it was intermittent. Digging in, it looks like it's triggered whenever there are multiple plug-ins in the Wireshark extcap directory. As far as I can tell it doesn't matter which plug-ins. For example, if you install two copies of the Wireshark extcat example plug-in (extcap_example.py) by creating 2 separate .bat/.py files, Wireshark will crash after stopping capturing for one of them.
@nono313 , I would be helpful if you could confirm this in your environment by removing any other plug-ins you have installed.
Assuming I'm right, this looks like an unfortunate bug in Wireshark that is unrelated to Sniffle. At some point I'll dust off my windbg skills and see what's going on. In the meantime, I can place a warning in the README about using multiple plug-ins.
That's it, thanks !
I did infact had another plugin (Nordic's Sniffer). If I remove the files from the extcap folder and then starts Wireshark, I can stop and start without any crash.
Thanks. Yes I guess a warning in the README should be enough for now.
This pull request introduces 7 alerts when merging 3716080acda3f0649c669efe53ac176fe427eb9c into 7f33e06596b05d6997fbe48480196644d58e0a5c - view on LGTM.com
new alerts:
This hasn't been forgotten, I just need to get around to trying it :)
Has the Windows extcap crash bug been fixed in upstream Wireshark? I don't normally use Windows, so I don't have everything set up to test.
Has the Windows extcap crash bug been fixed in upstream Wireshark?
I have finally had time to look at this again, and unfortunately the crash is still present in current Wireshark (v3.6.5-0-g21f79ddbefbd). I was able to capture a backtrace for the crash and have filed the following bug with the Wireshark project: https://gitlab.com/wireshark/wireshark/-/issues/18093. If I have time, I may poke around in the source and see if I can come up with a fix.
Thanks for looking into this. I'll rebase, experiment with, and merge this tonight or tomorrow.
Actually working on rebasing and testing this now. Sorry about the delay. I want to include this in the Version 1.7 release.
Cherry-picked/rebased in with some minor fixes and additions
Lately I've been using Sniffle quite a bit in a personal project I've been working on. Since I like to use Wireshark, I decided to make Wireshark extcap plugin to allow me to launch Sniffle directly from within the GUI. Lo and behold, I now see that there is a feature request (#38) asking for exactly this. Hopefully this fits the bill.
Pretty much all of the options available in the sniff_receiver script are accessible via the GUI with the exception of the '--mac top' feature. I wasn’t sure how useful that feature would be in this context.
I’ve also updated the README accordingly.
I've been using the plugin for a short while and so far it seems to be working well. But it surely could do with some more testing.