Copilot commented on code in PR #3866:
URL: https://github.com/apache/avro/pull/3866#discussion_r3566784327
##########
lang/js/lib/schemas.js:
##########
@@ -1162,12 +1188,21 @@ MapType.prototype._read = function (tap) {
MapType.prototype._skip = function (tap) {
var values = this._values;
+ var minBytes = 1 + getMinBytes(values); // Each entry carries a >=1-byte key.
+ var total = 0;
var len, n;
while ((n = tap.readLong())) {
if (n < 0) {
+ // Sized block: bound the item count too, so the cap cannot be bypassed
by
+ // encoding the block with a negative (byte-sized) count.
+ n = -n;
len = tap.readLong();
+ total += n;
+ checkCollectionBlock(tap, n, minBytes, total);
tap.pos += len;
Review Comment:
In the sized-block skip path, `len` (block byte-size) is not validated
before `tap.pos += len`. A malicious negative `len` can move `tap.pos`
backwards; since `Tap.isValid()` only checks `pos <= buf.length` (and treats
negative positions as valid), this can bypass truncation detection and lead to
incorrect decoding/skipping behavior.
##########
lang/js/test/test_schemas.js:
##########
@@ -939,6 +939,18 @@ describe('types', function () {
assert.strictEqual(t.getName(), undefined);
});
+ it('rejects a huge block count', function () {
+ var t = new types.MapType({type: 'map', values: 'long'});
+ var buf = Buffer.from([0x80, 0x88, 0xde, 0xbe, 0x01, 0x00]);
+ assert.throws(function () { t.fromBuffer(buf); }, /collection/);
+ });
+
+ it('reads a small map', function () {
+ var t = new types.MapType({type: 'map', values: 'int'});
+ var buf = t.toBuffer({a: 1, b: 2});
+ assert.deepEqual(t.fromBuffer(buf), {a: 1, b: 2});
+ });
Review Comment:
The PR description says tests were added "for both arrays and maps" covering
huge zero-byte counts, remaining-bytes rejection, skip-path bounding (including
negative/sized blocks), and schema-resolution cases. In this file, the new
MapType tests only cover a direct decode rejection and a small decode; there
are no analogous map skip/resolution tests for the new MapType._skip and
_updateResolver logic. Please add map coverage similar to the ArrayType cases
(or update the PR description if this was intentional).
##########
lang/js/lib/schemas.js:
##########
@@ -1303,14 +1346,24 @@ ArrayType.prototype._read = function (tap) {
};
ArrayType.prototype._skip = function (tap) {
+ var items = this._items;
+ var minBytes = getMinBytes(items);
+ var total = 0;
var len, n;
while ((n = tap.readLong())) {
if (n < 0) {
+ // Sized block: bound the item count too, so the cap cannot be bypassed
by
+ // encoding the block with a negative (byte-sized) count.
+ n = -n;
len = tap.readLong();
+ total += n;
+ checkCollectionBlock(tap, n, minBytes, total);
tap.pos += len;
Review Comment:
In the sized-block skip path, `len` (block byte-size) is not validated
before `tap.pos += len`. A malicious negative `len` can move `tap.pos`
backwards; since `Tap.isValid()` only checks `pos <= buf.length`, negative
positions are treated as valid and truncation can be bypassed.
--
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]
