Prohibited using of zero and negative filed number in ProtoNumber and zero field numbers in input bytes (#2766)

- implemented throwing error if decoded field number = 0
- prohibited using of zero and negative filed number in @ProtoNumber annotation
- optimized skipping of size delimited fields - removed the creation of an byte array in case of skipping

Fixes #2649
Fixes #1566

Co-authored-by: Leonid Startsev <[email protected]>

Author: shanshin

formats/protobuf/commonMain/src/kotlinx/serialization/protobuf/internal/Helpers.kt

@@ -87,6 +87,7 @@ internal fun SerialDescriptor.extractParameters(index: Int): ProtoDesc {
         val annotation = annotations[i]
         if (annotation is ProtoNumber) {
             protoId = annotation.number
+            checkFieldNumber(protoId, i, this)
         } else if (annotation is ProtoType) {
             format = annotation.type
         } else if (annotation is ProtoPacked) {
@@ -118,11 +119,21 @@ internal fun extractProtoId(descriptor: SerialDescriptor, index: Int, zeroBasedD
             return ID_HOLDER_ONE_OF
         } else if (annotation is ProtoNumber) {
             result = annotation.number
+            // 0 or negative numbers are acceptable for enums
+            if (!zeroBasedDefault) {
+                checkFieldNumber(result, i, descriptor)
+            }
         }
     }
     return result
 }
 
+private fun checkFieldNumber(fieldNumber: Int, propertyIndex: Int, descriptor: SerialDescriptor) {
+    if (fieldNumber <= 0) {
+        throw SerializationException("$fieldNumber is not allowed in ProtoNumber for property '${descriptor.getElementName(propertyIndex)}' of '${descriptor.serialName}', because protobuf supports field numbers in range 1..${Int.MAX_VALUE}")
+    }
+}
+
 internal class ProtobufDecodingException(message: String, e: Throwable? = null) : SerializationException(message, e)
 
 internal expect fun Int.reverseBytes(): Int

formats/protobuf/commonMain/src/kotlinx/serialization/protobuf/internal/ProtobufDecoding.kt

@@ -45,8 +45,11 @@ internal open class ProtobufDecoder(
              * If we have reasonably small count of elements, try to build sequential
              * array for the fast-path. Fast-path implies that elements are not marked with @ProtoId
              * explicitly or are monotonic and incremental (maybe, 1-indexed)
+             *
+             * Initialize all elements, because there will always be one extra element as arrays are numbered from 0
+             * but in protobuf field number starts from 1.
              */
-            val cache = IntArray(elements + 1)
+            val cache = IntArray(elements + 1) { -1 }
             for (i in 0 until elements) {
                 val protoId = extractProtoId(descriptor, i, false)
                 // If any element is marked as ProtoOneOf,
@@ -307,6 +310,9 @@ internal open class ProtobufDecoder(
                 if (protoId == -1) { // EOF
                     return elementMarker.nextUnmarkedIndex()
                 }
+                if (protoId == 0) {
+                    throw SerializationException("0 is not allowed as the protobuf field number in ${descriptor.serialName}, the input bytes may have been corrupted")
+                }
                 val index = getIndexByNum(protoId)
                 if (index == -1) { // not found
                     reader.skipElement()

formats/protobuf/commonMain/src/kotlinx/serialization/protobuf/internal/ProtobufReader.kt

@@ -59,7 +59,7 @@ internal class ProtobufReader(private val input: ByteArrayInput) {
         when (currentType) {
             ProtoWireType.VARINT -> readInt(ProtoIntegerType.DEFAULT)
             ProtoWireType.i64 -> readLong(ProtoIntegerType.FIXED)
-            ProtoWireType.SIZE_DELIMITED -> readByteArray()
+            ProtoWireType.SIZE_DELIMITED -> skipSizeDelimited()
             ProtoWireType.i32 -> readInt(ProtoIntegerType.FIXED)
             else -> throw ProtobufDecodingException("Unsupported start group or end group wire type: $currentType")
         }
@@ -75,6 +75,13 @@ internal class ProtobufReader(private val input: ByteArrayInput) {
         return readByteArrayNoTag()
     }
 
+    fun skipSizeDelimited() {
+        assertWireType(ProtoWireType.SIZE_DELIMITED)
+        val length = decode32()
+        checkLength(length)
+        input.skipExactNBytes(length)
+    }
+
     fun readByteArrayNoTag(): ByteArray {
         val length = decode32()
         checkLength(length)

formats/protobuf/commonMain/src/kotlinx/serialization/protobuf/internal/Streams.kt

@@ -33,6 +33,12 @@ internal class ByteArrayInput(private var array: ByteArray, private val endIndex
         return b
     }
 
+
+    fun skipExactNBytes(bytesCount: Int) {
+        ensureEnoughBytes(bytesCount)
+        position += bytesCount
+    }
+
     private fun ensureEnoughBytes(bytesCount: Int) {
         if (bytesCount > availableBytes) {
             throw SerializationException("Unexpected EOF, available $availableBytes bytes, requested: $bytesCount")

formats/protobuf/commonTest/src/kotlinx/serialization/protobuf/InvalidFieldNumberTest.kt

@@ -0,0 +1,70 @@
+/*
+ * Copyright 2017-2024 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license.
+ */
+
+package kotlinx.serialization.protobuf
+
+
+import kotlinx.serialization.*
+import kotlin.test.*
+
+class InvalidFieldNumberTest {
+
+    @Serializable
+    data class Holder(val value: Int)
+
+    @Serializable
+    data class ListHolder(val value: List<Int>)
+
+    @Serializable
+    data class ZeroProtoNumber(@ProtoNumber(0) val value: Int)
+
+    @Serializable
+    data class NegativeProtoNumber(@ProtoNumber(-5) val value: Int)
+
+    @Test
+    fun testDeserializeZeroInput() {
+        assertFailsWithMessage<SerializationException>("0 is not allowed as the protobuf field number in kotlinx.serialization.protobuf.InvalidFieldNumberTest.Holder, the input bytes may have been corrupted") {
+            // first value with field number = 0
+            val hexString = "000f"
+            ProtoBuf.decodeFromHexString<Holder>(hexString)
+        }
+    }
+
+    @Test
+    fun testDeserializeZeroInputForElement() {
+        assertFailsWithMessage<SerializationException>("0 is not allowed as the protobuf field number in kotlinx.serialization.protobuf.InvalidFieldNumberTest.ListHolder, the input bytes may have been corrupted") {
+            // first element with field number = 0
+            val hexString = "000f"
+            ProtoBuf.decodeFromHexString<ListHolder>(hexString)
+        }
+    }
+
+    @Test
+    fun testSerializeZeroProtoNumber() {
+        assertFailsWithMessage<SerializationException>("0 is not allowed in ProtoNumber for property 'value' of 'kotlinx.serialization.protobuf.InvalidFieldNumberTest.ZeroProtoNumber', because protobuf supports field numbers in range 1..2147483647") {
+            ProtoBuf.encodeToHexString(ZeroProtoNumber(42))
+        }
+    }
+
+    @Test
+    fun testDeserializeZeroProtoNumber() {
+        assertFailsWithMessage<SerializationException>("0 is not allowed in ProtoNumber for property 'value' of 'kotlinx.serialization.protobuf.InvalidFieldNumberTest.ZeroProtoNumber', because protobuf supports field numbers in range 1..2147483647") {
+            ProtoBuf.decodeFromHexString<ZeroProtoNumber>("000f")
+        }
+    }
+
+    @Test
+    fun testSerializeNegativeProtoNumber() {
+        assertFailsWithMessage<SerializationException>("-5 is not allowed in ProtoNumber for property 'value' of 'kotlinx.serialization.protobuf.InvalidFieldNumberTest.NegativeProtoNumber', because protobuf supports field numbers in range 1..2147483647") {
+            ProtoBuf.encodeToHexString(NegativeProtoNumber(42))
+        }
+    }
+
+    @Test
+    fun testDeserializeNegativeProtoNumber() {
+        assertFailsWithMessage<SerializationException>("-5 is not allowed in ProtoNumber for property 'value' of 'kotlinx.serialization.protobuf.InvalidFieldNumberTest.NegativeProtoNumber', because protobuf supports field numbers in range 1..2147483647") {
+            ProtoBuf.decodeFromHexString<NegativeProtoNumber>("000f")
+        }
+    }
+}

formats/protobuf/commonTest/src/kotlinx/serialization/protobuf/PackedArraySerializerTest.kt

@@ -47,7 +47,6 @@ class PackedArraySerializerTest {
 
     @Serializable
     data class PackedStringCarrier(
-        @ProtoNumber(0)
         @ProtoPacked
         val s: List<String>
     )
@@ -110,12 +109,12 @@ class PackedArraySerializerTest {
     @Test
     fun testEncodeAnnotatedStringList() {
         val obj = PackedStringCarrier(listOf("aaa", "bbb", "ccc"))
-        val expectedHex = "020361616102036262620203636363"
+        val expectedHex = "0a036161610a036262620a03636363"
         val encodedHex = ProtoBuf.encodeToHexString(obj)
         assertEquals(expectedHex, encodedHex)
         assertEquals(obj, ProtoBuf.decodeFromHexString<PackedStringCarrier>(expectedHex))
 
-        val invalidPackedHex = "020C036161610362626203636363"
+        val invalidPackedHex = "0a0C036161610362626203636363"
         val decoded = ProtoBuf.decodeFromHexString<PackedStringCarrier>(invalidPackedHex)
         val invalidString = "\u0003aaa\u0003bbb\u0003ccc"
         assertEquals(PackedStringCarrier(listOf(invalidString)), decoded)

formats/protobuf/commonTest/src/kotlinx/serialization/protobuf/SkipFieldsTest.kt

@@ -0,0 +1,31 @@
+/*
+ * Copyright 2017-2024 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license.
+ */
+
+package kotlinx.serialization.protobuf
+
+
+import kotlinx.serialization.*
+import kotlin.test.*
+
+class SkipFieldsTest {
+
+    @Serializable
+    data class Holder(val value: Int)
+
+    @Test
+    fun testSkipBigFieldNumber() {
+        // first value with id = 2047 and takes 2 bytes
+        val hexString = "f87f20082a"
+        val holder = ProtoBuf.decodeFromHexString<Holder>(hexString)
+        assertEquals(42, holder.value)
+    }
+
+    @Test
+    fun testSkipUnknownFiledNumberForString() {
+        // first value is size delimited (string) with id = 42
+        val hexString = "d2020c48656c6c6f20576f726c6421082a"
+        val holder = ProtoBuf.decodeFromHexString<Holder>(hexString)
+        assertEquals(42, holder.value)
+    }
+}