[
https://issues.apache.org/jira/browse/AVRO-4343?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18106896#comment-18106896
]
Ismaël Mejía commented on AVRO-4343:
------------------------------------
Verified the runtime behaviour against lang/perl (perl 5.40):
* Skipping a non-trailing {{bytes}}/{{string}}/{{fixed}} field returns
*corrupted* data (the absolute {{seek(\$size, 0)}} repositions the reader): a
follow-on long field decoded as 60 instead of 777.
* Skipping an {{array}} or {{map}} field *crashes* with "Can't call method
\"read\" on unblessed reference" because skip_array/skip_map call
skip_block(...) as a plain sub, so the \$class/\$reader/callback arguments are
misaligned. The negative-block-count seek+next loop is latent behind this
crash. So the practical impact is a correctness bug (crash + silent corruption)
on ordinary schema evolution rather than a live DoS.
Fix posted: https://github.com/apache/avro/pull/3947 (relative SEEK_CUR skips,
reject negative bytes/block sizes, invoke skip_block as a method, handle
negative block counts like decode_array/decode_map; adds regression tests).
> [perl] BinaryDecoder skip path broken: absolute seek corrupts decoding and
> skip_block infinite-loops on negative block count
> ----------------------------------------------------------------------------------------------------------------------------
>
> Key: AVRO-4343
> URL: https://issues.apache.org/jira/browse/AVRO-4343
> Project: Apache Avro
> Issue Type: Bug
> Components: perl
> Reporter: Ismaël Mejía
> Priority: Major
> Labels: pull-request-available
> Time Spent: 10m
> Remaining Estimate: 0h
>
> The Perl BinaryDecoder skip path (used during schema resolution when the
> writer's record contains a field absent from the reader's schema -- see
> decode_record -> skip) is broken in two ways. Both are reachable with ordinary
> schema evolution and one is a denial-of-service on crafted input. The code has
> been present since the initial Perl implementation (AVRO-974).
> *1. Absolute seek instead of relative in skip_bytes / skip_string /
> skip_fixed*
> {code:perl}
> sub skip_bytes {
> my $size = decode_long(...);
> $reader->seek($size, 0); # whence 0 = SEEK_SET (absolute!)
> }
> {code}
> To skip a value the decoder must advance the cursor by $size bytes
> (SEEK_CUR, whence 1). Using whence 0 seeks to *absolute* offset $size from the
> start of the stream, so any bytes/string/fixed field that is skipped
> mid-record
> repositions the reader and corrupts the decoding of every following field.
> skip_fixed (seek($schema->size, 0)) has the same defect.
> This is silently masked by t/03_bin_decode.t because the only skipped field in
> the test ("bonus") is the *last* writer field, so the wrong position is never
> observed. Reorder it (or skip a non-trailing field) and decoding breaks.
> *2. Infinite loop in skip_block on a negative block count (DoS)*
> {code:perl}
> sub skip_block {
> my $block_count = decode_long(...);
> while ($block_count) {
> if ($block_count < 0) {
> $reader->seek($block_count, 0); # negative absolute seek;
> block-size never read
> next; # re-loops WITHOUT re-reading
> $block_count
> }
> ...
> $block_count = decode_long(...);
> }
> }
> {code}
> Per the Avro spec, a negative array/map block count is followed by a long
> block-size (in bytes) to permit skipping. skip_block never reads that
> block-size,
> seeks to a negative absolute offset (a no-op / failure on a scalar or file
> handle), and then does `next` without re-reading $block_count -- so
> $block_count
> stays negative and the loop spins forever. An attacker who controls the
> serialized data (and knows the reader drops an array/map field) can encode a
> negative block count to hang the decoder -> CPU denial of service. This is the
> skip-path analogue of the Java issue fixed in AVRO-3635.
> *Reachability*: both paths are hit by decode_record whenever the writer schema
> has a field not present in the reader schema (a common schema-evolution case).
> *Suggested fix*:
> - skip_bytes/skip_string/skip_fixed: use a relative skip (seek($size, 1) /
> SEEK_CUR, or read-and-discard) and reject negative lengths.
> - skip_block: when block_count < 0, read the following block-size long and
> skip
> forward by that many bytes (or decode/skip abs(count) items), reject
> negative
> block-size, and always re-read block_count each iteration (drop the bare
> `next`).
> - Add resolution tests that skip a *non-trailing* bytes field and skip an
> array/map encoded with a negative block count.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
