From 6c1af18adc6da7b41e0d441b180e244eee3aa841 Mon Sep 17 00:00:00 2001 From: Georgiy Komarov Date: Thu, 26 Dec 2024 13:03:20 +0000 Subject: [PATCH] feat(unprotectedCall): naive path-sensitivity --- src/detectors/builtin/unprotectedCall.ts | 66 ++++++++++++++++++------ test/detectors/UnprotectedCall.tact | 20 ++++--- 2 files changed, 64 insertions(+), 22 deletions(-) diff --git a/src/detectors/builtin/unprotectedCall.ts b/src/detectors/builtin/unprotectedCall.ts index c9ae0b7..e4aa5b2 100644 --- a/src/detectors/builtin/unprotectedCall.ts +++ b/src/detectors/builtin/unprotectedCall.ts @@ -16,7 +16,7 @@ import { SEND_METHODS, } from "../../internals/tact"; import { Transfer } from "../../internals/transfer"; -import { unreachable, mergeLists, isListSubsetOf } from "../../internals/util"; +import { unreachable, mergeLists } from "../../internals/util"; import { MistiTactWarning, Severity } from "../../internals/warnings"; import { DataflowDetector } from "../detector"; import { @@ -29,17 +29,35 @@ import { } from "@tact-lang/compiler/dist/grammar/ast"; import { prettyPrint } from "@tact-lang/compiler/dist/prettyPrinter"; -type argTaint = { +type ArgTaint = { id: AstNode["id"]; parents: AstNode["id"][]; name: string; + // Poor man's path-sensitivity: we don't care about path contexts; we only + // need to set this flag and ensure the transfer function maintains + // monotonicity. + unprotected: boolean; }; -function createArgTaint(node: AstId, parents: AstNode["id"][] = []): argTaint { - return { id: node.id, name: idText(node), parents }; +function createArgTaint( + node: AstId, + { + parents = [], + unprotected = true, + }: Partial<{ parents: AstNode["id"][]; unprotected: boolean }> = {}, +): ArgTaint { + return { id: node.id, name: idText(node), parents, unprotected }; +} +function eqArgTaint(lhs: ArgTaint, rhs: ArgTaint): boolean { + return ( + lhs.id === rhs.id && + lhs.name === rhs.name && + lhs.parents.length === rhs.parents.length && + lhs.parents.every((v, i) => v === rhs.parents[i]) + ); } interface TaintState { - argTaints: argTaint[]; + argTaints: ArgTaint[]; } class TaintLattice implements JoinSemilattice { @@ -57,12 +75,12 @@ class TaintLattice implements JoinSemilattice { } leq(a: TaintState, b: TaintState): boolean { - return isListSubsetOf(a.argTaints, b.argTaints); + return a.argTaints.every((x) => b.argTaints.some((y) => eqArgTaint(x, y))); } } class UnprotectedCallTransfer implements Transfer { - constructor(private funArgs: argTaint[]) {} + constructor(private funArgs: ArgTaint[]) {} public transfer( inState: TaintState, @@ -77,12 +95,12 @@ class UnprotectedCallTransfer implements Transfer { } private findTaints( - acc: argTaint[] = [], + acc: ArgTaint[] = [], expr: AstExpression, out: TaintState, ): void { const getTaint = (expr: AstId) => - out.argTaints.find((t) => t.name === expr.text) || + out.argTaints.find((t) => t.name === expr.text && t.unprotected) || this.funArgs.find((t) => t.name === expr.text); switch (expr.kind) { case "id": @@ -117,6 +135,7 @@ class UnprotectedCallTransfer implements Transfer { // value and returns a new (tainted) result? break; case "struct_instance": + // TODO: Should we handle taints appearing in field defs? case "field_access": case "init_of": break; @@ -131,6 +150,23 @@ class UnprotectedCallTransfer implements Transfer { } private processStatement(out: TaintState, stmt: AstStatement) { + this.addNewTaints(out, stmt); + this.trackConditions(out, stmt); + } + + private trackConditions(out: TaintState, stmt: AstStatement) { + if (stmt.kind === "statement_condition") { + const argTaints: ArgTaint[] = []; + this.findTaints(argTaints, stmt.condition, out); + out.argTaints = out.argTaints.map((existing) => + argTaints.some((found) => found.id === existing.id) + ? { ...existing, unprotected: false } + : existing, + ); + } + } + + private addNewTaints(out: TaintState, stmt: AstStatement) { const lhsRhs = (() => { if ( (stmt.kind === "statement_assign" || @@ -144,13 +180,10 @@ class UnprotectedCallTransfer implements Transfer { })(); if (lhsRhs) { const { lhs, rhs } = lhsRhs; - const taints: argTaint[] = []; + const taints: ArgTaint[] = []; this.findTaints(taints, rhs, out); if (taints.length > 0) { - const taint = createArgTaint( - lhs, - taints.map((t) => t.id), - ); + const taint = createArgTaint(lhs, { parents: taints.map((t) => t.id) }); out.argTaints.push(taint); } } @@ -214,7 +247,7 @@ export class UnprotectedCall extends DataflowDetector { return warnings; } - private getArgTaints(f: AstStoreFunction): argTaint[] { + private getArgTaints(f: AstStoreFunction): ArgTaint[] { const taintOfTypedParam = (p: AstTypedParameter) => createArgTaint(p.name); switch (f.kind) { case "function_def": @@ -247,7 +280,8 @@ export class UnprotectedCall extends DataflowDetector { // TODO: Print the source of taint (using argTaint.parent) if (arg.kind !== "id") return; const name = idText(arg); - if (state.argTaints.find((at) => at.name === name)) { + // TODO: direct funargs usage + if (state.argTaints.find((at) => at.name === name && at.unprotected)) { acc.push(this.makeWarning(`${msg}: ${prettyPrint(arg)}`, arg.loc)); } }; diff --git a/test/detectors/UnprotectedCall.tact b/test/detectors/UnprotectedCall.tact index 3df6489..90d74f8 100644 --- a/test/detectors/UnprotectedCall.tact +++ b/test/detectors/UnprotectedCall.tact @@ -3,14 +3,22 @@ contract C { receive(s1: Slice) { let a = s1.loadAddress(); - send(SendParameters{ // Bad - to: a, - value: 0, - bounce: false, - body: emptyCell(), - }); + send(SendParameters{ to: a, value: 0, bounce: false, body: emptyCell() }); // Bad self.m.set(a, 42); // Bad self.m.del(a); // Bad + + if (a != newAddress(0, 0)) { + self.m.set(a, 42); // OK + } + self.m.set(a, 42); // OK: there was a condition above + + if (self.checkAddr(a)) { + self.m.set(a, 42); // OK + } + } + + fun checkAddr(a: Address): Bool { + return a != newAddress(0, 0); } }