From 07fc46922ea4963c5d0efa543a73d660204ec06b Mon Sep 17 00:00:00 2001 From: forefy Date: Fri, 16 Feb 2024 12:06:49 +0200 Subject: [PATCH] Added precision, improved SARIF output --- .gitignore | 3 +- eburger/main.py | 2 +- .../templates/missing_reentrancy_guard.yaml | 1 + .../templates/missing_zero_address_check.yaml | 1 + .../modifier_without_proper_enforcement.yaml | 3 +- .../tx_origin_used_for_access_control.yaml | 1 + eburger/templates/unbounded_loop.yaml | 3 +- eburger/templates/unchecked_call_return.yaml | 1 + .../use_of_transfer_or_send_on_payable.yaml | 1 + eburger/utils/outputs.py | 41 ++++++++++++++----- eburger/yaml_parser.py | 1 + 11 files changed, 43 insertions(+), 15 deletions(-) diff --git a/.gitignore b/.gitignore index f1b69e4..606ac57 100644 --- a/.gitignore +++ b/.gitignore @@ -12,4 +12,5 @@ dist/ **/cache .vscode *[0-9][0-9].py -*eburger-output* \ No newline at end of file +*eburger-output* +*.sarif \ No newline at end of file diff --git a/eburger/main.py b/eburger/main.py index 9705d9c..8f21cab 100644 --- a/eburger/main.py +++ b/eburger/main.py @@ -66,7 +66,7 @@ def main(): if len(project_paths) < 1: log( "info", - "No contract projects here, specifiy a path/single file via `-f` or run `eburger` in the smart contract project root.", + "No contract projects here, specifiy a path/single files via `-f` or run `eburger` in the smart contract project root.", ) sys.exit(0) elif len(project_paths) > 1: diff --git a/eburger/templates/missing_reentrancy_guard.yaml b/eburger/templates/missing_reentrancy_guard.yaml index d6f3daa..f1498f1 100644 --- a/eburger/templates/missing_reentrancy_guard.yaml +++ b/eburger/templates/missing_reentrancy_guard.yaml @@ -1,5 +1,6 @@ name: "Missing Reentracy Guard" severity: "Low" +precision: "Low" description: "Due to Solidity's design, when a function in your contract interacts with another contract's address, that contract gains the ability to recursively call your contract's function before handing back the control on the execution flow. If the reentrant function's logic did not expect the function to be called multiple times, it can be exposed to abuse." impact: "Potential draining of funds, depending on what happens when the function is retriggered." action-items: diff --git a/eburger/templates/missing_zero_address_check.yaml b/eburger/templates/missing_zero_address_check.yaml index 58b43ee..e0d4d10 100644 --- a/eburger/templates/missing_zero_address_check.yaml +++ b/eburger/templates/missing_zero_address_check.yaml @@ -1,5 +1,6 @@ name: "Missing Zero Address Check" severity: "Low" +precision: "Medium" description: "In Solidity, contracts often interact with external addresses. Failing to check for a possible 0 address input (especially in constructors, setters, and initializer functions) before such interactions can lead to unexpected dangerous behavior. A zero address check ensures that addresses are explicitly provided and not left uninitialized or set to a default, invalid state." impact: "Unexpected failed transactions and contract behavior." action-items: diff --git a/eburger/templates/modifier_without_proper_enforcement.yaml b/eburger/templates/modifier_without_proper_enforcement.yaml index 6378868..106aaa1 100644 --- a/eburger/templates/modifier_without_proper_enforcement.yaml +++ b/eburger/templates/modifier_without_proper_enforcement.yaml @@ -1,7 +1,8 @@ name: "Modifier Without Proper Enforcement" severity: "High" +precision: "Low" description: "Using the provided modifier onlyOwner for function access control without a proper enforcement mechanism like require or revert is a dire mistake because it fails to restrict access as intended. The modifier merely evaluates a condition (msg.sender == owner) without any action taken based on the result." -impact: "This means any user, regardless of whether they are the owner, can execute functions that are supposed to be restricted to the owner, potentially leading to unauthorized actions, such as withdrawing funds or altering critical contract settings." +impact: "Users may execute functions that are supposed to be restricted, potentially leading to unauthorized actions, such as withdrawing funds or altering critical contract settings." action-items: - "Change the modifier so that it will enforce the ownership check using a require/revert statement." references: diff --git a/eburger/templates/tx_origin_used_for_access_control.yaml b/eburger/templates/tx_origin_used_for_access_control.yaml index 686ec5b..2854971 100644 --- a/eburger/templates/tx_origin_used_for_access_control.yaml +++ b/eburger/templates/tx_origin_used_for_access_control.yaml @@ -1,5 +1,6 @@ name: "tx.origin Used for Access Control" severity: "Low" +precision: "Medium" description: "In a situation in which a user has interacted with a malicious contract, that contract can then impersonate as the victim user to perform actions in the current contract that are relying on the tx.origin." impact: "Malicious contracts can impersonate users to perform actions relying on the tx.origin." action-items: diff --git a/eburger/templates/unbounded_loop.yaml b/eburger/templates/unbounded_loop.yaml index 9631797..8c02784 100644 --- a/eburger/templates/unbounded_loop.yaml +++ b/eburger/templates/unbounded_loop.yaml @@ -1,5 +1,6 @@ -name: "Unbound Loop" +name: "Unbounded Loop" severity: "Low" +precision: "Low" description: "Unbounded loops, specifically, for loops that can modify state and have no apparent max restriction to the nunmber of iterations possible, can lead to excessive gas consumption, which may cause transactions to fail or become prohibitively expensive." impact: "Excessive gas consumption, contract denial of service." action-items: diff --git a/eburger/templates/unchecked_call_return.yaml b/eburger/templates/unchecked_call_return.yaml index 2876d7b..afa02aa 100644 --- a/eburger/templates/unchecked_call_return.yaml +++ b/eburger/templates/unchecked_call_return.yaml @@ -1,5 +1,6 @@ name: "Unchecked Call Return" severity: "Low" +precision: "Low" description: "A contract's call or send functions are used without checking the return value." impact: "Unexpected contract behavior." action-items: diff --git a/eburger/templates/use_of_transfer_or_send_on_payable.yaml b/eburger/templates/use_of_transfer_or_send_on_payable.yaml index bee233e..38fc8a6 100644 --- a/eburger/templates/use_of_transfer_or_send_on_payable.yaml +++ b/eburger/templates/use_of_transfer_or_send_on_payable.yaml @@ -1,5 +1,6 @@ name: "Use of transfer or send on a payable address" severity: "Medium" +precision: "Medium" description: "In Solidity, .transfer and .send both implement a risky gas limitation that reverts the transaction if the recipient's operations require more gas than the stipend of 2300 gas." impact: "Unexpected failed transactions and contract behavior." action-items: diff --git a/eburger/utils/outputs.py b/eburger/utils/outputs.py index 7ea32c7..57742e7 100644 --- a/eburger/utils/outputs.py +++ b/eburger/utils/outputs.py @@ -104,16 +104,25 @@ def save_as_markdown(output_file_path: Path, json_data: dict): def convert_severity_to_sarif_level(severity: str) -> str: - match severity: - case "High": - sarif_level = "error" - case "Medium": - sarif_level = "warning" - case "Low": - sarif_level = "note" - case _: - sarif_level = "note" - return sarif_level + mapping = {"High": "error", "Medium": "warning", "Low": "note"} + mapped_severity = mapping.get(severity, None) + if mapped_severity is None: + log( + "error", + "Failed converting a template's severity to the respective SARIF level", + ) + return mapped_severity + + +def convert_severity_to_sarif_security_severity(severity: str) -> str: + mapping = mapping = {"High": 8.0, "Medium": 5.0, "Low": 3.0} + mapped_severity = mapping.get(severity, None) + if mapped_severity is None: + log( + "error", + "Failed converting a template's severity to the respective SARIF/GitHub Code Scanning security severity", + ) + return mapped_severity def save_as_sarif(output_file_path: Path, insights: dict): @@ -129,15 +138,20 @@ def save_as_sarif(output_file_path: Path, insights: dict): "results": [], } + # precision - very-high, high, medium, or low + # security-severity - 9.0 is critical, 7.0 to 8.9 is high, 4.0 to 6.9 is medium and 3.9 or less is low + # simplified: 9.0, 8.0, 5.0, 3.0 sarif_rule_template = { "id": "", "name": "", "shortDescription": {"text": ""}, + "fullDescription": {"text": ""}, "helpUri": "", "help": { "text": "", "markdown": "", }, + "properties": {"precision": "", "problem": {"security-severity": ""}}, } if os.path.exists(output_file_path): @@ -175,12 +189,17 @@ def save_as_sarif(output_file_path: Path, insights: dict): new_rule = sarif_rule_template.copy() new_rule["id"] = rule_id new_rule["name"] = insight["name"] - new_rule["shortDescription"]["text"] = insight["description"] + new_rule["shortDescription"]["text"] = insight["name"] + new_rule["fullDescription"]["text"] = insight["description"] new_rule["helpUri"] = insight["references"][0] new_rule["help"]["text"] = insight["action-items"][0] new_rule["help"][ "markdown" ] = f"[{insight['action-items'][0]}]({insight['references'][0]})" + new_rule["properties"]["precision"] = insight["precision"].casefold() + new_rule["properties"]["problem"]["security-severity"] = ( + convert_severity_to_sarif_security_severity(insight["severity"]) + ) new_run["tool"]["driver"]["rules"].append(new_rule) rule_index = len(new_run["tool"]["driver"]["rules"]) - 1 diff --git a/eburger/yaml_parser.py b/eburger/yaml_parser.py index 19e0ecd..1579ed3 100644 --- a/eburger/yaml_parser.py +++ b/eburger/yaml_parser.py @@ -41,6 +41,7 @@ def process_yaml(file_path, ast_data, src_file_list): return { "name": yaml_data.get("name"), "severity": yaml_data.get("severity"), + "precision": yaml_data.get("precision"), "description": yaml_data.get("description"), "results": results, "action-items": yaml_data.get("action-items"),