BUG/MEDIUM: sample: reject the deprecated protobuf group wire types

protobuf_field_lookup() dispatches on the field's wire type through
protobuf_parser_defs[], but the entries for wire types 3 and 4
(START_GROUP/STOP_GROUP, deprecated) are zero-initialized:

    [PBUF_TYPE_START_GROUP     ] = {
            /* XXX Deprecated XXX */
    },
    [PBUF_TYPE_STOP_GROUP      ] = {
            /* XXX Deprecated XXX */
    },

while the only validation is the upper bound on the table:

    if (wire_type >= sizeof(protobuf_parser_defs) / sizeof(protobuf_parser_defs[0]))
            return 0;

A field using wire type 3 or 4 therefore passes the check and reaches a
call through the NULL .skip/.smp_store pointer: a request body
consisting of the single byte 0x0b (field 1, wire type 3) crashes the
process at pc=0. Any configuration routing a client body through the
"protobuf" converter (e.g. "http-request set-var(txn.f)
req.body,protobuf(1)") makes this remotely and unauthenticatedly
triggerable.

Let's reject wire types whose parser definition is missing and return 0
("field not found"), in line with the function's existing failure
contract.

The protobuf converter first appeared in 2.2; this should be backported
to all supported versions.

diff --git a/include/haproxy/protobuf.h b/include/haproxy/protobuf.h
index 2d5413d4c87f1ad9d35f3f74a10c685b2e0063f7..3274710422a2ae6a8e154af0da46f0960a0b57e7 100644 (file)
--- a/include/haproxy/protobuf.h
+++ b/include/haproxy/protobuf.h
@@ -541,6 +541,8 @@ static inline int protobuf_field_lookup(const struct arg *arg_p, struct sample *
                        return 0;
 
                p = &protobuf_parser_defs[wire_type];
+               if (!p->skip)
+                       return 0;
                /* If it is a length-delimited block (e.g., sub-message),
                 * read its size.
                 * Buffer overflow check (vlen > *len).