Detect generic uint 4112 v4 by catenacyber · Pull Request #7167 · OISF/suricata

catenacyber · 2022-03-23T20:56:06Z

Link to redmine ticket:
https://redmine.openinfosecfoundation.org/issues/4112

Describe changes:

Makes use of generic DetectUint structure for dsize and dcerpc, ttl, tcpmss, filesize (and template2)

Replaces #7150 with

commits history rework
incorporante ttl, tcpmss, template2 and filesize

Still TODO:

more keywords in C use specific versions, but they are more complex than just an integer
look for uses of set_uint in loggers to see if we can easily a new keywords

> git grep _LT src/*.h | grep DETEC
src/detect-ipproto.h:#define DETECT_IPPROTO_OP_LT     '<' /**< "less than" operator */
src/detect-iprep.h:#define DETECT_IPREP_OP_LT        0
src/detect-stream_size.h:#define DETECTSSIZE_LT 0
src/detect-tls-cert-validity.h:#define DETECT_TLS_VALIDITY_LT (1<<1) /* less than */
src/detect-urilen.h:#define DETECT_URILEN_LT   0   /**< "less than" operator */

catenacyber · 2022-03-23T20:56:55Z

Last version was +576 −1,110

Now it is +705 −2,371

catenacyber · 2022-03-23T21:49:11Z

The S-V failure indicates a real problem in etopen rules
ttl:<0 can never match for 2030055 and 2030056

Ticket: 4112 Move it away from http2 to generic core crate. And use it for DCERPC (and SMB) And remove the C version. Main change in API is the free function is not free itself, but a rust wrapper around unbox.

Ticket: 4112

victorjulien

Some inline comments and questions.

Really nice job. Esp love seeing so much parsing code get removed.

victorjulien · 2022-03-24T10:58:29Z

rust/src/dcerpc/detect.rs

-        assert_eq!(DETECT_DCE_IFACE_OP_NE, iface_data.op);
        assert_eq!(1, iface_data.any_frag);
-        assert_eq!(10, iface_data.version);
+        if let Some(x) = iface_data.du16 {


should we just use .unwrap() instead of conditions here?

victorjulien · 2022-03-24T10:59:32Z

rust/src/detect.rs

+    pub mode: DetectUintMode,
+}
+
+pub trait DetectIntType:


what are these empty constructs?

@jasonish advised me these...
I want a generic that provides what I can get for unsigned integers : comparison, max value, parsing from string...

What we're after is a type alias for a type that implements FromStr, PartialOrd, PrimInt, Bounded, however you can make a type alias (like a typedef) that specific multiple traits for whatever reason. The idiomatic workaround is to create a new trait that specifies all these traits.

It lets us specify function like fn detect_parse_uint_start_equal<T: DetectIntType> instead of fn detect_parse_uint_start_equal<T: std::str::FromStr + std::cmp::PartialOrd + num::PrimInt + num::Bounded>`.

victorjulien · 2022-03-24T11:03:06Z

rust/src/detect.rs

+pub unsafe extern "C" fn rs_detect_u64_parse(
+    str: *const std::os::raw::c_char,
+) -> *mut DetectUintData<u64> {
+    let ft_name: &CStr = CStr::from_ptr(str); //unsafe


if unsafe, should we do more to check? E.g. can ft_name be validated somehow, or can str be?

btw str is also a type, might be good to not use it as an argument value. Tricks code hilighing here for example

cc @jasonish
I think it is unsafe, because you have no assurance that C caller will provide a valid null-terminated string...

Changing str variable name

if unsafe, should we do more to check? E.g. can ft_name be validated somehow, or can str be?

We can check for null, thats about it.

victorjulien · 2022-03-24T11:05:34Z

rust/src/detect.rs

+) -> *mut DetectUintData<u64> {
+    let ft_name: &CStr = CStr::from_ptr(str); //unsafe
+    if let Ok(s) = ft_name.to_str() {
+        if let Ok((_, ctx)) = detect_parse_uint::<u64>(s) {


Is the first, ignored, argument here the remaining string? Should we check if that is empty? Could it be non-empty on trailing data?

Is the first, ignored, argument here the remaining string?

Yes

Should we check if that is empty? Could it be non-empty on trailing data?

I am not sure
There are other keywords such as urilen which have an integer, but not only. After, we can have norm|raw

victorjulien · 2022-03-24T11:22:25Z

src/detect-engine-uint.h

+#define DETECT_UINT_LT  DetectUintModeLt
+#define DETECT_UINT_LTE DetectUintModeLte
+
+typedef DetectUintData_u64 DetectU64Data;


These don't match our style, should probably be DetectUintDataU32

What do you mean ?
DetectUintData_u64 is generated by cbindgen
DetectU64Data is what is in the code currently

I added these typedef to minimize the diff.
But we can run sed 's/DetectU64Data/DetectUintData_u64/' and avoid these

catenacyber · 2022-03-24T21:11:35Z

Replaced by #7170

The "eve.version" field is not always log. Update the schema to enforce that it is, and fix it for records that don't log it. Ticket: OISF#7167

The "eve.version" field is not always logged. Update the schema to enforce that it is, and fix it for records that don't log it. Ticket: OISF#7167

The "eve.version" field is not always logged. Update the schema to enforce that it is, and fix it for records that don't log it. Ticket: OISF#7167 (cherry picked from commit fcc1b10)

catenacyber requested review from a team, jasonish and victorjulien as code owners March 23, 2022 20:56

catenacyber mentioned this pull request Mar 23, 2022

Detect generic uint 4112 v3 #7150

Closed

catenacyber added 6 commits March 23, 2022 22:49

detect: rust generic functions for integers

1544554

Ticket: 4112 Move it away from http2 to generic core crate. And use it for DCERPC (and SMB) And remove the C version. Main change in API is the free function is not free itself, but a rust wrapper around unbox.

detect: use generic integer functions for dsize

eede02f

Ticket: 4112

detect: use generic integer functions for ttl

e42b138

Ticket: 4112

detect: use generic integer functions for template2

303d808

detect: use generic integer functions for tcp mss

1d5704e

Ticket: 4112

detect: use generic integer functions for filesize

ebf6ee6

Ticket: 4112

catenacyber force-pushed the detect-generic-uint-4112-v4 branch from d275de5 to ebf6ee6 Compare March 23, 2022 21:50

victorjulien requested changes Mar 24, 2022

View reviewed changes

catenacyber mentioned this pull request Mar 24, 2022

Detect generic uint 4112 v5 #7170

Closed

catenacyber closed this Mar 24, 2022

jasonish added a commit to jasonish/suricata that referenced this pull request Jul 12, 2024

eve/dns: make version required

e4e1da2

The "eve.version" field is not always log. Update the schema to enforce that it is, and fix it for records that don't log it. Ticket: OISF#7167

jasonish mentioned this pull request Jul 12, 2024

eve/dns: make version required #11490

Closed

jasonish added a commit to jasonish/suricata that referenced this pull request Jul 12, 2024

eve/dns: make version required

dd8dd9b

The "eve.version" field is not always logged. Update the schema to enforce that it is, and fix it for records that don't log it. Ticket: OISF#7167

victorjulien pushed a commit to victorjulien/suricata that referenced this pull request Jul 15, 2024

eve/dns: make version required

fcc1b10

The "eve.version" field is not always logged. Update the schema to enforce that it is, and fix it for records that don't log it. Ticket: OISF#7167

jlucovsky mentioned this pull request Jul 30, 2024

eve/dns: make version required (cherry-pick from master) #11586

Closed

Conversation

catenacyber commented Mar 23, 2022

Uh oh!

catenacyber commented Mar 23, 2022

Uh oh!

catenacyber commented Mar 23, 2022

Uh oh!

victorjulien left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

catenacyber commented Mar 24, 2022

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants