diff --git a/lints/missing_owner_check/README.md b/lints/missing_owner_check/README.md index 0c394d1..8ad03e2 100644 --- a/lints/missing_owner_check/README.md +++ b/lints/missing_owner_check/README.md @@ -33,3 +33,35 @@ Use instead: See https://github.com/coral-xyz/sealevel-attacks/blob/master/programs/2-owner-checks/secure/src/lib.rs for a secure example. + +**How the lint is implemented:** +- for every function defined in the package +- exclude functions generated from macro expansion. +- Get a list of unique and unsafe AccountInfo's referenced in the body + - for each expression in the function body + - Ignore `.clone()` expressions as the expression referencing original account will be checked + - Check if the expression's type is Solana's AccountInfo (`solana_program::account_info::AccountInfo`) + - Ignore local variable expressions (`x` where x is defined in the function `let x = y`) + - Removes duplcate warnings: both `x` and `y` are reported by the lint. reporting `y` is sufficient. + - Also the owner could be checked on `y`. reporting `x` which a copy/ref of `y` would be false-positive. + - Determine using the expression kind (`.kind`): expr.kind = ExprKind::Path(QPath::Resolved(None, path)); path.segments.len() == 1 + - Ignore safe `.to_account_info()` expressions + - `.to_account_info()` method can be called to convert Anchor different account types to AccountInfo + - The Anchor account types such as `Account` implement `Owner` trait: The owner of the account is checked during deserialization + - The expressions `x.to_account_info` where `x` has one of following types are ignored: + - `Account` requires its type argument to implement `anchor_lang::Owner`. + - `Program`'s implementation of `try_from` checks the account's program id. So there is + no ambiguity in regard to the account's owner. + - `SystemAccount`'s implementation of `try_from` checks that the account's owner is the System Program. + - `AccountLoader` requires its type argument to implement `anchor_lang::Owner`. + - `Signer` are mostly accounts with a private key and most of the times owned by System Program. + - `Sysvar` type arguments checks the account key. + - Ignore `x.to_account_info()` expressions called on Anchor AccountInfo to remove duplicates. + - the lint checks the original expression `x`; no need for checking both. +- For each of the collected expressions, check if `owner` is accessed or if the `key` is compared + - Ignore the `account_expr` if any of the expressions in the function is `{account_expr}.owner` + - Ignore the `account_expr` if `key` is compared + - Check if there is a comparison expression (`==` or `!=`) and one of the expressions being compared accesses key on `account_expr`: + - lhs or rhs of the comparison is `{account_expr}.key()`; The key for Anchor's AccountInfo is accessed using `.key()` + - Or lhs or rhs is `{account_expr}.key`; The key of Solana AccountInfo are accessed using `.key` +- Report the remaining expressions diff --git a/lints/missing_owner_check/src/lib.rs b/lints/missing_owner_check/src/lib.rs index 9ba469d..3a08cc4 100644 --- a/lints/missing_owner_check/src/lib.rs +++ b/lints/missing_owner_check/src/lib.rs @@ -52,6 +52,38 @@ dylint_linting::declare_late_lint! { /// /// See https://github.com/coral-xyz/sealevel-attacks/blob/master/programs/2-owner-checks/secure/src/lib.rs /// for a secure example. + /// + /// **How the lint is implemented:** + /// - for every function defined in the package + /// - exclude functions generated from macro expansion. + /// - Get a list of unique and unsafe AccountInfo's referenced in the body + /// - for each expression in the function body + /// - Ignore `.clone()` expressions as the expression referencing original account will be checked + /// - Check if the expression's type is Solana's AccountInfo (`solana_program::account_info::AccountInfo`) + /// - Ignore local variable expressions (`x` where x is defined in the function `let x = y`) + /// - Removes duplcate warnings: both `x` and `y` are reported by the lint. reporting `y` is sufficient. + /// - Also the owner could be checked on `y`. reporting `x` which a copy/ref of `y` would be false-positive. + /// - Determine using the expression kind (`.kind`): expr.kind = ExprKind::Path(QPath::Resolved(None, path)); path.segments.len() == 1 + /// - Ignore safe `.to_account_info()` expressions + /// - `.to_account_info()` method can be called to convert Anchor different account types to AccountInfo + /// - The Anchor account types such as `Account` implement `Owner` trait: The owner of the account is checked during deserialization + /// - The expressions `x.to_account_info` where `x` has one of following types are ignored: + /// - `Account` requires its type argument to implement `anchor_lang::Owner`. + /// - `Program`'s implementation of `try_from` checks the account's program id. So there is + /// no ambiguity in regard to the account's owner. + /// - `SystemAccount`'s implementation of `try_from` checks that the account's owner is the System Program. + /// - `AccountLoader` requires its type argument to implement `anchor_lang::Owner`. + /// - `Signer` are mostly accounts with a private key and most of the times owned by System Program. + /// - `Sysvar` type arguments checks the account key. + /// - Ignore `x.to_account_info()` expressions called on Anchor AccountInfo to remove duplicates. + /// - the lint checks the original expression `x`; no need for checking both. + /// - For each of the collected expressions, check if `owner` is accessed or if the `key` is compared + /// - Ignore the `account_expr` if any of the expressions in the function is `{account_expr}.owner` + /// - Ignore the `account_expr` if `key` is compared + /// - Check if there is a comparison expression (`==` or `!=`) and one of the expressions being compared accesses key on `account_expr`: + /// - lhs or rhs of the comparison is `{account_expr}.key()`; The key for Anchor's AccountInfo is accessed using `.key()` + /// - Or lhs or rhs is `{account_expr}.key`; The key of Solana AccountInfo are accessed using `.key` + /// - Report the remaining expressions pub MISSING_OWNER_CHECK, Warn, "using an account without checking if its owner is as expected" @@ -67,9 +99,13 @@ impl<'tcx> LateLintPass<'tcx> for MissingOwnerCheck { span: Span, _: HirId, ) { + // exclude functions generated from macro expansions if !span.from_expansion() { + // get unique and unsafe AccountInfo's referenced in the body let accounts = get_referenced_accounts(cx, body); for account_expr in accounts { + // ignore the account_expr if `.owner` field is accessed in the function + // or key of account_expr is compared using `==` or `!=` in the function if !contains_owner_use(cx, body, account_expr) && !contains_key_check(cx, body, account_expr) { span_lint( cx, @@ -97,6 +133,7 @@ fn get_referenced_accounts<'tcx>( uses: Vec::new(), }; + // visit each expr in the body and collect AccountInfo's accounts.visit_expr(body.value); accounts.uses } @@ -107,10 +144,15 @@ impl<'cx, 'tcx> Visitor<'tcx> for AccountUses<'cx, 'tcx> { // s3v3ru5: the following check removes duplicate warnings where lint would report both `x` and `x.clone()` expressions. // ignore `clone()` expressions if !is_expr_method_call(self.cx, expr, &paths::CORE_CLONE).is_some(); + // type of the expression must be Solana's AccountInfo. let ty = self.cx.typeck_results().expr_ty(expr); if match_type(self.cx, ty, &paths::SOLANA_PROGRAM_ACCOUNT_INFO); + // ignore expressions which are local variables if !is_expr_local_variable(expr); + // `to_account_info()` returns AccountInfo. look for expressions calling `to_account_info` and ignore safe expressions + // expression is safe if `to_account_info` is called on Anchor "Owner" types such as Account, which check the owner during deserialization if !is_safe_to_account_info(self.cx, expr); + // check if this expression has been detected and is present in the already collected expressions Vec. let mut spanless_eq = SpanlessEq::new(self.cx); if !self.uses.iter().any(|e| spanless_eq.eq_expr(e, expr)); then { @@ -129,6 +171,10 @@ impl<'cx, 'tcx> Visitor<'tcx> for AccountUses<'cx, 'tcx> { // the lint reports uses of `x`. Having this check would remove such false positives. fn is_expr_local_variable<'tcx>(expr: &'tcx Expr<'tcx>) -> bool { if_chain! { + // The expressions accessing simple local variables have expr.kind = ExprKind::Path(QPath::Resolved(None, path)) + // where path only has one segment. + // Note: The check could be improved by including more checks on path/expr or following a different approach which uses Res. + // matches!(tcx.hir().qpath_res(qpath, expr.hir_id), Res::Local(_)) if let ExprKind::Path(QPath::Resolved(None, path)) = expr.kind; if path.segments.len() == 1; then { @@ -143,7 +189,8 @@ fn is_expr_local_variable<'tcx>(expr: &'tcx Expr<'tcx>) -> bool { fn is_safe_to_account_info<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> bool { if_chain! { // is the expression method call `to_account_info()` - if let Some(recv) = is_expr_method_call(cx, expr, &paths::ANCHOR_LANG_TO_ACCOUNT_INFO); + if let Some(recv) = is_expr_method_call(cx, expr, &paths::ANCHOR_LANG_TO_ACCOUNT_INFO); + // expr_ty_adjusted removes wrappers such as Box, any other implicit conversions and gives the base type if let ty::Ref(_, recv_ty, _) = cx.typeck_results().expr_ty_adjusted(recv).kind(); if let ty::Adt(adt_def, _) = recv_ty.kind(); // smoelius: @@ -185,10 +232,13 @@ fn is_expr_method_call<'tcx>( def_path: &[&str], ) -> Option<&'tcx Expr<'tcx>> { if_chain! { + // expr is a method call if let ExprKind::MethodCall(_, recv, _, _) = expr.kind; + // check the path of the method matches the given path; calling the given method if let Some(def_id) = cx.typeck_results().type_dependent_def_id(expr.hir_id); if match_def_path(cx, def_id, def_path); then { + // return receiver: the expression on which the method is being called. for `x.method()`, `x` is the receiver Some(recv) } else { None @@ -196,6 +246,7 @@ fn is_expr_method_call<'tcx>( } } +/// Check if any of the expressions in the body is `{account_expr}.owner` fn contains_owner_use<'tcx>( cx: &LateContext<'tcx>, body: &'tcx Body<'tcx>, @@ -204,7 +255,7 @@ fn contains_owner_use<'tcx>( visit_expr_no_bodies(body.value, |expr| uses_given_field(cx, expr, account_expr, "owner")) } -/// Checks if `expr` is references `field` on `account_expr` +/// Checks if `expr` references `field` on `account_expr`; `expr` is `{account_expr}.{field}` fn uses_given_field<'tcx>( cx: &LateContext<'tcx>, expr: &Expr<'tcx>, @@ -212,9 +263,11 @@ fn uses_given_field<'tcx>( field: &str, ) -> bool { if_chain! { + // if expression is field access if let ExprKind::Field(object, field_name) = expr.kind; // TODO: add check for key, is_signer if field_name.as_str() == field; + // check if the field is accessed on the expression `{account_expr}` let mut spanless_eq = SpanlessEq::new(cx); if spanless_eq.eq_expr(account_expr, object); then { @@ -233,7 +286,7 @@ fn calls_method_on_expr<'tcx>( def_path: &[&str], ) -> bool { if_chain! { - // check if expr is a method call + // check if expr is a method call to the given method if let Some(recv) = is_expr_method_call(cx, expr, def_path); // check if recv is same expression as account_expr let mut spanless_eq = SpanlessEq::new(cx); @@ -247,7 +300,7 @@ fn calls_method_on_expr<'tcx>( } -// Return true if the expr access key of account_expr(AccountInfo) +/// Return true if the expr access key on `account_expr` (AccountInfo.key) fn expr_accesses_key<'tcx>( cx: &LateContext<'tcx>, expr: &Expr<'tcx>, @@ -257,6 +310,7 @@ fn expr_accesses_key<'tcx>( calls_method_on_expr(cx, expr, account_expr, &paths::ANCHOR_LANG_KEY) || uses_given_field(cx, expr, account_expr, "key") } +/// Check if the key of account returned by account_expr is compared fn contains_key_check<'tcx>( cx: &LateContext<'tcx>, body: &'tcx Body<'tcx>, @@ -265,6 +319,7 @@ fn contains_key_check<'tcx>( visit_expr_no_bodies(body.value, |expr| compares_key(cx, expr, account_expr)) } +/// Check if expr is a comparison expression and one of expressions being compared accesses key on `account_expr` fn compares_key<'tcx>( cx: &LateContext<'tcx>, expr: &Expr<'tcx>,