Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ProtoBuf serialization incorrect behavior #2888

Closed
FerrumBrain opened this issue Dec 16, 2024 · 5 comments
Closed

ProtoBuf serialization incorrect behavior #2888

FerrumBrain opened this issue Dec 16, 2024 · 5 comments
Assignees
@shanshin
Labels
found-by-fuzzing

Comments

@FerrumBrain
Copy link

0. Setup

We will use the following structure of a Message:

@Serializable
sealed interface OneOfType

@OptIn(ExperimentalSerializationApi::class)
@Serializable
@JvmInline
value class FirstOption(val valueInt: Int) : OneOfType

@OptIn(ExperimentalSerializationApi::class)
@Serializable
@JvmInline
value class SecondOption(val valueDouble: Double) : OneOfType

@Serializable
data class ProtobufMessage<T> @OptIn(ExperimentalSerializationApi::class) constructor(
    @ProtoType(ProtoIntegerType.DEFAULT)
    val intFieldDefault: Int?,
    @ProtoType(ProtoIntegerType.FIXED)
    val intFieldFixed: Int?,
    @ProtoType(ProtoIntegerType.SIGNED)
    val intFieldSigned: Int?,
    var longField: Long? = 5,
    val floatField: Float?,
    val doubleField: Double?,
    val stringField: String?,
    val booleanField: Boolean?,
    val listField: List<T?> = emptyList(),
    @ProtoPacked val packedListField: List<T?> = emptyList(),
    val mapField: Map<String, T?> = emptyMap(),
    @ProtoPacked val packedMapField: Map<String, T?> = emptyMap(),
    val nestedMessageField: ProtobufMessage<T>?,
    val enumField: TestEnum?,
    @ProtoOneOf val oneOfField: OneOfType?,
)

It is slightly modified version of Value: added necessary annotations, unified types of lists and maps, added default value.

1. Empty messages can be decoded from various sources

If we try to deserialize some strings we will get empty message even if input wasn't empty.

val bytes = byteArrayOf(9)
val message = ProtoBuf.decodeFromByteArray<ProtobufMessage<Int>>(bytes)
assertTrue { bytes.contentEquals(ProtoBuf.encodeToByteArray(message)) } // Fails

2. Equal messages are encoded differently depending on type

If we try to serialize message with default values inclusion that is based on strings and message that is based on integers, we will get different results. And it works for all non-primitive and primitive types.

val messageInt = ProtobufMessage<Int>(
    intFieldDefault = null,
    intFieldFixed = null,
    intFieldSigned = null,
    // longField is 5 by default
    floatField = null,
    doubleField = null,
    stringField = null,
    booleanField = null,
    enumField = null,
    nestedMessageField = null,
    oneOfField = null,
    listField = emptyList(),
    packedListField = emptyList(),
    mapField = emptyMap(),
    packedMapField = emptyMap(),
)

val messageString = messageInt as ProtobufMessage<String>

val serializer = ProtoBuf { encodeDefaults = true }
val bytesForPrimitiveMessage = serializer.encodeToHexString<ProtobufMessage<Int>>(messageInt)
val bytesForNonPrimitiveMessages = serializer.encodeToHexString<ProtobufMessage<String>>(messageString)
assertTrue {bytesForPrimitiveMessage == bytesForNonPrimitiveMessages} // Fails

3. Decoding-encoding transformation is not an identity

For some not empty messages we can find byte sequence that will be decoded as a message that encodes into a different byte array.

val bytes = byteArrayOf(-30, 125, 0, 125)
val serializer = ProtoBuf { encodeDefaults = true }
val message = serializer.decodeFromByteArray<ProtobufMessage<ProtobufMessageInt>>(bytes)
assertTrue { bytes.contentEquals(serializer.encodeToByteArray(message)) } // Fails

4. Null cannot be assigned to a field with default value

If a field has default value you can't assign null to it. Even if null is default value, even if encodeDefaults is false

val message = ProtobufMessage<Int>(
    intFieldDefault = null,
    intFieldFixed = null,
    intFieldSigned = null,
    longField = null, // longField is 5 by default
    floatField = null,
    doubleField = null,
    stringField = null,
    booleanField = null,
    enumField = null,
    nestedMessageField = null,
    oneOfField = null,
    listField = emptyList(),
    packedListField = emptyList(),
    mapField = emptyMap(),
    packedMapField = emptyMap(),
)

assertDoesNotThrow { ProtoBuf.encodeToByteArray<ProtobufMessage<Int>>(message) } // Fails

Bugs are found by fuzzing team @ PLAN Lab

Environment

  • Kotlin version: 2.0.20
  • Library version: 1.7.3
  • Kotlin platforms: JVM
  • Gradle version: 8.8
@shanshin shanshin self-assigned this Jan 13, 2025
@shanshin
Copy link
Contributor

1.

Can't reproduce

val bytes = byteArrayOf(9)
val message = ProtoBuf.decodeFromByteArray<ProtobufMessage<Int>>(bytes)
assertTrue { bytes.contentEquals(ProtoBuf.encodeToByteArray(message)) } // Fails

Throws decoding error

kotlinx.serialization.protobuf.internal.ProtobufDecodingException: Error while decoding kotlinx.serialization.protobuf.FuzzerTest.ProtobufMessage
	at kotlinx.serialization.protobuf.internal.ProtobufDecoder.decodeSerializableValue(ProtobufDecoding.kt:278)
	at kotlinx.serialization.protobuf.internal.ProtobufDecoder.decodeSerializableValue(ProtobufDecoding.kt:243)
	at kotlinx.serialization.protobuf.ProtoBuf.decodeFromByteArray(ProtoBuf.kt:144)
	at kotlinx.serialization.protobuf.FuzzerTest.testEmpty(FuzzerTest.kt:64)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:59)
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:56)
	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
	at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306)
	at org.junit.runners.BlockJUnit4ClassRunner$1.evaluate(BlockJUnit4ClassRunner.java:100)
	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:366)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:103)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:63)
	at org.junit.runners.ParentRunner$4.run(ParentRunner.java:331)
	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:79)
	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:329)
	at org.junit.runners.ParentRunner.access$100(ParentRunner.java:66)
	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:293)
	at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306)
	at org.junit.runners.ParentRunner.run(ParentRunner.java:413)
	at org.gradle.api.internal.tasks.testing.junit.JUnitTestClassExecutor.runTestClass(JUnitTestClassExecutor.java:112)
	at org.gradle.api.internal.tasks.testing.junit.JUnitTestClassExecutor.execute(JUnitTestClassExecutor.java:58)
	at org.gradle.api.internal.tasks.testing.junit.JUnitTestClassExecutor.execute(JUnitTestClassExecutor.java:40)
	at org.gradle.api.internal.tasks.testing.junit.AbstractJUnitTestClassProcessor.processTestClass(AbstractJUnitTestClassProcessor.java:60)
	at org.gradle.api.internal.tasks.testing.SuiteTestClassProcessor.processTestClass(SuiteTestClassProcessor.java:52)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at org.gradle.internal.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:36)
	at org.gradle.internal.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:24)
	at org.gradle.internal.dispatch.ContextClassLoaderDispatch.dispatch(ContextClassLoaderDispatch.java:33)
	at org.gradle.internal.dispatch.ProxyDispatchAdapter$DispatchingInvocationHandler.invoke(ProxyDispatchAdapter.java:94)
	at com.sun.proxy.$Proxy2.processTestClass(Unknown Source)
	at org.gradle.api.internal.tasks.testing.worker.TestWorker$2.run(TestWorker.java:176)
	at org.gradle.api.internal.tasks.testing.worker.TestWorker.executeAndMaintainThreadName(TestWorker.java:129)
	at org.gradle.api.internal.tasks.testing.worker.TestWorker.execute(TestWorker.java:100)
	at org.gradle.api.internal.tasks.testing.worker.TestWorker.execute(TestWorker.java:60)
	at org.gradle.process.internal.worker.child.ActionExecutionWorker.execute(ActionExecutionWorker.java:56)
	at org.gradle.process.internal.worker.child.SystemApplicationClassLoaderWorker.call(SystemApplicationClassLoaderWorker.java:113)
	at org.gradle.process.internal.worker.child.SystemApplicationClassLoaderWorker.call(SystemApplicationClassLoaderWorker.java:65)
	at worker.org.gradle.process.internal.worker.GradleWorkerMain.run(GradleWorkerMain.java:69)
	at worker.org.gradle.process.internal.worker.GradleWorkerMain.main(GradleWorkerMain.java:74)
Caused by: kotlinx.serialization.protobuf.internal.ProtobufDecodingException: Error while decoding kotlin.Int at proto number 19500 of kotlinx.serialization.protobuf.FuzzerTest.ProtobufMessage
	at kotlinx.serialization.protobuf.internal.ProtobufDecoder.decodeSerializableValue(ProtobufDecoding.kt:278)
	at kotlinx.serialization.protobuf.internal.ProtobufTaggedDecoder.decodeNullableSerializableElement(ProtobufTaggedDecoder.kt:91)
	at kotlinx.serialization.protobuf.FuzzerTest$ProtobufMessage$$serializer.deserialize(FuzzerTest.kt:35)
	at kotlinx.serialization.protobuf.FuzzerTest$ProtobufMessage$$serializer.deserialize(FuzzerTest.kt:35)
	at kotlinx.serialization.protobuf.internal.ProtobufDecoder.decodeSerializableValue(ProtobufDecoding.kt:256)
	... 46 more
Caused by: kotlinx.serialization.protobuf.internal.ProtobufDecodingException: Error while decoding proto number 1 of kotlinx.serialization.protobuf.FuzzerTest.ProtobufMessage
	at kotlinx.serialization.protobuf.internal.ProtobufDecoder.decodeTaggedInt(ProtobufDecoding.kt:576)
	at kotlinx.serialization.protobuf.internal.ProtobufTaggedDecoder.decodeInt(ProtobufTaggedDecoder.kt:34)
	at kotlinx.serialization.internal.IntSerializer.deserialize(Primitives.kt:96)
	at kotlinx.serialization.internal.IntSerializer.deserialize(Primitives.kt:92)
	at kotlinx.serialization.protobuf.internal.ProtobufDecoder.decodeSerializableValue(ProtobufDecoding.kt:256)
	... 50 more
Caused by: kotlinx.serialization.protobuf.internal.ProtobufDecodingException: Expected wire type VARINT(0), but found i64(1)
	at kotlinx.serialization.protobuf.internal.ProtobufReader.readInt(ProtobufReader.kt:219)
	at kotlinx.serialization.protobuf.internal.ProtobufDecoder.decodeTaggedInt(ProtobufDecoding.kt:195)
	... 54 more

@shanshin
Copy link
Contributor

3.

Is not a bug.
Protobuf allows that the message may contain field values that are ignored during decoding. This happens when the field numbers in the message are unknown to the decoder.

As a result, different source messages may correspond to the same decoded object.

At the same time, kotlinx serialization always encodes the shortest version, so the original message and the encoded one are different.

In this case, the original message contains fields with numbers 2012 and 15, which are not in the kxSerialization descriptor

@shanshin
Copy link
Contributor

2.

An empty list is being encoded now for packed repeated fields, this should be removed.
#2906

@shanshin
Copy link
Contributor

3.

Related task #2908

@shanshin
Copy link
Contributor

Closed as processed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@shanshin shanshin

Labels
found-by-fuzzing
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@shanshin @fzhinkin @FerrumBrain