Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add SuspiciousMessageMode detector #193

Merged
merged 15 commits into from
Oct 24, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions src/cli/cli.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { Driver } from "./driver";
import { cliOptions, STDOUT_PATH } from "./options";
import { OutputFormat } from "../cli";
import { createDetector } from "../createDetector";
import { BuiltInDetectors } from "../detectors/detector";
import { unreachable } from "../internals/util";
import { generateToolsHelpMessage } from "../tools/tool";
import { MISTI_VERSION, TACT_VERSION } from "../version";
Expand All @@ -26,10 +27,15 @@ export function createMistiCommand(): Command {
.arguments("[TACT_CONFIG_PATH|TACT_FILE_PATH]");
cliOptions.forEach((option) => command.addOption(option));
command.action(async (_tactPath, options) => {
const logger = new Logger();
if (options.listTools) {
const toolsHelpMessage = await generateToolsHelpMessage();
// eslint-disable-next-line no-console
console.log(toolsHelpMessage);
logger.info(toolsHelpMessage);
process.exit(0);
}
if (options.listDetectors) {
Esorat marked this conversation as resolved.
Show resolved Hide resolved
const detectorNames = Object.keys(BuiltInDetectors);
detectorNames.forEach((name) => logger.info(`- ${name}`));
process.exit(0);
}
if (options.newDetector) {
Expand Down
5 changes: 5 additions & 0 deletions src/cli/options.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ export interface CLIOptions {
allDetectors: boolean;
config: string | undefined;
newDetector: string | undefined;
listDetectors: boolean;
}

export const cliOptionDefaults: Required<CLIOptions> = {
Expand All @@ -42,6 +43,7 @@ export const cliOptionDefaults: Required<CLIOptions> = {
allDetectors: false,
config: undefined,
newDetector: undefined,
listDetectors: false,
};

export const cliOptions = [
Expand Down Expand Up @@ -70,6 +72,9 @@ export const cliOptions = [
new Option("--list-tools", "List available tools and their options.").default(
cliOptionDefaults.listTools,
),
new Option("--list-detectors", "List available detectors.").default(
cliOptionDefaults.listDetectors,
),
new Option(
"-o, --output-format <json|plain>",
"Set the output format for all tools and warnings",
Expand Down
104 changes: 104 additions & 0 deletions src/detectors/builtin/suspiciousMessageMode.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
import { CompilationUnit } from "../../internals/ir";
import { forEachExpression } from "../../internals/tact";
import { MistiTactWarning, Severity } from "../../internals/warnings";
import { ASTDetector } from "../detector";
import {
AstExpression,
AstStructInstance,
idText,
AstOpBinary,
AstId,
} from "@tact-lang/compiler/dist/grammar/ast";

export class SuspiciousMessageMode extends ASTDetector {
Esorat marked this conversation as resolved.
Show resolved Hide resolved
severity = Severity.MEDIUM;

async check(cu: CompilationUnit): Promise<MistiTactWarning[]> {
Esorat marked this conversation as resolved.
Show resolved Hide resolved
const warnings: MistiTactWarning[] = [];
Array.from(cu.ast.getProgramEntries()).forEach((node) => {
forEachExpression(node, (expr) => {
if (this.isSendParametersStruct(expr)) {
this.checkSendParameters(expr, warnings);
}
});
});
return warnings;
}
private isSendParametersStruct(expr: AstExpression): boolean {
if (expr.kind === "struct_instance") {
Esorat marked this conversation as resolved.
Show resolved Hide resolved
return idText((expr as AstStructInstance).type) === "SendParameters";
}
return false;
}

private checkSendParameters(
expr: AstExpression,
Esorat marked this conversation as resolved.
Show resolved Hide resolved
warnings: MistiTactWarning[],
): void {
const args = (expr as AstStructInstance).args;
Esorat marked this conversation as resolved.
Show resolved Hide resolved
const modeField = args.find((arg) => idText(arg.field) === "mode");
if (modeField) {
this.checkModeExpression(modeField.initializer, warnings);
}
}

private checkModeExpression(
expr: AstExpression,
warnings: MistiTactWarning[],
): void {
const flagsUsed = new Set<string>();
const traverse = (expr: AstExpression): void => {
Esorat marked this conversation as resolved.
Show resolved Hide resolved
switch (expr.kind) {
case "op_binary":
const opBinary = expr as AstOpBinary;
if (opBinary.op !== "|") {
warnings.push(
this.makeWarning(
"Mode expression should only contain the '|' operator",
expr.loc,
{
suggestion:
"Use the '|' operator (bitwise OR) to combine flags",
},
),
);
}
traverse(opBinary.left);
traverse(opBinary.right);
break;
case "id":
const flagName = idText(expr as AstId);
Esorat marked this conversation as resolved.
Show resolved Hide resolved
if (flagsUsed.has(flagName)) {
warnings.push(
this.makeWarning(
`Flag '${flagName}' is used multiple times in mode expression`,
Esorat marked this conversation as resolved.
Show resolved Hide resolved
expr.loc,
{
suggestion:
"Use each flag at most once in the mode expression",
},
),
);
} else {
Esorat marked this conversation as resolved.
Show resolved Hide resolved
flagsUsed.add(flagName);
}
break;
case "number":
warnings.push(
this.makeWarning(
"Integer literals should not be used in mode expression; use symbolic constants instead",
expr.loc,
{
suggestion:
"Replace integer literals with symbolic flag constants",
},
),
);
break;
default:
break;
}
};
traverse(expr);
}
}
7 changes: 7 additions & 0 deletions src/detectors/detector.ts
Original file line number Diff line number Diff line change
Expand Up @@ -378,6 +378,13 @@ export const BuiltInDetectors: Record<string, DetectorEntry> = {
),
enabledByDefault: true,
},
SuspiciousMessageMode: {
loader: (ctx: MistiContext) =>
import("./builtin/suspiciousMessageMode").then(
(module) => new module.SuspiciousMessageMode(ctx),
),
enabledByDefault: true,
},
};

/**
Expand Down
71 changes: 71 additions & 0 deletions test/detectors/SuspiciousMessageMode.expected.out
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
[MEDIUM] SuspiciousMessageMode: Flag 'CustomSendRemainingValue' is used multiple times in mode expression
test/detectors/SuspiciousMessageMode.tact:21:46:
20 | value: 0,
> 21 | mode: CustomSendRemainingValue | CustomSendRemainingValue
^
22 | });
Help: Use each flag at most once in the mode expression
See: https://nowarp.io/tools/misti/docs/detectors/SuspiciousMessageMode

[MEDIUM] SuspiciousMessageMode: Mode expression should only contain the '|' operator
test/detectors/SuspiciousMessageMode.tact:30:19:
29 | value: 0,
> 30 | mode: CustomSendRemainingValue + CustomSendAllBalance
^
31 | });
Help: Use the '|' operator (bitwise OR) to combine flags
See: https://nowarp.io/tools/misti/docs/detectors/SuspiciousMessageMode

[MEDIUM] SuspiciousMessageMode: Integer literals should not be used in mode expression; use symbolic constants instead
test/detectors/SuspiciousMessageMode.tact:39:19:
38 | value: 0,
> 39 | mode: 64 // Integer literal instead of symbolic constant
^
40 | });
Help: Replace integer literals with symbolic flag constants
See: https://nowarp.io/tools/misti/docs/detectors/SuspiciousMessageMode

[MEDIUM] SuspiciousMessageMode: Mode expression should only contain the '|' operator
test/detectors/SuspiciousMessageMode.tact:57:19:
56 | value: 0,
> 57 | mode: CustomSendRemainingValue + CustomSendRemainingValue + 64 // Duplicate flags, '+' operator, integer literal
^
58 | });
Help: Use the '|' operator (bitwise OR) to combine flags
See: https://nowarp.io/tools/misti/docs/detectors/SuspiciousMessageMode

[MEDIUM] SuspiciousMessageMode: Flag 'CustomSendRemainingValue' is used multiple times in mode expression
test/detectors/SuspiciousMessageMode.tact:57:46:
56 | value: 0,
> 57 | mode: CustomSendRemainingValue + CustomSendRemainingValue + 64 // Duplicate flags, '+' operator, integer literal
^
58 | });
Help: Use each flag at most once in the mode expression
See: https://nowarp.io/tools/misti/docs/detectors/SuspiciousMessageMode

[MEDIUM] SuspiciousMessageMode: Integer literals should not be used in mode expression; use symbolic constants instead
test/detectors/SuspiciousMessageMode.tact:57:73:
56 | value: 0,
> 57 | mode: CustomSendRemainingValue + CustomSendRemainingValue + 64 // Duplicate flags, '+' operator, integer literal
^
58 | });
Help: Replace integer literals with symbolic flag constants
See: https://nowarp.io/tools/misti/docs/detectors/SuspiciousMessageMode

[MEDIUM] SuspiciousMessageMode: Mode expression should only contain the '|' operator
test/detectors/SuspiciousMessageMode.tact:66:19:
65 | value: 0,
> 66 | mode: (CustomSendRemainingValue * CustomSendAllBalance) - CustomSendIgnoreErrors
^
67 | });
Help: Use the '|' operator (bitwise OR) to combine flags
See: https://nowarp.io/tools/misti/docs/detectors/SuspiciousMessageMode

[MEDIUM] SuspiciousMessageMode: Mode expression should only contain the '|' operator
test/detectors/SuspiciousMessageMode.tact:66:20:
65 | value: 0,
> 66 | mode: (CustomSendRemainingValue * CustomSendAllBalance) - CustomSendIgnoreErrors
^
67 | });
Help: Use the '|' operator (bitwise OR) to combine flags
See: https://nowarp.io/tools/misti/docs/detectors/SuspiciousMessageMode
112 changes: 112 additions & 0 deletions test/detectors/SuspiciousMessageMode.tact
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
// Define custom constants
const CustomSendRemainingValue: Int = 64;
const CustomSendAllBalance: Int = 128;
const CustomSendIgnoreErrors: Int = 1;

contract SendParametersTestContract {
// Correct usage: should not trigger any warnings
fun correctUsage() {
send(SendParameters{
to: sender(),
value: 0,
mode: CustomSendRemainingValue
});
}

// Duplicate flag usage: should trigger a warning about flags used multiple times
fun duplicateFlagUsage() {
send(SendParameters{
to: sender(),
value: 0,
mode: CustomSendRemainingValue | CustomSendRemainingValue
});
}

// Invalid operator usage: should trigger a warning about using '+' instead of '|'
fun invalidOperatorUsage() {
send(SendParameters{
to: sender(),
value: 0,
mode: CustomSendRemainingValue + CustomSendAllBalance
});
}

// Integer literal usage: should trigger a warning about using integer literals
fun integerLiteralUsage() {
send(SendParameters{
to: sender(),
value: 0,
mode: 64 // Integer literal instead of symbolic constant
});
}

// Correct combination usage: should not trigger any warnings
fun correctCombinationUsage() {
send(SendParameters{
to: sender(),
value: 0,
mode: CustomSendRemainingValue | CustomSendAllBalance | CustomSendIgnoreErrors
Esorat marked this conversation as resolved.
Show resolved Hide resolved
});
}

// Multiple issues: should trigger multiple warnings
fun multipleIssues() {
send(SendParameters{
to: sender(),
value: 0,
mode: CustomSendRemainingValue + CustomSendRemainingValue + 64 // Duplicate flags, '+' operator, integer literal
});
}

// Complex expression with nested invalid operators
fun complexInvalidOperator() {
send(SendParameters{
to: sender(),
value: 0,
mode: (CustomSendRemainingValue * CustomSendAllBalance) - CustomSendIgnoreErrors
});
}

// Define functionReturningLiteral within the contract scope
fun functionReturningLiteral(): Int {
return 64;
}

// Using a function call that returns an integer literal: should trigger a warning
fun functionCallWithLiteral() {
send(SendParameters{
to: sender(),
value: 0,
mode: self.functionReturningLiteral() // Uses integer literal instead of symbolic constant
});
}

// Using undefined symbolic constants: should trigger a warning if the constant is not recognized
fun undefinedSymbolicConstant() {
send(SendParameters{
to: sender(),
value: 0,
mode: CustomSendIgnoreErrors | CustomSendAllBalance
});
}

// Correct usage with variables
fun correctUsageWithVariables() {
let modeFlag: Int = CustomSendRemainingValue | CustomSendIgnoreErrors;
send(SendParameters{
to: sender(),
value: 0,
mode: modeFlag
});
}

// Invalid usage with variables containing integer literals
fun invalidUsageWithVariableLiteral() {
let modeFlag: Int = 64;
send(SendParameters{
to: sender(),
value: 0,
mode: modeFlag // modeFlag contains an integer literal
Esorat marked this conversation as resolved.
Show resolved Hide resolved
});
}
}
Loading