Fix ICMP handling in probe modules #470
Conversation
…ey are. Also move to using PACKET_(IN)VALID instead of random constants for my sanity
…ocol as well as ICMP in most places
|
This branch solely handles ICMP validation and does not output any of the data. However, it's already a giant PR, so we might as well get through this one first. |
…to icmp-validation-abstraction
|
@zakird I'd be happy to take this over if you'd like, do you have an idea of what still needs to be done? Otherwise, I can just go through and figure out what was already done and proceed from there. |
|
If you want to take over and finish it off, that'd be amazing. I haven't had the time to look at this in quite a while. The idea of the PR was originally just to make sure that we included ICMP output consistently in all of our probe modules. This also meant cleaning up how we validated ICMP packets in all of the modules. As I started through this, I found that we had a bunch of duplicated code in all of the probe modules and I tried to pull all of these out into helper functions such that our probe modules looked sane. I think that a lot of what's there is OK, but there hasn't been much testing of the code (I wrote all of it on a transatlantic flight without Internet). I think that looking through diffs will probably drive you insane given how much has changed. I'd probably just start by getting it to compile (which I think should be pretty easily fixed) and then starting to run some scans and seeing what starts to come out. I'd probably start with the TCP module. |
| fs_add_null_icmp(fs); | ||
| } else if (ip_hdr->ip_p == IPPROTO_ICMP) { | ||
| // tcp | ||
| fs_add_null(fs, "sport"); |
There was a problem hiding this comment.
Is adding sport, dport, and classification here and in fs_populate_icmp_from_iphdr intentional? If not, I'll fix that.
|
I did a tcp synscan (port 80) using this branch to see what I'd find. I didn't find any icmp_unreach classifications in a 5%. I added an exit(1) condition to validate packet and process packet if the packet wasn't a tcp packet (just for santity checking), and redid a 5% scan. Zmap didn't exit, so no non-tcp packets found. I think my next step is to attempt to send non-tcp packets to a running zmap process and see what happens. Any thoughts/am I barking up the wrong tree? |
|
Is th pcap filter updated to allow icmp responses?
…On Tue, Jul 17, 2018 at 8:44 PM Alex Holland ***@***.***> wrote:
I did a tcp synscan (port 80) using this branch to see what I'd find. I
didn't find any icmp_unreach classifications in a 5%.
I added an exit(1) condition to validate packet and process packet if the
packet wasn't a tcp packet (just for santity checking), and redid a 5%
scan. Zmap didn't exit, so no non-tcp packets found. I think my next step
is to attempt to send non-tcp packets to a running zmap process and see
what happens.
Any thoughts/am I barking up the wrong tree?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#470 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAMSUKlwwarZLz0QzvYuWu30VX183mx7ks5uHpMFgaJpZM4PZ2S5>
.
|
…/zmap into icmp-validation-abstraction
| @@ -34,7 +33,6 @@ extern probe_module_t module_bacnet; | |||
| probe_module_t *probe_modules[] = { | |||
| &module_tcp_synscan, &module_tcp_synackscan, &module_icmp_echo, | |||
| &module_icmp_echo_time, &module_udp, &module_ntp, &module_upnp, &module_dns, | |||
| //&module_tcp_cisco_backdoor, | |||
There was a problem hiding this comment.
We need to move this to //examples
src/probe_modules/packet.h
Outdated
| return (struct icmp *)((char *)ip_hdr + 4 * ip_hdr->ip_hl); | ||
| } | ||
|
|
||
| static inline char *get_udp_payload(const struct udphdr *udp, |
There was a problem hiding this comment.
I'd do uint8_t instead of char, since we think of it as a byte array, not a string.
| static inline struct tcphdr *get_tcp_header(const struct ip *ip_hdr, | ||
| uint32_t len) | ||
| { | ||
| // buf not large enough to contain expected udp header |
| return (struct tcphdr *)((char *)ip_hdr + 4 * ip_hdr->ip_hl); | ||
| } | ||
|
|
||
| static inline struct udphdr *get_udp_header(const struct ip *ip_hdr, |
There was a problem hiding this comment.
Are there UDP options that influence the length of the header?
This PR represents the effort to standardize ICMP handling across all of our probe modules.
Fixes #655
Fixes #354