[CORE-15247] `kafka`: add size validation to string & arrays read off the wire by WillemKauf · Pull Request #29208 · redpanda-data/redpanda

WillemKauf · 2026-01-09T15:45:46Z

Backports Required

Release Notes

Bug Fixes

Adds better validation to arrays and strings being read off the wire from kafka clients.

Copilot

Pull request overview

This PR enhances input validation for Kafka protocol deserialization by adding bounds checks to prevent reading beyond available buffer space when parsing strings and arrays from client requests.

Changes:

Added validation to ensure string and array lengths don't exceed remaining bytes in the parser buffer
Enhanced error logging when disconnecting clients to include the exception details

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
src/v/kafka/protocol/wire.h	Added bounds validation for string and array deserialization to prevent buffer overruns
src/v/kafka/server/connection_context.cc	Enhanced disconnect logging to include exception details for better debugging

Copilot · 2026-01-09T15:46:28Z

+        if (static_cast<size_t>(len) > _parser.bytes_left()) {
+            throw std::out_of_range(
+              fmt::format(
+                "Array length {} exceeds remaining bytes {}",
+                len,
+                _parser.bytes_left()));
+        }
+


The validation compares the array length against remaining bytes, but this check is insufficient. The array length represents the number of elements, not the byte size. Each element will consume a variable number of bytes depending on type T. This validation will fail to catch cases where the total size of all elements exceeds remaining bytes. Consider validating during element-by-element parsing instead, or calculating the minimum required bytes based on element type.

Suggested change

if (static_cast<size_t>(len) > _parser.bytes_left()) {

throw std::out_of_range(

fmt::format(

"Array length {} exceeds remaining bytes {}",

len,

_parser.bytes_left()));

}

Yeah, this is fine though

dotnwat

is there a jira ticket or a description of how you encountered this?

dotnwat · 2026-01-09T15:54:19Z

+                _parser.bytes_left()));
+        }
+
        return _parser.read_string(n);


maybe the above validations should be inside the parser read_string method?

It could be, though iobuf_parser is used a lot internally and performing validation here at the kafka layer seems reasonable as well. WDYT?

iobuf_parser is used a lot internally and performing validation here at the kafka layer seems reasonable as well. WDYT?

maybe i'm missing something, but isn't this an argument for putting the validation in the parser itself, so that the validation happens for all users not just this one spot?

Not if two extra if statements is a concern for performance 👍

WDYT about putting these checks into iobuf_parser::read_string_safe() and calling that from wire.h instead?

oh sorry, you're right. i was thinking that the iobuf_parser interfaces were reading the size, but infact we are providing it as parsed out of the kafka protocol. so yeh, doing the check above the iobuf_parser makes sense!

WillemKauf · 2026-01-09T16:03:01Z

is there a jira ticket

https://redpandadata.atlassian.net/browse/CORE-15247

michael-redpanda

do we need to perform a remaining bytes check in read_tags and consume_unknown_tag as well?

michael-redpanda · 2026-01-09T16:42:39Z

            throw std::out_of_range("Asked to read a 0 byte flex string");
        }
+
+        if (static_cast<size_t>(n - 1) > _parser.bytes_left()) {


for performance resons should these be marked [[unlikely]]?

WillemKauf · 2026-01-09T17:08:15Z

do we need to perform a remaining bytes check in read_tags and consume_unknown_tag as well?

I haven't been able to hit any crashes within read_tags() or consume_unknown_tag() with my protocol fuzz testing, but I suppose it's easy enough to add a check there as well.

For string, array, and number of tags.

michael-redpanda

Would it be possible to get a unit test written that triggers this behavior?

WillemKauf · 2026-01-09T17:29:55Z

Would it be possible to get a unit test written that triggers this behavior?

Added one courtesy of Claude.

Add wire_validation_test.cc with tests that verify bounds checking and error handling in the Kafka protocol decoder. Tests cover: - Array length validation (exceeds buffer, negative, max int32) - Flex array validation (exceeds buffer, zero length) - String/flex string validation (exceeds buffer, negative/zero length) - Bytes/flex bytes validation - Tagged fields validation (count exceeds buffer, duplicate/non-ascending IDs, size exceeds buffer) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

WillemKauf · 2026-01-09T17:41:06Z

Alright done with force pushing (just wanted to make what Claude wrote for the unit test a tiny bit nicer). Approve away

dotnwat · 2026-01-09T17:43:39Z

+        if (unlikely(static_cast<size_t>(num_tags) > _parser.bytes_left())) {
+            throw std::out_of_range(
+              fmt::format(
+                "Number of tags {} exceeds remaining bytes {}",
+                num_tags,
+                _parser.bytes_left()));
+        }
        int64_t prev_tag_id = -1;
        while (num_tags-- > 0) {
            auto id = read_unsigned_varint(); // consume tag id


@michael-redpanda what was the concern for this case? it looks like if num_tags is too large then the loop will hit end-of-buffer exception. the other cases were large allocations because the bogus value was plugged directly into malloc.

Sorry my concern isn't here, but below in the iobuf_to_bytes(_parser.share(size)) call. Maybe that one is ok as well

Pretty sure _parser.share() won't malloc() either, it iterates over fragments.

WillemKauf · 2026-01-09T21:37:54Z

Any outstanding concerns here?

WillemKauf · 2026-01-10T00:08:50Z

@michael-redpanda I'll wait on your approval.

vbotbuildovich · 2026-01-12T13:34:24Z

/backport v25.3.x

vbotbuildovich · 2026-01-12T13:34:25Z

/backport v25.2.x

vbotbuildovich · 2026-01-12T13:34:26Z

/backport v25.1.x

WillemKauf requested review from Copilot, dotnwat, michael-redpanda and rockwotj January 9, 2026 15:45

github-actions Bot added the area/redpanda label Jan 9, 2026

WillemKauf changed the title ~~kafka: add size validation to string & arrays read off the wire~~ [CORE-15247] kafka: add size validation to string & arrays read off the wire Jan 9, 2026

Copilot AI reviewed Jan 9, 2026

View reviewed changes

dotnwat reviewed Jan 9, 2026

View reviewed changes

WillemKauf force-pushed the kafka_wire_validate_input branch from 3b7ad38 to a968756 Compare January 9, 2026 16:26

dotnwat previously approved these changes Jan 9, 2026

View reviewed changes

michael-redpanda reviewed Jan 9, 2026

View reviewed changes

WillemKauf dismissed dotnwat’s stale review via 943e418 January 9, 2026 17:16

WillemKauf force-pushed the kafka_wire_validate_input branch from a968756 to 943e418 Compare January 9, 2026 17:16

WillemKauf requested review from dotnwat and michael-redpanda January 9, 2026 17:17

WillemKauf added 2 commits January 9, 2026 12:17

kafka: pre-validate sizes when reading off the wire

13d3f76

For string, array, and number of tags.

kafka: add extra logging detail in connection_context

61bfa92

WillemKauf force-pushed the kafka_wire_validate_input branch from 943e418 to 61bfa92 Compare January 9, 2026 17:18

michael-redpanda reviewed Jan 9, 2026

View reviewed changes

Comment thread src/v/kafka/protocol/wire.h Outdated

github-actions Bot added the area/build label Jan 9, 2026

WillemKauf requested a review from michael-redpanda January 9, 2026 17:30

WillemKauf force-pushed the kafka_wire_validate_input branch from 8137870 to a78c238 Compare January 9, 2026 17:40

dotnwat reviewed Jan 9, 2026

View reviewed changes

dotnwat approved these changes Jan 9, 2026

View reviewed changes

michael-redpanda approved these changes Jan 12, 2026

View reviewed changes

WillemKauf merged commit d0f48f0 into redpanda-data:dev Jan 12, 2026
19 checks passed

WillemKauf mentioned this pull request Feb 10, 2026

Remove record copying in produce batch validation #29577

Merged

7 tasks

WillemKauf mentioned this pull request Apr 14, 2026

kafka/protocol: reject oversized tagged fields during parsing #30160

Merged

7 tasks

Conversation

WillemKauf commented Jan 9, 2026

Backports Required

Release Notes

Bug Fixes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 9, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dotnwat left a comment

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!

WillemKauf commented Jan 9, 2026

Uh oh!

michael-redpanda left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WillemKauf commented Jan 9, 2026

Uh oh!

michael-redpanda left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

WillemKauf commented Jan 9, 2026

Uh oh!

WillemKauf commented Jan 9, 2026

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!

WillemKauf commented Jan 9, 2026

Uh oh!

WillemKauf commented Jan 10, 2026

Uh oh!

Uh oh!

vbotbuildovich commented Jan 12, 2026

Uh oh!

vbotbuildovich commented Jan 12, 2026

Uh oh!

vbotbuildovich commented Jan 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants