Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
101 changes: 101 additions & 0 deletions yarn-project/foundation/src/serialize/buffer_reader.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -295,4 +295,105 @@ describe('buffer reader', () => {
);
});
});

describe('maxSize bounds checking', () => {
describe('readVector with maxSize', () => {
it('should read vector when size is within bounds', () => {
const items = [1, 2, 3];
const buffer = serializeToBuffer(items.length, items);
const reader = new BufferReader(buffer);

const result = reader.readVector({ fromBuffer: (r: BufferReader) => r.readNumber() }, 10);

expect(result).toEqual(items);
});

it('should throw when vector size exceeds maxSize', () => {
const items = [1, 2, 3, 4, 5];
const buffer = serializeToBuffer(items.length, items);
const reader = new BufferReader(buffer);

expect(() => {
reader.readVector({ fromBuffer: (r: BufferReader) => r.readNumber() }, 3);
}).toThrow('Vector size 5 exceeds maximum allowed 3');
});

it('should allow any size when maxSize is not provided', () => {
const items = [1, 2, 3, 4, 5];
const buffer = serializeToBuffer(items.length, items);
const reader = new BufferReader(buffer);

const result = reader.readVector({ fromBuffer: (r: BufferReader) => r.readNumber() });

expect(result).toEqual(items);
});
});

describe('readBuffer with maxSize', () => {
it('should read buffer when size is within bounds', () => {
const data = Buffer.from('hello');
// readBuffer expects length prefix + data
const buffer = serializeToBuffer(data.length, data);
const reader = new BufferReader(buffer);

const result = reader.readBuffer(10);

expect(result).toEqual(data);
});

it('should throw when buffer size exceeds maxSize', () => {
const data = Buffer.from('hello world');
// readBuffer expects length prefix + data
const buffer = serializeToBuffer(data.length, data);
const reader = new BufferReader(buffer);

expect(() => {
reader.readBuffer(5);
}).toThrow('Buffer size 11 exceeds maximum allowed 5');
});

it('should allow any size when maxSize is not provided', () => {
const data = Buffer.from('hello world');
// readBuffer expects length prefix + data
const buffer = serializeToBuffer(data.length, data);
const reader = new BufferReader(buffer);

const result = reader.readBuffer();

expect(result).toEqual(data);
});
});

describe('readString with maxSize', () => {
it('should read string when size is within bounds', () => {
const str = 'hello';
const buffer = serializeToBuffer(str);
const reader = new BufferReader(buffer);

const result = reader.readString(10);

expect(result).toEqual(str);
});

it('should throw when string size exceeds maxSize', () => {
const str = 'hello world';
const buffer = serializeToBuffer(str);
const reader = new BufferReader(buffer);

expect(() => {
reader.readString(5);
}).toThrow('Buffer size 11 exceeds maximum allowed 5');
});

it('should allow any size when maxSize is not provided', () => {
const str = 'hello world';
const buffer = serializeToBuffer(str);
const reader = new BufferReader(buffer);

const result = reader.readString();

expect(result).toEqual(str);
});
});
});
});
30 changes: 21 additions & 9 deletions yarn-project/foundation/src/serialize/buffer_reader.ts
Original file line number Diff line number Diff line change
Expand Up @@ -224,15 +224,22 @@ export class BufferReader {
* deserializing each one using the 'fromBuffer' method of 'itemDeserializer'.
*
* @param itemDeserializer - Object with 'fromBuffer' method to deserialize vector elements.
* @param maxSize - Optional maximum allowed size for the vector. If the size exceeds this, an error is thrown.
* @returns An array of deserialized elements of type T.
*/
public readVector<T>(itemDeserializer: {
/**
* A method to deserialize data from a buffer.
*/
fromBuffer: (reader: BufferReader) => T;
}): T[] {
public readVector<T>(
itemDeserializer: {
/**
* A method to deserialize data from a buffer.
*/
fromBuffer: (reader: BufferReader) => T;
},
maxSize?: number,
): T[] {
const size = this.readNumber();
if (maxSize !== undefined && size > maxSize) {
throw new Error(`Vector size ${size} exceeds maximum allowed ${maxSize}`);
}
const result = new Array<T>(size);
for (let i = 0; i < size; i++) {
result[i] = itemDeserializer.fromBuffer(this);
Expand Down Expand Up @@ -344,10 +351,11 @@ export class BufferReader {
* The method first reads the size of the string, then reads the corresponding
* number of bytes from the buffer and converts them to a string.
*
* @param maxSize - Optional maximum allowed size for the string buffer. If the size exceeds this, an error is thrown.
* @returns The read string from the buffer.
*/
public readString(): string {
return this.readBuffer().toString();
public readString(maxSize?: number): string {
return this.readBuffer(maxSize).toString();
}

/**
Expand All @@ -356,10 +364,14 @@ export class BufferReader {
* a Buffer with that size containing the bytes. Useful for reading variable-length
* binary data encoded as (size, data) format.
*
* @param maxSize - Optional maximum allowed size for the buffer. If the size exceeds this, an error is thrown.
* @returns A Buffer containing the read bytes.
*/
public readBuffer(): Buffer {
public readBuffer(maxSize?: number): Buffer {
const size = this.readNumber();
if (maxSize !== undefined && size > maxSize) {
throw new Error(`Buffer size ${size} exceeds maximum allowed ${maxSize}`);
}
this.#rangeCheck(size);
return this.readBytes(size);
}
Expand Down
14 changes: 14 additions & 0 deletions yarn-project/p2p/src/services/reqresp/constants.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
/**
* Constants for P2P message deserialization bounds checking.
* These constants define maximum allowed sizes during deserialization
* to prevent DoS attacks via maliciously crafted messages.
*/

/** Max transactions per block for deserialization validation (~300x default of 32) */
export { MAX_TXS_PER_BLOCK } from '@aztec/stdlib/deserialization';

/** Max version string length (e.g., "1.0.0-alpha.123") */
export const MAX_VERSION_STRING_LENGTH = 64;

/** Max block hash string length (hex: 0x + 64 chars, with generous headroom) */
export const MAX_BLOCK_HASH_STRING_LENGTH = 128;
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { MAX_TXS_PER_BLOCK } from '../../constants.js';
import { BitVector } from './bitvector.js';

describe('BitVector', () => {
Expand Down Expand Up @@ -82,4 +83,26 @@ describe('BitVector', () => {
expect(bitVector.isSet(100)).toBe(false);
});
});

describe('fromBuffer validation', () => {
it('should throw when length exceeds MAX_TXS_PER_BLOCK', () => {
const length = MAX_TXS_PER_BLOCK + 1;
const buffer = Buffer.alloc(4 + Math.ceil(length / 8));
buffer.writeUInt32BE(length, 0);

expect(() => BitVector.fromBuffer(buffer)).toThrow(
`BitVector length ${length} exceeds maximum ${MAX_TXS_PER_BLOCK}`,
);
});

it('should accept length at MAX_TXS_PER_BLOCK boundary', () => {
const length = MAX_TXS_PER_BLOCK;
const byteLength = Math.ceil(length / 8);
const buffer = Buffer.alloc(4 + byteLength);
buffer.writeUInt32BE(length, 0);

const bitVector = BitVector.fromBuffer(buffer);
expect(bitVector.getLength()).toBe(length);
});
});
});
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import { BufferReader, serializeToBuffer } from '@aztec/foundation/serialize';

import { MAX_TXS_PER_BLOCK } from '../../constants.js';

/**
* BitVector helper class for representing and serializing bit vectors
*/
Expand Down Expand Up @@ -80,6 +82,13 @@ export class BitVector {
const reader = BufferReader.asReader(buffer);
const length = reader.readNumber();

if (length < 0) {
throw new Error(`BitVector length ${length} cannot be negative`);
}
if (length > MAX_TXS_PER_BLOCK) {
throw new Error(`BitVector length ${length} exceeds maximum ${MAX_TXS_PER_BLOCK}`);
}

const bitBuffer = reader.readBytes(BitVector.byteLength(length));
return new BitVector(bitBuffer, length);
}
Expand Down
8 changes: 5 additions & 3 deletions yarn-project/p2p/src/services/reqresp/protocols/status.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ import type { WorldStateSyncStatus, WorldStateSynchronizer } from '@aztec/stdlib

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

import { MAX_BLOCK_HASH_STRING_LENGTH, MAX_VERSION_STRING_LENGTH } from '../constants.js';

/*
* P2P Status Message
* It is used to establish Status handshake between to peers
Expand All @@ -32,12 +34,12 @@ export class StatusMessage {
static fromBuffer(buffer: Buffer | BufferReader): StatusMessage {
const reader = BufferReader.asReader(buffer);
return new StatusMessage(
reader.readString(), // compressedComponentsVersion
reader.readString(MAX_VERSION_STRING_LENGTH), // compressedComponentsVersion
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just check this? This version isn't a semver (i.e. 1.2.3-alpha.123). It's our compressed components version which incorporates a number of items. Can you verify that MAX_VERSION_STRING_LENGTH is sufficient for this?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is an example:

p2p:7:libp2p_service:libp2p_service Started libp2p service with protocol version 00-31337-00000000-0-14d6d44e-01eff751

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should be fine as MAX_VERSION_STRING_LENGTH is 64, but would like to check.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PhilWindle 64 seems to be enough for MAX_VERSION_STRING_LENGTH looking at how the version string is created:

  return [
    '00',
    versions.l1ChainId,
    versions.l1RollupAddress.toString().slice(2, 10),
    versions.rollupVersion,
    versions.l2ProtocolContractsHash.toString().slice(2, 10),
    versions.l2CircuitsVkTreeRoot.toString().slice(2, 10),
  ].join('-');

64 seems to be 2x as long as expected length

BlockNumber(reader.readNumber()), // latestBlockNumber
reader.readString(), // latestBlockHash
reader.readString(MAX_BLOCK_HASH_STRING_LENGTH), // latestBlockHash
BlockNumber(reader.readNumber()), // finalizedBlockNumber
//TODO: add finalizedBlockHash
//reader.readString(), // finalizedBlockHash
//reader.readString(MAX_BLOCK_HASH_STRING_LENGTH), // finalizedBlockHash
);
}

Expand Down
1 change: 1 addition & 0 deletions yarn-project/stdlib/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
"./package.local.json"
],
"exports": {
"./deserialization": "./dest/deserialization/index.js",
"./aztec-address": "./dest/aztec-address/index.js",
"./abi": "./dest/abi/index.js",
"./abi/function-selector": "./dest/abi/function_selector.js",
Expand Down
3 changes: 2 additions & 1 deletion yarn-project/stdlib/src/block/body.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { BufferReader, serializeToBuffer } from '@aztec/foundation/serialize';
import { inspect } from 'util';
import { z } from 'zod';

import { MAX_TX_EFFECTS_PER_BODY } from '../deserialization/index.js';
import type { ZodFor } from '../schemas/index.js';
import { TxEffect } from '../tx/tx_effect.js';

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

return new this(reader.readVector(TxEffect));
return new this(reader.readVector(TxEffect, MAX_TX_EFFECTS_PER_BODY));
}

/**
Expand Down
9 changes: 5 additions & 4 deletions yarn-project/stdlib/src/block/checkpointed_l2_block.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import type { FieldsOf } from '@aztec/foundation/types';
import { z } from 'zod';

import { L1PublishedData, PublishedCheckpoint } from '../checkpoint/published_checkpoint.js';
import { MAX_BLOCK_HASH_STRING_LENGTH, MAX_COMMITTEE_SIZE } from '../deserialization/index.js';
import { L2Block } from './l2_block.js';
import { L2BlockNew } from './l2_block_new.js';
import { CommitteeAttestation } from './proposal/committee_attestation.js';
Expand Down Expand Up @@ -36,9 +37,9 @@ export class CheckpointedL2Block {
const checkpointNumber = reader.readNumber();
const block = reader.readObject(L2BlockNew);
const l1BlockNumber = reader.readBigInt();
const l1BlockHash = reader.readString();
const l1BlockHash = reader.readString(MAX_BLOCK_HASH_STRING_LENGTH);
const l1Timestamp = reader.readBigInt();
const attestations = reader.readVector(CommitteeAttestation);
const attestations = reader.readVector(CommitteeAttestation, MAX_COMMITTEE_SIZE);
return new CheckpointedL2Block(
CheckpointNumber(checkpointNumber),
block,
Expand Down Expand Up @@ -89,9 +90,9 @@ export class PublishedL2Block {
const reader = BufferReader.asReader(bufferOrReader);
const block = reader.readObject(L2Block);
const l1BlockNumber = reader.readBigInt();
const l1BlockHash = reader.readString();
const l1BlockHash = reader.readString(MAX_BLOCK_HASH_STRING_LENGTH);
const l1Timestamp = reader.readBigInt();
const attestations = reader.readVector(CommitteeAttestation);
const attestations = reader.readVector(CommitteeAttestation, MAX_COMMITTEE_SIZE);
return new PublishedL2Block(block, new L1PublishedData(l1BlockNumber, l1Timestamp, l1BlockHash), attestations);
}

Expand Down
9 changes: 5 additions & 4 deletions yarn-project/stdlib/src/block/validate_block_result.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
deserializeCheckpointInfo,
serializeCheckpointInfo,
} from '../checkpoint/checkpoint_info.js';
import { MAX_COMMITTEE_SIZE } from '../deserialization/index.js';
import { CommitteeAttestation } from './proposal/committee_attestation.js';

/** Subtype for invalid checkpoint validation results */
Expand Down Expand Up @@ -109,13 +110,13 @@ export function deserializeValidateCheckpointResult(bufferOrReader: Buffer | Buf
if (valid) {
return { valid };
}
const reason = reader.readString() as 'insufficient-attestations' | 'invalid-attestation';
const reason = reader.readString(64) as 'insufficient-attestations' | 'invalid-attestation';
const checkpoint = deserializeCheckpointInfo(reader.readBuffer());
const committee = reader.readVector(EthAddress);
const committee = reader.readVector(EthAddress, MAX_COMMITTEE_SIZE);
const epoch = EpochNumber(reader.readNumber());
const seed = reader.readBigInt();
const attestors = reader.readVector(EthAddress);
const attestations = reader.readVector(CommitteeAttestation);
const attestors = reader.readVector(EthAddress, MAX_COMMITTEE_SIZE);
const attestations = reader.readVector(CommitteeAttestation, MAX_COMMITTEE_SIZE);
const invalidIndex = reader.readNumber();
if (reason === 'insufficient-attestations') {
return { valid, reason, checkpoint, committee, epoch, seed, attestors, attestations };
Expand Down
3 changes: 2 additions & 1 deletion yarn-project/stdlib/src/checkpoint/checkpoint.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import type { FieldsOf } from '@aztec/foundation/types';
import { z } from 'zod';

import { L2BlockNew } from '../block/l2_block_new.js';
import { MAX_BLOCKS_PER_CHECKPOINT } from '../deserialization/index.js';
import { CheckpointHeader } from '../rollup/checkpoint_header.js';
import { AppendOnlyTreeSnapshot } from '../trees/append_only_tree_snapshot.js';
import type { CheckpointInfo } from './checkpoint_info.js';
Expand Down Expand Up @@ -48,7 +49,7 @@ export class Checkpoint {
return new Checkpoint(
reader.readObject(AppendOnlyTreeSnapshot),
reader.readObject(CheckpointHeader),
reader.readVector(L2BlockNew),
reader.readVector(L2BlockNew, MAX_BLOCKS_PER_CHECKPOINT),
CheckpointNumber(reader.readNumber()),
);
}
Expand Down
7 changes: 4 additions & 3 deletions yarn-project/stdlib/src/checkpoint/published_checkpoint.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import type { FieldsOf } from '@aztec/foundation/types';
import { z } from 'zod';

import { CommitteeAttestation } from '../block/proposal/committee_attestation.js';
import { MAX_BLOCK_HASH_STRING_LENGTH, MAX_COMMITTEE_SIZE } from '../deserialization/index.js';
import { Checkpoint } from './checkpoint.js';

export class L1PublishedData {
Expand Down Expand Up @@ -42,7 +43,7 @@ export class L1PublishedData {
static fromBuffer(bufferOrReader: Buffer | BufferReader): L1PublishedData {
const reader = BufferReader.asReader(bufferOrReader);
const l1BlockNumber = reader.readBigInt();
const l1BlockHash = reader.readString();
const l1BlockHash = reader.readString(MAX_BLOCK_HASH_STRING_LENGTH);
const l1Timestamp = reader.readBigInt();
return new L1PublishedData(l1BlockNumber, l1Timestamp, l1BlockHash);
}
Expand Down Expand Up @@ -82,9 +83,9 @@ export class PublishedCheckpoint {
const reader = BufferReader.asReader(bufferOrReader);
const checkpoint = reader.readObject(Checkpoint);
const l1BlockNumber = reader.readBigInt();
const l1BlockHash = reader.readString();
const l1BlockHash = reader.readString(MAX_BLOCK_HASH_STRING_LENGTH);
const l1Timestamp = reader.readBigInt();
const attestations = reader.readVector(CommitteeAttestation);
const attestations = reader.readVector(CommitteeAttestation, MAX_COMMITTEE_SIZE);
return new PublishedCheckpoint(
checkpoint,
new L1PublishedData(l1BlockNumber, l1Timestamp, l1BlockHash),
Expand Down
Loading
Loading