diff --git a/CHANGELOG.md b/CHANGELOG.md index e82af15..a2247a1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,10 @@ Notable changes are grouped by release. See [all releases](https://github.com/Co ## Unreleased - Added repository contribution and community documentation. +- SARIF: `tool.driver.rules` lists each rule reported with a title and a description, and each + result carries its confidence (`high`, `medium` or `low`). +- A vtable box whose only warning is `missing typeinfo`, as a valid `-fno-rtti` program logs, is + now a `vtable-diagnostic` warning instead of a `vtable-corrupted` error. ## v0.1.0 diff --git a/README.md b/README.md index 9c7d616..372939d 100644 --- a/README.md +++ b/README.md @@ -31,12 +31,15 @@ captures its output, and turns each report of the CoreTrace runtime into a findi | `vtable-corrupted` | CWE-843 | virtual call site | | `vcall-invalid-target` | CWE-843 | virtual call site | | `vtable-type-mismatch` | CWE-843, warning | virtual call site | +| `vtable-diagnostic` | CWE-843, warning | virtual call site | The memory rules come from the `alloc` and `bounds` modules, which the default module set enables. The vtable rules come from the `vtable` module's diagnostics (`--ct-modules=alloc,vtable --ct-vtable-diag`): each box the runtime logs at `WARN` level is one -finding, its message listing the box's warnings and its rule the most specific one. Boxes logged -at `INFO` level are tracing and produce no finding. +finding, its message listing the box's warnings and its rule the most specific one. A box with +no more specific warning is `vtable-diagnostic`: `missing typeinfo` alone, which a valid program +built with `-fno-rtti` produces, is one. Boxes logged at `INFO` level are tracing and produce no +finding. Sites carry the path the source was compiled from, so run the analyzer from the directory the static analyzers report paths from. @@ -125,9 +128,13 @@ $ echo $? ``` `--format sarif` prints a SARIF 2.1.0 log instead of the text summary: one result per finding -with `ruleId`, `level`, `message.text` and `properties.cwe`, the faulting access as -`locations[0]` and the allocation site as `relatedLocations[0]` (`id` 0, "allocated here"). In -`--test-dir` mode, the log holds the findings of every program in one run. +with `ruleId`, `level`, `message.text`, `properties.cwe` and `properties.confidence`, the faulting +access as `locations[0]` and the allocation site as `relatedLocations[0]` (`id` 0, "allocated +here"). The confidence is `high` for a fault observed while the program ran, `medium` for a vtable +type mismatch, which may be a legitimate cast, and `low` for a vtable report without a more +specific rule. `tool.driver.rules` lists each rule reported, with a title (`shortDescription`) and +what it detects (`fullDescription`), for a consumer such as GitHub code scanning to explain the +finding. In `--test-dir` mode, the log holds the findings of every program in one run. ```console $ runtime-analyzer --format sarif -- --ct-modules=alloc,bounds findings/heap_overflow_write.c @@ -141,12 +148,14 @@ $ runtime-analyzer --format sarif -- --ct-modules=alloc,bounds findings/heap_ove "locations": [ { "physicalLocation": { "artifactLocation": { "uri": "findings/heap_overflow_write.c" }, "region": { "startColumn": 15, "startLine": 7 } } } ], "message": { "text": "heap-buffer-overflow WRITE of size 4" }, - "properties": { "cwe": "CWE-122" }, + "properties": { "confidence": "high", "cwe": "CWE-122" }, "relatedLocations": [ { "id": 0, "message": { "text": "allocated here" }, ... } ], "ruleId": "heap-buffer-overflow" } ], - "tool": { "driver": { "name": "coretrace-runtime-analyzer", ... } } + "tool": { "driver": { "name": "coretrace-runtime-analyzer", + "rules": [ { "id": "heap-buffer-overflow", + "shortDescription": { "text": "Heap buffer overflow" }, ... } ], ... } } } ], "version": "2.1.0" @@ -230,7 +239,7 @@ ctest --test-dir build --output-on-failure | `findings_unit`, `process_unit` | the report parser and the process runner (`unittests/`) | | `cli_args_unit` | argument parsing, every option and every invalid spelling, the compiler's `-o`, the compile-only and `-x` flags, the C-as-C++ and `DEBUG` heuristics, the compiler arguments of a run, the artifact names of a batch (`src/cli_args.hpp`) | | `events_unit` | the CoreTrace event lines of a run and their summary, ANSI colours stripped, and the verdict drawn from a result (`src/events.hpp`) | -| `sarif_unit` | the SARIF writer against the exact log a consumer reads: empty, an error with its locations, a warning without one | +| `sarif_unit` | the SARIF writer against the exact log a consumer reads: empty, an error with its locations, a warning without one; every rule listed with a title and a description, and each finding's confidence | | `api_unit` | the library API: `Run` and `RunBatch` on real programs, a heap overflow, a compile-only run, a hang against the timeout, a batch, the refused and empty cases | | `consumer` | a project fetching this repository the way ctrace does (`unittests/consumer/`) configures, builds, links `coretrace::runtime-analyzer_lib` and analyzes a program | | `fixtures` | every source under `test/` states what it proves in its header (`// args:`, `// expect:`), and the analyzer proves it: findings with rule, CWE, level, faulting line and allocation site, or none; the program's exit status and output when stated. A fixture without an expectation fails (`test/check_fixtures.py`) | diff --git a/src/findings.cpp b/src/findings.cpp index 1375ccf..77ff84d 100644 --- a/src/findings.cpp +++ b/src/findings.cpp @@ -102,10 +102,11 @@ namespace coretrace::runtime_analyzer {"vptr on freed object", "vtable-use-after-free", "CWE-416", Severity::Error}, {"vtable resolve failed", "vtable-corrupted", "CWE-843", Severity::Error}, {"no vptr", "vtable-corrupted", "CWE-843", Severity::Error}, - {"missing typeinfo", "vtable-corrupted", "CWE-843", Severity::Error}, {"target in non-exec memory", "vcall-invalid-target", "CWE-843", Severity::Error}, {"static!=dynamic type", "vtable-type-mismatch", "CWE-843", Severity::Warning}, {"module mismatch: ", "vtable-type-mismatch", "CWE-843", Severity::Warning}, + // Valid under -fno-rtti, so it never outranks another warning of the box. + {"missing typeinfo", "vtable-diagnostic", "CWE-843", Severity::Warning}, }; // A box the runtime logged at WARN level: "[VTABLE]" or "[VCALL]", then "│ label : value │" @@ -251,6 +252,72 @@ namespace coretrace::runtime_analyzer } } // namespace + std::optional DescribeRule(std::string_view rule) + { + struct DescribedRule + { + std::string_view rule; + RuleDescription description; + }; + // Every rule ParseFindings reports: the bounds rules, double-free and memory-leak, and the + // vtable rules with their fallback. + static constexpr DescribedRule kRules[] = { + {"heap-buffer-overflow", + {"Heap buffer overflow", + "A read or a write past the bounds of a heap allocation, " + "observed while the program ran.", + "high"}}, + {"stack-buffer-overflow", + {"Stack buffer overflow", + "A read or a write past the bounds of a stack object, " + "observed while the program ran.", + "high"}}, + {"heap-use-after-free", + {"Use of freed heap memory", + "A read or a write to heap memory after it was freed, " + "observed while the program ran.", + "high"}}, + {"double-free", + {"Double free", "Heap memory freed a second time, observed while the program ran.", + "high"}}, + {"memory-leak", + {"Memory leak", "Heap memory the program allocated and had not freed when it ended.", + "high"}}, + {"vtable-null-this", + {"Virtual call through a null object", + "A virtual call made through a null pointer to an object.", "high"}}, + {"vtable-use-after-free", + {"Virtual call through a freed object", + "A virtual call made through an object whose memory was already freed.", "high"}}, + {"vtable-corrupted", + {"Virtual call through a corrupted vtable", + "A virtual call made through an object whose vtable pointer is missing or " + "invalid.", + "high"}}, + {"vcall-invalid-target", + {"Virtual call to non-executable memory", + "A virtual call whose target address is not in executable memory.", "high"}}, + {"vtable-type-mismatch", + {"Object of an unexpected dynamic type", + "At a virtual call, the object's dynamic type differs from its static type, or " + "comes from another module: a possible type confusion, or a legitimate cast.", + "medium"}}, + {"vtable-diagnostic", + {"Other virtual call issue", + "A vtable or virtual call problem the runtime reported without a more specific " + "rule, such as a vtable without type information, as built with -fno-rtti.", + "low"}}, + }; + for (const DescribedRule& described : kRules) + { + if (described.rule == rule) + { + return described.description; + } + } + return std::nullopt; + } + std::vector ParseFindings(std::string_view output) { std::vector findings; diff --git a/src/findings.hpp b/src/findings.hpp index 678799a..004e461 100644 --- a/src/findings.hpp +++ b/src/findings.hpp @@ -4,6 +4,7 @@ #include "runtime_analyzer.hpp" +#include #include #include @@ -11,6 +12,18 @@ namespace coretrace::runtime_analyzer { // The memory errors the CoreTrace runtime reported in `output`, without ANSI escapes. [[nodiscard]] std::vector ParseFindings(std::string_view output); + + // What a rule checks, for a SARIF consumer to explain the rule's findings, and how sure a + // finding of it is: "high" for a fault observed while the program ran, lower for an inference. + struct RuleDescription + { + std::string_view title; + std::string_view description; + std::string_view confidence; + }; + + // The description of `rule`; nothing for a rule this analyzer does not report. + [[nodiscard]] std::optional DescribeRule(std::string_view rule); } // namespace coretrace::runtime_analyzer #endif // CORETRACE_RUNTIME_ANALYZER_FINDINGS_HPP diff --git a/src/sarif.cpp b/src/sarif.cpp index 45371da..c7beb11 100644 --- a/src/sarif.cpp +++ b/src/sarif.cpp @@ -1,10 +1,14 @@ // SPDX-License-Identifier: Apache-2.0 #include "sarif.hpp" +#include "findings.hpp" + #include #include #include +#include +#include #include namespace coretrace::runtime_analyzer @@ -27,13 +31,32 @@ namespace coretrace::runtime_analyzer }; } + // A rule as the log lists it: its id, and its title and description when it has them. + [[nodiscard]] llvm::json::Object Rule(const std::string& rule) + { + llvm::json::Object listed{{"id", rule}}; + if (const std::optional described = DescribeRule(rule)) + { + listed["shortDescription"] = + llvm::json::Object{{"text", std::string(described->title)}}; + listed["fullDescription"] = + llvm::json::Object{{"text", std::string(described->description)}}; + } + return listed; + } + [[nodiscard]] llvm::json::Object Result(const Finding& finding) { + llvm::json::Object properties{{"cwe", finding.cwe}}; + if (const std::optional described = DescribeRule(finding.rule)) + { + properties["confidence"] = std::string(described->confidence); + } llvm::json::Object result{ {"ruleId", finding.rule}, {"level", finding.severity == Severity::Error ? "error" : "warning"}, {"message", llvm::json::Object{{"text", finding.message}}}, - {"properties", llvm::json::Object{{"cwe", finding.cwe}}}, + {"properties", std::move(properties)}, }; if (finding.location) { @@ -53,14 +76,21 @@ namespace coretrace::runtime_analyzer void WriteSarif(std::ostream& out, const std::vector& findings) { llvm::json::Array results; + llvm::json::Array rules; + std::set listed; for (const Finding& finding : findings) { results.push_back(Result(finding)); + if (listed.insert(finding.rule).second) + { + rules.push_back(Rule(finding.rule)); + } } llvm::json::Object driver{ {"name", "coretrace-runtime-analyzer"}, {"informationUri", "https://github.com/CoreTrace/coretrace-runtime-analyzer"}, + {"rules", std::move(rules)}, }; llvm::json::Object run{ {"tool", llvm::json::Object{{"driver", std::move(driver)}}}, diff --git a/test/ct_vtable_diag_fake.cpp b/test/ct_vtable_diag_fake.cpp index b8b0f56..1e43e22 100644 --- a/test/ct_vtable_diag_fake.cpp +++ b/test/ct_vtable_diag_fake.cpp @@ -22,7 +22,7 @@ int main() } table[0] = nullptr; // offset-to-top - table[1] = nullptr; // missing typeinfo + table[1] = nullptr; // missing typeinfo, outranked by "vtable resolve failed" table[2] = nullptr; // fake entry FakeObject obj{}; diff --git a/test/ct_vtable_no_rtti.cpp b/test/ct_vtable_no_rtti.cpp new file mode 100644 index 0000000..9c58cdb --- /dev/null +++ b/test/ct_vtable_no_rtti.cpp @@ -0,0 +1,32 @@ +// SPDX-License-Identifier: Apache-2.0 +// args: --ct-modules=alloc,vtable --ct-vtable-diag -fno-rtti +// expect: rule=vtable-diagnostic cwe=CWE-843 level=warning line=24 +#include + +struct Base +{ + virtual int Value() const + { + return 1; + } +}; + +struct Derived : Base +{ + int Value() const override + { + return 2; + } +}; + +[[gnu::noinline]] int Call(const Base& base) +{ + return base.Value(); +} + +int main() +{ + Derived derived; + std::printf("%d\n", Call(derived)); + return 0; +} diff --git a/unittests/findings_test.cpp b/unittests/findings_test.cpp index a59e9db..e2be70d 100644 --- a/unittests/findings_test.cpp +++ b/unittests/findings_test.cpp @@ -265,6 +265,7 @@ namespace }; constexpr Case cases[] = { {"vptr on freed object", "vtable-use-after-free", "CWE-416", Severity::Error}, + {"missing typeinfo", "vtable-diagnostic", "CWE-843", Severity::Warning}, {"missing typeinfo|vtable resolve failed", "vtable-corrupted", "CWE-843", Severity::Error}, {"no vptr", "vtable-corrupted", "CWE-843", Severity::Error}, @@ -276,6 +277,8 @@ namespace {"no vptr|vptr on freed object", "vtable-use-after-free", "CWE-416", Severity::Error}, {"static!=dynamic type|target in non-exec memory", "vcall-invalid-target", "CWE-843", Severity::Error}, + {"missing typeinfo|target in non-exec memory", "vcall-invalid-target", "CWE-843", + Severity::Error}, }; for (const Case& c : cases) { diff --git a/unittests/sarif_test.cpp b/unittests/sarif_test.cpp index 4a27b40..543c590 100644 --- a/unittests/sarif_test.cpp +++ b/unittests/sarif_test.cpp @@ -4,6 +4,8 @@ #include "expect.hpp" +#include + #include #include #include @@ -29,11 +31,14 @@ namespace { "results": )"; - constexpr std::string_view kTail = R"(, + constexpr std::string_view kDriver = R"(, "tool": { "driver": { "informationUri": "https://github.com/CoreTrace/coretrace-runtime-analyzer", - "name": "coretrace-runtime-analyzer" + "name": "coretrace-runtime-analyzer", + "rules": )"; + + constexpr std::string_view kTail = R"( } } } @@ -42,14 +47,16 @@ namespace } )"; - [[nodiscard]] std::string Log(std::string_view results) + // The log of `results`, its driver listing `rules`. + [[nodiscard]] std::string Log(std::string_view results, std::string_view rules = "[]") { - return std::string(kHead) + std::string(results) + std::string(kTail); + return std::string(kHead) + std::string(results) + std::string(kDriver) + + std::string(rules) + std::string(kTail); } void EmptyLog() { - Expect(Sarif({}) == Log("[]"), "no finding: an empty results array"); + Expect(Sarif({}) == Log("[]"), "no finding: empty results and rules arrays"); } void FindingWithLocations() @@ -80,6 +87,7 @@ namespace "text": "heap-buffer-overflow WRITE of size 4" }, "properties": { + "confidence": "high", "cwe": "CWE-122" }, "relatedLocations": [ @@ -101,7 +109,18 @@ namespace ], "ruleId": "heap-buffer-overflow" } - ])"); + ])", + R"([ + { + "fullDescription": { + "text": "A read or a write past the bounds of a heap allocation, observed while the program ran." + }, + "id": "heap-buffer-overflow", + "shortDescription": { + "text": "Heap buffer overflow" + } + } + ])"); Expect(Sarif({finding}) == expected, "one error: location, allocation site as related location 0, rule, CWE"); } @@ -117,6 +136,7 @@ namespace "text": "leak of 16 bytes" }, "properties": { + "confidence": "high", "cwe": "CWE-401" }, "relatedLocations": [ @@ -137,10 +157,96 @@ namespace ], "ruleId": "memory-leak" } - ])"); + ])", + R"([ + { + "fullDescription": { + "text": "Heap memory the program allocated and had not freed when it ended." + }, + "id": "memory-leak", + "shortDescription": { + "text": "Memory leak" + } + } + ])"); Expect(Sarif({finding}) == expected, "one warning: no locations, no column when the runtime gave none"); } + + // The one run of the log Sarif() writes for `findings`. + [[nodiscard]] llvm::json::Object Run(const std::vector& findings) + { + llvm::Expected log = llvm::json::parse(Sarif(findings)); + if (!log) + { + llvm::consumeError(log.takeError()); + return {}; + } + const llvm::json::Array* runs = log->getAsObject()->getArray("runs"); + return runs != nullptr && !runs->empty() ? *(*runs)[0].getAsObject() : llvm::json::Object{}; + } + + [[nodiscard]] Finding OfRule(std::string rule) + { + return Finding{std::move(rule), "CWE-0", Severity::Error, "m", std::nullopt, std::nullopt}; + } + + // A SARIF consumer (GitHub code scanning, an IDE, CoreTrace's merged log) explains a finding + // from its rule: every rule the analyzer reports is listed once, with a title and a + // description. + void EveryRuleIsDescribed() + { + const std::vector rules = { + "heap-buffer-overflow", "stack-buffer-overflow", "heap-use-after-free", + "double-free", "memory-leak", "vtable-null-this", + "vtable-use-after-free", "vtable-corrupted", "vcall-invalid-target", + "vtable-type-mismatch", "vtable-diagnostic"}; + for (const std::string& rule : rules) + { + llvm::json::Object run = Run({OfRule(rule), OfRule(rule)}); + const llvm::json::Object* driver = run.getObject("tool") != nullptr + ? run.getObject("tool")->getObject("driver") + : nullptr; + const llvm::json::Array* listed = + driver != nullptr ? driver->getArray("rules") : nullptr; + const llvm::json::Object* entry = + listed != nullptr && listed->size() == 1 ? (*listed)[0].getAsObject() : nullptr; + const auto text = [&](llvm::StringRef key) -> llvm::StringRef + { + const llvm::json::Object* message = + entry != nullptr ? entry->getObject(key) : nullptr; + const std::optional value = + message != nullptr ? message->getString("text") : std::nullopt; + return value.value_or(""); + }; + Expect(entry != nullptr && entry->getString("id") == rule && + !text("shortDescription").empty() && !text("fullDescription").empty() && + text("shortDescription") != rule, + rule + ": listed once, with a title and a description"); + } + } + + // How sure a finding is: a fault observed while the program ran is certain; a vtable type + // mismatch may be a legitimate cast; an unrecognized vtable report says little. + void EveryFindingHasAConfidence() + { + const std::vector> expected = { + {"heap-buffer-overflow", "high"}, + {"memory-leak", "high"}, + {"vtable-type-mismatch", "medium"}, + {"vtable-diagnostic", "low"}}; + for (const auto& [rule, confidence] : expected) + { + llvm::json::Object run = Run({OfRule(rule)}); + const llvm::json::Array* results = run.getArray("results"); + const llvm::json::Object* result = + results != nullptr && results->size() == 1 ? (*results)[0].getAsObject() : nullptr; + const llvm::json::Object* properties = + result != nullptr ? result->getObject("properties") : nullptr; + Expect(properties != nullptr && properties->getString("confidence") == confidence, + rule + ": confidence " + confidence); + } + } } // namespace int main() @@ -148,5 +254,7 @@ int main() EmptyLog(); FindingWithLocations(); WarningWithoutLocation(); + EveryRuleIsDescribed(); + EveryFindingHasAConfidence(); return unittests::Finish("sarif_test"); }