Use ICMP instead of HTTP for testing the connection #1261
Conversation
d35ha
force-pushed
the
internet-detector-dns
branch
3 times, most recently
from
537515e to
8c2dde2
Compare
d35ha
added
💎 enhancement
There was a problem hiding this comment.
Nice improvement @d35ha! I think you should document (both in the code as comment and in the commit message description), why this is an enhacement (how this reduces the fingerprint of the tools and is a better option than HTTPS and TCP), as this is what makes this a nice improvement and we want to ensure others understand it (now and in the future to avoid it is changed back).
d35ha
force-pushed
the
internet-detector-dns
branch
from
8c2dde2 to
cb0eefc
Compare
d35ha
force-pushed
the
internet-detector-dns
branch
from
cb0eefc to
d84a5a2
Compare
There was a problem hiding this comment.
Thank you for the changes! As already mentioned in my last review: You need to document why this is an enhacement (how this reduces the fingerprint of the tools and is a better option than HTTPS and TCP). Please do this in the following ways:
- Add a description to your commit. I recommend reading https://cbea.ms/git-commit for more info on best practices to write good commit messages
- Update the comment at the beginning of
internet_detector.pyw. Note it is wrong ATM:This tool checks if internet connectivity exists by reaching out to specific website.
|
I tested locally and it works as expected. As suggested by @Ana06 it is not known why this implementation poses an advantage over HTTPS |
There was a problem hiding this comment.
tested locally, it works fine!! Thanks @d35ha.
Just to add to my previous comment, for someone without a background perhaps it is hard to know why this implementation is more efficient than the previous one. Just a bit of explanation would be good.
d35ha
force-pushed
the
internet-detector-dns
branch
from
d84a5a2 to
69ed6d9
Compare
d35ha
changed the title
There was a problem hiding this comment.
Thanks for adding documentation in internet_detector.vm/tools/internet_detector.pyw @d35ha! 💐
Please always test your changes locally. The package fails to install.
I would appreciate if you could add a description to your commit messages. This is an important change and it would be helpful when checking the commit history to know why a change was introduced. As already suggested in my previous reviews, I recommend you check https://cbea.ms/git-commit. Concretely:
- the first line (the title) should ideally be <=50 characters
- after the title leave an empty line. The rest is the description, which should explain what and why.
An example of one of my last PRs with several commits (every of them with a commit description): mandiant/flare-vm#645
d35ha
force-pushed
the
internet-detector-dns
branch
2 times, most recently
from
44c11d6 to
3ec3fd1
Compare
Using ping instead of checking the status of multiple websites for detecting the internet connection: - The original approach to detect the internet connection was to send HTTP requests to a hard-coded list of websites and check the return (ex. status code, website content, ...). - The original approach produces much traces (HTTP requests/responses with so many packets and DNS resolution) that interferes with the dynamic malware analysis tools causing a lot of confusion. - In this commit, ICMP is used instead by pinging a hard-coded list of public DNS servers and to check if any of them is alive. - The new approach ensures less traces (2 packets/request) and efficient detection (no DNS resolution is needed).
d35ha
force-pushed
the
internet-detector-dns
branch
from
3ec3fd1 to
706f1fc
Compare
There was a problem hiding this comment.
@d35ha thank you for the changes! I appreciate the documentation addition and the detailed commit description.
I have done some more testing and this change is very noisy in fakenet:
It seems the ProcessBlackList: internet_detector.exe fakenet exclusion is not working properly for ICMP. The HTTP communication was excluded from the output, while ICMP is still there. @d35ha can you reproduce this? Am I doing something wrong?
|
@sara-rn you also tested the change. Did you also get the noise in fakenet? |
@Ana06 |
It seems that fakenet failes to (or doesn't) identify the source process for the ICMP requests.
|
|
@d35ha I think it is not related to mandiant/flare-vm#630 as fakenet does ignore the process when using HTTP. I think the issue is how fakenet is implemented. When @emtuls first implemented the internet detector using HTTP requests we realised the process was not being ignored: mandiant/flare-fakenet-ng#190. Supporting ignoring HTTP requests coming from a black listed process required structural changes in fakenet implemented in mandiant/flare-fakenet-ng#192. It seems that we now have the same issue with ICMP and this would requiring modifying fakenet again, not sure if it is even possible. 🤔 |
|
PR to add ICMP ID-based filtering in fakenet that allows us to ignore the ICMP traffic from the internet detector: mandiant/flare-fakenet-ng#204 |
|
We would still need to adapt this PR after mandiant/flare-fakenet-ng#204 has been merged to add the ID, @d35ha do you plan to do this now to illustrate how this is exactly implemented (concretely how the ID is generate), which will help understanding what we want to address with mandiant/flare-fakenet-ng#204 (and may help with the PR review)? Or are you planning to wait for feedback on mandiant/flare-fakenet-ng#204 before making changes here? |
d35ha
force-pushed
the
internet-detector-dns
branch
2 times, most recently
from
6651997 to
ca9e491
Compare
d35ha
force-pushed
the
internet-detector-dns
branch
from
ca9e491 to
1e1fe10
Compare
No description provided.