Add: PoC for alive test. #1822
Add: PoC for alive test. #1822
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Snapshot WarningsEnsure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. OpenSSF Scorecard
Scanned Files
|
jjnicola
force-pushed
the
rs-alive-detection
branch
from
29593d9 to
0388b4a
Compare
github-actions
bot
added
minor_release
jjnicola
force-pushed
the
rs-alive-detection
branch
from
0388b4a to
ac1a0d0
Compare
github-actions
bot
added
minor_release
jjnicola
force-pushed
the
rs-alive-detection
branch
from
ac1a0d0 to
0fdda51
Compare
github-actions
bot
added
minor_release
🔍 Vulnerabilities of
|
| digest | sha256:ab1046adf36fe950a0973416719f8a78f6c11a7c9882d2ca907456a8c4085f0d |
| vulnerabilities |      |
| size | 144 MB |
| packages | 261 |
📦 Base Image debian:stable-20250113-slim
| also known as |
|
| digest | sha256:9dfddad9f09eadd2541a567e0865bd223387cf490b1c8d9d1f08d3b413766841 |
| vulnerabilities |   |
Description
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
Description
Description
Description
Description
Description
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
Description
Description
Description
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
Description
Description
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
Description
Description
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
Description
Description
Description | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
Description
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
|
rust/src/alive_test/alive_test.rs
Outdated
|
|
||
| use tracing::debug; | ||
|
|
||
| /// Define IPPROTO_RAW |
There was a problem hiding this comment.
I propose removing this docstring. It doesn't say anything about what IPPROTO_RAW is, which it should if we really wanted a docstring, so it is probably meant as a comment, but as a comment it just states the obvious.
rust/src/alive_test/alive_test.rs
Outdated
| /// Define IPPROTO_RAW | ||
| const IPPROTO_RAW: i32 = 255; | ||
|
|
||
| /// Default timeout |
rust/src/alive_test/alive_test.rs
Outdated
| enum AliveTestCtl { | ||
| Stop, | ||
| // (IP and succesful detection method) | ||
| Alive(String, AliveTestMethods), |
There was a problem hiding this comment.
Personally, I prefer using either a new type inside the enum variant (Alive(Alive)) or, probably cleaner:
enum AliveTestCtl {
Stop,
Alive {
ip: String,
detection_method: AliveTestMethods,
}
}Moreover, I think representing the IP by a IpAddr instead of String makes the intent clear immediately without the need for a comment and moves the parsing logic to an earlier point in the code, which I strongly prefer.
rust/src/alive_test/alive_test.rs
Outdated
| match Socket::new_raw( | ||
| Domain::IPV4, | ||
| socket2::Type::RAW, | ||
| Some(Protocol::from(IPPROTO_RAW)), | ||
| ) { | ||
| Ok(s) => Ok(s), | ||
| Err(_) => Err(AliveTestError::NoSocket("no socket".to_string())), | ||
| } |
There was a problem hiding this comment.
This is
fn new_raw_socket() -> Result<Socket, AliveTestError> {
Socket::new_raw(
Domain::IPV4,
socket2::Type::RAW,
Some(Protocol::from(IPPROTO_RAW)),
).map_err(|_| AliveTestError::NoSocket("no socket".to_string()))
}Even better, make the error message of the NoSocket variant useful by adding the context (and removing the custom string):
.map_err(|e| AliveTestError::NoSocket(e))
There was a problem hiding this comment.
Oh! thanks! Didn't know how to use it!
rust/src/alive_test/alive_test.rs
Outdated
| async fn forge_icmp(dst: IpAddr) -> Result<Vec<u8>, AliveTestError> { | ||
| if dst.is_ipv6() { | ||
| return Err(AliveTestError::InvalidDestinationAddr( | ||
| "Invalid destination address".to_string(), |
There was a problem hiding this comment.
Again, I think adding the error message to the error variant here just makes everything more verbose without adding any value. I think this should be something like
return Err(AliveTestError::IPv6AddressInICMPDestination)
with error message defined on the AliveTestError struct itself, to keep the application logic separated from the error messages.
rust/src/alive_test/alive_test.rs
Outdated
| let _ = worker_handle.await; | ||
| let _ = worker_capture_handle.await; |
There was a problem hiding this comment.
These results should be handled and not ignored
github-actions
bot
added
minor_release
jjnicola
force-pushed
the
rs-alive-detection
branch
from
7b2a31d to
17a6712
Compare
github-actions
bot
added
minor_release
jjnicola
force-pushed
the
rs-alive-detection
branch
from
17a6712 to
e26ca26
Compare
github-actions
bot
added
minor_release
jjnicola
force-pushed
the
rs-alive-detection
branch
2 times, most recently
from
f9edf9a to
27ee979
Compare
github-actions
bot
added
minor_release
jjnicola
force-pushed
the
rs-alive-detection
branch
from
27ee979 to
76f95d0
Compare
jjnicola
force-pushed
the
rs-alive-detection
branch
from
76f95d0 to
7f3859f
Compare
github-actions
bot
added
minor_release
jjnicola
force-pushed
the
rs-alive-detection
branch
from
7f3859f to
32be533
Compare
github-actions
bot
added
minor_release
jjnicola
force-pushed
the
rs-alive-detection
branch
from
32be533 to
c449a97
Compare
github-actions
bot
added
minor_release
rust/src/alive_test/alive_test.rs
Outdated
|
|
||
| fn forge_icmp(dst: IpAddr) -> Result<Vec<u8>, AliveTestError> { | ||
| if dst.is_ipv6() { | ||
| return Err(AliveTestError::InvalidDestinationAddr); |
There was a problem hiding this comment.
I would have this method take Ipv4Addr to get rid of this check (and the superfluous one below)
rust/src/alive_test/alive_test.rs
Outdated
| // Create an icmp packet from a buffer and modify it. | ||
| let mut buf = vec![0; ICMP_LENGTH]; | ||
| let mut icmp_pkt = make_mut_icmp_packet(&mut buf)?; | ||
| icmp_pkt.set_icmp_type(IcmpTypes::EchoRequest); | ||
| icmp_pkt.set_icmp_code(IcmpCode::new(0u8)); | ||
|
|
||
| // Require an unmutable ICMP packet for checksum calculation. | ||
| // We create an unmutable from the buffer for this purpose | ||
| let icmp_aux = IcmpPacket::new(&buf).ok_or_else(|| AliveTestError::CreateIcmpPacket)?; | ||
| let chksum = pnet::packet::icmp::checksum(&icmp_aux); | ||
|
|
||
| // Because the buffer of original mutable icmp packet is borrowed, | ||
| // create a new mutable icmp packet to set the checksum in the original buffer. | ||
| let mut icmp_pkt = make_mut_icmp_packet(&mut buf)?; | ||
| icmp_pkt.set_checksum(chksum); |
There was a problem hiding this comment.
You can save all of the auxilliary packets via
| // Create an icmp packet from a buffer and modify it. | |
| let mut buf = vec![0; ICMP_LENGTH]; | |
| let mut icmp_pkt = make_mut_icmp_packet(&mut buf)?; | |
| icmp_pkt.set_icmp_type(IcmpTypes::EchoRequest); | |
| icmp_pkt.set_icmp_code(IcmpCode::new(0u8)); | |
| // Require an unmutable ICMP packet for checksum calculation. | |
| // We create an unmutable from the buffer for this purpose | |
| let icmp_aux = IcmpPacket::new(&buf).ok_or_else(|| AliveTestError::CreateIcmpPacket)?; | |
| let chksum = pnet::packet::icmp::checksum(&icmp_aux); | |
| // Because the buffer of original mutable icmp packet is borrowed, | |
| // create a new mutable icmp packet to set the checksum in the original buffer. | |
| let mut icmp_pkt = make_mut_icmp_packet(&mut buf)?; | |
| icmp_pkt.set_checksum(chksum); | |
| // Create an icmp packet from a buffer and modify it. | |
| let mut buf = vec![0; ICMP_LENGTH]; | |
| let mut icmp_pkt = make_mut_icmp_packet(&mut buf)?; | |
| icmp_pkt.set_icmp_type(IcmpTypes::EchoRequest); | |
| icmp_pkt.set_icmp_code(IcmpCode::new(0u8)); | |
| icmp_pkt.set_checksum(icmp::checksum(&icmp_pkt.to_immutable())); |
(note: I imported pnet::packet::icmp, but we can also qualify the path if you prefer)
rust/src/alive_test/alive_test.rs
Outdated
| let mut buf = vec![0; ICMP_LENGTH]; | ||
| let mut icmp_pkt = make_mut_icmp_packet(&mut buf)?; | ||
| icmp_pkt.set_icmp_type(IcmpTypes::EchoRequest); | ||
| icmp_pkt.set_icmp_code(IcmpCode::new(0u8)); |
There was a problem hiding this comment.
What's 0u8 here? Why do we set this code in particular? Would appreciate a comment or extracting the 0u8 to a descriptive constant
rust/src/alive_test/alive_test.rs
Outdated
| MutableIpv4Packet::new(&mut ip_buf).ok_or_else(|| AliveTestError::CreateIcmpPacket)?; | ||
|
|
||
| pkt.set_header_length(HEADER_LENGTH); | ||
| pkt.set_next_level_protocol(IpNextHeaderProtocol(IpNextHeaderProtocols::Icmp.0)); |
There was a problem hiding this comment.
This is a complicated way of just writing
| pkt.set_next_level_protocol(IpNextHeaderProtocol(IpNextHeaderProtocols::Icmp.0)); | |
| pkt.set_next_level_protocol(IpNextHeaderProtocols::Icmp); |
rust/src/alive_test/alive_test.rs
Outdated
| pkt.set_header_length(HEADER_LENGTH); | ||
| pkt.set_next_level_protocol(IpNextHeaderProtocol(IpNextHeaderProtocols::Icmp.0)); | ||
| pkt.set_ttl(DEFAULT_TTL); | ||
| match dst.to_string().parse::<Ipv4Addr>() { |
There was a problem hiding this comment.
It's already parsed? Simply match on the ip addr. In fact we've already checked that it's ipv4 above, so there is no need for any of this.
rust/src/alive_test/alive_test.rs
Outdated
| }, | ||
| } | ||
| fn make_mut_icmp_packet(buf: &mut Vec<u8>) -> Result<MutableIcmpPacket, AliveTestError> { | ||
| MutableIcmpPacket::new(buf).ok_or_else(|| AliveTestError::CreateIcmpPacket) |
There was a problem hiding this comment.
Note that the new method returns None only if the provided buffer is not large enough to contain the packet. Since we control the buffer size, we should be able to safely unwrap on this call to the new method (that includes some of the calls to Ipv4Packet::new and IcmpPacket::new below).
Also I regret suggesting the creation of this method, since it will only ever be called once in the revised version of the code, so maybe inlining is better after all.
rust/src/alive_test/alive_test.rs
Outdated
| enum AliveTestCtl { | ||
| Stop, | ||
| // (IP and successful detection method) | ||
| Alive { | ||
| ip: String, | ||
| detection_method: AliveTestMethods, | ||
| }, | ||
| } |
There was a problem hiding this comment.
I like having a type for this overall, but I would split this into two types, since, as far as I can tell, only the capturing thread ever sends the Alive variant and only the other task ever sends Stop. So I'd go with
pub struct Stop;
pub struct AliveHost {
ip: IpAddr, // It's an IpAddr, not a string.
detection_method: AliveTestMethods,
}
rust/src/alive_test/alive_test.rs
Outdated
| match send_handle.await { | ||
| Ok(Ok(())) => (), | ||
| Ok(Err(e)) => return Err(e), | ||
| Err(e) => return Err(AliveTestError::JoinError(e.to_string())), | ||
| }; | ||
| match capture_handle.await { | ||
| Ok(Ok(())) => (), | ||
| Ok(Err(e)) => return Err(e), | ||
| Err(e) => return Err(AliveTestError::JoinError(e.to_string())), | ||
| }; |
There was a problem hiding this comment.
I think it's safe enough to unwrap the .await call here, since the JoinError means that something failed within the task, so gracefully returning doesn't seem too useful.
I'd go with
| match send_handle.await { | |
| Ok(Ok(())) => (), | |
| Ok(Err(e)) => return Err(e), | |
| Err(e) => return Err(AliveTestError::JoinError(e.to_string())), | |
| }; | |
| match capture_handle.await { | |
| Ok(Ok(())) => (), | |
| Ok(Err(e)) => return Err(e), | |
| Err(e) => return Err(AliveTestError::JoinError(e.to_string())), | |
| }; | |
| send_handle.await.unwrap()?; | |
| capture_handle.await.unwrap()?; |
rust/src/alive_test/alive_test.rs
Outdated
| // TODO: Replace with a Storage type to store the alive host list | ||
| let mut alive = Vec::<(String, String)>::new(); | ||
|
|
||
| if self.methods.contains(&AliveTestMethods::ConsiderAlive) { | ||
| for t in self.target.iter() { | ||
| alive.push((t.clone(), AliveTestMethods::ConsiderAlive.to_string())); | ||
| println!("{t} via {}", AliveTestMethods::ConsiderAlive.to_string()) | ||
| } | ||
| return Ok(()); | ||
| }; |
There was a problem hiding this comment.
What if alive simply were Vec<AliveHost> (with AliveHost defined as above, i.e. the Alive variant in AliveTestCtl)? That seems to contain all the data we need and be cleaner than a collection of anonymous strings.
|
|
||
| let timeout = self.timeout.unwrap_or(DEFAULT_TIMEOUT_MS); | ||
| let methods = self.methods.clone(); | ||
| let send_handle = tokio::spawn(send_task(methods, trgt, timeout, tx_ctl)); |
There was a problem hiding this comment.
Note that, in the current implementation, a malformed ip address in the trgt vector leads to an error much later in the program, when in fact we already know that this isn't supposed to happen. I propose doing something like
let trgt = trgt.into_iter().map(|ip_str| Ipv4Addr::from(ip_str)).collect::<Result<Vec<_>, _>>().map_err(|e| AliveTestError::InvalidDestinationAddr)?;
and then passing that into send_task, so that we do the parsing as early as possible (and make the send_task method more explicit and cleaner as a result.
There was a problem hiding this comment.
I would like to leave this as it is for now, since it is a PoC. Later, we need to introduce a better target handling. Currently, it only supports a comma separated IPv4 address list. But it should accept a Vec<Target>, where Target is an enum representing an IPv4, an IPv6 or a Hostname.
There was a problem hiding this comment.
That's a good point. In that case, I'd prefer doing exactly that: Make the argument a Vec<Target>, and define Target (for the PoC) as
enum Target {
Ipv4(Ipv4Address),
}This way, we already do the "right thing" and can simply add the new variants when we want to support Ipv6 etc. In the current code, you'd need to remember that you need to replace every instance of parsing Ipv4 from a String with the appropriate logic. But if you prefer leaving it as it is, that is fine too
ArnoStiefvater
added
no_release
It only support icmpv4 For testing, compile with nasl-builtin-raw-ip feature and run `sudo target/debug/scannerctl alivetest --icmp -t 192.168.0.1,192.168.0.2,192.168.0.3 --timeout 5000 --verbose`
jjnicola
force-pushed
the
rs-alive-detection
branch
from
c449a97 to
c24c433
Compare
| @@ -80,7 +80,8 @@ walkdir = "2" | |||
| x509-certificate = "0.23.1" | |||
| x509-parser = "0.16.0" | |||
|
|
|||
| pcap = { version = "1.0.0", optional = true } | |||
| rayon = { version = "1.8.0", optional = true } | |||
| ip_buf.append(icmp_buf); | ||
| let total_length = ip_buf.len(); | ||
| let mut pkt = | ||
| MutableIpv4Packet::new(&mut ip_buf).ok_or_else(|| AliveTestError::CreateIcmpPacket)?; |
There was a problem hiding this comment.
It should be the same as above here, we know the buffer has IP_LENGTH, so we can unwrap here.
| } | ||
|
|
||
|
|
||
| fn forge_icmp_packet() -> Result<Vec<u8>, AliveTestError> { |
There was a problem hiding this comment.
This function always returns Ok() so we can get rid of the result
| let chksum = checksum(&pkt.to_immutable()); | ||
| pkt.set_checksum(chksum); | ||
|
|
||
| Ipv4Packet::owned(ip_buf).ok_or_else(|| AliveTestError::CreateIcmpPacket) |
There was a problem hiding this comment.
Same here, this is always safe to unwrap. As a result, this method will never fail, so we can change the return type to Ipv4Packet<'static>
|
|
||
| } | ||
|
|
||
| fn forge_icmp(dst: Ipv4Addr) -> Result<Ipv4Packet<'static>, AliveTestError> { |
There was a problem hiding this comment.
This method will also never fail afterwards.
| } | ||
|
|
||
| fn process_ip_packet(packet: &[u8]) -> Result<Option<AliveHostCtl>, AliveTestError> { | ||
| let pkt = Ipv4Packet::new(&packet[16..]).ok_or_else(|| AliveTestError::CreateIcmpPacket)?; |
There was a problem hiding this comment.
The error message here will say
"Wrong buffer size. Not possible to create an ICMP packet"
but we're trying to create an IP packet. I think this should be AliveTestError::CreateIpPacket or even AliveTestError::WrongBufferSizeForIpPacket(packet.len() - 16) (something like that)
| methods: Vec<AliveTestMethods>, | ||
| ) -> Result<(), CliError> { | ||
| let s = Scanner::new(target, methods, timeout); | ||
| let _ = s.run_alive_test().await; |
There was a problem hiding this comment.
This ignores the result. We should at least print the error message in an Err case.
What:
PoC for alive test.
It only support icmpv4
Jira: SC-1104
Why:
How:
For testing, compile with nasl-builtin-raw-ip feature and run
sudo target/debug/scannerctl alivetest --icmp -t 192.168.0.1,192.168.0.2,192.168.0.3 --timeout 5000 --verboseChecklist: