Extract IPs and CIDRs from SPF TXT records#3144
Conversation
|
I have read the CLA Document and I hereby sign the CLA 1 out of 2 committers have signed the CLA. |
|
I have read the CLA Document and I hereby sign the CLA |
|
recheck |
64cb3c1 to
430e1e8
Compare
|
hey @thunderstornX, thanks for the PR. There are some issues with your module as-written now we'd need to address though: Redundant ResolutionsThe biggest thing is you are doing redundant DNS resolution. BBOT will have already resolved every single DNS_NAME including all the rdtypes (by default), to include txt. So the: line is doing completely redundant work. We have some modules that re-resolve, but they are all doing that to some modified version of the domain, not the same domain again. You should be able to read TXT data directly from `event.raw_dns_records["TXT"] Feature (mostly) already implementedDNS_NAMEs are already being extracted from txt records. this happens in extract_targets() in dns/helpers.py. It is not getting IP addresses or CIDRs, though - which is what that issue you referenced was talking about. I am not sure of the utility of those, we can certain emit them as IPs, but if they aren't in scope, nothing will happen to them anyway. I think the value is probably just the added context they provide, more as an end product than something to feed back into the scan. There are a few other minor things, but given the other ones I pointed out, they will probably not remain relevant. I think ultimately this probably needs to be a much smaller change, modifying extract_targets so it also uses our pre-existing cidr/ip regexes, etc. |
extract_targets already pulled hostnames from TXT content; it now also extracts the ip4:/ip6: IPs and CIDR ranges using bbot's existing ip/cidr regexes, reading from the already-resolved TXT (no extra resolution). The mechanism prefix is stripped before matching so it doesn't pollute the IPv6 match, and a CIDR's bare network address is not emitted alongside it. Closes blacklanternsecurity#2545
430e1e8 to
1ad043f
Compare
|
Thanks for the thorough review @liquidsec — all fair points. Reworked per your suggestion:
Happy to gate it to |
|
Alright, theres some more things to go over: Don't use stdlib re, use import regex as reBBOT uses the third-party regex package everywhere, not stdlib re. It's a drop-in replacement but releases the GIL during matching, which matters for BBOT's async/threaded architecture. All existing code (regexes.py, misc.py, regex.py) uses import regex as re. Any new regex patterns should either go in regexes.py and be imported, or use regex if compiled locally. CIDRs are silently dropped currentlyextract_targets would now return CIDRs, but its only consumer, which turns it into an event hardcodes to DNS_NAME as event_type. This will mean it will fail validation and just do nothing. Instead, we should be abel to set None as the event type, and let the event auto detection handle assign it a type. Gate on v=spf1We should do a cheap check for v=spf1 before we do any regexes. bbot/core/helpers/regexes.pywe generally centralize our regexes here. If theres even a remote chance it will be used again, put it there and import it over TestsLets make a test that validates all the way from coming in via a txt record to being emitted as a an IP_ADDRESS or IP_RANGE. Such a test would have caught the second issue i mentioned, i think. Also a test to verify the macros are skipped |
The SPF mechanism prefix regex moves to regexes.py (compiled with the regex package like the rest). IP/CIDR extraction now only runs on records containing v=spf1, and SPF macro tokens are skipped. DNS child events no longer hardcode DNS_NAME: an IP was coerced anyway, but a CIDR failed validation and was silently dropped, so the type is now auto-detected (DNS_NAME / IP_ADDRESS / IP_RANGE). Adds an end-to-end test from TXT record to emitted IP_ADDRESS / IP_RANGE events, plus macro and gating coverage.
|
Addressed all of it:
Unrelated heads-up from running the suite: |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #3144 +/- ##
======================================
+ Coverage 90% 91% +1%
======================================
Files 448 448
Lines 42614 42732 +118
======================================
+ Hits 38340 38478 +138
+ Misses 4274 4254 -20 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
CodeQL's incomplete-url-substring-sanitization query flags hostname string literals in membership asserts; these are exact set-membership checks, so switch the three flagged asserts to subset comparisons.
|
CodeQL's three alerts were false positives on exact set-membership asserts in the new tests; switched those to subset comparisons. The benchmark job's failure is only the comment-on-PR step (fork token), and ubuntu:24.04 died on Docker Hub registry timeouts before running anything. |
|
looking good! little swamped at the moment but will give this a final check and hopefully merge soon |
|
Tested against some records i threw up onto one of my domains. I think everything is working perfectly. We can merge pending @ausmaster final check. One thing to keep in mind: with the default report distance of 0 - these usually wont be visible at all, unless they happen to land in scope. They will still be going to modules regardless. This is fine and expected, just wanted to throw that out there. There are definitely some edge cases where they will feed discovery of new in scope assets, particularly when the scope is a giant ip range. |
Prevents speculate from expanding out-of-scope CIDRs (e.g. from SPF records) into individual IP_ADDRESS events that no downstream module can use. Children land at distance+1, so enumerating a range beyond search_distance just produces unreachable pipeline churn.
|
Hey, we discovered a pre-existing potential issue that this was kindof adjacent to, so we went ahead and tacked the fix onto this PR to make sure all tests pass together. Will be merging once tests pass. |
ausmaster
left a comment
There was a problem hiding this comment.
A great PR, thanks very much @thunderstornX for your contribution!
|
Hey @liquidsec, thank you for this, and for the patience across both rounds. The depth of the review genuinely helped: the redundant resolution, the CIDR event type that was getting silently dropped, and centralizing the regex all made the change tighter and taught me a lot about how BBOT fits together. Good point on the default report distance of 0. The IPs and ranges still reach the modules even when they aren't surfaced, and the giant-IP-range scope is a neat case where they could pull in new in-scope assets. And no problem at all tacking the adjacent fix onto the PR, glad if it helps everything land together. Really appreciate it, this has been a great first experience contributing here. |
Replaces the full inline CLA workflow with a thin caller that references the reusable workflow in blacklanternsecurity/.github. CLA logic is now maintained in one place across all BLS repos. This also fixes the bug where org members (e.g. liquidsec) were flagged as unsigned when they committed to an external contributor's PR (#3144). Depends on: blacklanternsecurity/.github#1
|
Thank you @ausmaster, that means a lot. Really glad it came together well. |
Replaces the full inline CLA workflow with a thin caller that references the reusable workflow in blacklanternsecurity/.github. CLA logic is now maintained in one place across all BLS repos. This also fixes the bug where org members (e.g. liquidsec) were flagged as unsigned when they committed to an external contributor's PR (#3144). Depends on: blacklanternsecurity/.github#1
thanks for sticking with it! Hope we see more from you. |
|
recheck |
|
recheck |
1 similar comment
|
recheck |
Summary
Adds a passive
dnsspfmodule that parses SPF (Sender Policy Framework) DNSTXTrecords and emits the infrastructure they disclose. Closes #2545.SPF records frequently expose target-owned infrastructure — individual IPs, whole subnets, and related domains — which is useful for attack-surface mapping. The subnets in particular (the original ask in #2545) are often owned by the target.
Implementation
The issue suggested doing this in
excavate, butexcavateonly watchesHTTP_RESPONSE/RAW_TEXTand never sees DNS records. bbot already has a clear pattern of per-record DNS-TXTparser modules —dnscaa,dnstlsrpt,dnsbimi— so I implemented this as a dedicateddnsspfmodule following that same pattern. Happy to fold it intoexcavateinstead if you'd prefer.The module watches
DNS_NAME, resolves the host's SPF record, and emits:IP_NETWORKforip4:/ip6:CIDR subnetsIP_ADDRESSfor individualip4:/ip6:hostsDNS_NAMEfor domains referenced viainclude:/a:/mx:/redirect=/exists:RAW_DNS_RECORD(optional, off by default)Each output type is individually toggleable via module options. SPF macros (
%{...}) are skipped, and malformed addresses are ignored.Testing
bbot/test/test_step_2/module_tests/test_module_dnsspf.py— covers a single IPv4, an IPv4 subnet, an IPv6 subnet, and theinclude:/a:domains. Passing.ruff check/ruff format --checkclean.