-
Notifications
You must be signed in to change notification settings - Fork 1.8k
AVRO-4301: [js] Bound collection allocation when decoding arrays and maps #3866
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
1dd095f
7fe9fc3
6ae9c61
b50e676
27b6f73
85f05d8
fa77337
e87dcda
ecedd26
7d73199
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -65,6 +65,26 @@ var PATH = []; | |
| // Currently active logical type, used for name redirection. | ||
| var LOGICAL_TYPE = null; | ||
|
|
||
| // Collection allocation limits, guarding against a block-count DoS. | ||
| // | ||
| // An `array` or `map` block is encoded as an item count followed by that many | ||
| // items. A malicious or truncated input can declare a very large count while | ||
| // carrying little or no data. Elements that encode to zero bytes (e.g. `null`) | ||
| // consume no input, so the block count cannot be bounded by the bytes actually | ||
| // remaining in the buffer alone; such a block is capped by item count instead. | ||
| // Both limits can be overridden (to the same value) via the | ||
| // `AVRO_MAX_COLLECTION_ITEMS` environment variable. | ||
| var MAX_COLLECTION_ITEMS = 10000000; // Zero-byte element cap. | ||
| var MAX_COLLECTION_STRUCTURAL = 2147483639; // Integer.MAX_VALUE - 8. | ||
| if (typeof process !== 'undefined' && process.env && | ||
| process.env.AVRO_MAX_COLLECTION_ITEMS) { | ||
| var envItems = parseInt(process.env.AVRO_MAX_COLLECTION_ITEMS, 10); | ||
| if (envItems >= 0) { | ||
| MAX_COLLECTION_ITEMS = envItems; | ||
| MAX_COLLECTION_STRUCTURAL = envItems; | ||
| } | ||
| } | ||
|
|
||
|
|
||
| /** | ||
| * Schema parsing entry point. | ||
|
|
@@ -1149,9 +1169,15 @@ MapType.prototype._check = function (val, cb) { | |
|
|
||
| MapType.prototype._read = function (tap) { | ||
| var values = this._values; | ||
| var minBytes = this._valuesMinBytes !== undefined ? | ||
| this._valuesMinBytes : | ||
| 1 + getMinBytes(values); // Each entry carries a >=1-byte key. | ||
| var val = {}; | ||
| var total = 0; | ||
| var n; | ||
| while ((n = readArraySize(tap))) { | ||
| total += n; | ||
| checkCollectionBlock(tap, n, minBytes, total); | ||
| while (n--) { | ||
| var key = tap.readString(); | ||
| val[key] = values._read(tap); | ||
|
|
@@ -1162,12 +1188,16 @@ 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) { | ||
| len = tap.readLong(); | ||
| tap.pos += len; | ||
|
Comment on lines
1204
to
1223
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in MapType._skip — the sized-block path now rejects a |
||
| } else { | ||
|
Comment on lines
1199
to
1224
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 6ae9c61: the negative (byte-sized) block branch of _skip now applies the cumulative total and checkCollectionBlock too, so the caps can't be bypassed with a negative count. Added a regression test. |
||
| total += n; | ||
| checkCollectionBlock(tap, n, minBytes, total); | ||
| while (n--) { | ||
| tap.skipString(); | ||
| values._skip(tap); | ||
|
|
@@ -1203,6 +1233,8 @@ MapType.prototype._match = function () { | |
| MapType.prototype._updateResolver = function (resolver, type, opts) { | ||
| if (type instanceof MapType) { | ||
| resolver._values = this._values.createResolver(type._values, opts); | ||
| // Bound the block count using the writer's entry size (see ArrayType). | ||
| resolver._valuesMinBytes = 1 + getMinBytes(type._values); | ||
| resolver._read = this._read; | ||
|
Comment on lines
1259
to
1264
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed — added |
||
| } | ||
| }; | ||
|
|
@@ -1288,13 +1320,19 @@ ArrayType.prototype._check = function (val, cb) { | |
|
|
||
| ArrayType.prototype._read = function (tap) { | ||
| var items = this._items; | ||
| var minBytes = this._itemsMinBytes !== undefined ? | ||
| this._itemsMinBytes : | ||
| getMinBytes(items); | ||
| var val = []; | ||
| var total = 0; | ||
| var n; | ||
| while ((n = tap.readLong())) { | ||
| if (n < 0) { | ||
| n = -n; | ||
| tap.skipLong(); // Skip size. | ||
| } | ||
| total += n; | ||
| checkCollectionBlock(tap, n, minBytes, total); | ||
| while (n--) { | ||
| val.push(items._read(tap)); | ||
| } | ||
|
|
@@ -1303,14 +1341,19 @@ 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) { | ||
| len = tap.readLong(); | ||
| tap.pos += len; | ||
|
Comment on lines
+1378
to
1398
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in b50e676: same negative-byte-size guard added to the other collection's _skip sized-block branch.
Comment on lines
1379
to
1398
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed the same way in ArrayType._skip — added the |
||
| } else { | ||
|
Comment on lines
1374
to
1399
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 6ae9c61: same fix applied to the other collection's _skip negative-block branch. |
||
| total += n; | ||
| checkCollectionBlock(tap, n, minBytes, total); | ||
| while (n--) { | ||
| this._items._skip(tap); | ||
| items._skip(tap); | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -1354,6 +1397,9 @@ ArrayType.prototype._match = function (tap1, tap2) { | |
| ArrayType.prototype._updateResolver = function (resolver, type, opts) { | ||
| if (type instanceof ArrayType) { | ||
| resolver._items = this._items.createResolver(type._items, opts); | ||
| // Bound the block count using the writer's element size, which is what is | ||
| // actually on the wire (the reader/resolved type may be larger). | ||
| resolver._itemsMinBytes = getMinBytes(type._items); | ||
| resolver._read = this._read; | ||
| } | ||
| }; | ||
|
|
@@ -2153,6 +2199,74 @@ function readArraySize(tap) { | |
| return n; | ||
| } | ||
|
|
||
| /** | ||
| * Minimum number of bytes a value of the given type can occupy on the wire. | ||
| * | ||
| * Used to bound collection block counts against the bytes actually remaining. | ||
| * Memoized on the type; recursive records are handled with a `seen` guard | ||
| * (a directly self-referential required field cannot encode in finite bytes, | ||
| * and self-references reached through an array/map/union already contribute at | ||
| * least one byte before recursing, so returning 0 on a cycle is safe). | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reworded the docstring — it now says the 0-on-cycle result is a conservative lower bound that may under-estimate (e.g. a union whose only small branch is a recursive record) but only ever makes the check more permissive, never rejecting valid data. |
||
| */ | ||
| function getMinBytes(type, seen) { | ||
| if (type._minBytes !== undefined) { | ||
| return type._minBytes; | ||
| } | ||
| var mb; | ||
| if (type instanceof NullType) { | ||
| mb = 0; | ||
| } else if (type instanceof FixedType) { | ||
| mb = type._size; | ||
| } else if (type instanceof FloatType) { | ||
| mb = 4; | ||
| } else if (type instanceof DoubleType) { | ||
| mb = 8; | ||
| } else if (type instanceof LogicalType) { | ||
| mb = getMinBytes(type._underlyingType, seen); | ||
| } else if (type instanceof RecordType) { | ||
| seen = seen || []; | ||
| if (~seen.indexOf(type)) { | ||
| return 0; // Cycle; see note above. | ||
| } | ||
|
Comment on lines
+2296
to
+2299
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a deliberate conservative lower bound: for a cyclic schema whose true minimum can't be computed without unbounded recursion, returning 0 can only make the bytes-remaining check more permissive (it never falsely rejects valid data) — matching how the other SDKs treat recursion. Computing the exact minimum for a recursive union branch isn't decidable in general here. I've reworded the docstring to state this explicitly rather than implying the bound is tight. |
||
| seen.push(type); | ||
| mb = 0; | ||
| var fields = type._fields; | ||
| for (var i = 0, l = fields.length; i < l; i++) { | ||
| mb += getMinBytes(fields[i]._type, seen); | ||
| } | ||
| seen.pop(); | ||
| } else { | ||
| // Booleans, ints, longs, strings, bytes, enums (index), unions (index), | ||
| // and arrays/maps (empty block terminator) all occupy at least one byte. | ||
| mb = 1; | ||
| } | ||
|
Comment on lines
+2307
to
+2311
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 6ae9c61: getMinBytes now handles unions as 1 (branch index) + the smallest branch minimum, so ['int','long'] is at least 2 bytes and the available-bytes check isn't underestimated. |
||
| type._minBytes = mb; | ||
| return mb; | ||
| } | ||
|
|
||
| /** | ||
| * Reject a collection block whose declared item count cannot be backed by the | ||
| * input, guarding against unbounded allocation from a tiny payload. | ||
| * | ||
| * `total` is the cumulative item count across all blocks read so far (including | ||
| * the current one). Elements with a positive on-wire minimum are bounded by the | ||
| * bytes remaining in the buffer; zero-byte elements are bounded by the item | ||
| * cap; and every collection is bounded by the structural cap. | ||
| */ | ||
| function checkCollectionBlock(tap, n, minBytes, total) { | ||
| if (total > MAX_COLLECTION_STRUCTURAL) { | ||
| throw new Error('collection size exceeds maximum allowed'); | ||
| } | ||
| if (minBytes > 0) { | ||
| // Divide (rather than multiply) to avoid any overflow on large counts. | ||
| if (n > (tap.buf.length - tap.pos) / minBytes) { | ||
| throw new Error('collection size exceeds remaining buffer'); | ||
| } | ||
| } else if (total > MAX_COLLECTION_ITEMS) { | ||
| throw new Error('collection of zero-byte items exceeds maximum allowed'); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Correctly stringify an object which contains types. | ||
| * | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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}); | ||
| }); | ||
|
Comment on lines
+951
to
+961
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in b50e676: added MapType coverage mirroring the array cases — huge block count while skipping, huge negative (sized) block count while skipping, a rejected negative block byte-size, and a small map decoded under schema resolution. |
||
|
|
||
| }); | ||
|
|
||
| describe('ArrayType', function () { | ||
|
|
@@ -971,6 +983,60 @@ describe('types', function () { | |
| assert.deepEqual(t.fromBuffer(buf), [1]); | ||
| }); | ||
|
|
||
| it('rejects a huge zero-byte block count', function () { | ||
| // 6-byte payload declaring a block count of 200,000,000 nulls. | ||
| var t = new types.ArrayType({type: 'array', items: 'null'}); | ||
| var buf = Buffer.from([0x80, 0x88, 0xde, 0xbe, 0x01, 0x00]); | ||
| assert.throws(function () { t.fromBuffer(buf); }, /collection/); | ||
| }); | ||
|
|
||
| it('reads a small zero-byte collection', function () { | ||
| var t = new types.ArrayType({type: 'array', items: 'null'}); | ||
| var buf = Buffer.from([6, 0]); // Block count 3, then terminator. | ||
| assert.deepEqual(t.fromBuffer(buf), [null, null, null]); | ||
| }); | ||
|
|
||
| it('rejects a block count above the remaining bytes', function () { | ||
| var t = new types.ArrayType({type: 'array', items: 'long'}); | ||
| var buf = Buffer.from([0x80, 0x88, 0xde, 0xbe, 0x01, 0x00]); | ||
| assert.throws(function () { t.fromBuffer(buf); }, /collection/); | ||
| }); | ||
|
|
||
| it('bounds a huge zero-byte block count while skipping', function () { | ||
| var v1 = createType({ | ||
| name: 'Foo', | ||
| type: 'record', | ||
| fields: [ | ||
| {name: 'array', type: {type: 'array', items: 'null'}}, | ||
| {name: 'val', type: 'int'} | ||
| ] | ||
| }); | ||
| var v2 = createType({ | ||
| name: 'Foo', | ||
| type: 'record', | ||
| fields: [{name: 'val', type: 'int'}] | ||
| }); | ||
| var buf = Buffer.from([0x80, 0x88, 0xde, 0xbe, 0x01, 0x00, 6]); | ||
| var resolver = v2.createResolver(v1); | ||
| assert.throws(function () { v2.fromBuffer(buf, resolver); }, /collection/); | ||
| }); | ||
|
|
||
| it('bounds a huge block count under resolution', function () { | ||
| var t1 = new types.ArrayType({type: 'array', items: 'null'}); | ||
| var t2 = createType({type: 'array', items: ['null', 'long']}); | ||
| var buf = Buffer.from([0x80, 0x88, 0xde, 0xbe, 0x01, 0x00]); | ||
| var resolver = t2.createResolver(t1); | ||
| assert.throws(function () { t2.fromBuffer(buf, resolver); }, /collection/); | ||
| }); | ||
|
|
||
| it('reads a small collection under resolution', function () { | ||
| var t1 = new types.ArrayType({type: 'array', items: 'null'}); | ||
| var t2 = createType({type: 'array', items: ['null', 'long']}); | ||
| var buf = t1.toBuffer([null, null, null, null, null]); | ||
| var resolver = t2.createResolver(t1); | ||
| assert.equal(t2.fromBuffer(buf, resolver).length, 5); | ||
| }); | ||
|
|
||
| it('skip', function () { | ||
| var v1 = createType({ | ||
| name: 'Foo', | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in b50e676: the sized-block skip path now rejects a negative block byte-size before
tap.pos += len, so it can't move the position backwards and bypass truncation detection (Tap.isValid only checks pos <= buf.length).