From 8750729837d92d3faec0f6ff63fc828743bdf285 Mon Sep 17 00:00:00 2001 From: Adam Welc Date: Thu, 20 Jul 2023 11:25:46 -0700 Subject: [PATCH] [move-compiler] Added a linter flagging potential unenforceable custom transfer/share/freeze policy (#13029) ## Description This adds another linter, this time to flag a potential unenforceable transfer/share/freeze policy - a developer may implement a custom policy for objects with the `store` ability which can be transferred/shared/frozen using public function variants. ## Test Plan New tests have been added. --- If your changes are not user-facing and not a breaking change, you can skip the following section. Otherwise, please indicate what changed, and then add to the Release Notes section as highlighted during the release process. ### Type of Change (Check all that apply) - [ ] protocol change - [x] user-visible impact - [ ] breaking change for a client SDKs - [ ] breaking change for FNs (FN binary must upgrade) - [ ] breaking change for validators or node operators (must upgrade binaries) - [ ] breaking change for on-chain data layout - [ ] necessitate either a data wipe or data migration ### Release notes When building Move code, additional linter warnings related to implementing custom transfer/share/freeze functions may appear. These functions are created to enforce a custom transfer/share/freeze policy but implementing them to work with instances of a type with the `store` ability makes the policy unenforceable (these type instances can be transferred/shared/frozen using public variants of transfer/share/freeze functions). --- crates/sui-move-build/src/lib.rs | 10 +- .../src/linters/custom_state_change.rs | 272 ++++++++++++++++++ crates/sui-move-build/src/linters/mod.rs | 15 + .../src/linters/self_transfer.rs | 8 +- .../sui-move-build/src/linters/share_owned.rs | 13 +- .../tests/linter/redundant_custom.exp | 39 +++ .../tests/linter/redundant_custom.move | 26 ++ crates/sui-move-build/tests/linter_tests.rs | 10 +- 8 files changed, 375 insertions(+), 18 deletions(-) create mode 100644 crates/sui-move-build/src/linters/custom_state_change.rs create mode 100644 crates/sui-move-build/tests/linter/redundant_custom.exp create mode 100644 crates/sui-move-build/tests/linter/redundant_custom.move diff --git a/crates/sui-move-build/src/lib.rs b/crates/sui-move-build/src/lib.rs index ce3ed525cfc38..b0d02d430ea64 100644 --- a/crates/sui-move-build/src/lib.rs +++ b/crates/sui-move-build/src/lib.rs @@ -54,7 +54,8 @@ use sui_types::{ use sui_verifier::verifier as sui_bytecode_verifier; use crate::linters::{ - known_filters, self_transfer::SelfTransferVerifier, share_owned::ShareOwnedVerifier, + custom_state_change::CustomStateChangeVerifier, known_filters, + self_transfer::SelfTransferVerifier, share_owned::ShareOwnedVerifier, }; #[cfg(test)] @@ -134,8 +135,11 @@ impl BuildConfig { let mut fn_info = None; let compiled_pkg = build_plan.compile_with_driver(writer, |compiler| { let (files, units_res) = if lint { - let lint_visitors = - vec![ShareOwnedVerifier.visitor(), SelfTransferVerifier.visitor()]; + let lint_visitors = vec![ + ShareOwnedVerifier.visitor(), + SelfTransferVerifier.visitor(), + CustomStateChangeVerifier.visitor(), + ]; let (filter_attr_name, filters) = known_filters(); compiler .add_visitors(lint_visitors) diff --git a/crates/sui-move-build/src/linters/custom_state_change.rs b/crates/sui-move-build/src/linters/custom_state_change.rs new file mode 100644 index 0000000000000..a89879cb9fecb --- /dev/null +++ b/crates/sui-move-build/src/linters/custom_state_change.rs @@ -0,0 +1,272 @@ +// Copyright (c) Mysten Labs, Inc. +// SPDX-License-Identifier: Apache-2.0 + +//! This analysis flags potential custom implementations of transfer/share/freeze calls on objects +//! that already have a store ability and where "public" variants of these calls can be used. This +//! can be dangerous as custom transfer/share/freeze operation is becoming unenforceable in this +//! situation. A function is considered a potential custom implementation if it takes as a +//! parameter an instance of a struct type defined in a given module with a store ability and passes +//! it as an argument to a "private" transfer/share/freeze call. + +use move_ir_types::location::*; + +use move_compiler::{ + cfgir::{ + absint::JoinResult, + ast::Program, + visitor::{ + LocalState, SimpleAbsInt, SimpleAbsIntConstructor, SimpleDomain, SimpleExecutionContext, + }, + CFGContext, MemberName, + }, + diag, + diagnostics::{ + codes::{custom, DiagnosticInfo, Severity}, + Diagnostic, Diagnostics, + }, + hlir::ast::{ + BaseType_, Command, Exp, LValue, Label, ModuleCall, SingleType, SingleType_, Type, + TypeName_, Type_, Var, + }, + parser::ast::Ability_, + shared::{CompilationEnv, Identifier}, +}; +use std::collections::BTreeMap; + +use super::{ + CUSTOM_STATE_CHANGE_DIAG_CATEGORY, CUSTOM_STATE_CHANGE_DIAG_CODE, INVALID_LOC, + LINT_WARNING_PREFIX, +}; + +const TRANSFER_FUN: &str = "transfer"; +const SHARE_FUN: &str = "share_object"; +const FREEZE_FUN: &str = "freeze_object"; + +const PRIVATE_OBJ_FUNCTIONS: &[(&str, &str, &str)] = &[ + ("sui", "transfer", TRANSFER_FUN), + ("sui", "transfer", SHARE_FUN), + ("sui", "transfer", FREEZE_FUN), +]; + +const CUSTOM_STATE_CHANGE_DIAG: DiagnosticInfo = custom( + LINT_WARNING_PREFIX, + Severity::Warning, + CUSTOM_STATE_CHANGE_DIAG_CATEGORY, + CUSTOM_STATE_CHANGE_DIAG_CODE, + "potentially unenforceable custom transfer/share/freeze policy", +); + +//************************************************************************************************** +// types +//************************************************************************************************** + +pub struct CustomStateChangeVerifier; +pub struct CustomStateChangeVerifierAI { + fn_name_loc: Loc, +} + +#[derive(Copy, Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Default)] +pub enum Value { + /// An instance of a struct defined within a given module with a store ability. + LocalObjWithStore(Loc), + #[default] + Other, +} + +pub struct ExecutionContext { + diags: Diagnostics, +} + +#[derive(Clone, Debug)] +pub struct State { + locals: BTreeMap>, +} + +//************************************************************************************************** +// impls +//************************************************************************************************** + +impl SimpleAbsIntConstructor for CustomStateChangeVerifier { + type AI<'a> = CustomStateChangeVerifierAI; + + fn new<'a>( + _env: &CompilationEnv, + _program: &'a Program, + context: &'a CFGContext<'a>, + _init_state: &mut as SimpleAbsInt>::State, + ) -> Option> { + let Some(_) = &context.module else { + return None + }; + let MemberName::Function(fn_name) = context.member else { + return None; + }; + + Some(CustomStateChangeVerifierAI { + fn_name_loc: fn_name.loc, + }) + } +} + +impl SimpleAbsInt for CustomStateChangeVerifierAI { + type State = State; + type ExecutionContext = ExecutionContext; + + fn finish(&mut self, _final_states: BTreeMap, diags: Diagnostics) -> Diagnostics { + diags + } + + fn start_command(&self, _: &mut State) -> ExecutionContext { + ExecutionContext { + diags: Diagnostics::new(), + } + } + + fn finish_command(&self, context: ExecutionContext, _state: &mut State) -> Diagnostics { + let ExecutionContext { diags } = context; + diags + } + + fn exp_custom( + &self, + _context: &mut ExecutionContext, + _state: &mut State, + _e: &Exp, + ) -> Option> { + None + } + + fn call_custom( + &self, + context: &mut ExecutionContext, + _state: &mut State, + _loc: &Loc, + return_ty: &Type, + f: &ModuleCall, + args: Vec, + ) -> Option> { + if let Some((_, _, fname)) = PRIVATE_OBJ_FUNCTIONS + .iter() + .find(|(addr, module, fun)| f.is(addr, module, fun)) + { + if let Value::LocalObjWithStore(obj_addr_loc) = args[0] { + let msg = format!( + "Potential unintended implementation of a custom {} function.", + fname + ); + let (op, action) = if *fname == TRANSFER_FUN { + ("transfer", "transferred") + } else if *fname == SHARE_FUN { + ("share", "shared") + } else { + ("freeze", "frozen") + }; + let uid_msg = format!( + "Instances of a type with a store ability can be {action} using \ + the public_{fname} function which often negates the intent \ + of enforcing a custom {op} policy" + ); + let note_msg = format!("A custom {op} policy for a given type is implemented through calling \ + the private {fname} function variant in the module defining this type"); + let mut d = diag!( + CUSTOM_STATE_CHANGE_DIAG, + (self.fn_name_loc, msg), + (f.name.loc(), uid_msg) + ); + d.add_note(note_msg); + if obj_addr_loc != INVALID_LOC { + let loc_msg = format!("An instance of a module-private type with a store ability to be {} coming from here", action); + d.add_secondary_label((obj_addr_loc, loc_msg)); + } + context.add_diag(d) + } + } + Some(match &return_ty.value { + Type_::Unit => vec![], + Type_::Single(_) => vec![Value::Other], + Type_::Multiple(types) => vec![Value::Other; types.len()], + }) + } + + fn command_custom(&self, _: &mut ExecutionContext, _: &mut State, _: &Command) -> bool { + false + } + + fn lvalue_custom( + &self, + _context: &mut ExecutionContext, + _state: &mut State, + _l: &LValue, + _value: &Value, + ) -> bool { + false + } +} + +fn is_local_obj_with_store(sp!(_, st_): &SingleType, context: &CFGContext) -> bool { + let sp!(_, bt_) = match st_ { + SingleType_::Base(v) => v, + // transfer/share/freeze take objects by value so even if by-reference object has store and + // is module-local, it could not end up being an argument to one of these functions + SingleType_::Ref(_, _) => return false, + }; + if let BaseType_::Apply(abilities, sp!(_, tname), _) = bt_ { + if !abilities.has_ability_(Ability_::Store) { + // no store ability + return false; + } + if let TypeName_::ModuleType(mident, _) = tname { + if let Some(current_mident) = context.module { + if mident.value == current_mident.value { + return true; + } + } + } + } + false +} + +impl SimpleDomain for State { + type Value = Value; + + fn new(context: &CFGContext, mut locals: BTreeMap>) -> Self { + for (v, st) in &context.signature.parameters { + if is_local_obj_with_store(st, context) { + let local_state = locals.get_mut(v).unwrap(); + if let LocalState::Available(loc, _) = local_state { + *local_state = LocalState::Available(*loc, Value::LocalObjWithStore(*loc)); + } + } + } + State { locals } + } + + fn locals_mut(&mut self) -> &mut BTreeMap> { + &mut self.locals + } + + fn locals(&self) -> &BTreeMap> { + &self.locals + } + + fn join_value(v1: &Value, v2: &Value) -> Value { + match (v1, v2) { + (obj @ Value::LocalObjWithStore(loc1), Value::LocalObjWithStore(loc2)) => { + if loc1 == loc2 { + *obj + } else { + Value::LocalObjWithStore(INVALID_LOC) + } + } + (Value::Other, _) | (_, Value::Other) => Value::Other, + } + } + + fn join_impl(&mut self, _: &Self, _: &mut JoinResult) {} +} + +impl SimpleExecutionContext for ExecutionContext { + fn add_diag(&mut self, diag: Diagnostic) { + self.diags.add(diag) + } +} diff --git a/crates/sui-move-build/src/linters/mod.rs b/crates/sui-move-build/src/linters/mod.rs index 21bf1f21d652f..f52b31c427441 100644 --- a/crates/sui-move-build/src/linters/mod.rs +++ b/crates/sui-move-build/src/linters/mod.rs @@ -5,7 +5,9 @@ use move_compiler::{ diagnostics::codes::{DiagnosticsID, WarningFilter}, expansion::ast as E, }; +use move_ir_types::location::Loc; +pub mod custom_state_change; pub mod self_transfer; pub mod share_owned; @@ -13,12 +15,17 @@ pub const SHARE_OWNED_DIAG_CATEGORY: u8 = 1; pub const SHARE_OWNED_DIAG_CODE: u8 = 1; pub const SELF_TRANSFER_DIAG_CATEGORY: u8 = 2; pub const SELF_TRANSFER_DIAG_CODE: u8 = 1; +pub const CUSTOM_STATE_CHANGE_DIAG_CATEGORY: u8 = 3; +pub const CUSTOM_STATE_CHANGE_DIAG_CODE: u8 = 1; pub const ALLOW_ATTR_NAME: &str = "lint_allow"; pub const LINT_WARNING_PREFIX: &str = "Lint "; pub const SHARE_OWNED_FILTER_NAME: &str = "share_owned"; pub const SELF_TRANSFER_FILTER_NAME: &str = "self_transfer"; +pub const CUSTOM_STATE_CHANGE_FILTER_NAME: &str = "custom_state_change"; + +pub const INVALID_LOC: Loc = Loc::invalid(); pub fn known_filters() -> (E::AttributeName_, Vec) { ( @@ -41,6 +48,14 @@ pub fn known_filters() -> (E::AttributeName_, Vec) { ), Some(SELF_TRANSFER_FILTER_NAME), ), + WarningFilter::Code( + DiagnosticsID::new( + CUSTOM_STATE_CHANGE_DIAG_CATEGORY, + CUSTOM_STATE_CHANGE_DIAG_CODE, + Some(LINT_WARNING_PREFIX), + ), + Some(CUSTOM_STATE_CHANGE_FILTER_NAME), + ), ], ) } diff --git a/crates/sui-move-build/src/linters/self_transfer.rs b/crates/sui-move-build/src/linters/self_transfer.rs index 9059d04b9a292..d91cb2ae1b04f 100644 --- a/crates/sui-move-build/src/linters/self_transfer.rs +++ b/crates/sui-move-build/src/linters/self_transfer.rs @@ -26,17 +26,17 @@ use move_compiler::{ use move_symbol_pool::Symbol; use std::collections::BTreeMap; -use super::{SELF_TRANSFER_DIAG_CATEGORY, SELF_TRANSFER_DIAG_CODE}; +use super::{ + INVALID_LOC, LINT_WARNING_PREFIX, SELF_TRANSFER_DIAG_CATEGORY, SELF_TRANSFER_DIAG_CODE, +}; const TRANSFER_FUNCTIONS: &[(&str, &str, &str)] = &[ ("sui", "transfer", "public_transfer"), ("sui", "transfer", "transfer"), ]; -const INVALID_LOC: Loc = Loc::invalid(); - const SELF_TRANSFER_DIAG: DiagnosticInfo = custom( - "Lint ", + LINT_WARNING_PREFIX, Severity::Warning, SELF_TRANSFER_DIAG_CATEGORY, SELF_TRANSFER_DIAG_CODE, diff --git a/crates/sui-move-build/src/linters/share_owned.rs b/crates/sui-move-build/src/linters/share_owned.rs index 9ba57f466f52b..ee681e1b2738c 100644 --- a/crates/sui-move-build/src/linters/share_owned.rs +++ b/crates/sui-move-build/src/linters/share_owned.rs @@ -30,7 +30,7 @@ use move_compiler::{ }; use std::collections::BTreeMap; -use super::{SHARE_OWNED_DIAG_CATEGORY, SHARE_OWNED_DIAG_CODE}; +use super::{LINT_WARNING_PREFIX, SHARE_OWNED_DIAG_CATEGORY, SHARE_OWNED_DIAG_CODE}; const SHARE_FUNCTIONS: &[(&str, &str, &str)] = &[ ("sui", "transfer", "public_share_object"), @@ -38,7 +38,7 @@ const SHARE_FUNCTIONS: &[(&str, &str, &str)] = &[ ]; const SHARE_OWNED_DIAG: DiagnosticInfo = custom( - "Lint ", + LINT_WARNING_PREFIX, Severity::Warning, SHARE_OWNED_DIAG_CATEGORY, SHARE_OWNED_DIAG_CODE, @@ -206,21 +206,18 @@ impl SimpleAbsInt for ShareOwnedVerifierAI { } } -fn is_obj(l: &LValue) -> bool { - let sp!(_, l_) = l; +fn is_obj(sp!(_, l_): &LValue) -> bool { if let LValue_::Var(_, st) = l_ { return is_obj_type(st); } false } -fn is_obj_type(st: &SingleType) -> bool { - let sp!(_, st_) = st; - let bt = match st_ { +fn is_obj_type(sp!(_, st_): &SingleType) -> bool { + let sp!(_, bt_) = match st_ { SingleType_::Base(v) => v, SingleType_::Ref(_, v) => v, }; - let sp!(_, bt_) = bt; if let BaseType_::Apply(abilities, _, _) = bt_ { if abilities.has_ability_(Ability_::Key) { return true; diff --git a/crates/sui-move-build/tests/linter/redundant_custom.exp b/crates/sui-move-build/tests/linter/redundant_custom.exp new file mode 100644 index 0000000000000..55d0885311de1 --- /dev/null +++ b/crates/sui-move-build/tests/linter/redundant_custom.exp @@ -0,0 +1,39 @@ +warning[Lint W03001]: potentially unenforceable custom transfer/share/freeze policy + ┌─ tests/linter/redundant_custom.move:14:16 + │ +14 │ public fun custom_transfer_bad(o: S1, ctx: &mut TxContext) { + │ ^^^^^^^^^^^^^^^^^^^ - An instance of a module-private type with a store ability to be transferred coming from here + │ │ + │ Potential unintended implementation of a custom transfer function. +15 │ transfer::transfer(o, tx_context::sender(ctx)) + │ -------- Instances of a type with a store ability can be transferred using the public_transfer function which often negates the intent of enforcing a custom transfer policy + │ + = A custom transfer policy for a given type is implemented through calling the private transfer function variant in the module defining this type + = This warning can be suppressed with '#[lint_allow(custom_state_change)]' applied to the 'module' or module member ('const', 'fun', or 'struct') + +warning[Lint W03001]: potentially unenforceable custom transfer/share/freeze policy + ┌─ tests/linter/redundant_custom.move:19:16 + │ +19 │ public fun custom_share_bad(o: S1) { + │ ^^^^^^^^^^^^^^^^ - An instance of a module-private type with a store ability to be shared coming from here + │ │ + │ Potential unintended implementation of a custom share_object function. +20 │ transfer::share_object(o) + │ ------------ Instances of a type with a store ability can be shared using the public_share_object function which often negates the intent of enforcing a custom share policy + │ + = A custom share policy for a given type is implemented through calling the private share_object function variant in the module defining this type + = This warning can be suppressed with '#[lint_allow(custom_state_change)]' applied to the 'module' or module member ('const', 'fun', or 'struct') + +warning[Lint W03001]: potentially unenforceable custom transfer/share/freeze policy + ┌─ tests/linter/redundant_custom.move:23:16 + │ +23 │ public fun custom_freeze_bad(o: S1) { + │ ^^^^^^^^^^^^^^^^^ - An instance of a module-private type with a store ability to be frozen coming from here + │ │ + │ Potential unintended implementation of a custom freeze_object function. +24 │ transfer::freeze_object(o) + │ ------------- Instances of a type with a store ability can be frozen using the public_freeze_object function which often negates the intent of enforcing a custom freeze policy + │ + = A custom freeze policy for a given type is implemented through calling the private freeze_object function variant in the module defining this type + = This warning can be suppressed with '#[lint_allow(custom_state_change)]' applied to the 'module' or module member ('const', 'fun', or 'struct') + diff --git a/crates/sui-move-build/tests/linter/redundant_custom.move b/crates/sui-move-build/tests/linter/redundant_custom.move new file mode 100644 index 0000000000000..b93ba3313ca29 --- /dev/null +++ b/crates/sui-move-build/tests/linter/redundant_custom.move @@ -0,0 +1,26 @@ +// Copyright (c) Mysten Labs, Inc. +// SPDX-License-Identifier: Apache-2.0 + +module 0x42::test { + use sui::object::UID; + use sui::transfer; + use sui::tx_context::{Self, TxContext}; + + struct S1 has key, store { + id: UID + } + + #[lint_allow(self_transfer)] + public fun custom_transfer_bad(o: S1, ctx: &mut TxContext) { + transfer::transfer(o, tx_context::sender(ctx)) + } + + #[lint_allow(share_owned)] + public fun custom_share_bad(o: S1) { + transfer::share_object(o) + } + + public fun custom_freeze_bad(o: S1) { + transfer::freeze_object(o) + } +} diff --git a/crates/sui-move-build/tests/linter_tests.rs b/crates/sui-move-build/tests/linter_tests.rs index 1b04f1a2cbf77..05387f657b149 100644 --- a/crates/sui-move-build/tests/linter_tests.rs +++ b/crates/sui-move-build/tests/linter_tests.rs @@ -18,8 +18,8 @@ use move_compiler::{ }; use sui_move_build::linters::{ - known_filters, self_transfer::SelfTransferVerifier, share_owned::ShareOwnedVerifier, - LINT_WARNING_PREFIX, + custom_state_change::CustomStateChangeVerifier, known_filters, + self_transfer::SelfTransferVerifier, share_owned::ShareOwnedVerifier, LINT_WARNING_PREFIX, }; const SUI_FRAMEWORK_PATH: &str = "../sui-framework/packages/sui-framework"; @@ -62,7 +62,11 @@ fn run_tests(path: &Path) -> anyhow::Result<()> { let exp_path = path.with_extension(EXP_EXT); let targets: Vec = vec![path.to_str().unwrap().to_owned()]; - let lint_visitors = vec![ShareOwnedVerifier.visitor(), SelfTransferVerifier.visitor()]; + let lint_visitors = vec![ + ShareOwnedVerifier.visitor(), + SelfTransferVerifier.visitor(), + CustomStateChangeVerifier.visitor(), + ]; let (filter_attr_name, filters) = known_filters_for_test(); let (files, comments_and_compiler_res) = Compiler::from_files( targets,