Skip to content

Commit 11f73b1

Browse files
mrzeszutkomrzeszutko
committed
Adding lenght validation on deserialization
1 parent 4802c84 commit 11f73b1

File tree

16 files changed

+231
-28
lines changed

16 files changed

+231
-28
lines changed

‎yarn-project/foundation/src/serialize/buffer_reader.test.ts‎

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -295,4 +295,105 @@ describe('buffer reader', () => {
295295
);
296296
});
297297
});
298+
299+
describe('maxSize bounds checking', () => {
300+
describe('readVector with maxSize', () => {
301+
it('should read vector when size is within bounds', () => {
302+
const items = [1, 2, 3];
303+
const buffer = serializeToBuffer(items.length, items);
304+
const reader = new BufferReader(buffer);
305+
306+
const result = reader.readVector({ fromBuffer: (r: BufferReader) => r.readNumber() }, 10);
307+
308+
expect(result).toEqual(items);
309+
});
310+
311+
it('should throw when vector size exceeds maxSize', () => {
312+
const items = [1, 2, 3, 4, 5];
313+
const buffer = serializeToBuffer(items.length, items);
314+
const reader = new BufferReader(buffer);
315+
316+
expect(() => {
317+
reader.readVector({ fromBuffer: (r: BufferReader) => r.readNumber() }, 3);
318+
}).toThrow('Vector size 5 exceeds maximum allowed 3');
319+
});
320+
321+
it('should allow any size when maxSize is not provided', () => {
322+
const items = [1, 2, 3, 4, 5];
323+
const buffer = serializeToBuffer(items.length, items);
324+
const reader = new BufferReader(buffer);
325+
326+
const result = reader.readVector({ fromBuffer: (r: BufferReader) => r.readNumber() });
327+
328+
expect(result).toEqual(items);
329+
});
330+
});
331+
332+
describe('readBuffer with maxSize', () => {
333+
it('should read buffer when size is within bounds', () => {
334+
const data = Buffer.from('hello');
335+
// readBuffer expects length prefix + data
336+
const buffer = serializeToBuffer(data.length, data);
337+
const reader = new BufferReader(buffer);
338+
339+
const result = reader.readBuffer(10);
340+
341+
expect(result).toEqual(data);
342+
});
343+
344+
it('should throw when buffer size exceeds maxSize', () => {
345+
const data = Buffer.from('hello world');
346+
// readBuffer expects length prefix + data
347+
const buffer = serializeToBuffer(data.length, data);
348+
const reader = new BufferReader(buffer);
349+
350+
expect(() => {
351+
reader.readBuffer(5);
352+
}).toThrow('Buffer size 11 exceeds maximum allowed 5');
353+
});
354+
355+
it('should allow any size when maxSize is not provided', () => {
356+
const data = Buffer.from('hello world');
357+
// readBuffer expects length prefix + data
358+
const buffer = serializeToBuffer(data.length, data);
359+
const reader = new BufferReader(buffer);
360+
361+
const result = reader.readBuffer();
362+
363+
expect(result).toEqual(data);
364+
});
365+
});
366+
367+
describe('readString with maxSize', () => {
368+
it('should read string when size is within bounds', () => {
369+
const str = 'hello';
370+
const buffer = serializeToBuffer(str);
371+
const reader = new BufferReader(buffer);
372+
373+
const result = reader.readString(10);
374+
375+
expect(result).toEqual(str);
376+
});
377+
378+
it('should throw when string size exceeds maxSize', () => {
379+
const str = 'hello world';
380+
const buffer = serializeToBuffer(str);
381+
const reader = new BufferReader(buffer);
382+
383+
expect(() => {
384+
reader.readString(5);
385+
}).toThrow('Buffer size 11 exceeds maximum allowed 5');
386+
});
387+
388+
it('should allow any size when maxSize is not provided', () => {
389+
const str = 'hello world';
390+
const buffer = serializeToBuffer(str);
391+
const reader = new BufferReader(buffer);
392+
393+
const result = reader.readString();
394+
395+
expect(result).toEqual(str);
396+
});
397+
});
398+
});
298399
});

‎yarn-project/foundation/src/serialize/buffer_reader.ts‎

Lines changed: 21 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -224,15 +224,22 @@ export class BufferReader {
224224
* deserializing each one using the 'fromBuffer' method of 'itemDeserializer'.
225225
*
226226
* @param itemDeserializer - Object with 'fromBuffer' method to deserialize vector elements.
227+
* @param maxSize - Optional maximum allowed size for the vector. If the size exceeds this, an error is thrown.
227228
* @returns An array of deserialized elements of type T.
228229
*/
229-
public readVector<T>(itemDeserializer: {
230-
/**
231-
* A method to deserialize data from a buffer.
232-
*/
233-
fromBuffer: (reader: BufferReader) => T;
234-
}): T[] {
230+
public readVector<T>(
231+
itemDeserializer: {
232+
/**
233+
* A method to deserialize data from a buffer.
234+
*/
235+
fromBuffer: (reader: BufferReader) => T;
236+
},
237+
maxSize?: number,
238+
): T[] {
235239
const size = this.readNumber();
240+
if (maxSize !== undefined && size > maxSize) {
241+
throw new Error(`Vector size ${size} exceeds maximum allowed ${maxSize}`);
242+
}
236243
const result = new Array<T>(size);
237244
for (let i = 0; i < size; i++) {
238245
result[i] = itemDeserializer.fromBuffer(this);
@@ -344,10 +351,11 @@ export class BufferReader {
344351
* The method first reads the size of the string, then reads the corresponding
345352
* number of bytes from the buffer and converts them to a string.
346353
*
354+
* @param maxSize - Optional maximum allowed size for the string buffer. If the size exceeds this, an error is thrown.
347355
* @returns The read string from the buffer.
348356
*/
349-
public readString(): string {
350-
return this.readBuffer().toString();
357+
public readString(maxSize?: number): string {
358+
return this.readBuffer(maxSize).toString();
351359
}
352360

353361
/**
@@ -356,10 +364,14 @@ export class BufferReader {
356364
* a Buffer with that size containing the bytes. Useful for reading variable-length
357365
* binary data encoded as (size, data) format.
358366
*
367+
* @param maxSize - Optional maximum allowed size for the buffer. If the size exceeds this, an error is thrown.
359368
* @returns A Buffer containing the read bytes.
360369
*/
361-
public readBuffer(): Buffer {
370+
public readBuffer(maxSize?: number): Buffer {
362371
const size = this.readNumber();
372+
if (maxSize !== undefined && size > maxSize) {
373+
throw new Error(`Buffer size ${size} exceeds maximum allowed ${maxSize}`);
374+
}
363375
this.#rangeCheck(size);
364376
return this.readBytes(size);
365377
}

‎yarn-project/p2p/src/services/reqresp/constants.ts‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
/**
2+
* Constants for P2P message deserialization bounds checking.
3+
* These constants define maximum allowed sizes during deserialization
4+
* to prevent DoS attacks via maliciously crafted messages.
5+
*/
6+
7+
/** Max transactions per block for deserialization validation (~300x default of 32) */
8+
export { MAX_TXS_PER_BLOCK } from '@aztec/stdlib/deserialization';
9+
10+
/** Max version string length (e.g., "1.0.0-alpha.123") */
11+
export const MAX_VERSION_STRING_LENGTH = 64;
12+
13+
/** Max block hash string length (hex: 0x + 64 chars, with generous headroom) */
14+
export const MAX_BLOCK_HASH_STRING_LENGTH = 128;

‎yarn-project/p2p/src/services/reqresp/protocols/block_txs/bitvector.test.ts‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import { MAX_TXS_PER_BLOCK } from '../../constants.js';
12
import { BitVector } from './bitvector.js';
23

34
describe('BitVector', () => {
@@ -82,4 +83,26 @@ describe('BitVector', () => {
8283
expect(bitVector.isSet(100)).toBe(false);
8384
});
8485
});
86+
87+
describe('fromBuffer validation', () => {
88+
it('should throw when length exceeds MAX_TXS_PER_BLOCK', () => {
89+
const length = MAX_TXS_PER_BLOCK + 1;
90+
const buffer = Buffer.alloc(4 + Math.ceil(length / 8));
91+
buffer.writeUInt32BE(length, 0);
92+
93+
expect(() => BitVector.fromBuffer(buffer)).toThrow(
94+
`BitVector length ${length} exceeds maximum ${MAX_TXS_PER_BLOCK}`,
95+
);
96+
});
97+
98+
it('should accept length at MAX_TXS_PER_BLOCK boundary', () => {
99+
const length = MAX_TXS_PER_BLOCK;
100+
const byteLength = Math.ceil(length / 8);
101+
const buffer = Buffer.alloc(4 + byteLength);
102+
buffer.writeUInt32BE(length, 0);
103+
104+
const bitVector = BitVector.fromBuffer(buffer);
105+
expect(bitVector.getLength()).toBe(length);
106+
});
107+
});
85108
});

‎yarn-project/p2p/src/services/reqresp/protocols/block_txs/bitvector.ts‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
import { BufferReader, serializeToBuffer } from '@aztec/foundation/serialize';
22

3+
import { MAX_TXS_PER_BLOCK } from '../../constants.js';
4+
35
/**
46
* BitVector helper class for representing and serializing bit vectors
57
*/
@@ -80,6 +82,13 @@ export class BitVector {
8082
const reader = BufferReader.asReader(buffer);
8183
const length = reader.readNumber();
8284

85+
if (length < 0) {
86+
throw new Error(`BitVector length ${length} cannot be negative`);
87+
}
88+
if (length > MAX_TXS_PER_BLOCK) {
89+
throw new Error(`BitVector length ${length} exceeds maximum ${MAX_TXS_PER_BLOCK}`);
90+
}
91+
8392
const bitBuffer = reader.readBytes(BitVector.byteLength(length));
8493
return new BitVector(bitBuffer, length);
8594
}

‎yarn-project/p2p/src/services/reqresp/protocols/status.ts‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,8 @@ import type { WorldStateSyncStatus, WorldStateSynchronizer } from '@aztec/stdlib
77

88
import type { PeerId } from '@libp2p/interface';
99

10+
import { MAX_BLOCK_HASH_STRING_LENGTH, MAX_VERSION_STRING_LENGTH } from '../constants.js';
11+
1012
/*
1113
* P2P Status Message
1214
* It is used to establish Status handshake between to peers
@@ -32,12 +34,12 @@ export class StatusMessage {
3234
static fromBuffer(buffer: Buffer | BufferReader): StatusMessage {
3335
const reader = BufferReader.asReader(buffer);
3436
return new StatusMessage(
35-
reader.readString(), // compressedComponentsVersion
37+
reader.readString(MAX_VERSION_STRING_LENGTH), // compressedComponentsVersion
3638
BlockNumber(reader.readNumber()), // latestBlockNumber
37-
reader.readString(), // latestBlockHash
39+
reader.readString(MAX_BLOCK_HASH_STRING_LENGTH), // latestBlockHash
3840
BlockNumber(reader.readNumber()), // finalizedBlockNumber
3941
//TODO: add finalizedBlockHash
40-
//reader.readString(), // finalizedBlockHash
42+
//reader.readString(MAX_BLOCK_HASH_STRING_LENGTH), // finalizedBlockHash
4143
);
4244
}
4345

‎yarn-project/stdlib/package.json‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
"./package.local.json"
88
],
99
"exports": {
10+
"./deserialization": "./dest/deserialization/index.js",
1011
"./aztec-address": "./dest/aztec-address/index.js",
1112
"./abi": "./dest/abi/index.js",
1213
"./abi/function-selector": "./dest/abi/function_selector.js",

‎yarn-project/stdlib/src/block/body.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import { BufferReader, serializeToBuffer } from '@aztec/foundation/serialize';
55
import { inspect } from 'util';
66
import { z } from 'zod';
77

8+
import { MAX_TX_EFFECTS_PER_BODY } from '../deserialization/index.js';
89
import type { ZodFor } from '../schemas/index.js';
910
import { TxEffect } from '../tx/tx_effect.js';
1011

@@ -40,7 +41,7 @@ export class Body {
4041
static fromBuffer(buf: Buffer | BufferReader) {
4142
const reader = BufferReader.asReader(buf);
4243

43-
return new this(reader.readVector(TxEffect));
44+
return new this(reader.readVector(TxEffect, MAX_TX_EFFECTS_PER_BODY));
4445
}
4546

4647
/**

‎yarn-project/stdlib/src/block/checkpointed_l2_block.ts‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import type { FieldsOf } from '@aztec/foundation/types';
66
import { z } from 'zod';
77

88
import { L1PublishedData, PublishedCheckpoint } from '../checkpoint/published_checkpoint.js';
9+
import { MAX_BLOCK_HASH_STRING_LENGTH, MAX_COMMITTEE_SIZE } from '../deserialization/index.js';
910
import { L2Block } from './l2_block.js';
1011
import { L2BlockNew } from './l2_block_new.js';
1112
import { CommitteeAttestation } from './proposal/committee_attestation.js';
@@ -36,9 +37,9 @@ export class CheckpointedL2Block {
3637
const checkpointNumber = reader.readNumber();
3738
const block = reader.readObject(L2BlockNew);
3839
const l1BlockNumber = reader.readBigInt();
39-
const l1BlockHash = reader.readString();
40+
const l1BlockHash = reader.readString(MAX_BLOCK_HASH_STRING_LENGTH);
4041
const l1Timestamp = reader.readBigInt();
41-
const attestations = reader.readVector(CommitteeAttestation);
42+
const attestations = reader.readVector(CommitteeAttestation, MAX_COMMITTEE_SIZE);
4243
return new CheckpointedL2Block(
4344
CheckpointNumber(checkpointNumber),
4445
block,
@@ -89,9 +90,9 @@ export class PublishedL2Block {
8990
const reader = BufferReader.asReader(bufferOrReader);
9091
const block = reader.readObject(L2Block);
9192
const l1BlockNumber = reader.readBigInt();
92-
const l1BlockHash = reader.readString();
93+
const l1BlockHash = reader.readString(MAX_BLOCK_HASH_STRING_LENGTH);
9394
const l1Timestamp = reader.readBigInt();
94-
const attestations = reader.readVector(CommitteeAttestation);
95+
const attestations = reader.readVector(CommitteeAttestation, MAX_COMMITTEE_SIZE);
9596
return new PublishedL2Block(block, new L1PublishedData(l1BlockNumber, l1Timestamp, l1BlockHash), attestations);
9697
}
9798

‎yarn-project/stdlib/src/block/validate_block_result.ts‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import { BufferReader, serializeToBuffer } from '@aztec/foundation/serialize';
55

66
import { z } from 'zod';
77

8+
import { MAX_COMMITTEE_SIZE } from '../deserialization/index.js';
89
import {
910
type CheckpointInfo,
1011
CheckpointInfoSchema,
@@ -109,13 +110,13 @@ export function deserializeValidateCheckpointResult(bufferOrReader: Buffer | Buf
109110
if (valid) {
110111
return { valid };
111112
}
112-
const reason = reader.readString() as 'insufficient-attestations' | 'invalid-attestation';
113+
const reason = reader.readString(64) as 'insufficient-attestations' | 'invalid-attestation';
113114
const checkpoint = deserializeCheckpointInfo(reader.readBuffer());
114-
const committee = reader.readVector(EthAddress);
115+
const committee = reader.readVector(EthAddress, MAX_COMMITTEE_SIZE);
115116
const epoch = EpochNumber(reader.readNumber());
116117
const seed = reader.readBigInt();
117-
const attestors = reader.readVector(EthAddress);
118-
const attestations = reader.readVector(CommitteeAttestation);
118+
const attestors = reader.readVector(EthAddress, MAX_COMMITTEE_SIZE);
119+
const attestations = reader.readVector(CommitteeAttestation, MAX_COMMITTEE_SIZE);
119120
const invalidIndex = reader.readNumber();
120121
if (reason === 'insufficient-attestations') {
121122
return { valid, reason, checkpoint, committee, epoch, seed, attestors, attestations };

0 commit comments

Comments
 (0)