Fix input handling around -P flag with the DNS module, Resolves #746#757
Fix input handling around -P flag with the DNS module, Resolves #746#757phillip-stephens merged 20 commits intomainfrom
-P flag with the DNS module, Resolves #746#757Conversation
|
I don't think that I'm fully parsing how this is supposed to work. I would expect That said, I don't love the idea of probe modules changing global configuration — that seems very messy to me. I think it would make sure sense to instead throw a Last note, I could imagine that someone might want to send a multiple of the number of probes (e.g., if there are two names, and you want to send a packet to each twice, then you set the number of probes to be 4). |
|
@zakird You're right, I've updated the file to allow the user to use a multiple of the DNS question count as the probe number. Added details in the PR description. WDYT about the pstephens@scratch-04-a:~/zmap-rsync$ sudo ./src/zmap -p 53 --probe-module=dns -O json -o logfile.json -P 2 1.1.1.1 --sender-threads=1 --probe-args="A,yahoo.com;AAAA,google.com" --output-fields=* -c 3
Dec 19 19:14:24.923 [INFO] dedup: Response deduplication method is full
Dec 19 19:14:24.923 [INFO] filter: No output filter provided. ZMap will output all results, including duplicate and non-successful responses (e.g., RST and ICMP packets). If you want a filter similar to ZMap's default behavior, you can set an output filter similar to the following: --output-filter="success=1 && repeat=0".
Dec 19 19:14:24.978 [INFO] recv: duplicate responses will be passed to the output module
Dec 19 19:14:24.978 [INFO] recv: unsuccessful responses will be passed to the output module
0:00 0%; sent: 0 0 p/s (0 p/s avg); recv: 0 0 p/s (0 p/s avg); app success: 0 0 p/s (0 p/s avg); drops: 0 p/s (0 p/s avg); hitrate: 0.00% app hitrate: 0.00%
0:00 0%; sent: 0 0 p/s (0 p/s avg); recv: 0 0 p/s (0 p/s avg); app success: 0 0 p/s (0 p/s avg); drops: 0 p/s (0 p/s avg); hitrate: 0.00% app hitrate: 0.00%
0:01 34%; sent: 2 done (39 p/s avg); recv: 2 2 p/s (1 p/s avg); app success: 1 0 p/s (0 p/s avg); drops: 0 p/s (0 p/s avg); hitrate: 100.00% app hitrate: 50.00%
0:02 67%; sent: 2 done (39 p/s avg); recv: 2 0 p/s (0 p/s avg); app success: 1 0 p/s (0 p/s avg); drops: 0 p/s (0 p/s avg); hitrate: 100.00% app hitrate: 50.00%
0:03 101%; sent: 2 done (39 p/s avg); recv: 2 0 p/s (0 p/s avg); app success: 1 0 p/s (0 p/s avg); drops: 0 p/s (0 p/s avg); hitrate: 100.00% app hitrate: 50.00%
Dec 19 19:14:29.022 [INFO] zmap: completedNotice the Seems it'd be a bit confusing to users if they didn't go look at the |
zakird
left a comment
zakird
left a comment
There was a problem hiding this comment.
Overall this seems fine, but personally, would have tried to have the string parsing logic in one place in the probe module instead of having counting in one place and parsing in another place such that both always have to be updated. There's a comment inline about this.
src/probe_modules/module_dns.c
Outdated
| // find how many probe_args the user wants to query | ||
| if (conf->probe_args) { | ||
| int arg_strlen = strlen(conf->probe_args); | ||
| for (int i = 0; i < arg_strlen; i++) { |
There was a problem hiding this comment.
I think that the more canonical way to do this would be to use strtok or strtok. More broadly though, I do wonder if it makes sense that this is logic is different from the logic we use for actually parsing out the questions. It seems that when parsing, we would know this information already and then wouldn't have this logic in different places.
There was a problem hiding this comment.
Fair, and thanks for the suggestion to look at stroke, seems very much like the correct way to go about this. I've re-done the logic and it seems much cleaner. I've re-tested the scenarios I outlined in the PR, mind taking a second look and we can merge if it looks good to you? @zakird
There was a problem hiding this comment.
Update: let me integrate the changes made here, they'll conflict.
There was a problem hiding this comment.
I'll re-assign you when done.
|
|
||
| const char *qopts_rn = "rn"; | ||
| static uint8_t *rdbits; | ||
| const char *qopts_rn = "nr"; // used in query to disable recursion bit in DNS header |
There was a problem hiding this comment.
Updated this, realized that we're using nr in the --help text, and I think it makes more sense as an abbreviation for no recursion
…nters no-recursion bit incorrectly
|
Ready for a re-review, logic is all in one place now @zakird |
zakird
left a comment
zakird
left a comment
There was a problem hiding this comment.
This looks sane, but I could 100% also have missed something here. Assuming that you have tested though and it works, LGTM to merge.
2 participants
Context available in Issue #746
--probe-argsthe user passed in based on count of semi-colons-Pthat isn't a multiple of the number of DNS questions-P 4 --probe-args="A,google.com;AAAA,cloudflare.com"is acceptable since 4 probes is a multiple of 2 DNS questionsNote: The
--dedupuses the scanned host's source IP address to de-duplicate responses. This means that if you send 4 probes and 2 DNS queries to8.8.8.8(2 probes per question), then ZMap only marks 1 of the 4 as a "application success". This seems mis-leading, but perhaps this is just how ZMap is designed? The de-dupeing is baked intorecv.c, so it's not probe-module specific.Tests
A,google.com;A,google.comA,google.com;A,google.com;AAAA,yahoo.comA,Notice how there's 2 responses in the
logfile.json, and that theapp hitratebeing printed maxes out at 50%