NASL builtin function for open privileged sockets with udp and tcp #1757
Conversation
There were some issues with the socket functions: - The position of the context argument must be the last argument of a nasl function - The named argument `len` for `recv()` must be called `length` instead
Before it was not possible anymore to receive the target, as it was neer set within the context
Also implements the buffer for TCP connections, as received messages must be buffered in order to read them line by line.
Split socket into different modules for handling tcp and udp connections. This reduces complexety of the socket module.
recv may return before length bytes have been read: as soon as at least one byte has been received, the timeout is lowered to 1 second. If no data is received during that time, the function returns the already read data; otherwise, if the full initial timeout has not been reached, a 1 second timeout is re-armed and the script tries to receive more data from the socket. This special feature was implemented to get a good compromise between reliability and speed when openvas-scanner talks to unknown or complex protocols. This functionality was missing.
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
|
🔍 Vulnerabilities of
|
| digest | sha256:07736a1ac2e79119ab253700cc52d80551a24d42bf5be75a30d2dc30c9da3502 |
| vulnerabilities |      |
| size | 138 MB |
| packages | 262 |
📦 Base Image debian:stable-20241016-slim
| also known as |
|
| digest | sha256:d337e8c1770de491cd165b65051f49455ff20fdd26b296d24650876aefaa445f |
| 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
|
Kraemii
force-pushed
the
rs-priv-sockets
branch
from
f4ed020
to
14d740b
Compare
| display("was socket created: ", !isnull(sock)); | ||
| display("fd: ", sock); | ||
| display("is function ftp_log_in defined: ", defined_func("ftp_log_in")); | ||
| # Login data for ftp://ftp.dlptest.com/ provided by https://dlptest.com/ftp-test/ |
There was a problem hiding this comment.
q: are we allowed to just use that in our public repository?
| .and_then(|_| local_socket.local_addr().ok()) | ||
| .and_then(|l_addr| IpAddr::from_str(&l_addr.ip().to_string()).ok()) | ||
| .ok_or_else(|| FunctionErrorKind::Diagnostic("No route to destination".to_string(), None)) | ||
| local_socket.connect(sd)?; |
There was a problem hiding this comment.
q: don't we need a disconnect/close before we return?
| // TLS/SSL | ||
| Some(tls_version) => match tls_version { | ||
| OpenvasEncaps::Tls12 | OpenvasEncaps::Tls13 => { | ||
| if let Ok(tls_conf) = Self::get_tls_conf(context) { |
There was a problem hiding this comment.
nit: can you separate those blocks and may give them a name? This is hard to read and the else branch is always continue.
There was a problem hiding this comment.
I agree. Might be a case for .and_then
| } | ||
| _ => { | ||
| return Err(FunctionErrorKind::WrongArgument(format!( | ||
| "unsupported transport layer: {transport}{tls_version}" |
There was a problem hiding this comment.
q: although it is a wrong argument, that wrong argument could have been valid on a previously installed version (when e.g. a tls_version drops a transport layer, hence I think it should be a Diagnostic instead?
| impl Write for UdpConnection { | ||
| fn write(&mut self, buf: &[u8]) -> io::Result<usize> { | ||
| let mtu = mtu(self.socket.peer_addr()?.ip()); | ||
| if buf.len() < mtu { |
There was a problem hiding this comment.
nit: I think guardian instruction are easier to read when the return early, so instead of writing it like this I would propose to switch the order:
if buf.len() >= mtu {
return Err(io::Error::new(...);
}
result = ...
if result < 0 {
Err(...)
} else {
Ok(result as usize)
}
That way a reader can immediately skip the guardian check without having to keep the branch in mind and concentrate on the result generation.
| } | ||
|
|
||
| fn flush(&mut self) -> io::Result<()> { | ||
| Ok(()) |
There was a problem hiding this comment.
q: don't we need flushing for the case the device waits until it sends for optimization or something?
| // TLS/SSL | ||
| Some(tls_version) => match tls_version { | ||
| OpenvasEncaps::Tls12 | OpenvasEncaps::Tls13 => { | ||
| if let Ok(tls_conf) = Self::get_tls_conf(context) { |
There was a problem hiding this comment.
I agree. Might be a case for .and_then
| let cert_path = match get_kb_item(context, "Secret/tls_cert")? { | ||
| Some(x) => Ok(x.to_string()), | ||
| None => Err(FunctionErrorKind::Diagnostic( | ||
| "KB key 'Secret/tls_cert' is not set".to_string(), | ||
| None, | ||
| )), | ||
| }?; | ||
|
|
||
| let key_path = match get_kb_item(context, "Secret/tls_key")? { | ||
| Some(x) => Ok(x.to_string()), | ||
| None => Err(FunctionErrorKind::Diagnostic( | ||
| "KB key 'Secret/tls_key' is not set".to_string(), | ||
| None, | ||
| )), | ||
| }?; | ||
|
|
||
| let password = match get_kb_item(context, "Secret/tls_password")? { | ||
| Some(x) => Ok(x.to_string()), | ||
| None => Err(FunctionErrorKind::Diagnostic( | ||
| "KB key 'Secret/tls_password' is not set".to_string(), | ||
| None, | ||
| )), | ||
| }?; | ||
|
|
||
| let cafile_path = match get_kb_item(context, "Secret/tls_cafile")? { | ||
| Some(x) => Ok(x.to_string()), | ||
| None => Err(FunctionErrorKind::Diagnostic( | ||
| "KB key 'Secret/tls_cafile' is not set".to_string(), | ||
| None, | ||
| )), | ||
| }?; |
There was a problem hiding this comment.
| let cert_path = match get_kb_item(context, "Secret/tls_cert")? { | |
| Some(x) => Ok(x.to_string()), | |
| None => Err(FunctionErrorKind::Diagnostic( | |
| "KB key 'Secret/tls_cert' is not set".to_string(), | |
| None, | |
| )), | |
| }?; | |
| let key_path = match get_kb_item(context, "Secret/tls_key")? { | |
| Some(x) => Ok(x.to_string()), | |
| None => Err(FunctionErrorKind::Diagnostic( | |
| "KB key 'Secret/tls_key' is not set".to_string(), | |
| None, | |
| )), | |
| }?; | |
| let password = match get_kb_item(context, "Secret/tls_password")? { | |
| Some(x) => Ok(x.to_string()), | |
| None => Err(FunctionErrorKind::Diagnostic( | |
| "KB key 'Secret/tls_password' is not set".to_string(), | |
| None, | |
| )), | |
| }?; | |
| let cafile_path = match get_kb_item(context, "Secret/tls_cafile")? { | |
| Some(x) => Ok(x.to_string()), | |
| None => Err(FunctionErrorKind::Diagnostic( | |
| "KB key 'Secret/tls_cafile' is not set".to_string(), | |
| None, | |
| )), | |
| }?; | |
| let get_kb_string = |name| { | |
| match get_kb_item(context, name)? { | |
| Some(x) => Ok(x.to_string()), | |
| None => Err(FunctionErrorKind::Diagnostic( | |
| format!("KB key {} is not set", name), | |
| None, | |
| )), | |
| } | |
| }; | |
| let cert_path = get_kb_string("Secret/tls_cert")?; | |
| let key_path = get_kb_string("Secret/tls_key")?; | |
| let password = get_kb_string("Secret/tls_password")?; | |
| let cafile_path = get_kb_string("Secret/tls_cafile")?; |
There was a problem hiding this comment.
or better yet make the closure a function and use it in open_sock_kdc as well
| let mut sport = 1023; | ||
|
|
||
| while sport > 0 { | ||
| if let Ok(udp) = UdpConnection::new_priv(addr, sport, dport) { | ||
| let fd = self.add(NaslSocket::Udp(udp)); | ||
| return Ok(NaslValue::Number(fd as i64)); | ||
| } | ||
| sport -= 1; | ||
| } |
There was a problem hiding this comment.
I think
for sport in (1..=1023).rev() {
}is a little cleaner, but its debatable
| Tcp(Box<TCPConnection>), | ||
| Udp(UDPConnection), | ||
| Tcp(Box<TcpConnection>), | ||
| Udp(UdpConnection), | ||
| Close, |
There was a problem hiding this comment.
Nitpick: Rename this to Closed?
What:
Adds the NASL functions for open privileged sockets for TCP and UDP:
I added two NASL script for testing TCP and UDP. Note that you need privileged
This PR depends on #1727
Jira: SC-1120
Why:
How:
Checklist: