Bug #7323
closedmqtt: wrong and missing direction for keywords
Description
As reported https://forum.suricata.io/t/question-about-mqtt-detection/4890/3
@Sascha Steinbiss do you want to fix this ?
diff --git a/rust/src/mqtt/detect.rs b/rust/src/mqtt/detect.rs
index c7dedc7ee8..b65d70686b 100644
--- a/rust/src/mqtt/detect.rs
+++ b/rust/src/mqtt/detect.rs
@@ -1127,7 +1127,7 @@ pub unsafe extern "C" fn ScDetectMqttRegister() {
G_MQTT_TYPE_BUFFER_ID = DetectHelperBufferRegister(
b"mqtt.type\0".as_ptr() as *const libc::c_char,
ALPROTO_MQTT,
- false, // only to server
+ true,
true,
);
@@ -1172,7 +1172,7 @@ pub unsafe extern "C" fn ScDetectMqttRegister() {
G_MQTT_REASON_CODE_BUFFER_ID = DetectHelperBufferRegister(
b"mqtt.reason_code\0".as_ptr() as *const libc::c_char,
ALPROTO_MQTT,
- false, // only to server
+ true, // only to client
true,
);
let kw = SCSigTableElmt {
allows to have more alerts for SV test about unsub
Also SUBACK case seems to be missing for reason code
Files
Updated by Sascha Steinbiss 2 months ago
I'm still not able to reproduce the original poster's issues with the CONNACK reason code and the unsubscribe topic. I modified one of the S-V pcaps to have their reason code 134 (Bad Username or Password) in the CONNACK response and was properly able to alert on it, even with the original code without your patch above. See attachment.
That's why I was asking about a pcap from the OP, to get more insight into what users may be capturing in the real world.
allows to have more alerts for SV test about unsub
What do you mean? Adding your changes and re-running S-V for mqtt does not result in any changes to the eve.json for mqtt-unsub-rules for me (apart from flow IDs obviously).
Also SUBACK case seems to be missing for reason code
Oh, yes. Good catch. That's because I implemented these not as "reason code" but as "QOS granted" as that's what it was meant to denote back in MQTT 3. Changed this to also match these properly in mqtt_tx_suback_unsuback_has_reason_code (formerly mqtt_tx_unsuback_has_reason_code). Also extended S-V tests a bit to cover this better.
Updated by Sascha Steinbiss 2 months ago
See https://github.com/OISF/suricata/pull/11995 and https://github.com/OISF/suricata-verify/pull/2102 providing some tests.
Updated by Philippe Antoine 2 months ago
allows to have more alerts for SV test about unsub
What do you mean? Adding your changes and re-running S-V for mqtt does not result in any changes to the
eve.jsonfor mqtt-unsub-rules for me (apart from flow IDs obviously).
Hmm.. Was I having a weird suricata.yaml at the time ?
Oh maybe I was using https://github.com/OISF/suricata/pull/11976 instead of master...
Directions look like they need fixing indeed
Updated by Sascha Steinbiss 2 months ago
Philippe Antoine wrote in #note-6:
[...]
Directions look like they need fixing indeed
Sure. Will update my PR.
Updated by Sascha Steinbiss 2 months ago
Sascha Steinbiss wrote in #note-8:
Philippe Antoine wrote in #note-6:
[...]Directions look like they need fixing indeed
Sure. Will update my PR.
Well, AFAICS there is also more to do, because within a session (say, between client A and broker/"server" B), things like PUBLISH can go in both directions -- the server can also PUBLISH towards the client if the message in question is in a subscribed topic. This means that it is likely there will have to be some more adjustments to directions. This must have slipped my mind back then.
Updated by Sascha Steinbiss 2 months ago
I just took a look at the keyword directions, updated the above PR and removed it from draft status.
Updated by Philippe Antoine about 1 month ago
- Status changed from In Progress to Resolved
https://github.com/OISF/suricata/pull/11995 was merged