KRPC-564: Reject >10-byte varint tags in protobuf parser (#672)

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-authored-by: Alexander Sysoev <Alexander.Sysoev@jetbrains.com>

Author: 3 people

native-deps/shims/protobuf/src/cpp/protowire.cpp

@@ -235,47 +235,75 @@ extern "C" {
     }
 
     int pw_decoder_read_validated_tag(pw_decoder_t *self, uint32_t *tag_out) {
-        int pos_before = self->codedInputStream.CurrentPosition();
-
-        uint64_t raw64;
-        if (!self->codedInputStream.ReadVarint64(&raw64)) {
-            // Use ConsumedEntireMessage() to distinguish
-            // legitimate end-of-stream from actual errors (like >10-byte varints).
-            // Note: CurrentPosition() alone is insufficient because ReadVarint64's
-            // fast-path array reader does not advance the buffer pointer on failure.
-            if (self->codedInputStream.ConsumedEntireMessage()) {
-                return 0; // legitimate end of stream
-            }
-            return -1; // error (>10-byte varint, truncated varint, etc.)
+        // Zero-initialize so the Kotlin caller always has a meaningful value
+        // for error diagnostics, even on early-return error paths.
+        *tag_out = 0;
+
+        // Sub-message boundary: BytesUntilLimit() == 0 when a PushLimit scope
+        // is active and all bytes within it have been consumed. This mirrors
+        // ReadTag()'s internal limit check.
+        if (self->codedInputStream.BytesUntilLimit() == 0) {
+            return 0;
         }
 
-        int pos_after = self->codedInputStream.CurrentPosition();
-        int bytes_used = pos_after - pos_before;
-
-        // A zero tag value read from actual bytes is invalid (field number 0).
-        if (raw64 == 0) {
-            return -1;
+        // Read the first byte via ReadRaw to reliably distinguish top-level
+        // EOF from a varint start. BytesUntilLimit() is either -1 (no limit,
+        // top-level context where current_limit_ is INT_MAX) or positive
+        // (inside a PushLimit scope with remaining data). In either case,
+        // ReadRaw(1) failure means the underlying stream is exhausted.
+        //
+        // This avoids ReadTag()'s ambiguous return-0-for-both-EOF-and-errors
+        // and ConsumedEntireMessage()'s broken behavior at the top level (no
+        // limit set → legitimate_message_end_ is never set).
+        uint8_t b;
+        if (!self->codedInputStream.ReadRaw(&b, 1)) {
+            // BytesUntilLimit == -1: top-level EOF (no limit active, stream
+            // exhausted). BytesUntilLimit > 0: the limit says data should be
+            // available but the stream is exhausted — I/O error or truncation.
+            if (self->codedInputStream.BytesUntilLimit() > 0) {
+                return -1; // truncated stream
+            }
+            return 0; // top-level EOF
         }
 
-        // Tag must fit in 32 bits (29-bit field number + 3-bit wire type).
-        if (raw64 > UINT32_MAX) {
-            return -1;
+        // Parse the varint manually, tracking byte count for overlong detection.
+        // Tags are uint32 (max 5 varint bytes), but we must read up to 10 bytes
+        // to detect the >10-byte varint conformance case.
+        uint64_t result = b & 0x7F;
+        int bytes_used = 1;
+
+        while (b >= 0x80) {
+            if (bytes_used >= 10) {
+                // Write partial result for diagnostics before returning error.
+                *tag_out = static_cast<uint32_t>(result & UINT32_MAX);
+                return -1; // varint exceeds 10 bytes
+            }
+            if (!self->codedInputStream.ReadRaw(&b, 1)) {
+                *tag_out = static_cast<uint32_t>(result & UINT32_MAX);
+                return -1; // truncated varint
+            }
+            result |= static_cast<uint64_t>(b & 0x7F) << (7 * bytes_used);
+            bytes_used++;
         }
 
+        // Write the decoded value for diagnostics on all remaining error paths.
+        *tag_out = static_cast<uint32_t>(result & UINT32_MAX);
+
+        if (result == 0) return -1;         // zero tag (invalid field number 0)
+        if (result > UINT32_MAX) return -1;  // exceeds 32-bit tag range
+
         // Reject overlong varint encoding: the varint used more bytes than the
         // minimum required for its value. Each varint byte carries 7 payload bits.
         int min_bytes;
-        if      (raw64 < (1ULL <<  7)) min_bytes = 1;
-        else if (raw64 < (1ULL << 14)) min_bytes = 2;
-        else if (raw64 < (1ULL << 21)) min_bytes = 3;
-        else if (raw64 < (1ULL << 28)) min_bytes = 4;
-        else                           min_bytes = 5;
-
-        if (bytes_used > min_bytes) {
-            return -1;
-        }
+        if      (result < (1ULL <<  7)) min_bytes = 1;
+        else if (result < (1ULL << 14)) min_bytes = 2;
+        else if (result < (1ULL << 21)) min_bytes = 3;
+        else if (result < (1ULL << 28)) min_bytes = 4;
+        else                            min_bytes = 5;
+
+        if (bytes_used > min_bytes) return -1;
 
-        *tag_out = static_cast<uint32_t>(raw64);
+        // Success — *tag_out already set above.
         return 1;
     }
 

protobuf/protobuf-api/src/commonMain/kotlin/kotlinx/rpc/protobuf/ProtobufException.kt

@@ -32,8 +32,8 @@ public class ProtobufDecodingException : ProtobufException {
             "Decoder encountered an embedded string or message which claimed to have negative size."
         )
 
-        internal fun invalidTag() = ProtobufDecodingException(
-            "Protocol message contained an invalid tag (zero)."
+        internal fun invalidTag(tag: UInt = 0u) = ProtobufDecodingException(
+            "Protocol message contained an invalid tag ($tag)."
         )
 
         internal fun truncatedMessage() = ProtobufDecodingException(

protobuf/protobuf-api/src/commonTest/kotlin/kotlinx/rpc/protobuf/test/WireMarshallerTest.kt

@@ -829,7 +829,7 @@ class WireMarshallerTest {
 
 
     @Test
-    fun testInvalidTag() {
+    fun testInvalidTagZero() {
         val buffer = Buffer()
         buffer.writeByte(0)
 
@@ -840,6 +840,37 @@ class WireMarshallerTest {
         }
     }
 
+    @Test
+    fun testInvalidTagVarintMoreThanTenBytes() {
+        // 11 bytes: 10 continuation bytes (0x80) + 1 terminator (0x01)
+        // This exceeds the protobuf varint maximum of 10 bytes.
+        val buffer = Buffer()
+        repeat(10) { buffer.writeByte(0x80.toByte()) }
+        buffer.writeByte(0x01)
+
+        assertFailsWith<ProtobufDecodingException> {
+            checkForPlatformDecodeException {
+                WireDecoder(buffer).readTag()
+            }
+        }
+    }
+
+    @Test
+    fun testInvalidTagOverlongEncoding() {
+        // Field number 1, wire type 0 (VARINT) = tag value 8.
+        // Minimal encoding: single byte 0x08.
+        // Overlong encoding: two bytes 0x88 0x00 (value 8 in 2 varint bytes).
+        val buffer = Buffer()
+        buffer.writeByte(0x88.toByte())
+        buffer.writeByte(0x00)
+
+        assertFailsWith<ProtobufDecodingException> {
+            checkForPlatformDecodeException {
+                WireDecoder(buffer).readTag()
+            }
+        }
+    }
+
     /**
      * Writes a raw protobuf LENGTH_DELIMITED field with the given [data] bytes
      * at field number [fieldNr] into the buffer.

protobuf/protobuf-api/src/jvmMain/kotlin/kotlinx/rpc/protobuf/internal/WireDecoder.jvm.kt

@@ -24,7 +24,13 @@ internal class WireDecoderJvm(source: Source) : WireDecoder {
         if (codedInputStream.isAtEnd) return null
 
         val posBefore = codedInputStream.totalBytesRead
-        val raw64 = codedInputStream.readRawVarint64()
+        val raw64 = try {
+            codedInputStream.readRawVarint64()
+        } catch (e: InvalidProtocolBufferException) {
+            // readRawVarint64() throws for varints exceeding 10 bytes.
+            // Convert to ProtobufDecodingException so callers only need to handle one type.
+            throw ProtobufDecodingException(e.message ?: "Malformed varint", e)
+        }
         val bytesUsed = codedInputStream.totalBytesRead - posBefore
 
         // A valid tag must fit in 32 bits (29-bit field number + 3-bit wire type).
@@ -49,7 +55,7 @@ internal class WireDecoderJvm(source: Source) : WireDecoder {
 
         val tag = raw64.toUInt()
         if (tag == 0u) {
-            throw ProtobufDecodingException.invalidTag()
+            throw ProtobufDecodingException.invalidTag(tag)
         }
 
         return KTag.from(tag)

protobuf/protobuf-api/src/nativeMain/kotlin/kotlinx/rpc/protobuf/internal/WireDecoder.native.kt

@@ -67,11 +67,11 @@ internal class WireDecoderNative(private val source: Buffer) : WireDecoder {
     }
 
     override fun readTag(): KTag? = memScoped {
-        val tagOut = alloc<UIntVar>()
-        when (pw_decoder_read_validated_tag(raw, tagOut.ptr)) {
-            0 -> null // end of stream
-            1 -> KTag.from(tagOut.value)
-            else -> throw ProtobufDecodingException.invalidTag()
+        val tag = alloc<UIntVar>()
+        when (pw_decoder_read_validated_tag(raw, tag.ptr)) {
+            0 -> null // end of stream or sub-message boundary
+            1 -> KTag.from(tag.value)
+            else -> throw ProtobufDecodingException.invalidTag(tag.value)
         }
     }
 

tests/protobuf-conformance/src/jvmTest/resources/native_known_failures.txt

@@ -2,9 +2,3 @@
 # Tests listed here are excluded from native conformance JUnit assertions.
 # Each line is a test name (text after '#' is a comment).
 
-# >10-byte varint tag rejection not yet handled by C++ ReadVarint64 (pre-existing, also fails on JVM)
-Required.Proto2.ProtobufInput.BadTag_VarintMoreThanTenBytes
-Required.Proto3.ProtobufInput.BadTag_VarintMoreThanTenBytes
-Required.Editions_Proto2.ProtobufInput.BadTag_VarintMoreThanTenBytes
-Required.Editions_Proto3.ProtobufInput.BadTag_VarintMoreThanTenBytes
-

versions-root/libs.versions.toml

@@ -11,7 +11,7 @@ kotlin-compiler = "0.0.0" # default to kotlin-lang or env.KOTLIN_COMPILER_VERSIO
 # the version scheme is "<upstream-grpc-version>-<shim-version>"
 internal-native-grpc-shim = "1.74.1-2"
 # the version scheme is "<upstream-protobuf-version>-<shim-version>"
-internal-native-protobuf-shim = "31.1-2"
+internal-native-protobuf-shim = "31.1-5"
 # the version numbers for the shim annotation
 internal-native-shim-annotation = "0.1.0"