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.

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

Malformed packed varints read beyond field length #3050

Open
jrose-signal opened this issue Jul 25, 2024 · 2 comments
Open

Malformed packed varints read beyond field length #3050

jrose-signal opened this issue Jul 25, 2024 · 2 comments

Comments

@jrose-signal
Copy link

jrose-signal commented Jul 25, 2024

With the following protobuf

syntax = "proto3";

message RepeatedBug {
  bytes name = 1;
  repeated int32 values = 2;
  int32 id = 3;
  bytes should_never_appear = 4;
}

And the following (malformed) input:

12       // (tag 2 | Length Delimited)
01       // Length of 'values'
80       // *** Invalid truncated varint ***
18       // (tag 3 | Varint)
22       // ID value 34, which happens to be equivalent to (tag 4 | Length Delimited)
0A       // (tag 1 | Length Delimited), which happens to be equivalent to a length of 10
09       // Length of 'name'
313233   // UTF-8 Value "123456789"
343536
373839

(which can be generated by this convenient protoscope):

2:LEN {`80`}
3:VARINT 4:LEN
1:LEN {"123456789"}

Wire-Swift treats the 3:VARINT as part of the length-delimited field, and produces RepeatedBug(values: [3072], should_never_appear: "\u{09}123456789"), instead of rejecting the input as malformed. (Most instances of this bug would result in an unexpected error somewhere else, as the input stream has been desynchronized, but I managed to find this one that puns to a different "valid" structure instead.)

As far as I can tell the Kotlin implementation has the same bug, but I didn't test it. Similarly, both implementations seem vulnerable to the same issue when it's a nested message that's truncated, rather than a packed field. (It would not surprise me if these were covered by Google's protobuf conformance tests, but I didn't check that either.)

@jrose-signal
Copy link
Author

Here's the test I added for Swift (in addition to the above proto):

func testDecodePackedRepeatedVarintMalformed() throws {
    // Protoscope input:
    // 2:LEN {`80`}
    // 3: 4:LEN
    // 1:LEN {"123456789"}
    let data = Foundation.Data(hexEncoded: """
        12       // (tag 2 | Length Delimited)
        01       // Length of 'values'
        80       // Invalid truncated varint
        18       // (tag 3 | Varint)
        22       // ID value 34, which happens to be equivalent to (tag 4 | Length Delimited)
        0A       // (tag 1 | Length Delimited), which happens to be equivalent to a length of 10
        09       // Length of 'name'
        313233   // UTF-8 Value "123456789"
        343536
        373839
    """)!
    try test(data: data) { reader in
        do {
            let result = try reader.decode(RepeatedBug.self)
            XCTFail("should have failed to parse, but got \(result)")
        } catch is ProtoDecoder.Error {
            // Okay, we got the expected error.
        }
    }
}

@oldergod
Copy link
Member

oldergod commented Aug 6, 2024

Thanks for reporting. I didn't work much on the Swift side so I cannot really help here.
I've checked what Kotlin does and we fail like so:

java.io.IOException: Expected to end at 3 but was 4
	at com.squareup.wire.ProtoReader.afterPackableScalar(ProtoReader.kt:433)
	at com.squareup.wire.ProtoReader.readVarint32(ProtoReader.kt:325)
	at com.squareup.wire.ProtoAdapterKt$commonInt32$1.decode(ProtoAdapter.kt:906)
	at com.squareup.wire.ProtoAdapterKt$commonInt32$1.decode(ProtoAdapter.kt:886)

And protoc failed as follows:

While parsing a protocol message, the input ended unexpectedly in the middle of a field.  This could mean either that the input has been truncated or that an embedded message misreported its own length.
com.google.protobuf.InvalidProtocolBufferException: While parsing a protocol message, the input ended unexpectedly in the middle of a field.  This could mean either that the input has been truncated or that an embedded message misreported its own length.
	at com.google.protobuf.InvalidProtocolBufferException.truncatedMessage(InvalidProtocolBufferException.java:92)
	at com.google.protobuf.CodedInputStream$ArrayDecoder.readRawByte(CodedInputStream.java:1217)
	at com.google.protobuf.CodedInputStream$ArrayDecoder.readRawVarint64SlowPath(CodedInputStream.java:1102)
	at com.google.protobuf.CodedInputStream$ArrayDecoder.readRawVarint32(CodedInputStream.java:996)
	at com.google.protobuf.CodedInputStream$ArrayDecoder.readInt32(CodedInputStream.java:747)

If you feel like fixing the Swift adapter, please do! Otherwise, we'll try to fix it later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants