BUG/MINOR: mqtt: set the type of the mqtt_field_value output

sample_conv_mqtt_field_value() never set the sample type, which was
thus left to the binary input type although the converter is declared
to return a string. Because of this, a string field holding a \0 byte,
forbidden by the MQTT specification ([MQTT-1.5.4-2]), was returned as
is without anyone noticing.

Set the type according to the field: password, will payload,
correlation data and authentication data are binary data as defined by
the specification and keep all their bytes, the other fields are
strings and the converter now fails when they hold a \0 byte, as for a
malformed packet.

The documentation is updated to describe the output type per field,
and a reg-test is added to check that a "public\0x" username does not
match "public".

This must be backported to all stable branches.

diff --git a/doc/configuration.txt b/doc/configuration.txt
index bc6361bffb646ce6da9621f4eb1821cedfcb5be0..c8480f87e3b260bfbb413e504c5d2f528847d636 100644 (file)
--- a/doc/configuration.txt
+++ b/doc/configuration.txt
@@ -21429,7 +21429,7 @@ map(map_name[,default_value])                      string       string
 map_match(map_name[,default_value])                _match_      string
 map_match_output(map_name[,default_value])         _match_      _output_
 mod(value)                                         integer      integer
-mqtt_field_value(pkt_type,fieldname_or_prop_ID)    binary       binary
+mqtt_field_value(pkt_type,fieldname_or_prop_ID)    binary       string / binary
 mqtt_is_valid                                      binary       boolean
 ms_ltime(format[,offset])                          integer      string
 ms_utime(format[,offset])                          integer      string
@@ -22764,6 +22764,13 @@ mqtt_field_value(<packettype>,<fieldname_or_property_ID>)
                     Not supported yet:
                        38: User Property
 
+  The output is a string, except for the password, will_payload, Correlation
+  Data (9) and Authentication Data (22) fields which are binary data as
+  defined by the MQTT specification. A string field holding a NUL byte, which
+  the specification forbids, is considered malformed and the converter fails.
+  See:
+  https://docs.oasis-open.org/mqtt/mqtt/v5.0/os/mqtt-v5.0-os.html#_Toc3901010
+
   Due to current HAProxy design, only the first message sent by the client and
   the server can be parsed. Thus this converter can extract data only from
   CONNECT and CONNACK packet types. CONNECT is the first message sent by the

diff --git a/reg-tests/converter/mqtt_embedded_nul.vtc b/reg-tests/converter/mqtt_embedded_nul.vtc
new file mode 100644 (file)
index 0000000..fc14d11
--- /dev/null
+++ b/reg-tests/converter/mqtt_embedded_nul.vtc
@@ -0,0 +1,54 @@
+varnishtest "mqtt_field_value must not return a string with a \0 byte"
+feature ignore_unknown_macro
+
+# A string sample cannot contain a \0 byte. MQTT string fields are copied as
+# is from the packet, so a client can put a \0 in them. Such a value must not
+# be matched by "-m str" on its prefix.
+
+syslog S1 -level info {
+    # c1: username "public"
+    recv
+    expect ~ "[^:\\[ ]\\[${h1_pid}\\]: user=match$"
+    # c2: username "public\0x" must not match "public"
+    recv
+    expect ~ "[^:\\[ ]\\[${h1_pid}\\]: user=nomatch$"
+} -start
+
+haproxy h1 -conf {
+    global
+    .if feature(THREAD)
+        thread-groups 1
+    .endif
+        nbthread 1
+
+    defaults
+        mode tcp
+        timeout connect "${HAPROXY_TEST_TIMEOUT-5s}"
+        timeout client  "${HAPROXY_TEST_TIMEOUT-5s}"
+        timeout server  "${HAPROXY_TEST_TIMEOUT-5s}"
+
+    frontend fe1
+        bind "fd@${fe1}"
+        log ${S1_addr}:${S1_port} local0
+        log-format "user=%[var(txn.user,nomatch)]"
+
+        tcp-request inspect-delay 100ms
+        tcp-request content set-var-fmt(txn.user) match if { req.payload(0,0),mqtt_field_value(connect,username) -m str public }
+        tcp-request content reject
+} -start
+
+# MQTT 3.1.1 CONNECT packet: empty client id, username, password "passwd"
+
+# username "public"
+client c1 -connect ${h1_fe1_sock} {
+    sendhex "101c 00044d515454 04 c2 003c 0000 0006 7075626c6963 0006 706173737764"
+    delay 0.3
+} -run
+
+# username "public\0x"
+client c2 -connect ${h1_fe1_sock} {
+    sendhex "101e 00044d515454 04 c2 003c 0000 0008 7075626c69630078 0006 706173737764"
+    delay 0.3
+} -run
+
+syslog S1 -wait

diff --git a/src/sample.c b/src/sample.c
index 9d29baa57f97c1f279f63b2a39ae94d9a60e8e84..a229c2f34cad2853b90c47cbe4f32609688e107b 100644 (file)
--- a/src/sample.c
+++ b/src/sample.c
@@ -4451,6 +4451,20 @@ static int sample_conv_mqtt_field_value(const struct arg *arg_p, struct sample *
        }
 
        smp->data.u.str = ist2buf(value);
+
+       /* the password, will payload, correlation data and authentication
+        * data are binary, the other fields are strings and cannot contain
+        * a NUL byte
+        */
+       if (fieldname_id == MQTT_FN_PASSWORD || fieldname_id == MQTT_FN_WILL_PAYLOAD ||
+           fieldname_id == MQTT_FN_CORRELATION_DATA || fieldname_id == MQTT_FN_AUTHENTICATION_DATA)
+               smp->data.type = SMP_T_BIN;
+       else {
+               if (memchr(istptr(value), 0, istlen(value)))
+                       return 0;
+               smp->data.type = SMP_T_STR;
+       }
+
        smp->flags |= SMP_F_CONST;
        return 1;
 }