diff --git a/.github/workflows/reusable-tests.yaml b/.github/workflows/reusable-tests.yaml index 163fb79fb5..d5aedd07ad 100644 --- a/.github/workflows/reusable-tests.yaml +++ b/.github/workflows/reusable-tests.yaml @@ -458,6 +458,8 @@ jobs: path: tests/pyth - cmd: cd tests/realloc && anchor test --skip-lint && npx tsc --noEmit path: tests/realloc + - cmd: cd tests/accountloader-realloc && anchor test --skip-lint && npx tsc --noEmit + path: tests/accountloader-realloc - cmd: cd tests/system-accounts && anchor test --skip-lint path: tests/system-accounts - cmd: cd tests/misc && anchor test --skip-lint && npx tsc --noEmit diff --git a/CHANGELOG.md b/CHANGELOG.md index 85a8ccdc2a..836831e3d5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,6 +31,7 @@ The minor version will be incremented upon a breaking change and the patch versi ### Fixes +- lang: Guard `AccountLoader::exit` against zero-copy buffer truncation and bail with `AccountDidNotDeserialize` instead of rewriting the discriminator over an undersized buffer ([#4633](https://github.com/otter-sec/anchor/pull/4633)). - lang: Set `anchor-lang` Minimum Supported Rust Version to `1.89` ([#4638](https://github.com/otter-sec/anchor/pull/4638)). - lang: Shorten invariant lifetimes during `Context` creation ([#4363](https://github.com/solana-foundation/anchor/pull/4363)). - ts: Guard recursive IDL layouts against stack overflows while preserving supported recursive types ([#4604](https://github.com/solana-foundation/anchor/pull/4604)). diff --git a/lang/src/accounts/account_loader.rs b/lang/src/accounts/account_loader.rs index 24c9ccca49..67819a4e20 100644 --- a/lang/src/accounts/account_loader.rs +++ b/lang/src/accounts/account_loader.rs @@ -268,6 +268,11 @@ impl<'info, T: ZeroCopy + Owner> AccountsExit<'info> for AccountLoader<'info, T> fn exit(&self, program_id: &Pubkey) -> Result<()> { // Only persist if the owner is the current program and the account is not closed. if &T::owner() == program_id && !crate::common::is_closed(self.acc_info) { + // Guard against truncation: refuse to rewrite the discriminator over an undersized buffer. + let required = T::DISCRIMINATOR.len() + mem::size_of::(); + if self.acc_info.try_data_len()? < required { + return Err(ErrorCode::AccountDidNotDeserialize.into()); + } let mut data = self.acc_info.try_borrow_mut_data()?; let dst: &mut [u8] = &mut data; let mut writer = BpfWriter::new(dst); diff --git a/lang/tests/account_loader_truncation.rs b/lang/tests/account_loader_truncation.rs index 6578564685..3aa5a41a58 100644 --- a/lang/tests/account_loader_truncation.rs +++ b/lang/tests/account_loader_truncation.rs @@ -33,7 +33,8 @@ macro_rules! setup_truncated_account { #[test] fn test_load_truncated() { setup_truncated_account!(key, owner, lamports, data, account_info); - let loader: AccountLoader = AccountLoader::try_from(&account_info).unwrap(); + let loader: AccountLoader = + AccountLoader::try_from_unchecked(&crate::ID, &account_info).unwrap(); assert_eq!( loader.load().unwrap_err(), ErrorCode::AccountDidNotDeserialize.into() @@ -43,7 +44,8 @@ fn test_load_truncated() { #[test] fn test_load_mut_truncated() { setup_truncated_account!(key, owner, lamports, data, account_info); - let loader: AccountLoader = AccountLoader::try_from(&account_info).unwrap(); + let loader: AccountLoader = + AccountLoader::try_from_unchecked(&crate::ID, &account_info).unwrap(); assert_eq!( loader.load_mut().unwrap_err(), ErrorCode::AccountDidNotDeserialize.into() diff --git a/tests/accountloader-realloc/Anchor.toml b/tests/accountloader-realloc/Anchor.toml new file mode 100644 index 0000000000..aa21e1b1cd --- /dev/null +++ b/tests/accountloader-realloc/Anchor.toml @@ -0,0 +1,9 @@ +[programs.localnet] +accountloader_realloc = "8GM8KqKaxYb1jEbn5TiqqPqJYsChhjYvfpT2f6KTmUKb" + +[provider] +cluster = "localnet" +wallet = "~/.config/solana/id.json" + +[scripts] +test = "yarn run ts-mocha -p ./tsconfig.json -t 1000000 tests/**/*.ts" diff --git a/tests/accountloader-realloc/Cargo.toml b/tests/accountloader-realloc/Cargo.toml new file mode 100644 index 0000000000..97d6280542 --- /dev/null +++ b/tests/accountloader-realloc/Cargo.toml @@ -0,0 +1,8 @@ +[workspace] +members = [ + "programs/*" +] +resolver = "2" + +[profile.release] +overflow-checks = true diff --git a/tests/accountloader-realloc/package.json b/tests/accountloader-realloc/package.json new file mode 100644 index 0000000000..76c5ecd9a3 --- /dev/null +++ b/tests/accountloader-realloc/package.json @@ -0,0 +1,19 @@ +{ + "name": "accountloader-realloc", + "version": "0.1.0", + "license": "(MIT OR Apache-2.0)", + "homepage": "https://github.com/otter-sec/anchor#readme", + "bugs": { + "url": "https://github.com/otter-sec/anchor/issues" + }, + "repository": { + "type": "git", + "url": "https://github.com/otter-sec/anchor.git" + }, + "engines": { + "node": ">=17" + }, + "scripts": { + "test": "anchor test" + } +} diff --git a/tests/accountloader-realloc/programs/accountloader-realloc/Cargo.toml b/tests/accountloader-realloc/programs/accountloader-realloc/Cargo.toml new file mode 100644 index 0000000000..98b250d9ca --- /dev/null +++ b/tests/accountloader-realloc/programs/accountloader-realloc/Cargo.toml @@ -0,0 +1,22 @@ +[package] +name = "accountloader-realloc" +version = "0.1.0" +edition = "2021" + +[lib] +crate-type = ["cdylib", "lib"] +name = "accountloader_realloc" + +[features] +no-entrypoint = [] +no-log-ix-name = [] +cpi = ["no-entrypoint"] +default = [] +idl-build = ["anchor-lang/idl-build"] + +[profile.release] +overflow-checks = true + +[dependencies] +anchor-lang = { path = "../../../../lang" } +bytemuck = { version = "1.4.0", features = ["derive", "min_const_generics"] } diff --git a/tests/accountloader-realloc/programs/accountloader-realloc/src/lib.rs b/tests/accountloader-realloc/programs/accountloader-realloc/src/lib.rs new file mode 100644 index 0000000000..14f667a059 --- /dev/null +++ b/tests/accountloader-realloc/programs/accountloader-realloc/src/lib.rs @@ -0,0 +1,156 @@ +use anchor_lang::prelude::*; + +declare_id!("8GM8KqKaxYb1jEbn5TiqqPqJYsChhjYvfpT2f6KTmUKb"); + +#[program] +pub mod accountloader_realloc { + use super::*; + + /// Create the zero-copy account with the full required footprint + /// (`DISCRIMINATOR.len() + size_of::()`). + pub fn initialize(ctx: Context) -> Result<()> { + let mut data = ctx.accounts.data.load_init()?; + data.value = 42; + Ok(()) + } + + /// Shrink the `AccountLoader` to `new_len` bytes + pub fn shrink(_ctx: Context, _new_len: u16) -> Result<()> { + Ok(()) + } + + /// Verify the account stays readable after the shrink. + pub fn read(ctx: Context) -> Result { + let data = ctx.accounts.data.load()?; + Ok(data.value) + } + + /// Creates an account as an older program version left it: v1 footprint + /// with the current discriminator. (In a real upgrade the struct name — + /// and therefore the discriminator — doesn't change; the V1/V2 split + /// exists only so both layouts can coexist in this test.) + pub fn initialize_legacy(ctx: Context) -> Result<()> { + let mut data = ctx.accounts.counter.try_borrow_mut_data()?; + data[..8].copy_from_slice(CounterV2::DISCRIMINATOR); + let v1 = CounterV1 { value: 42 }; + data[8..].copy_from_slice(bytemuck::bytes_of(&v1)); + Ok(()) + } + + /// Grows the legacy account to the v2 footprint via the `realloc` + /// constraint, then fills the new field from existing v1 data. + pub fn migrate(ctx: Context) -> Result<()> { + let mut counter = ctx.accounts.counter.load_mut()?; + counter.extra = counter.value * 2; + Ok(()) + } + + pub fn read_extra(ctx: Context) -> Result { + Ok(ctx.accounts.counter.load()?.extra) + } +} + +#[derive(Accounts)] +pub struct Initialize<'info> { + #[account(mut)] + pub authority: Signer<'info>, + + #[account( + init, + payer = authority, + seeds = [b"data"], + bump, + space = 8 + core::mem::size_of::(), + )] + pub data: AccountLoader<'info, Data>, + + pub system_program: Program<'info, System>, +} + +#[derive(Accounts)] +#[instruction(new_len: u16)] +pub struct Shrink<'info> { + #[account(mut)] + pub authority: Signer<'info>, + #[account( + mut, + seeds = [b"data"], + bump, + realloc = new_len as usize, + realloc::payer = authority, + realloc::zero = false, + )] + pub data: AccountLoader<'info, Data>, + + pub system_program: Program<'info, System>, +} + +#[derive(Accounts)] +pub struct Read<'info> { + #[account(seeds = [b"data"], bump)] + pub data: AccountLoader<'info, Data>, +} + +#[derive(Accounts)] +pub struct InitializeLegacy<'info> { + #[account(mut)] + pub authority: Signer<'info>, + + /// CHECK: holds the v1 layout; written manually in the handler. + #[account( + init, + payer = authority, + seeds = [b"legacy"], + bump, + space = 8 + core::mem::size_of::(), + owner = crate::ID, + )] + pub counter: UncheckedAccount<'info>, + + pub system_program: Program<'info, System>, +} + +#[derive(Accounts)] +pub struct Migrate<'info> { + #[account(mut)] + pub authority: Signer<'info>, + + #[account( + mut, + seeds = [b"legacy"], + bump, + realloc = 8 + core::mem::size_of::(), + realloc::payer = authority, + realloc::zero = false, + )] + pub counter: AccountLoader<'info, CounterV2>, + + pub system_program: Program<'info, System>, +} + +#[derive(Accounts)] +pub struct ReadCounter<'info> { + #[account(seeds = [b"legacy"], bump)] + pub counter: AccountLoader<'info, CounterV2>, +} + +#[account(zero_copy)] +#[repr(C)] +pub struct Data { + pub value: u64, + pub padding: [u8; 64], +} + +/// The original account layout (8-bytes body + 8-bytes discriminator = 16 bytes). +#[zero_copy] +pub struct CounterV1 { + pub value: u64, +} + +/// The upgraded layout: `extra` was added. (16 bytes body + 8 bytes discriminator = 24 bytes) +#[account(zero_copy)] +#[repr(C)] +pub struct CounterV2 { + pub value: u64, + pub extra: u64, +} diff --git a/tests/accountloader-realloc/tests/accountloader-realloc.ts b/tests/accountloader-realloc/tests/accountloader-realloc.ts new file mode 100644 index 0000000000..dc03dc194f --- /dev/null +++ b/tests/accountloader-realloc/tests/accountloader-realloc.ts @@ -0,0 +1,144 @@ +import * as anchor from "@anchor-lang/core"; +import { AnchorError, Program } from "@anchor-lang/core"; +import { assert } from "chai"; +import { AccountloaderRealloc } from "../target/types/accountloader_realloc"; + +describe("accountloader-realloc", () => { + anchor.setProvider(anchor.AnchorProvider.env()); + + const program = anchor.workspace + .AccountloaderRealloc as Program; + const authority = (program.provider as any).wallet + .payer as anchor.web3.Keypair; + + const DISCRIMINATOR_LEN = 8; + + let data: anchor.web3.PublicKey; + let legacy: anchor.web3.PublicKey; + + before(async () => { + [data] = anchor.web3.PublicKey.findProgramAddressSync( + [Buffer.from("data")], + program.programId + ); + [legacy] = anchor.web3.PublicKey.findProgramAddressSync( + [Buffer.from("legacy")], + program.programId + ); + }); + + it("initializes the zero-copy account at full size", async () => { + await program.methods + .initialize() + .accounts({ authority: authority.publicKey, data }) + .rpc(); + + const account = await program.account.data.fetch(data); + assert.strictEqual(account.value.toString(), "42"); + }); + + it("under-sized AccountLoader realloc aborts the tx with AccountDidNotDeserialize (3003) and does not brick", async () => { + try { + await program.methods + .shrink(DISCRIMINATOR_LEN) + .accounts({ authority: authority.publicKey, data }) + .rpc(); + assert.fail( + "expected shrink of AccountLoader below 8 + size_of::() to abort the tx" + ); + } catch (e) { + assert.isTrue(e instanceof AnchorError, `unexpected error: ${e}`); + const err = e as AnchorError; + // 3003 = AccountDidNotDeserialize + assert.strictEqual( + err.error.errorCode.number, + 3003, + `expected AccountDidNotDeserialize (3003), got ${err.error.errorCode.number} (${err.error.errorCode.code})` + ); + assert.strictEqual(err.error.errorCode.code, "AccountDidNotDeserialize"); + } + + // Account body must be unchanged (Solana rolls back state on tx failure). + const info = await program.provider.connection.getAccountInfo(data); + assert.isNotNull(info, "account should still exist"); + assert.isAbove( + info!.data.length, + DISCRIMINATOR_LEN, + "account body must NOT have been truncated" + ); + + // `load()` continues to work — value preserved. + const value = await program.methods.read().accounts({ data }).view(); + assert.strictEqual(value.toString(), "42", "zero-copy value preserved"); + }); + + it("accepts AccountLoader realloc at exactly DISCRIMINATOR + size_of::()", async () => { + const minSize = 8 + 8 + 64; // disc + value + padding + + // Grow above the minimum first so the next realloc is an actual shrink + await program.methods + .shrink(minSize + 20) + .accounts({ authority: authority.publicKey, data }) + .rpc(); + + let info = await program.provider.connection.getAccountInfo(data); + assert.isNotNull(info); + assert.strictEqual(info!.data.length, minSize + 20); + + // Oversized account stays readable. + let value = await program.methods.read().accounts({ data }).view(); + assert.strictEqual(value.toString(), "42"); + + // Real shrink down to the exact minimum must succeed. + await program.methods + .shrink(minSize) + .accounts({ authority: authority.publicKey, data }) + .rpc(); + + info = await program.provider.connection.getAccountInfo(data); + assert.isNotNull(info); + assert.strictEqual(info!.data.length, minSize); + + // Value still readable after the shrink. + value = await program.methods.read().accounts({ data }).view(); + assert.strictEqual(value.toString(), "42"); + }); + + it("migrates a legacy zero-copy account by growing it with realloc", async () => { + const V1_LEN = 8 + 8; // disc + value + const V2_LEN = 8 + 16; // disc + value + extra + + // Account as an older program version left it: v1 footprint, valid + // discriminator. + await program.methods + .initializeLegacy() + .accounts({ authority: authority.publicKey, counter: legacy }) + .rpc(); + + let info = await program.provider.connection.getAccountInfo(legacy); + assert.isNotNull(info); + assert.strictEqual(info!.data.length, V1_LEN); + assert.strictEqual(info!.data.readBigUInt64LE(8).toString(), "42"); + + // The migrate ix takes the account as AccountLoader and its + // realloc constraint grows it to the v2 footprint. + await program.methods + .migrate() + .accounts({ authority: authority.publicKey, counter: legacy }) + .rpc(); + + info = await program.provider.connection.getAccountInfo(legacy); + assert.strictEqual(info!.data.length, V2_LEN, "account grown to v2 size"); + assert.strictEqual( + info!.data.readBigUInt64LE(8).toString(), + "42", + "v1 data preserved" + ); + + const extra = await program.methods + .readExtra() + .accounts({ counter: legacy }) + .view(); + assert.strictEqual(extra.toString(), "84", "v2 field populated by migrate"); + }); +}); diff --git a/tests/accountloader-realloc/tsconfig.json b/tests/accountloader-realloc/tsconfig.json new file mode 100644 index 0000000000..cd5d2e3d06 --- /dev/null +++ b/tests/accountloader-realloc/tsconfig.json @@ -0,0 +1,10 @@ +{ + "compilerOptions": { + "types": ["mocha", "chai"], + "typeRoots": ["./node_modules/@types"], + "lib": ["es2015"], + "module": "commonjs", + "target": "es6", + "esModuleInterop": true + } +} diff --git a/tests/package.json b/tests/package.json index d58abf7523..907f0efe2f 100644 --- a/tests/package.json +++ b/tests/package.json @@ -24,6 +24,7 @@ "lint": "prettier */*.js \"*/**/*{.js,.ts}\" --check" }, "workspaces": [ + "accountloader-realloc", "anchor-cli-account", "anchor-cli-idl", "anchor-cli-legacy-idl",