Add host nasl built-in functions #1758
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 |
🔍 Vulnerabilities of
|
| digest | sha256:e47ae0cf4c56fade3133467b16e1aa866a6a8677cbc18a4240400571be8d8c37 |
| vulnerabilities |      |
| size | 143 MB |
| packages | 262 |
📦 Base Image debian:stable-20241111-slim
| also known as |
|
| digest | sha256:4821d386c4d96ba06dbaa992f225af5b7aa6d5cc400dfbf993df44526bb843d0 |
| 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
|
rust/src/nasl/builtin/host/mod.rs
Outdated
| let mut v = Vec::new(); | ||
| if let Some(vh) = context.target_vhosts() { | ||
| v = vh | ||
| .into_iter() | ||
| .map(|(v, _s)| NaslValue::String(v)) | ||
| .collect::<Vec<_>>(); | ||
| } |
There was a problem hiding this comment.
| let mut v = Vec::new(); | |
| if let Some(vh) = context.target_vhosts() { | |
| v = vh | |
| .into_iter() | |
| .map(|(v, _s)| NaslValue::String(v)) | |
| .collect::<Vec<_>>(); | |
| } | |
| let v = if let Some(vh) = context.target_vhosts() { | |
| vh.into_iter() | |
| .map(|(v, _s)| NaslValue::String(v)) | |
| .collect::<Vec<_>>() | |
| } else { | |
| vec![] | |
| }; |
| if !v.is_empty() { | ||
| return Ok(NaslValue::Fork(v)); | ||
| } |
There was a problem hiding this comment.
Where is the difference between context.target_vhosts() = None and context.target_vhosts() = Some(vec![])? Should we really treat them the same way?
rust/src/nasl/builtin/host/mod.rs
Outdated
| if let Ok(ip) = get_host_ip(context) { | ||
| match lookup_addr(&ip) { | ||
| Ok(host) => Ok(NaslValue::String(host)), | ||
| Err(_) => Ok(NaslValue::String(ip.to_string())), | ||
| } | ||
| } else { | ||
| Ok(NaslValue::String(context.target().to_string())) | ||
| } |
There was a problem hiding this comment.
Nit: I think doing the conversion to NaslValue at the last possible place makes this a little nicer to read. I also prefer using two matches.
| if let Ok(ip) = get_host_ip(context) { | |
| match lookup_addr(&ip) { | |
| Ok(host) => Ok(NaslValue::String(host)), | |
| Err(_) => Ok(NaslValue::String(ip.to_string())), | |
| } | |
| } else { | |
| Ok(NaslValue::String(context.target().to_string())) | |
| } | |
| let host = match get_host_ip(context) { | |
| Ok(ip) => match lookup_addr(&ip) { | |
| Ok(host) => host, | |
| Err(_) => ip.to_string(), | |
| }, | |
| Err(_) => context.target().to_string(), | |
| }; | |
| Ok(NaslValue::String(host)) |
rust/src/nasl/builtin/host/mod.rs
Outdated
| ///Expands the vHosts list with the given hostname. | ||
| ///The mandatory parameter hostname is of type string. It contains the hostname which should be added to the list of vHosts | ||
| ///Additionally a source, how the hostname was detected can be added with the named argument source as a string. If it is not given, the value NASL is set as default. | ||
| pub fn add_host_name( | ||
| register: &Register, | ||
| context: &Context, | ||
| ) -> Result<NaslValue, FunctionErrorKind> { | ||
| let hostname = match register.named("hostname") { | ||
| Some(ContextType::Value(NaslValue::String(x))) if !x.is_empty() => x.clone(), | ||
| _ => { | ||
| return Err(FunctionErrorKind::diagnostic_ret_null("Empty Hostname")); | ||
| } | ||
| }; | ||
| let source = match register.named("source") { | ||
| Some(ContextType::Value(NaslValue::String(x))) if !x.is_empty() => x.clone(), | ||
| _ => "NASL".to_string(), | ||
| }; | ||
|
|
||
| context.add_hostname(hostname, source); | ||
| Ok(NaslValue::Null) | ||
| } |
There was a problem hiding this comment.
I realized that this pattern appears more than once, so I think the cleanest is to implement a custom struct that performs the non-empty check and let the type system do the work for us
| ///Expands the vHosts list with the given hostname. | |
| ///The mandatory parameter hostname is of type string. It contains the hostname which should be added to the list of vHosts | |
| ///Additionally a source, how the hostname was detected can be added with the named argument source as a string. If it is not given, the value NASL is set as default. | |
| pub fn add_host_name( | |
| register: &Register, | |
| context: &Context, | |
| ) -> Result<NaslValue, FunctionErrorKind> { | |
| let hostname = match register.named("hostname") { | |
| Some(ContextType::Value(NaslValue::String(x))) if !x.is_empty() => x.clone(), | |
| _ => { | |
| return Err(FunctionErrorKind::diagnostic_ret_null("Empty Hostname")); | |
| } | |
| }; | |
| let source = match register.named("source") { | |
| Some(ContextType::Value(NaslValue::String(x))) if !x.is_empty() => x.clone(), | |
| _ => "NASL".to_string(), | |
| }; | |
| context.add_hostname(hostname, source); | |
| Ok(NaslValue::Null) | |
| } | |
| struct Hostname(String); | |
| impl<'a> FromNaslValue<'a> for Hostname { | |
| fn from_nasl_value(value: &'a NaslValue) -> Result<Self, FunctionErrorKind> { | |
| let str = String::from_nasl_value(value)?; | |
| if str.is_empty() { | |
| Err(FunctionErrorKind::diagnostic_ret_null("Empty hostname.")) | |
| } | |
| else { | |
| Ok(Self(str)) | |
| } | |
| } | |
| } | |
| ///Expands the vHosts list with the given hostname. | |
| ///The mandatory parameter hostname is of type string. It contains the hostname which should be added to the list of vHosts | |
| ///Additionally a source, how the hostname was detected can be added with the named argument source as a string. If it is not given, the value NASL is set as default. | |
| #[nasl_function] | |
| pub fn add_host_name( | |
| context: &Context, | |
| hostname: Hostname, | |
| source: Option<&str>, | |
| ) -> Result<(), FunctionErrorKind> { | |
| let source = source.filter(|x| x.is_empty()).unwrap_or("NASL"); | |
| context.add_hostname(hostname.0.into(), source.into()); | |
| Ok(()) | |
| } |
rust/src/nasl/builtin/host/mod.rs
Outdated
| let hostname = match register.named("hostname") { | ||
| Some(ContextType::Value(NaslValue::String(x))) if !x.is_empty() => x.clone(), | ||
| _ => { | ||
| return Err(FunctionErrorKind::diagnostic_ret_null("Empty Hostname")); | ||
| } | ||
| }; |
There was a problem hiding this comment.
If you define the Hostname type as above and use the nasl_function macro, all of this can go.
rust/src/nasl/builtin/host/mod.rs
Outdated
| let mut flag = false; | ||
| for a1 in addr1.iter() { | ||
| for a2 in addr2.iter() { | ||
| if a1.eq(a2) { | ||
| flag = true; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if cmp_hostname { | ||
| for hn1 in hostnames1.iter() { | ||
| for hn2 in hostnames2.iter() { | ||
| if hn1.is_ok() && hn2.is_ok() && hn1.as_ref().unwrap() == hn2.as_ref().unwrap() { | ||
| flag = true; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| Ok(NaslValue::Boolean(flag)) |
There was a problem hiding this comment.
Using the filter_map expression from above, you can rewrite this entire thing as
| let mut flag = false; | |
| for a1 in addr1.iter() { | |
| for a2 in addr2.iter() { | |
| if a1.eq(a2) { | |
| flag = true; | |
| } | |
| } | |
| } | |
| if cmp_hostname { | |
| for hn1 in hostnames1.iter() { | |
| for hn2 in hostnames2.iter() { | |
| if hn1.is_ok() && hn2.is_ok() && hn1.as_ref().unwrap() == hn2.as_ref().unwrap() { | |
| flag = true; | |
| } | |
| } | |
| } | |
| } | |
| Ok(NaslValue::Boolean(flag)) | |
| let any_ip_address_matches = addr1.iter().any(|a1| addr2.contains(a1)); | |
| let any_hostname_matches = hostnames1.iter().any(|h1| hostnames2.contains(h1)); | |
| Ok(NaslValue::Boolean( | |
| any_ip_address_matches || (cmp_hostname && any_hostname_matches), | |
| )) |
If we then use nasl_function! and make the function infallible and simply return bool, the Ok(NaslValue::Boolean(...)) can go too
rust/src/nasl/utils/hosts.rs
Outdated
|
|
||
| pub fn resolve( | ||
| mut hostname: String, | ||
| ) -> Result<Option<Box<dyn Iterator<Item = SocketAddr>>>, FunctionErrorKind> { |
There was a problem hiding this comment.
Is this really the return type we want?
- Why the
Option? The None variant is never even constructed. - Why the boxed dyn iterator?
impl Iteratorshould work here
| mut hostname: String, | ||
| ) -> Result<Option<Box<dyn Iterator<Item = SocketAddr>>>, FunctionErrorKind> { | ||
| //std::net to_socket_addrs() requires a port. Therefore, using a dummy port | ||
| hostname.push_str(":5000"); |
There was a problem hiding this comment.
Given that we add a dummy port, we shouldn't be returning SocketAddr from this function, but rather return an IpAddr, since that is the only valuable information a caller can extract.
| // is considered a "blocking" operation and `tokio::task::spawn_blocking` | ||
| // should be used. | ||
| /// vhost list which resolve to the IP address and their sources. | ||
| vhosts: Mutex<Vec<(String, String)>>, |
There was a problem hiding this comment.
Comment: I know we talked about this and didnt find a short-term solution that was any better than this mutex, but I think it is a shame that we have to resort to mutexes all over the place instead of finding a more robust solution that passes the responsibility of handling shared/mutable state to the interpreter/executor.
rust/src/nasl/builtin/host/mod.rs
Outdated
| match resolve(hostname)? { | ||
| Some(mut a) => { | ||
| let address = a.next().map_or_else(String::new, |x| x.to_string()); | ||
| let address = &address[..(address.len() - 5)]; |
There was a problem hiding this comment.
What is the 5? I think what youre trying to do is strip the port from the ip addr? This should be changed by returning IpAddr from resolve (which is the only thing that makes sense anyways, see below) and not by doing custom parsing logic on the Display result of SocketAddr
rust/src/nasl/builtin/host/mod.rs
Outdated
| fn resolve_host_name(hostname: Hostname) -> String { | ||
| resolve(hostname.0).map_or_else( | ||
| |_| "127.0.0.1".to_string(), | ||
| |x| x.first().unwrap().to_string(), |
There was a problem hiding this comment.
Is this unwrap here ok? I don't see an immediate reason why resolve can't return an empty Vec.
rust/src/nasl/builtin/host/mod.rs
Outdated
| h1: &str, | ||
| h2: &str, | ||
| cmp_hostname: Option<bool>, | ||
| ) -> Result<NaslValue, FunctionErrorKind> { |
There was a problem hiding this comment.
Nit: You could change the return type to Result<bool, _>
rust/src/nasl/builtin/host/mod.rs
Outdated
| .clone() | ||
| .into_iter() | ||
| .map(|x| lookup_addr(&x)) | ||
| .filter_map(|x| lookup_addr(&x).ok()) |
There was a problem hiding this comment.
Haven't tested, but you should be able to get rid of the clone by using .iter() instead. Something like this:
| .clone() | |
| .into_iter() | |
| .map(|x| lookup_addr(&x)) | |
| .filter_map(|x| lookup_addr(&x).ok()) | |
| let hostnames1 = addr1 | |
| .iter() | |
| .filter_map(|x| lookup_addr(x).ok()) |
rust/src/nasl/utils/hosts.rs
Outdated
| Ok(addr) => { | ||
| let ips = addr | ||
| .into_iter() | ||
| .map(|x| x.ip().to_string()) |
There was a problem hiding this comment.
Why the .to_string()? It makes more sense to me to be explicit about what this function returns and to simply return the IpAddr instead.
rust/src/nasl/utils/hosts.rs
Outdated
| let ips = addr | ||
| .into_iter() | ||
| .map(|x| x.ip().to_string()) | ||
| .collect::<Vec<_>>(); |
There was a problem hiding this comment.
If I am not mistaken, every call site of this function immediately turns the Vec into an iterator anyways, so we can save the alloc and return the iterator (but using impl Iterator<Item=IpAddr> instead of Box<dyn Iterator<...>>)
What:
Add host nasl built-in functions
Jira: SC-1189
Why:
How:
Checklist: