lang: Fix unexpected account substitution in InterfaceAccount (#4139)

* tests: Add basic security checks tests for `InterfaceAccount`

* ci: Add `interface-account` tests

* lang: Fix `InterfaceAccount::try_from`

* lang: Fix `InterfaceAccount::reload`

* lang: Allow multiple owners in `InterfaceAccount::reload`

* lang: Remove the unnecessary clone to get the account info

* lang: Fix compile error about mutable borrow

* lang: Remove the unused imports

Author: acheroncrypto

.github/workflows/reusable-tests.yaml

@@ -472,6 +472,8 @@ jobs:
             path: tests/bench
           - cmd: cd tests/idl && ./test.sh
             path: tests/idl
+          - cmd: cd tests/interface-account && anchor test
+            path: tests/interface-account
           - cmd: cd tests/lazy-account && anchor test --skip-lint
             path: tests/lazy-account
           - cmd: cd tests/test-instruction-validation && ./test.sh

lang/src/accounts/interface_account.rs

@@ -1,14 +1,14 @@
 //! Account container that checks ownership on deserialization.
 
 use crate::accounts::account::Account;
-use crate::error::{Error, ErrorCode};
+use crate::error::ErrorCode;
 use crate::solana_program::account_info::AccountInfo;
 use crate::solana_program::instruction::AccountMeta;
 use crate::solana_program::pubkey::Pubkey;
 use crate::solana_program::system_program;
 use crate::{
     AccountDeserialize, AccountSerialize, Accounts, AccountsClose, AccountsExit, CheckOwner, Key,
-    Owners, Result, ToAccountInfo, ToAccountInfos, ToAccountMetas,
+    Owners, Result, ToAccountInfos, ToAccountMetas,
 };
 use std::collections::BTreeSet;
 use std::fmt;
@@ -182,32 +182,6 @@ impl<'a, T: AccountSerialize + AccountDeserialize + Clone> InterfaceAccount<'a,
         }
     }
 
-    /// Reloads the account from storage. This is useful, for example, when
-    /// observing side effects after CPI.
-    ///
-    /// No Anchor discriminator is checked during reload. Instead, this method enforces
-    /// owner stability by verifying that `info.owner == self.owner` (i.e., the pubkey
-    /// validated at construction) to avoid TOCTOU issues, and then re-deserializes `T`
-    /// from the updated bytes.
-    ///
-    /// If you need discriminator validation on reload, use `Account<T>` with an Anchor
-    /// #[account] type.
-    pub fn reload(&mut self) -> Result<()> {
-        let info = self.account.to_account_info();
-
-        // Enforce owner stability: must match the one validated at construction.
-        if info.owner != &self.owner {
-            return Err(Error::from(ErrorCode::AccountOwnedByWrongProgram)
-                .with_pubkeys((*info.owner, self.owner)));
-        }
-
-        // Re-deserialize fresh data into the inner account.
-        let mut data: &[u8] = &info.try_borrow_data()?;
-        let new_val = T::try_deserialize_unchecked(&mut data)?;
-        self.account.set_inner(new_val);
-        Ok(())
-    }
-
     pub fn into_inner(self) -> T {
         self.account.into_inner()
     }
@@ -234,28 +208,19 @@ impl<'a, T: AccountSerialize + AccountDeserialize + Clone> InterfaceAccount<'a,
 }
 
 impl<'a, T: AccountSerialize + AccountDeserialize + CheckOwner + Clone> InterfaceAccount<'a, T> {
-    /// Deserializes the given `info` into a `InterfaceAccount`.
-    ///
-    /// This **does not** check an Anchor discriminator. It first validates
-    /// program ownership via `T::check_owner`, then deserializes using
-    /// `AccountDeserialize::try_deserialize_unchecked`.
+    /// Deserializes the given `info` into an `InterfaceAccount`.
     #[inline(never)]
     pub fn try_from(info: &'a AccountInfo<'a>) -> Result<Self> {
-        // `InterfaceAccount` targets foreign program accounts (e.g., SPL Token
-        // accounts) that do not have Anchor discriminators. Because of that, we
-        // intentionally skip the Anchor discriminator check here and instead:
-        //
-        // 1) Validate program ownership via `T::check_owner(info.owner)?`
-        // 2) Deserialize without a discriminator by delegating to
-        //    `T::try_deserialize_unchecked`
-        Self::try_from_unchecked(info)
+        if info.owner == &system_program::ID && info.lamports() == 0 {
+            return Err(ErrorCode::AccountNotInitialized.into());
+        }
+        T::check_owner(info.owner)?;
+        let mut data: &[u8] = &info.try_borrow_data()?;
+        Ok(Self::new(info, T::try_deserialize(&mut data)?))
     }
 
-    /// Deserializes the given `info` into a `InterfaceAccount` **without** checking
-    /// the account discriminator. This is intended for foreign program accounts.
-    /// Prefer `Self::try_from` when you also want the ownership check, but note
-    /// that both skip Anchor discriminator checks, and `try_from` additionally
-    /// enforces ownership.
+    /// Deserializes the given `info` into an `InterfaceAccount` without checking the account
+    /// discriminator. Be careful when using this and avoid it if possible.
     #[inline(never)]
     pub fn try_from_unchecked(info: &'a AccountInfo<'a>) -> Result<Self> {
         if info.owner == &system_program::ID && info.lamports() == 0 {
@@ -265,6 +230,22 @@ impl<'a, T: AccountSerialize + AccountDeserialize + CheckOwner + Clone> Interfac
         let mut data: &[u8] = &info.try_borrow_data()?;
         Ok(Self::new(info, T::try_deserialize_unchecked(&mut data)?))
     }
+
+    /// Reloads the account from storage. This is useful, for example, when observing side effects
+    /// after CPI.
+    ///
+    /// This method also validates that the account is owned by one of the expected programs.
+    pub fn reload(&mut self) -> Result<()> {
+        let info: &AccountInfo = self.account.as_ref();
+        T::check_owner(info.owner)?;
+
+        // Re-deserialize fresh data into the inner account.
+        self.account.set_inner({
+            let mut data: &[u8] = &info.try_borrow_data()?;
+            T::try_deserialize(&mut data)?
+        });
+        Ok(())
+    }
 }
 
 impl<'info, B, T: AccountSerialize + AccountDeserialize + CheckOwner + Clone> Accounts<'info, B>

tests/interface-account/Anchor.toml

@@ -0,0 +1,11 @@
+[programs.localnet]
+interface_account = "interfaceAccount111111111111111111111111111"
+new = "New1111111111111111111111111111111111111111"
+old = "oLd1111111111111111111111111111111111111111"
+
+[provider]
+cluster = "localnet"
+wallet = "~/.config/solana/id.json"
+
+[scripts]
+test = "yarn run ts-mocha -p ./tsconfig.json -t 1000000 tests/**/*.ts"

tests/interface-account/Cargo.toml

@@ -0,0 +1,14 @@
+[workspace]
+members = [
+    "programs/*"
+]
+resolver = "2"
+
+[profile.release]
+overflow-checks = true
+lto = "fat"
+codegen-units = 1
+[profile.release.build-override]
+opt-level = 3
+incremental = false
+codegen-units = 1

tests/interface-account/package.json

@@ -0,0 +1,16 @@
+{
+  "name": "interface-account",
+  "version": "0.32.1",
+  "license": "(MIT OR Apache-2.0)",
+  "homepage": "https://github.com/coral-xyz/anchor#readme",
+  "bugs": {
+    "url": "https://github.com/coral-xyz/anchor/issues"
+  },
+  "repository": {
+    "type": "git",
+    "url": "https://github.com/coral-xyz/anchor.git"
+  },
+  "engines": {
+    "node": ">=17"
+  }
+}

tests/interface-account/programs/interface-account/Cargo.toml

@@ -0,0 +1,21 @@
+[package]
+name = "interface-account"
+version = "0.1.0"
+description = "Created with Anchor"
+edition = "2021"
+
+[lib]
+crate-type = ["cdylib", "lib"]
+name = "interface_account"
+
+[features]
+default = []
+cpi = ["no-entrypoint"]
+no-entrypoint = []
+no-idl = []
+idl-build = ["anchor-lang/idl-build"]
+
+[dependencies]
+anchor-lang = { path = "../../../../lang" }
+new = { path = "../new", features = ["cpi"] }
+old = { path = "../old", features = ["cpi"] }

tests/interface-account/programs/interface-account/src/lib.rs

@@ -0,0 +1,52 @@
+#![allow(warnings)]
+
+use anchor_lang::prelude::*;
+
+declare_id!("interfaceAccount111111111111111111111111111");
+
+#[program]
+pub mod interface_account {
+    use super::*;
+
+    pub fn test(ctx: Context<Test>) -> Result<()> {
+        Ok(())
+    }
+}
+
+#[derive(Accounts)]
+pub struct Test<'info> {
+    pub expected_account: InterfaceAccount<'info, interface::ExpectedAccount>,
+}
+
+mod interface {
+    #[derive(Clone)]
+    pub struct ExpectedAccount(new::ExpectedAccount);
+
+    impl anchor_lang::AccountDeserialize for ExpectedAccount {
+        fn try_deserialize(buf: &mut &[u8]) -> anchor_lang::Result<Self> {
+            new::ExpectedAccount::try_deserialize(buf).map(Self)
+        }
+
+        fn try_deserialize_unchecked(buf: &mut &[u8]) -> anchor_lang::Result<Self> {
+            new::ExpectedAccount::try_deserialize_unchecked(buf).map(Self)
+        }
+    }
+
+    impl anchor_lang::AccountSerialize for ExpectedAccount {}
+
+    impl anchor_lang::Owners for ExpectedAccount {
+        fn owners() -> &'static [anchor_lang::prelude::Pubkey] {
+            &[old::ID_CONST, new::ID_CONST]
+        }
+    }
+
+    #[cfg(feature = "idl-build")]
+    mod idl_impls {
+        use super::ExpectedAccount;
+
+        impl anchor_lang::IdlBuild for ExpectedAccount {}
+        impl anchor_lang::Discriminator for ExpectedAccount {
+            const DISCRIMINATOR: &'static [u8] = &[];
+        }
+    }
+}

tests/interface-account/programs/new/Cargo.toml

@@ -0,0 +1,18 @@
+[package]
+name = "new"
+version = "0.1.0"
+description = "Created with Anchor"
+edition = "2021"
+
+[lib]
+crate-type = ["cdylib", "lib"]
+
+[features]
+default = []
+cpi = ["no-entrypoint"]
+no-entrypoint = []
+no-idl = []
+idl-build = ["anchor-lang/idl-build"]
+
+[dependencies]
+anchor-lang = { path = "../../../../lang" }

tests/interface-account/programs/new/src/lib.rs

@@ -0,0 +1,47 @@
+#![allow(warnings)]
+
+use anchor_lang::prelude::*;
+
+declare_id!("New1111111111111111111111111111111111111111");
+
+#[program]
+pub mod new {
+    use super::*;
+
+    pub fn init(ctx: Context<Init>) -> Result<()> {
+        Ok(())
+    }
+
+    pub fn init_another(ctx: Context<InitAnother>) -> Result<()> {
+        Ok(())
+    }
+}
+
+#[derive(Accounts)]
+pub struct Init<'info> {
+    #[account(mut)]
+    pub authority: Signer<'info>,
+    #[account(init, payer = authority, space = 40)]
+    pub expected_account: Account<'info, ExpectedAccount>,
+    pub system_program: Program<'info, System>,
+}
+
+#[derive(Accounts)]
+pub struct InitAnother<'info> {
+    #[account(mut)]
+    pub authority: Signer<'info>,
+    #[account(init, payer = authority, space = 72)]
+    pub another_account: Account<'info, AnotherAccount>,
+    pub system_program: Program<'info, System>,
+}
+
+#[account]
+pub struct ExpectedAccount {
+    pub data: Pubkey,
+}
+
+#[account]
+pub struct AnotherAccount {
+    pub a: Pubkey,
+    pub b: Pubkey,
+}

tests/interface-account/programs/old/Cargo.toml

@@ -0,0 +1,18 @@
+[package]
+name = "old"
+version = "0.1.0"
+description = "Created with Anchor"
+edition = "2021"
+
+[lib]
+crate-type = ["cdylib", "lib"]
+
+[features]
+default = []
+cpi = ["no-entrypoint"]
+no-entrypoint = []
+no-idl = []
+idl-build = ["anchor-lang/idl-build"]
+
+[dependencies]
+anchor-lang = { path = "../../../../lang" }