BUG/MINOR: sample: realign the ungrpc converter on message boundaries

sample_conv_ungrpc() walks the GRPC messages of the body with two
variables: <pos>, the read cursor, and <grpc_left>, the bytes left.
They are supposed to describe the same position.

When protobuf_field_lookup() fails, it does not always leave <pos> at the
end of the message. For a length-delimited field it stops on the value,
after the key and the length. But the loop still subtracts the whole
message length from <grpc_left>, without moving <pos>. The cursor and the
counter then describe different parts of the input, and the next
iteration reads a "gRPC header" from the previous message's payload.

A 23-byte body shows it:

  00 0000000a 0a 08 00 00 00 00 03 08 b9 0a 00 00000003 08 8f 4e

ungrpc(1,int64) returns 1337, which is the value of the first message's
field. The second message holds the real field 1 = 9999 but is never
reached. The counter can also over-count, and skip real messages.

Save the end of the message rigght after the "grpc_left < left"
check, then set <pos> to it when the lookup fails, so the next iteration
starts on a real message boundary.

Nothing changes on the success path, nor for fully parseable bodies,
where the lookup already stops at the message end. The multi-message walk
is intended for streaming bodies and is kept.

This is not a memory-safety issue: <pos> and the counter advance by the
header together, and the lookup moves forward by at most the message
length, which is the same amount removed from the counter.

This is also not a policy bypass like a4f5eea841 ("BUG/MEDIUM: protobuf:
fix nested path bypass in field lookup"). There, a flat sibling field
could satisfy a nested path that the configuration explicitly
restricted, so the client gained a capability it did not have. Here the
client writes the whole body and can already obtain any value with a
single well-formed message. The bug only makes the proxy read a value
that is the field of no message of the body, while the backend reads
another one. Hence MINOR and not MEDIUM.

The loop was written this way when the converter was added in 2.0
(1fceee8316) and did not change since 5f33f85ce8, apart from the
unaligned access fix. Not a regression. Backport to 2.0 and above.

Thank you to Owais Lone for having reported this bug and provided this
patch.

Must be backported to all stable versions.

Reported-by: Owais Lone <thesecguy>

diff --git a/src/sample.c b/src/sample.c
index ba50695df50fb1aacf7bf32528e88c707476ab5b..e5f7343f9f1a4faa1114991406df36bbd06dad0f 100644 (file)
--- a/src/sample.c
+++ b/src/sample.c
@@ -4402,6 +4402,7 @@ static int sample_conv_ungrpc(const struct arg *arg_p, struct sample *smp, void
 
        while (grpc_left > GRPC_MSG_HEADER_SZ) {
                size_t grpc_msg_len, left;
+               unsigned char *msg_end;
 
                grpc_msg_len = left = ntohl(read_u32(pos + GRPC_MSG_COMPRESS_FLAG_SZ));
 
@@ -4411,9 +4412,19 @@ static int sample_conv_ungrpc(const struct arg *arg_p, struct sample *smp, void
                if (grpc_left < left)
                        return 0;
 
+               /* protobuf_field_lookup() walks the message and advances <pos>,
+                * but on failure it leaves it wherever it stopped, for instance
+                * on the value of a length-delimited field whose type does not
+                * match, or at the end of a narrowed sub-message. The next
+                * message must be decoded from the end of the current one, so
+                * remember that position rather than relying on the lookup.
+                */
+               msg_end = pos + grpc_msg_len;
+
                if (protobuf_field_lookup(arg_p, smp, &pos, &left))
                        return 1;
 
+               pos = msg_end;
                grpc_left -= grpc_msg_len;
        }