From 3a33c4c42534fa6e2a8c08051f7b74785ca581c2 Mon Sep 17 00:00:00 2001 From: Vara Prasad Bandaru Date: Fri, 8 Mar 2024 18:53:55 +0530 Subject: [PATCH] Update arbitrary_cpi lint to check for Instruction{..} and CpiContext::new, CpiContext::new_with_signer calls --- crate/src/paths.rs | 9 +- lints/arbitrary_cpi/README.md | 38 +- lints/arbitrary_cpi/src/lib.rs | 576 ++++++++---------- lints/arbitrary_cpi/ui/insecure-2/Cargo.toml | 1 + lints/arbitrary_cpi/ui/insecure-2/src/lib.rs | 50 +- .../ui/insecure-2/src/lib.stderr | 34 +- 6 files changed, 365 insertions(+), 343 deletions(-) diff --git a/crate/src/paths.rs b/crate/src/paths.rs index aca07e88..b4d235a5 100644 --- a/crate/src/paths.rs +++ b/crate/src/paths.rs @@ -6,6 +6,7 @@ pub const ANCHOR_LANG_ACCOUNT: [&str; 4] = ["anchor_lang", "accounts", "account" pub const ANCHOR_LANG_ACCOUNT_LOADER: [&str; 4] = ["anchor_lang", "accounts", "account_loader", "AccountLoader"]; pub const ANCHOR_LANG_PROGRAM: [&str; 4] = ["anchor_lang", "accounts", "program", "Program"]; +pub const ANCHOR_LANG_INTERFACE: [&str; 4] = ["anchor_lang", "accounts", "interface", "Interface"]; pub const ANCHOR_LANG_SYSTEM_ACCOUNT: [&str; 4] = ["anchor_lang", "accounts", "system_account", "SystemAccount"]; pub const ANCHOR_LANG_ACCOUNT_DESERIALIZE: [&str; 2] = ["anchor_lang", "AccountDeserialize"]; @@ -20,7 +21,11 @@ pub const ANCHOR_LANG_TRY_DESERIALIZE: [&str; 3] = // key() method call path pub const ANCHOR_LANG_KEY: [&str; 3] = ["anchor_lang", "Key", "key"]; pub const ANCHOR_LANG_TO_ACCOUNT_INFOS_TRAIT: [&str; 2] = ["anchor_lang", "ToAccountInfos"]; - +// CpiContext::new() +pub const ANCHOR_CPI_CONTEXT_NEW: [&str; 4] = ["anchor_lang", "context", "CpiContext", "new"]; +// CpiContext::new_with_signer() +pub const ANCHOR_CPI_CONTEXT_NEW_SIGNER: [&str; 4] = + ["anchor_lang", "context", "CpiContext", "new_with_signer"]; pub const BORSH_TRY_FROM_SLICE: [&str; 4] = ["borsh", "de", "BorshDeserialize", "try_from_slice"]; pub const CORE_BRANCH: [&str; 5] = ["core", "ops", "try_trait", "Try", "branch"]; @@ -29,6 +34,8 @@ pub const CORE_CLONE: [&str; 4] = ["core", "clone", "Clone", "clone"]; pub const SOLANA_PROGRAM_ACCOUNT_INFO: [&str; 3] = ["solana_program", "account_info", "AccountInfo"]; pub const SOLANA_PROGRAM_INVOKE: [&str; 3] = ["solana_program", "program", "invoke"]; +// Instruction {..} +pub const SOLANA_PROGRAM_INSTRUCTION: [&str; 3] = ["solana_program", "instruction", "Instruction"]; pub const SOLANA_PROGRAM_CREATE_PROGRAM_ADDRESS: [&str; 4] = [ "solana_program", "pubkey", diff --git a/lints/arbitrary_cpi/README.md b/lints/arbitrary_cpi/README.md index eefb1b54..e923e43e 100644 --- a/lints/arbitrary_cpi/README.md +++ b/lints/arbitrary_cpi/README.md @@ -42,22 +42,22 @@ invoke(&ix, accounts.clone()); **How the lint is implemented:** -- For every function containing calls to `solana_program::program::invoke` -- find the definition of `Instruction` argument passed to `invoke`; first argument -- If the `Instruction` argument is result of a function call - - If the function is whitelisted, do not report; only functions defined in - `spl_token::instruction` are whitelisted. - - Else report the call to `invoke` as vulnerable -- Else if the `Instruction` is initialized in the function itself - - find the assign statement assigning to the `program_id` field, assigning to - field at `0`th index - - find all the aliases of `program_id`. Use the rhs of the assignment as initial - alias and look for all assignments assigning to the locals recursively. - - If `program_id` is compared using any of aliases ignore the call to `invoke`. - - Look for calls to `core::cmp::PartialEq{ne, eq}` where one of arg is moved - from an alias. - - If one of the arg accesses `program_id` and if the basic block containing the - comparison dominates the basic block containing call to `invoke` ensuring the - `program_id` is checked in all execution paths Then ignore the call to `invoke`. - - Else report the call to `invoke`. - - Else report the call to `invoke`. +- For every function + - For every statement in the function initializing `Instruction {..}` + - Get the place being assigned to `program_id` field + - find all the aliases of `program_id`. Use the rhs of the assignment as initial + alias and look for all assignments assigning to the locals recursively. + - If `program_id` is compared using any of aliases ignore the call to `invoke`. + - Look for calls to `core::cmp::PartialEq{ne, eq}` where one of arg is moved + from an alias. + - If one of the arg accesses `program_id` and if the basic block containing the + comparison dominates the basic block containing call to `invoke` ensuring the + `program_id` is checked in all execution paths Then ignore the call to `invoke`. + - Else report the statement initializing `Instruction`. + - Else report the statement initializing `Instruction`. + - For every call to `CpiContext::new` or `CpiContext::new_with_signer` + - Get the place of the first argument (program's account info) + - find all aliases of `program's` place. + - If the `program` is a result of calling `to_account_info` on Anchor `Program`/`Interface` + - continue + - Else report the call to `CpiContext::new`/`CpiContext::new_with_signer` diff --git a/lints/arbitrary_cpi/src/lib.rs b/lints/arbitrary_cpi/src/lib.rs index a3182263..08639e23 100644 --- a/lints/arbitrary_cpi/src/lib.rs +++ b/lints/arbitrary_cpi/src/lib.rs @@ -2,23 +2,22 @@ #![feature(box_patterns)] #![warn(unused_extern_crates)] -use clippy_utils::{diagnostics::span_lint, match_def_path}; +use clippy_utils::{diagnostics::span_lint, match_any_def_paths, match_def_path}; use if_chain::if_chain; use rustc_hir::Body; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::{ mir, mir::{ - BasicBlock, Local, Operand, Place, ProjectionElem, Rvalue, StatementKind, TerminatorKind, + AggregateKind, BasicBlock, Local, Operand, Place, Rvalue, Statement, StatementKind, + TerminatorKind, }, - ty::TyKind, + ty::{self, TyKind}, }; -use rustc_span::symbol::Symbol; use solana_lints::paths; extern crate rustc_hir; extern crate rustc_middle; -extern crate rustc_span; dylint_linting::declare_late_lint! { /// **What it does:** @@ -63,25 +62,25 @@ dylint_linting::declare_late_lint! { /// /// **How the lint is implemented:** /// - /// - For every function containing calls to `solana_program::program::invoke` - /// - find the definition of `Instruction` argument passed to `invoke`; first argument - /// - If the `Instruction` argument is result of a function call - /// - If the function is whitelisted, do not report; only functions defined in - /// `spl_token::instruction` are whitelisted. - /// - Else report the call to `invoke` as vulnerable - /// - Else if the `Instruction` is initialized in the function itself - /// - find the assign statement assigning to the `program_id` field, assigning to - /// field at `0`th index - /// - find all the aliases of `program_id`. Use the rhs of the assignment as initial - /// alias and look for all assignments assigning to the locals recursively. - /// - If `program_id` is compared using any of aliases ignore the call to `invoke`. - /// - Look for calls to `core::cmp::PartialEq{ne, eq}` where one of arg is moved - /// from an alias. - /// - If one of the arg accesses `program_id` and if the basic block containing the - /// comparison dominates the basic block containing call to `invoke` ensuring the - /// `program_id` is checked in all execution paths Then ignore the call to `invoke`. - /// - Else report the call to `invoke`. - /// - Else report the call to `invoke`. + /// - For every function + /// - For every statement in the function initializing `Instruction {..}` + /// - Get the place being assigned to `program_id` field + /// - find all the aliases of `program_id`. Use the rhs of the assignment as initial + /// alias and look for all assignments assigning to the locals recursively. + /// - If `program_id` is compared using any of aliases ignore the call to `invoke`. + /// - Look for calls to `core::cmp::PartialEq{ne, eq}` where one of arg is moved + /// from an alias. + /// - If one of the arg accesses `program_id` and if the basic block containing the + /// comparison dominates the basic block containing call to `invoke` ensuring the + /// `program_id` is checked in all execution paths Then ignore the call to `invoke`. + /// - Else report the statement initializing `Instruction`. + /// - Else report the statement initializing `Instruction`. + /// - For every call to `CpiContext::new` or `CpiContext::new_with_signer` + /// - Get the place of the first argument (program's account info) + /// - find all aliases of `program's` place. + /// - If the `program` is a result of calling `to_account_info` on Anchor `Program`/`Interface` + /// - continue + /// - Else report the call to `CpiContext::new`/`CpiContext::new_with_signer` pub ARBITRARY_CPI, Warn, "Finds unconstrained inter-contract calls" @@ -89,6 +88,9 @@ dylint_linting::declare_late_lint! { impl<'tcx> LateLintPass<'tcx> for ArbitraryCpi { fn check_body(&mut self, cx: &LateContext<'tcx>, body: &'tcx Body<'tcx>) { + if body.value.span.from_expansion() { + return; + } let hir_map = cx.tcx.hir(); let body_did = hir_map.body_owner_def_id(body.id()).to_def_id(); // The body is the body of function whose mir is available @@ -98,331 +100,285 @@ impl<'tcx> LateLintPass<'tcx> for ArbitraryCpi { } let body_mir = cx.tcx.optimized_mir(body_did); // list of block id and the terminator of the basic blocks in the CFG - let terminators = body_mir - .basic_blocks - .iter_enumerated() - .map(|(block_id, block)| (block_id, &block.terminator)); - for (block_id, terminator) in terminators { - if_chain! { - if let t = terminator.as_ref().unwrap(); - // The terminator is a call to a function; the function is defined and is not function pointer or function object - // i.e The function is not copied or moved. Generic functions, trait methods are not Constant. - if let TerminatorKind::Call { - func: func_operand, - args, - .. - } = &t.kind; - if let mir::Operand::Constant(box func) = func_operand; - if let TyKind::FnDef(def_id, _callee_substs) = func.const_.ty().kind(); - then { - // Static call - let callee_did = *def_id; - // Calls `invoke` - if match_def_path(cx, callee_did, &paths::SOLANA_PROGRAM_INVOKE) { - // Get the `Instruction`, instruction is the first argument of `invoke` function. - let inst_arg = &args[0]; - if let Operand::Move(p) = inst_arg { - // Check if the Instruction is returned from a whitelisted function (is_whitelist = true) - // if `Instruction` is defined in this function, find all the locals/places the program_id is defined - let (is_whitelist, programid_places) = - Self::find_program_id_for_instru(cx, body_mir, block_id, p); - let likely_programid_locals: Vec = - programid_places.iter().map(|pl| pl.local).collect(); - // if not whitelisted, check if the program_id is compared using one of the locals. - if !is_whitelist - && !Self::is_programid_checked( - cx, - body_mir, - block_id, - likely_programid_locals.as_ref(), - ) - { - span_lint( - cx, - ARBITRARY_CPI, - t.source_info.span, - "program_id may not be checked", - ); - } - } + for (block_id, block_data) in body_mir.basic_blocks.iter_enumerated() { + // find the Instruction {...} initialization statements and check if program id is validated + // Note: Because the lint looks for `Instruction {..}` construction instead of going through `invoke`, `invoke_signed`, + // the lint will not report when the `Instruction` of `invoke` calls is returned by a function defined in a dependency. + // However, if the `Instruction` is returned by a function defined in this crate then that function will get checked by the + // lint and the statement initializing the `Instruction` will be reported. + for stmt in &block_data.statements { + if_chain! { + if let Some(program_id_place) = is_instruction_init_stmt(cx, stmt); + if !is_program_id_verified(cx, body_mir, block_id, &program_id_place); + then { + span_lint( + cx, + ARBITRARY_CPI, + stmt.source_info.span, + "program_id may not be checked", + ) } } } - } - } -} - -impl ArbitraryCpi { - // This function is passed the Place corresponding to the 1st argument to invoke, and is - // responsible for tracing the definition of that local back to the point where the instruction - // is defined. - // - // We handle two cases: - // 1. The instruction is initialized within this Body, in which case the returned - // likely_program_id_places will contain all the Places containing a program_id, - // and we can then look for comparisons with those places to see if the program id is - // checked. - // - // 2. The instruction passed to invoke is returned from a function call. In the general case, - // we want to raise a warning since the program ID still might not be checked (the function - // that is called may or may not check it). However, if this came from a call to - // spl_token::instruction::... then it will be checked and we will ignore it - // - // Returns (is_whitelisted, likely_program_id_places) - fn find_program_id_for_instru<'tcx>( - cx: &LateContext, - body: &'tcx mir::Body<'tcx>, - block: BasicBlock, - mut inst_arg: &Place<'tcx>, - ) -> (bool, Vec>) { - let preds = body.basic_blocks.predecessors(); - let mut cur_block = block; - loop { - // Walk the bb in reverse, starting with the terminator - if let Some(t) = &body.basic_blocks[cur_block].terminator { - // the terminator is a call; the return value of the call is assigned to `inst_arg.local` - match &t.kind { - TerminatorKind::Call { - func: mir::Operand::Constant(box func), - destination: dest, + // if the terminator is a call to CpiContext::new or CpiContext::new_with_signer: + // - if program id is not verified + // - report error + if let Some(t) = &block_data.terminator { + if_chain! { + if let TerminatorKind::Call { + func: func_operand, args, .. - } if dest.local_or_deref_local() == Some(inst_arg.local) => { - if_chain! { - // function definition - if let TyKind::FnDef(def_id, _callee_substs) = func.const_.ty().kind(); - // non-zero args are passed in the call - if !args.is_empty(); - if let Operand::Copy(arg0_pl) | Operand::Move(arg0_pl) = &args[0]; - then { - // in order to trace back to the call which creates the - // instruction, we have to trace through a call to Try::branch - // Expressions such as `call()?` with try operator will have Try::branch - // call and the first argument is the return value of actual call `call()`. - // If the call is Try::branch, look for the first arg which will have the return - // value of `call()`. - if match_def_path(cx, *def_id, &paths::CORE_BRANCH) { - inst_arg = arg0_pl; - } else { - // If this is not Try::branch, check if its a call to a function in `spl_token::instruction` module - let path = cx.get_def_path(*def_id); - let token_path = - paths::SPL_TOKEN_INSTRUCTION.map(Symbol::intern); - // if the instruction is constructed by a function in `spl_token::instruction`, assume program_id is checked - if path.iter().take(2).eq(&token_path) { - return (true, Vec::new()); - } - // if the called function is not the whitelisted one, then we assume it to be vulnerable - return (false, Vec::new()); - } - } - } + } = &t.kind; + if let mir::Operand::Constant(box func) = func_operand; + if let TyKind::FnDef(def_id, _callee_substs) = func.const_.ty().kind(); + if match_any_def_paths( + cx, + *def_id, + &[ + &paths::ANCHOR_CPI_CONTEXT_NEW, + &paths::ANCHOR_CPI_CONTEXT_NEW_SIGNER, + ], + ) + .is_some(); + if let Operand::Move(program_place) = &args[0]; + if !is_program_safe_account_info(cx, body_mir, block_id, program_place); + then { + span_lint( + cx, + ARBITRARY_CPI, + t.source_info.span, + "program_id may not be checked", + ) } - _ => {} } } - // check every statement - for stmt in body.basic_blocks[cur_block].statements.iter().rev() { - match &stmt.kind { - // if the statement assigns to `inst_arg`, update `inst_arg` to the rhs - StatementKind::Assign(box (assign_place, rvalue)) - if assign_place.local == inst_arg.local => - { - // Check if assign_place is assignment to a field. if not then this is not the initialization of the struct - // have to check further - if_chain! { - if assign_place.projection.len() == 1; - if let proj = assign_place.projection[0]; - // the projection could be deref etc - if let ProjectionElem::Field(f, ty) = proj; - then { - // stmt is an assignment to a field. - // there will be 3 statements(for 3 fields), ensure this statement is assignment - // to the first field `program_id` - // Also, do not update inst_arg, as this is just field assignment. - if_chain! { - // program_id is the first field; index = 0 - if f.index() == 0; - if let Some(adtdef) = ty.ty_adt_def(); - if match_def_path( - cx, - adtdef.did(), - &["solana_program", "pubkey", "Pubkey"], - ); - then { - if let Rvalue::Use(Operand::Copy(pl) | Operand::Move(pl)) - | Rvalue::Ref(_, _, pl) = rvalue - { - // found the program_id. now look for all assignments/aliases to program_id. - let likely_program_id_aliases = Self::find_program_id_aliases(body, cur_block, pl); - return (false, likely_program_id_aliases); - } - } - } - } else { - // inst_arg is defined using this statement. rvalue could store the actual value. - if let Rvalue::Use(Operand::Copy(pl) | Operand::Move(pl)) - | Rvalue::Ref(_, _, pl) = rvalue - { - inst_arg = pl; - } - } - } + } + } +} + +/// Return the place of program id if the statement initializes Instruction i.e stmt is _x = Instruction {...} +fn is_instruction_init_stmt<'tcx>(cx: &LateContext, stmt: &Statement<'tcx>) -> Option> { + if_chain! { + if let StatementKind::Assign(box (_, rvalue)) = &stmt.kind; + // The MIR generated for the `insecure-2` and other programs shows that the entire struct is initialized at once. + // Note: Its unknown in what cases the struct initialization is deaggregated. Assuming here that + // the struct is initialized at once till a counter example is found. + if let Rvalue::Aggregate(box AggregateKind::Adt(def_id, variant_idx, _, _, _), fields) = + rvalue; + // The Adt is a struct + if variant_idx.index() == 0; + // The struct is `solana_program::instruction::Instruction` + if match_def_path(cx, *def_id, &paths::SOLANA_PROGRAM_INSTRUCTION); + // program id is the first field. Assuming its operand is at the start of the fields IndexVec. + if let Some(Operand::Move(pl) | Operand::Copy(pl)) = fields.iter().next(); + then { + Some(*pl) + } else { + None + } + } +} + +/// Given the place corresponding to `program_id` of CPI call, return true if `program_id` is validated else false +/// +/// The `program_id` is the place of operand used to initialize `Instruction`: +/// - `let _x = Instruction { program_id: program_id_place, accounts: _, data: _ }` +fn is_program_id_verified<'tcx>( + cx: &LateContext, + body: &'tcx mir::Body<'tcx>, + block_id: BasicBlock, + program_id_place: &Place<'tcx>, +) -> bool { + let program_id_aliases = find_place_aliases(body, block_id, program_id_place); + let likely_program_id_locals: Vec = + program_id_aliases.iter().map(|pl| pl.local).collect(); + is_programid_checked(cx, body, block_id, likely_program_id_locals.as_ref()) +} + +/// Given the place corresponding to `program` account info, return true if the `AccountInfo` is of a `Program`. +fn is_program_safe_account_info<'tcx>( + cx: &LateContext<'tcx>, + body: &'tcx mir::Body<'tcx>, + block_id: BasicBlock, + program_place: &Place<'tcx>, +) -> bool { + let program_aliases = find_place_aliases(body, block_id, program_place); + // This function at the moment only checks if the program is a result of calling `to_account_info`. + // The aliases returned by `find_place_aliases` are of form where there is an assignment statement `alias[i] = alias[i+1]`. + // As we are only looking for `to_account_info` calls, it is sufficient to check for assignment to the last alias. + let program = program_aliases.last().unwrap(); + + for (_, block_data) in body.basic_blocks.iter_enumerated() { + match &block_data.terminator.as_ref().unwrap().kind { + TerminatorKind::Call { + func: mir::Operand::Constant(box func), + destination: dest, + args, + .. + } if dest.local_or_deref_local() == program.local_or_deref_local() => { + if_chain! { + // the func is a call to `.to_account_info()` on type `Program` or `Interface` + if let TyKind::FnDef(def_id, _) = func.const_.ty().kind(); + if match_def_path(cx, *def_id, &paths::ANCHOR_LANG_TO_ACCOUNT_INFO); + if !args.is_empty(); + if let Operand::Copy(arg0_pl) | Operand::Move(arg0_pl) = &args[0]; + if let ty::Adt(adt_def, _) = arg0_pl.ty(body, cx.tcx).ty.peel_refs().kind(); + if match_any_def_paths( + cx, + adt_def.did(), + &[&paths::ANCHOR_LANG_PROGRAM, &paths::ANCHOR_LANG_INTERFACE], + ) + .is_some(); + then { + // The program is a result of calling `to_account_info` on `Program` or `Interface` + return true; } - _ => {} - } - } - match preds.get(cur_block) { - // traverse the CFG. Only predecessor is being considered. - Some(cur_preds) if !cur_preds.is_empty() => cur_block = cur_preds[0], - _ => { - break; } } + _ => {} } - // we did not find the statement assigning to the program_id of `Instruction`. report as vulnerable - (false, Vec::new()) } + false +} - fn find_program_id_aliases<'tcx>( - body: &'tcx mir::Body<'tcx>, - block: BasicBlock, - mut id_arg: &Place<'tcx>, - ) -> Vec> { - let preds = body.basic_blocks.predecessors(); - let mut cur_block = block; - let mut likely_program_id_aliases = Vec::::new(); - likely_program_id_aliases.push(*id_arg); - loop { - // check every stmt - for stmt in body.basic_blocks[cur_block].statements.iter().rev() { - match &stmt.kind { - // if the statement assigns to `inst_arg`, update `inst_arg` to the rhs - StatementKind::Assign(box (assign_place, rvalue)) - if assign_place.local_or_deref_local() == id_arg.local_or_deref_local() => +/// Given a place, find other places which are an alias to this place +fn find_place_aliases<'tcx>( + body: &'tcx mir::Body<'tcx>, + block: BasicBlock, + mut id_arg: &Place<'tcx>, +) -> Vec> { + let preds = body.basic_blocks.predecessors(); + let mut cur_block = block; + let mut likely_program_id_aliases = Vec::::new(); + likely_program_id_aliases.push(*id_arg); + loop { + // check every stmt + for stmt in body.basic_blocks[cur_block].statements.iter().rev() { + match &stmt.kind { + // if the statement assigns to `inst_arg`, update `inst_arg` to the rhs + StatementKind::Assign(box (assign_place, rvalue)) + if assign_place.local_or_deref_local() == id_arg.local_or_deref_local() => + { + if let Rvalue::Use(Operand::Copy(pl) | Operand::Move(pl)) + | Rvalue::Ref(_, _, pl) = rvalue { - if let Rvalue::Use(Operand::Copy(pl) | Operand::Move(pl)) - | Rvalue::Ref(_, _, pl) = rvalue - { - id_arg = pl; - likely_program_id_aliases.push(*pl); - } + id_arg = pl; + likely_program_id_aliases.push(*pl); } - _ => {} } + _ => {} } - match preds.get(cur_block) { - Some(cur_preds) if !cur_preds.is_empty() => cur_block = cur_preds[0], - _ => { - break; - } + } + match preds.get(cur_block) { + Some(cur_preds) if !cur_preds.is_empty() => cur_block = cur_preds[0], + _ => { + break; } } - likely_program_id_aliases } + likely_program_id_aliases +} - // This function takes the list of programid_locals and a starting block, and searches for a - // check elsewhere in the Body that would compare the program_id with something else. - fn is_programid_checked<'tcx>( - cx: &LateContext, - body: &'tcx mir::Body<'tcx>, - block: BasicBlock, - programid_locals: &[Local], - ) -> bool { - let preds = body.basic_blocks.predecessors(); - let mut cur_block = block; - loop { - // check every statement - if_chain! { - // is terminator a call `core::cmp::PartialEq{ne, eq}`? - if let Some(t) = &body.basic_blocks[cur_block].terminator; - if let TerminatorKind::Call { - func: func_operand, - args, - .. - } = &t.kind; - if let mir::Operand::Constant(box func) = func_operand; - if let TyKind::FnDef(def_id, _callee_substs) = func.const_.ty().kind(); - if match_def_path(cx, *def_id, &["core", "cmp", "PartialEq", "ne"]) - || match_def_path(cx, *def_id, &["core", "cmp", "PartialEq", "eq"]); - // check if any of the args accesses program_id - if let Operand::Copy(arg0_pl) | Operand::Move(arg0_pl) = args[0]; - if let Operand::Copy(arg1_pl) | Operand::Move(arg1_pl) = args[1]; - then { - // if either arg0 or arg1 came from one of the programid_locals, then we know - // this eq/ne check was operating on the program_id. - if Self::is_moved_from(cx, body, cur_block, &arg0_pl, programid_locals) - || Self::is_moved_from(cx, body, cur_block, &arg1_pl, programid_locals) - { - // we found the check. if it dominates the call to invoke, then the check - // is assumed to be sufficient! - return body.basic_blocks.dominators().dominates(cur_block, block); - } +// This function takes the list of programid_locals and a starting block, and searches for a +// check elsewhere in the Body that would compare the program_id with something else. +fn is_programid_checked<'tcx>( + cx: &LateContext, + body: &'tcx mir::Body<'tcx>, + block: BasicBlock, + programid_locals: &[Local], +) -> bool { + let preds = body.basic_blocks.predecessors(); + let mut cur_block = block; + loop { + // check every statement + if_chain! { + // is terminator a call `core::cmp::PartialEq{ne, eq}`? + if let Some(t) = &body.basic_blocks[cur_block].terminator; + if let TerminatorKind::Call { + func: func_operand, + args, + .. + } = &t.kind; + if let mir::Operand::Constant(box func) = func_operand; + if let TyKind::FnDef(def_id, _callee_substs) = func.const_.ty().kind(); + if match_def_path(cx, *def_id, &["core", "cmp", "PartialEq", "ne"]) + || match_def_path(cx, *def_id, &["core", "cmp", "PartialEq", "eq"]); + // check if any of the args accesses program_id + if let Operand::Copy(arg0_pl) | Operand::Move(arg0_pl) = args[0]; + if let Operand::Copy(arg1_pl) | Operand::Move(arg1_pl) = args[1]; + then { + // if either arg0 or arg1 came from one of the programid_locals, then we know + // this eq/ne check was operating on the program_id. + if is_moved_from(cx, body, cur_block, &arg0_pl, programid_locals) + || is_moved_from(cx, body, cur_block, &arg1_pl, programid_locals) + { + // we found the check. if it dominates the call to invoke, then the check + // is assumed to be sufficient! + return body.basic_blocks.dominators().dominates(cur_block, block); } } + } - match preds.get(cur_block) { - Some(cur_preds) if !cur_preds.is_empty() => cur_block = cur_preds[0], - _ => { - break; - } + match preds.get(cur_block) { + Some(cur_preds) if !cur_preds.is_empty() => cur_block = cur_preds[0], + _ => { + break; } } - false } + false +} - // helper function - // Given the Place search_place, check if it was defined using one of the locals in search_list - fn is_moved_from<'tcx>( - _: &LateContext, - body: &'tcx mir::Body<'tcx>, - block: BasicBlock, - mut search_place: &Place<'tcx>, - search_list: &[Local], - ) -> bool { - let preds = body.basic_blocks.predecessors(); - let mut cur_block = block; - if let Some(search_loc) = search_place.local_or_deref_local() { - if search_list.contains(&search_loc) { - return true; - } +// helper function +// Given the Place search_place, check if it was defined using one of the locals in search_list +fn is_moved_from<'tcx>( + _: &LateContext, + body: &'tcx mir::Body<'tcx>, + block: BasicBlock, + mut search_place: &Place<'tcx>, + search_list: &[Local], +) -> bool { + let preds = body.basic_blocks.predecessors(); + let mut cur_block = block; + if let Some(search_loc) = search_place.local_or_deref_local() { + if search_list.contains(&search_loc) { + return true; } - // look for chain of assign statements whose value is eventually assigned to the `search_place` and - // see if any of the intermediate local is in the search_list. - loop { - for stmt in body.basic_blocks[cur_block].statements.iter().rev() { - match &stmt.kind { - StatementKind::Assign(box (assign_place, rvalue)) - if assign_place.local_or_deref_local() - == search_place.local_or_deref_local() => - { - match rvalue { - Rvalue::Use( - Operand::Copy(rvalue_place) | Operand::Move(rvalue_place), - ) - | Rvalue::Ref(_, _, rvalue_place) => { - search_place = rvalue_place; - if let Some(search_loc) = search_place.local_or_deref_local() { - if search_list.contains(&search_loc) { - return true; - } + } + // look for chain of assign statements whose value is eventually assigned to the `search_place` and + // see if any of the intermediate local is in the search_list. + loop { + for stmt in body.basic_blocks[cur_block].statements.iter().rev() { + match &stmt.kind { + StatementKind::Assign(box (assign_place, rvalue)) + if assign_place.local_or_deref_local() + == search_place.local_or_deref_local() => + { + match rvalue { + Rvalue::Use(Operand::Copy(rvalue_place) | Operand::Move(rvalue_place)) + | Rvalue::Ref(_, _, rvalue_place) => { + search_place = rvalue_place; + if let Some(search_loc) = search_place.local_or_deref_local() { + if search_list.contains(&search_loc) { + return true; } } - _ => {} } + _ => {} } - _ => {} } + _ => {} } - match preds.get(cur_block) { - Some(cur_preds) if !cur_preds.is_empty() => cur_block = cur_preds[0], - _ => { - break; - } + } + match preds.get(cur_block) { + Some(cur_preds) if !cur_preds.is_empty() => cur_block = cur_preds[0], + _ => { + break; } } - false } + false } // We do not test the sealevel-attacks 'insecure' example, because it calls diff --git a/lints/arbitrary_cpi/ui/insecure-2/Cargo.toml b/lints/arbitrary_cpi/ui/insecure-2/Cargo.toml index d857c233..908950b2 100644 --- a/lints/arbitrary_cpi/ui/insecure-2/Cargo.toml +++ b/lints/arbitrary_cpi/ui/insecure-2/Cargo.toml @@ -17,5 +17,6 @@ default = [] [dependencies] anchor-lang = "0.29.0" +anchor-spl = "0.29.0" [workspace] diff --git a/lints/arbitrary_cpi/ui/insecure-2/src/lib.rs b/lints/arbitrary_cpi/ui/insecure-2/src/lib.rs index 0dd00b0b..86e01dd0 100644 --- a/lints/arbitrary_cpi/ui/insecure-2/src/lib.rs +++ b/lints/arbitrary_cpi/ui/insecure-2/src/lib.rs @@ -2,6 +2,7 @@ use anchor_lang::prelude::*; use anchor_lang::solana_program; use anchor_lang::solana_program::entrypoint::ProgramResult; use anchor_lang::solana_program::instruction::Instruction; +use anchor_spl::token::{self, Token}; declare_id!("Fg6PaFpoGXkYsidMpWTK6W2BeZ7FEfcYkg476zPFsLnS"); @@ -10,19 +11,59 @@ pub mod arbitrary_cpi_insecure { use super::*; pub fn cpi(ctx: Context, _amount: u64) -> ProgramResult { + // Instruction {...}; Lint reports the ins let ins = Instruction { - program_id: *ctx.accounts.token_program.key, + program_id: *ctx.accounts.some_program.key, accounts: vec![], data: vec![], }; - solana_program::program::invoke( + solana_program::program::invoke_signed( &ins, &[ ctx.accounts.source.clone(), ctx.accounts.destination.clone(), ctx.accounts.authority.clone(), ], - ) + &[&[]], + )?; + + // CpiContext::new(); Lint reports this + let accounts = token::Transfer { + from: ctx.accounts.source.to_account_info(), + to: ctx.accounts.destination.to_account_info(), + authority: ctx.accounts.authority.to_account_info(), + }; + let cpi_ctx = CpiContext::new(ctx.accounts.some_program.to_account_info(), accounts); + token::transfer(cpi_ctx, 100)?; + + // CpiContext::new_with_signer(); Lint reports this + let accounts2 = token::Transfer { + from: ctx.accounts.destination.to_account_info(), + to: ctx.accounts.source.to_account_info(), + authority: ctx.accounts.authority.to_account_info(), + }; + + let cpi_ctx_signers = CpiContext::new_with_signer( + ctx.accounts.some_program.to_account_info(), + accounts2, + &[&[]], + ); + token::transfer(cpi_ctx_signers, 1000)?; + + // CpiContext::new_with_signer() + // The lint does not report this because `token_program` is of type `Program` + let t = ctx.accounts.token_program.to_account_info(); + + let accounts3 = token::Transfer { + from: ctx.accounts.destination.to_account_info(), + to: ctx.accounts.source.to_account_info(), + authority: ctx.accounts.authority.to_account_info(), + }; + + let cpi_ctx_signers_crct = CpiContext::new_with_signer(t, accounts3, &[&[]]); + token::transfer(cpi_ctx_signers_crct, 1000)?; + + Ok(()) } } @@ -31,7 +72,8 @@ pub struct Cpi<'info> { source: AccountInfo<'info>, destination: AccountInfo<'info>, authority: AccountInfo<'info>, - token_program: AccountInfo<'info>, + token_program: Program<'info, Token>, + some_program: AccountInfo<'info>, } #[allow(dead_code)] diff --git a/lints/arbitrary_cpi/ui/insecure-2/src/lib.stderr b/lints/arbitrary_cpi/ui/insecure-2/src/lib.stderr index 38c26a5e..932a967f 100644 --- a/lints/arbitrary_cpi/ui/insecure-2/src/lib.stderr +++ b/lints/arbitrary_cpi/ui/insecure-2/src/lib.stderr @@ -1,17 +1,33 @@ error: program_id may not be checked - --> $DIR/lib.rs:18:9 + --> $DIR/lib.rs:15:19 | -LL | / solana_program::program::invoke( -LL | | &ins, -LL | | &[ -LL | | ctx.accounts.source.clone(), -... | -LL | | ], -LL | | ) +LL | let ins = Instruction { + | ___________________^ +LL | | program_id: *ctx.accounts.some_program.key, +LL | | accounts: vec![], +LL | | data: vec![], +LL | | }; | |_________^ | = note: `-D arbitrary-cpi` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(arbitrary_cpi)]` -error: aborting due to 1 previous error +error: program_id may not be checked + --> $DIR/lib.rs:36:23 + | +LL | let cpi_ctx = CpiContext::new(ctx.accounts.some_program.to_account_info(), accounts); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: program_id may not be checked + --> $DIR/lib.rs:46:31 + | +LL | let cpi_ctx_signers = CpiContext::new_with_signer( + | _______________________________^ +LL | | ctx.accounts.some_program.to_account_info(), +LL | | accounts2, +LL | | &[&[]], +LL | | ); + | |_________^ + +error: aborting due to 3 previous errors