From 77d935221a4805107b20b60ae7c1148725e4e2d0 Mon Sep 17 00:00:00 2001 From: steveluscher Date: Mon, 1 Apr 2024 14:33:05 +0000 Subject: [PATCH] fix: bounds check --- packages/library-legacy/src/message/legacy.ts | 17 +++--- packages/library-legacy/src/message/v0.ts | 41 ++++++++++----- .../library-legacy/src/transaction/legacy.ts | 3 +- .../src/transaction/versioned.ts | 3 +- .../src/utils/guarded-array-utils.ts | 34 ++++++++++++ packages/library-legacy/src/validator-info.ts | 7 ++- .../test/guarded-array-utils.test.ts | 52 +++++++++++++++++++ 7 files changed, 133 insertions(+), 24 deletions(-) create mode 100644 packages/library-legacy/src/utils/guarded-array-utils.ts create mode 100644 packages/library-legacy/test/guarded-array-utils.test.ts diff --git a/packages/library-legacy/src/message/legacy.ts b/packages/library-legacy/src/message/legacy.ts index 587c6df5d15b..6b8ee132967a 100644 --- a/packages/library-legacy/src/message/legacy.ts +++ b/packages/library-legacy/src/message/legacy.ts @@ -16,6 +16,7 @@ import { import {TransactionInstruction} from '../transaction'; import {CompiledKeys} from './compiled-keys'; import {MessageAccountKeys} from './account-keys'; +import {guardedShift, guardedSplice} from '../utils/guarded-array-utils'; /** * An instruction to execute by a program @@ -268,7 +269,7 @@ export class Message { // Slice up wire data let byteArray = [...buffer]; - const numRequiredSignatures = byteArray.shift()!; + const numRequiredSignatures = guardedShift(byteArray); if ( numRequiredSignatures !== (numRequiredSignatures & VERSION_PREFIX_MASK) @@ -278,26 +279,26 @@ export class Message { ); } - const numReadonlySignedAccounts = byteArray.shift()!; - const numReadonlyUnsignedAccounts = byteArray.shift()!; + const numReadonlySignedAccounts = guardedShift(byteArray); + const numReadonlyUnsignedAccounts = guardedShift(byteArray); const accountCount = shortvec.decodeLength(byteArray); let accountKeys = []; for (let i = 0; i < accountCount; i++) { - const account = byteArray.splice(0, PUBLIC_KEY_LENGTH); + const account = guardedSplice(byteArray, 0, PUBLIC_KEY_LENGTH); accountKeys.push(new PublicKey(Buffer.from(account))); } - const recentBlockhash = byteArray.splice(0, PUBLIC_KEY_LENGTH); + const recentBlockhash = guardedSplice(byteArray, 0, PUBLIC_KEY_LENGTH); const instructionCount = shortvec.decodeLength(byteArray); let instructions: CompiledInstruction[] = []; for (let i = 0; i < instructionCount; i++) { - const programIdIndex = byteArray.shift()!; + const programIdIndex = guardedShift(byteArray); const accountCount = shortvec.decodeLength(byteArray); - const accounts = byteArray.splice(0, accountCount); + const accounts = guardedSplice(byteArray, 0, accountCount); const dataLength = shortvec.decodeLength(byteArray); - const dataSlice = byteArray.splice(0, dataLength); + const dataSlice = guardedSplice(byteArray, 0, dataLength); const data = bs58.encode(Buffer.from(dataSlice)); instructions.push({ programIdIndex, diff --git a/packages/library-legacy/src/message/v0.ts b/packages/library-legacy/src/message/v0.ts index 10156568676c..c4263389aee0 100644 --- a/packages/library-legacy/src/message/v0.ts +++ b/packages/library-legacy/src/message/v0.ts @@ -16,6 +16,7 @@ import {TransactionInstruction} from '../transaction'; import {AddressLookupTableAccount} from '../programs'; import {CompiledKeys} from './compiled-keys'; import {AccountKeysFromLookups, MessageAccountKeys} from './account-keys'; +import {guardedShift, guardedSplice} from '../utils/guarded-array-utils'; /** * Message constructor arguments @@ -426,7 +427,7 @@ export class MessageV0 { static deserialize(serializedMessage: Uint8Array): MessageV0 { let byteArray = [...serializedMessage]; - const prefix = byteArray.shift() as number; + const prefix = guardedShift(byteArray); const maskedPrefix = prefix & VERSION_PREFIX_MASK; assert( prefix !== maskedPrefix, @@ -440,29 +441,35 @@ export class MessageV0 { ); const header: MessageHeader = { - numRequiredSignatures: byteArray.shift() as number, - numReadonlySignedAccounts: byteArray.shift() as number, - numReadonlyUnsignedAccounts: byteArray.shift() as number, + numRequiredSignatures: guardedShift(byteArray), + numReadonlySignedAccounts: guardedShift(byteArray), + numReadonlyUnsignedAccounts: guardedShift(byteArray), }; const staticAccountKeys = []; const staticAccountKeysLength = shortvec.decodeLength(byteArray); for (let i = 0; i < staticAccountKeysLength; i++) { staticAccountKeys.push( - new PublicKey(byteArray.splice(0, PUBLIC_KEY_LENGTH)), + new PublicKey(guardedSplice(byteArray, 0, PUBLIC_KEY_LENGTH)), ); } - const recentBlockhash = bs58.encode(byteArray.splice(0, PUBLIC_KEY_LENGTH)); + const recentBlockhash = bs58.encode( + guardedSplice(byteArray, 0, PUBLIC_KEY_LENGTH), + ); const instructionCount = shortvec.decodeLength(byteArray); const compiledInstructions: MessageCompiledInstruction[] = []; for (let i = 0; i < instructionCount; i++) { - const programIdIndex = byteArray.shift() as number; + const programIdIndex = guardedShift(byteArray); const accountKeyIndexesLength = shortvec.decodeLength(byteArray); - const accountKeyIndexes = byteArray.splice(0, accountKeyIndexesLength); + const accountKeyIndexes = guardedSplice( + byteArray, + 0, + accountKeyIndexesLength, + ); const dataLength = shortvec.decodeLength(byteArray); - const data = new Uint8Array(byteArray.splice(0, dataLength)); + const data = new Uint8Array(guardedSplice(byteArray, 0, dataLength)); compiledInstructions.push({ programIdIndex, accountKeyIndexes, @@ -473,11 +480,21 @@ export class MessageV0 { const addressTableLookupsCount = shortvec.decodeLength(byteArray); const addressTableLookups: MessageAddressTableLookup[] = []; for (let i = 0; i < addressTableLookupsCount; i++) { - const accountKey = new PublicKey(byteArray.splice(0, PUBLIC_KEY_LENGTH)); + const accountKey = new PublicKey( + guardedSplice(byteArray, 0, PUBLIC_KEY_LENGTH), + ); const writableIndexesLength = shortvec.decodeLength(byteArray); - const writableIndexes = byteArray.splice(0, writableIndexesLength); + const writableIndexes = guardedSplice( + byteArray, + 0, + writableIndexesLength, + ); const readonlyIndexesLength = shortvec.decodeLength(byteArray); - const readonlyIndexes = byteArray.splice(0, readonlyIndexesLength); + const readonlyIndexes = guardedSplice( + byteArray, + 0, + readonlyIndexesLength, + ); addressTableLookups.push({ accountKey, writableIndexes, diff --git a/packages/library-legacy/src/transaction/legacy.ts b/packages/library-legacy/src/transaction/legacy.ts index ee11644cd8c3..3dd3810673d5 100644 --- a/packages/library-legacy/src/transaction/legacy.ts +++ b/packages/library-legacy/src/transaction/legacy.ts @@ -12,6 +12,7 @@ import type {Signer} from '../keypair'; import type {Blockhash} from '../blockhash'; import type {CompiledInstruction} from '../message'; import {sign, verify} from '../utils/ed25519'; +import {guardedSplice} from '../utils/guarded-array-utils'; /** @internal */ type MessageSignednessErrors = { @@ -904,7 +905,7 @@ export class Transaction { const signatureCount = shortvec.decodeLength(byteArray); let signatures = []; for (let i = 0; i < signatureCount; i++) { - const signature = byteArray.splice(0, SIGNATURE_LENGTH_IN_BYTES); + const signature = guardedSplice(byteArray, 0, SIGNATURE_LENGTH_IN_BYTES); signatures.push(bs58.encode(Buffer.from(signature))); } diff --git a/packages/library-legacy/src/transaction/versioned.ts b/packages/library-legacy/src/transaction/versioned.ts index 53e78ade1fff..a810beff7986 100644 --- a/packages/library-legacy/src/transaction/versioned.ts +++ b/packages/library-legacy/src/transaction/versioned.ts @@ -8,6 +8,7 @@ import * as shortvec from '../utils/shortvec-encoding'; import * as Layout from '../layout'; import {sign} from '../utils/ed25519'; import {PublicKey} from '../publickey'; +import {guardedSplice} from '../utils/guarded-array-utils'; export type TransactionVersion = 'legacy' | 0; @@ -82,7 +83,7 @@ export class VersionedTransaction { const signaturesLength = shortvec.decodeLength(byteArray); for (let i = 0; i < signaturesLength; i++) { signatures.push( - new Uint8Array(byteArray.splice(0, SIGNATURE_LENGTH_IN_BYTES)), + new Uint8Array(guardedSplice(byteArray, 0, SIGNATURE_LENGTH_IN_BYTES)), ); } diff --git a/packages/library-legacy/src/utils/guarded-array-utils.ts b/packages/library-legacy/src/utils/guarded-array-utils.ts new file mode 100644 index 000000000000..847f1f9aae43 --- /dev/null +++ b/packages/library-legacy/src/utils/guarded-array-utils.ts @@ -0,0 +1,34 @@ +const END_OF_BUFFER_ERROR_MESSAGE = 'Reached end of buffer unexpectedly'; + +/** + * Delegates to `Array#shift`, but throws if the array is zero-length. + */ +export function guardedShift(byteArray: T[]): T { + if (byteArray.length === 0) { + throw new Error(END_OF_BUFFER_ERROR_MESSAGE); + } + return byteArray.shift() as T; +} + +/** + * Delegates to `Array#splice`, but throws if the section being spliced out extends past the end of + * the array. + */ +export function guardedSplice( + byteArray: T[], + ...args: + | [start: number, deleteCount?: number] + | [start: number, deleteCount: number, ...items: T[]] +): T[] { + const [start] = args; + if ( + args.length === 2 // Implies that `deleteCount` was supplied + ? start + (args[1] ?? 0) > byteArray.length + : start >= byteArray.length + ) { + throw new Error(END_OF_BUFFER_ERROR_MESSAGE); + } + return byteArray.splice( + ...(args as Parameters), + ); +} diff --git a/packages/library-legacy/src/validator-info.ts b/packages/library-legacy/src/validator-info.ts index 2412b0a75e42..7e681003d705 100644 --- a/packages/library-legacy/src/validator-info.ts +++ b/packages/library-legacy/src/validator-info.ts @@ -9,6 +9,7 @@ import { import * as Layout from './layout'; import * as shortvec from './utils/shortvec-encoding'; import {PublicKey, PUBLIC_KEY_LENGTH} from './publickey'; +import {guardedShift, guardedSplice} from './utils/guarded-array-utils'; export const VALIDATOR_INFO_KEY = new PublicKey( 'Va1idator1nfo111111111111111111111111111111', @@ -83,8 +84,10 @@ export class ValidatorInfo { const configKeys: Array = []; for (let i = 0; i < 2; i++) { - const publicKey = new PublicKey(byteArray.splice(0, PUBLIC_KEY_LENGTH)); - const isSigner = byteArray.splice(0, 1)[0] === 1; + const publicKey = new PublicKey( + guardedSplice(byteArray, 0, PUBLIC_KEY_LENGTH), + ); + const isSigner = guardedShift(byteArray) === 1; configKeys.push({publicKey, isSigner}); } diff --git a/packages/library-legacy/test/guarded-array-utils.test.ts b/packages/library-legacy/test/guarded-array-utils.test.ts new file mode 100644 index 000000000000..06afec819a16 --- /dev/null +++ b/packages/library-legacy/test/guarded-array-utils.test.ts @@ -0,0 +1,52 @@ +import {expect} from 'chai'; +import {spy} from 'sinon'; + +import {guardedShift, guardedSplice} from '../src/utils/guarded-array-utils'; + +describe('guardedShift', () => { + it('delegates to Array#shift', () => { + const arr = [1, 2, 3]; + const shiftSpy = spy(arr, 'shift'); + const result = guardedShift(arr); + expect(shiftSpy).is.calledWithExactly(); + expect(result).to.eq(shiftSpy.returnValues[0]); + }); + it('throws when the array is zero-length', () => { + const arr: number[] = []; + expect(() => guardedShift(arr)).to.throw(); + }); +}); + +describe('guardedSplice', () => { + it('delegates to Array#splice', () => { + const arr = [1, 2, 3]; + const spliceSpy = spy(arr, 'splice'); + const result = guardedSplice( + arr, + /* start */ 0, + /* deleteCount */ 3, + /* ...items */ + 100, + 101, + 102, + ); + expect(spliceSpy).is.calledWithExactly(0, 3, 100, 101, 102); + expect(result).to.eq(spliceSpy.returnValues[0]); + }); + it('allows zero-length splices', () => { + const arr: number[] = [1, 2, 3]; + expect(guardedSplice(arr, 0, 0)).to.be.an.empty('array'); + }); + it('allows zero-length splices via the `deleteCount` argument being the explicit value `undefined`', () => { + const arr: number[] = [1, 2, 3]; + expect(guardedSplice(arr, 0, undefined)).to.be.an.empty('array'); + }); + it('throws when the `start` would take you past the end of the array', () => { + const arr: number[] = [1, 2, 3]; + expect(() => guardedSplice(arr, 3)).to.throw(); + }); + it('throws when the `deleteCount` and `start` would take you past the end of the array', () => { + const arr: number[] = [1, 2, 3]; + expect(() => guardedSplice(arr, 1, 3)).to.throw(); + }); +});