iemejia commented on code in PR #3861:
URL: https://github.com/apache/avro/pull/3861#discussion_r3612690299
##########
lang/py/avro/io.py:
##########
@@ -198,6 +199,13 @@ class BinaryDecoder:
_reader: IO[bytes]
+ #: Reads with a declared length above this many bytes are validated against
Review Comment:
Good catch — `#:` is Sphinx/autodoc syntax that documents the attribute on
the following line, but you're right the project doesn't use it anywhere else
and doesn't render autodoc attribute docs, so it was just inconsistent.
Switched all of these to plain `#` comments (and dropped the couple of `:data:`
roles for the same reason).
##########
lang/py/avro/io.py:
##########
@@ -393,12 +459,28 @@ def skip_double(self) -> None:
self.skip(8)
def skip_bytes(self) -> None:
- self.skip(self.read_long())
+ # The length prefix is attacker-controlled: a negative value would seek
+ # backwards (corrupting the decoder position, e.g. an infinite loop
+ # during schema resolution) and an oversized value past EOF. Validate
it
+ # the same way ``read`` does before skipping.
+ n = self.read_long()
+ if n < 0:
+ raise avro.errors.InvalidAvroBinaryEncoding(f"Requested {n} bytes
to skip, expected positive integer.")
Review Comment:
Removed the redundant check — a negative length now falls through to
`skip()`, which already rejects backward seeks, so there's a single enforcement
point. (`read()` keeps its own check since it doesn't delegate to `skip()`.)
##########
lang/py/avro/io.py:
##########
@@ -246,6 +286,11 @@ def read_long(self) -> int:
n = b & 0x7F
shift = 7
while (b & 0x80) != 0:
+ # A 64-bit value needs at most 10 bytes (shifts 0..63); reject an
+ # overlong varint rather than accepting a malformed, arbitrarily
+ # large value.
+ if shift >= 70:
+ raise avro.errors.InvalidAvroBinaryEncoding("Varint is too
long")
b = ord(self.read(1))
n |= (b & 0x7F) << shift
shift += 7
Review Comment:
Took a look and compared against Java's
`BinaryDecoder.readLong`/`innerLongDecode`:
- Our 10-byte cap matches Java: both reject an 11th continuation byte, so
neither accepts a varint longer than 10 bytes.
- On the 10th byte we're actually *stricter* than Java — we reject payload
bits above bit 63 (`b & 0x7E`), whereas Java `^=`s them shifted past the 64-bit
boundary and silently discards the overflow. Nothing a conformant encoder emits
is rejected by our check.
- You're right that the spec doesn't mandate minimal encoding, so
non-minimal forms within the 10-byte budget (trailing zero-continuation groups)
are accepted by both SDKs and decode identically — a real determinism gap, but
a cross-language one that would need a spec change to require canonical varints.
For this PR the 10-byte bound is the security-relevant part (it bounds both
the work and the value range), so I'd prefer to keep that here. How would you
suggest handling the determinism/canonical-encoding question — is it worth a
separate JIRA + spec PR to require minimal varint encoding across languages,
and do you think we should also relax our extra 10th-byte strictness to exactly
match Java's behavior in the meantime? Happy to go either way.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
