-
Notifications
You must be signed in to change notification settings - Fork 27
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
pivot.arp_scan() #258
pivot.arp_scan() #258
Conversation
Codecov Report
@@ Coverage Diff @@
## main #258 +/- ##
==========================================
+ Coverage 73.24% 74.13% +0.89%
==========================================
Files 88 88
Lines 5490 5781 +291
==========================================
+ Hits 4021 4286 +265
- Misses 1383 1409 +26
Partials 86 86
... and 13 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🌶️ feature very excited to have this.
Few change requests.
Also please add tests and docs. there may also be a few unwraps I didn’t tag. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First pass.
Few thoughts on error handling.
Please update the arp scan docs.
implants/lib/eldritch/Cargo.toml
Outdated
@@ -40,6 +40,8 @@ windows-sys = { workspace = true, features = [ | |||
"Win32_Security", | |||
]} | |||
whoami = { workspace = true } | |||
pnet = "0.34.0" | |||
ipnetwork = "0.20.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add these to the workspace root and use workspace true in the package. This way if another package wants to use a crate they'll be kept in step with the main version. If specific libs need to break with the current workspace version we can do that but should avoid it.
implants/lib/eldritch/src/pivot.rs
Outdated
starlark_heap: &'v Heap, | ||
target_cidrs: Vec<String>, | ||
) -> anyhow::Result<Vec<Dict<'v>>> { | ||
if false { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we leave this as a single line for now. Would like to maintain consistency even while we look to auto formatting.
) { | ||
if interface.ips.iter().filter(|ip| ip.is_ipv4()).count() == 0 { | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to return an error? Might be nice in case something breaks to know why.
let (mut tx, mut rx) = match channel(&interface, Default::default()) { | ||
Ok(Ethernet(tx, rx)) => (tx, rx), | ||
Ok(_) => panic!("Unhandled channel type"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe if you return an error here instead of panicking the thread will close and pass back the error message to join similar to panic.
If we can do that instead that would be my preference even if it only achieves the same effect as panic it will be more reusable and also clearer that the function itself shouldn't panic.
Ok(elapsed) => elapsed, | ||
Err(err) => { | ||
println!("Failed to get elapsed time on {}: {}", interface.name, err); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want to avoid println in production code eldritch should only print when a user calls print (print calls in eldritch have a special handler to ensure they get back to the c2 rust prints wont get recorded). If you want to preserve debug strings like this you can wrap them in an if debug and set debug to false.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's one println left and please add docs to the docs/_docs/user-guide/eldritch.md#pivot.arp_scan
currently just has boiler plate.
When adding docs make sure to include an example of the Dict output and document any edge cases or OS requirements.
Once those two are done go ahead and merge o7
* ARP Scan * Fixup Error Messages * Remove all unwraps * Add Tests * Fix Windows Compile Error * Fix Windows Test * Change listener thread to return result * Update Dependency Location and Pivot.rs format * Fixup Thread Failure * Docs and Limiting Println to Debug --------- Co-authored-by: Hulto <7121375+hulto@users.noreply.github.com>
* ARP Scan * Fixup Error Messages * Remove all unwraps * Add Tests * Fix Windows Compile Error * Fix Windows Test * Change listener thread to return result * Update Dependency Location and Pivot.rs format * Fixup Thread Failure * Docs and Limiting Println to Debug --------- Co-authored-by: Hulto <7121375+hulto@users.noreply.github.com>
* ARP Scan * Fixup Error Messages * Remove all unwraps * Add Tests * Fix Windows Compile Error * Fix Windows Test * Change listener thread to return result * Update Dependency Location and Pivot.rs format * Fixup Thread Failure * Docs and Limiting Println to Debug --------- Co-authored-by: Hulto <7121375+hulto@users.noreply.github.com>
pivot.arp_scan() (#258) * ARP Scan * Fixup Error Messages * Remove all unwraps * Add Tests * Fix Windows Compile Error * Fix Windows Test * Change listener thread to return result * Update Dependency Location and Pivot.rs format * Fixup Thread Failure * Docs and Limiting Println to Debug --------- Co-authored-by: Hulto <7121375+hulto@users.noreply.github.com>
What type of PR is this?
/kind feature
What this PR does / why we need it:
Adds ARP scan capability.
Will scan given subnets' IPs using ARP requests and checking for ARP replies. Does this multithreaded, one thread per subnet.
Which issue(s) this PR fixes:
Fixes #239
Note:
I will take any advice on how to test this!