Iprep feature 6857/v1 #11057
Iprep feature 6857/v1 #11057
Conversation
No need to init ptrs to NULL after SCCalloc.
Implement special "isset" and "isnotset" modes. "isset" matches if an IP address is part of an iprep category with any value. It is internally implemented as ">=,0", which should always be true if there is a value to evaluate, as valid reputation values are 0-127. "isnotset" matches if an IP address is not part of an iprep category. It is internally stored as "=,255", but has some special case handling in the `DetectIPRepMatch` function. Ticket: OISF#6857.
|
Information: QA ran without warnings. Pipeline 20591 |
| } | ||
| let (_, values) = nom7::multi::separated_list1( | ||
| tag(","), | ||
| preceded(multispace0, nom7::bytes::complete::is_not(",")), |
There was a problem hiding this comment.
Why this change ?
This is normally used if you do not care about the subparts of a keyword
There was a problem hiding this comment.
Guess I'm not really sure what other ways there are to do it. I just want to split by , or end of string.
There was a problem hiding this comment.
btw took this directly from the bytemath parsing code, even though that has a slightly different layout
There was a problem hiding this comment.
btw took this directly from the bytemath parsing code
bytemath is right using this, because bytemath is a list of sub keywords like endian or relative, whose order is not relevant. bytemath parses kind of a list of key+values.
Here, you want to parse something different, you parse a list of specific things...
I just want to split by , or end of string.
I think it is easier to have some linear code like currently, but yes, if you really want to first split into words/fields, this is good.
But then, you should not duplicate code, cf below...
| )(i)?; | ||
|
|
||
| if values.len() == 4 { | ||
| let cmd = match DetectIPRepDataCmd::from_str(values[0].trim()) { |
There was a problem hiding this comment.
If let Ok(cmd) = else return Err
There was a problem hiding this comment.
do you mean something like this?
- if values.len() == 4 {
- let cmd = match DetectIPRepDataCmd::from_str(values[0].trim()) {
- Ok(val) => val,
- Err(_) => return Err(make_error("invalid command".to_string())),
+ let args = values.len();
+ if args == 4 || args == 3 {
+ let cmd = if let Ok(cmd) = DetectIPRepDataCmd::from_str(values[0].trim()) {
+ cmd
+ } else {
+ return Err(make_error("invalid command".to_string()));
};Gets more verbose, not sure I see this as being better.
There was a problem hiding this comment.
Would something like let cmd = DetectIPRepDataCmd::from_str(values[0].trim())? work ?
Current code is let (i, cmd) = map_res(alpha0, DetectIPRepDataCmd::from_str)(i)?;
So, maybe something like et (_, cmd) = map_res(alpha0, DetectIPRepDataCmd::from_str)(values[0].trim())?;
| }; | ||
| return Ok((i, DetectIPRepData { du8, cat, cmd })); | ||
| } else { | ||
| return Err(make_error("too many arguments".to_string())); |
| @@ -112,6 +112,28 @@ pub fn detect_parse_iprep(i: &str) -> IResult<&str, DetectIPRepData, RuleParseEr | |||
| mode, | |||
| }; | |||
| return Ok((i, DetectIPRepData { du8, cat, cmd })); | |||
| } else if values.len() == 3 { | |||
| let cmd = match DetectIPRepDataCmd::from_str(values[0].trim()) { | |||
There was a problem hiding this comment.
This code looks duplicated from the case values.len() == 4
| } | ||
| let (mode, arg1) = match values[2].trim() { | ||
| "isset" => { (DetectUintMode::DetectUintModeGte, 0)}, // any value means it's set | ||
| "isnotset" => { (DetectUintMode::DetectUintModeEqual, 255) }, // impossible value means not set |
There was a problem hiding this comment.
Why not DetectUintModeGt ?
There was a problem hiding this comment.
Also I think 255 can be written as u8::MAX or something similar ;-)
There was a problem hiding this comment.
Another solution would be to create DetectUintModeAny and DetectUintModeNone ?
Or to have the wrapper structure DetectIPRepData use booleans justest and isnotset
There was a problem hiding this comment.
DetectUintModeGt 255 is an invalid setting for u8, even if it is perhaps not enforced anywhere.
Would like 255 to explicit, or even use a named constant to make it easy to understand what it signifies.
There was a problem hiding this comment.
I guess DetectUintModeAny could make some sense, though it could just be conceptualized as true, but less sure about DetectUintModeNone. I think your suggestion of not overloading the uint support is probably the cleanest.
There was a problem hiding this comment.
have the wrapper structure DetectIPRepData use booleans justest and isnotset
Looks cleanest indeed
|
replaced by #11091 |
Implementation of
issetandisnotsetfor theiprepkeyword.https://redmine.openinfosecfoundation.org/issues/6857
Looking for: