Mqtt rust keywords 4863 v1 by catenacyber · Pull Request #11316 · OISF/suricata

catenacyber · 2024-06-18T05:17:48Z

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

Describe changes:

mqtt: move keywords to pure rust

SV_BRANCH=OISF/suricata-verify#1920

cc @satta

So that MQTTTypeCode::CONNECT does not become c_o_n_n_e_c_t

As needed for MQTTTypeCode which accepts both CONNECT uppercase and unassigned lowercase

for easier later matching

Ticket: 4863 On the way, convert some keywords to use the first-class integer support. And imports in pure rust the support for multi-buffer.

catenacyber · 2024-06-18T05:18:22Z

            transform_name("UnassignedMsgType"),
            "unassigned_msg_type".to_string()
        );
+        assert_eq!(transform_name("SAMECASE"), "samecase".to_string());


cc @jasonish

Fine in a test.

catenacyber · 2024-06-18T05:19:17Z


 Match on the numeric value of the reason code that is used in MQTT 5.0 for some message types. Please refer to the specification for the meaning of these values, which are often specific to the message type in question.

+mqtt.reason_code uses an :ref:`unsigned 8-bits integer <rules-integer-keywords>`.


We can now use all the operators

catenacyber · 2024-06-18T05:19:40Z


 Match on a combination of MQTT CONNECT flags, separated by commas (``,``). Flags may be prefixed by ``!`` to indicate negation, i.e. a flag prefixed by ``!`` must `not` be set to match.

+mqtt.connect.flags uses an :ref:`unsigned 8-bits integer <rules-integer-keywords>`


We can now match on the unassigned bit and the qos 2 bits if we want

catenacyber · 2024-06-18T05:19:56Z

    "IPPROTO_TCP",
    "IPPROTO_UDP",
    "SRepCatGetByShortname",
+    "SIG_FLAG_TOSERVER",


@satta why is MQTT detection only to server ?

Good question. I just grepped through detect-mqtt-* and saw many instances of SIG_FLAG_TOSERVER... well, not all of the things that are alerted on in the detection keywords are sent to the broker, but most are (e.g. anything with CONNECT or PUBLISH, but not CONNACK, the reason code, etc.).
Could be a copy-paste artifact; interesting that these keywords still seem to work though.

Maybe I should also revisit the code and check if maybe the transactions the parser produces are deliberately registered as being toserver because I didn't know an alternative (even though they likely contain messages in various directions) and that's why we want to apply the keywords to toserver.

And there is no SIG_FLAG_TOCLIENT

catenacyber · 2024-06-18T05:20:22Z

 #[no_mangle]
 pub unsafe extern "C" fn rs_mqtt_tx_is_toclient(
-    tx: *const std::os::raw::c_void,
+    tx:  &MQTTTransaction,


needed so that MQTTTransaction is exported by cbindgen

catenacyber · 2024-06-18T05:21:18Z

+    FAIL_IF_NULL(sig);
+
+    sig = DetectEngineAppendSig(
+            de_ctx, "alert ip any any -> any any (mqtt.protocol_version:3; sid:2; rev:1;)");


@satta why put another time the same signature with a different sid ?

The second one should certainly have been mqtt.protocol_version:5.

will fix in next version

catenacyber · 2024-06-18T05:23:33Z

+                    return Some(rcode);
+                }
+            }
+            _ => return None,


@satta why do we not continue with the loop ?

Right, we should. Good catch! This is likely an oversight when extending transactions to multi-operations.

will fix in next version

catenacyber · 2024-06-18T05:25:14Z

+        if let MQTTOperation::UNSUBSCRIBE(ref unsubv) = msg.op {
+            if (local_id as usize) < unsubv.topics.len() + offset {
+                let topic = &unsubv.topics[(local_id as usize) - offset];
+                if !topic.is_empty() {


I wonder if we treat right the empty topic case here

You mean, in that empty topics should be returned nevertheless? Well, the spec does not forbid zero length topic strings (e.g. in MQTT5 if the topic alias property is used) so we might just return length of 0 and a null pointer, right?

yes, we should...

catenacyber · 2024-06-18T05:28:22Z

This is a follow up on #11291

There are more protocols to do...

codecov · 2024-06-18T05:38:22Z

Codecov Report

Attention: Patch coverage is 87.52108% with 148 lines in your changes missing coverage. Please review.

Project coverage is 82.48%. Comparing base (49ecf37) to head (959b28a).
Report is 3 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #11316      +/-   ##
==========================================
+ Coverage   82.47%   82.48%   +0.01%     
==========================================
  Files         934      918      -16     
  Lines      252270   252086     -184     
==========================================
- Hits       208055   207944     -111     
+ Misses      44215    44142      -73

Flag	Coverage Δ
fuzzcorpus	`60.29% <85.01%> (+0.03%)`	⬆️
livemode	`18.78% <32.24%> (+0.02%)`	⬆️
pcap	`43.77% <33.73%> (-0.01%)`	⬇️
suricata-verify	`61.37% <78.76%> (+0.05%)`	⬆️
unittests	`59.86% <54.04%> (-0.05%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

suricata-qa · 2024-06-18T07:40:02Z

Information: QA ran without warnings.

Pipeline 21114

suricata-qa · 2024-06-18T21:55:03Z

Information: QA ran without warnings.

Pipeline 21128

satta · 2024-06-19T07:50:04Z

Thanks for the porting work and also for taking a closer look at the code! Will give it a deeper look later and reply here.

catenacyber · 2024-06-19T08:03:45Z

Thanks for the porting work and also for taking a closer look at the code! Will give it a deeper look later and reply here.

ok waiting for it then, thanks

catenacyber · 2024-06-27T09:03:22Z

Continued in #11374

catenacyber added 5 commits June 17, 2024 18:52

detect: helper function for multibuffer

7266b69

rust/derive: transform all uppercase names the right way

0ee6cda

So that MQTTTypeCode::CONNECT does not become c_o_n_n_e_c_t

rust/derive: string enumeration become case insensitive

13e828f

As needed for MQTTTypeCode which accepts both CONNECT uppercase and unassigned lowercase

mqtt: parse and store raw connect flags

b439fdf

for easier later matching

detect/mqtt: move keywords to rust

2fc612a

Ticket: 4863 On the way, convert some keywords to use the first-class integer support. And imports in pure rust the support for multi-buffer.

catenacyber requested review from jasonish, jufajardini and victorjulien as code owners June 18, 2024 05:17

catenacyber commented Jun 18, 2024

View reviewed changes

fixup! rust/derive: transform all uppercase names the right way

e543b59

catenacyber added 2 commits June 18, 2024 15:44

fixup! detect: helper function for multibuffer

0c2aee2

fixup! detect: helper function for multibuffer

959b28a

catenacyber mentioned this pull request Jun 19, 2024

Applayer plugin 5053 v1 #11321

Closed

This was referenced Jun 20, 2024

detect/sip: add sticky buffers to match headers v4 #11330

Closed

Mqtt rust keywords 4863 v2 #11374

Closed

catenacyber closed this Jun 27, 2024


		Match on the numeric value of the reason code that is used in MQTT 5.0 for some message types. Please refer to the specification for the meaning of these values, which are often specific to the message type in question.

		mqtt.reason_code uses an :ref:`unsigned 8-bits integer <rules-integer-keywords>`.


		Match on a combination of MQTT CONNECT flags, separated by commas (``,``). Flags may be prefixed by ``!`` to indicate negation, i.e. a flag prefixed by ``!`` must `not` be set to match.

		mqtt.connect.flags uses an :ref:`unsigned 8-bits integer <rules-integer-keywords>`

Conversation

catenacyber commented Jun 18, 2024

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!

satta Jun 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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 Jun 18, 2024

Uh oh!

codecov Bot commented Jun 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

suricata-qa commented Jun 18, 2024

Uh oh!

suricata-qa commented Jun 18, 2024

Uh oh!

satta commented Jun 19, 2024

Uh oh!

catenacyber commented Jun 19, 2024

Uh oh!

catenacyber commented Jun 27, 2024

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

satta Jun 19, 2024 •

edited

Loading

codecov Bot commented Jun 18, 2024 •

edited

Loading