diff --git a/CHANGELOG.md b/CHANGELOG.md index 68859ce..56586b3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,9 @@ Notable changes are grouped by release. See [all releases](https://github.com/Co - **SARIF:** a rule of the merged log carries the title, description and documentation link its tool gives, so that GitHub code scanning can explain the finding; flawfinder's rule titles and CWE links come first, the analyzers' when they describe their rules. +- **SARIF:** the related locations and the confidence a tool writes in its SARIF reach the merged + log (the runtime analyzer's allocation site, the Python analyzer's sink), and a result's + message links to its related locations, which GitHub code scanning then shows as links. ## v0.85.0 diff --git a/CMakeLists.txt b/CMakeLists.txt index 9ac4562..0e9dde6 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -870,6 +870,12 @@ add_test(NAME ctrace_dyn_findings_merge_with_static_ones WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}) # A finding the runtime analyzer reports without a location keeps none: no file is made up for # it, and the run does not fail on it. +# The allocation of a heap overflow, which the runtime analyzer gives as a related location, and +# the finding's confidence reach the merged log, whose message links to the allocation. +set(ctrace_runtime_related_sarif ${CMAKE_BINARY_DIR}/runtime-related.sarif) +add_test(NAME ctrace_sarif_keeps_the_runtime_allocation_site + COMMAND sh -c "rm -f '${ctrace_runtime_related_sarif}'; $ --sarif-format --fail-on none --dyn --config ${CMAKE_BINARY_DIR}/fake-runtime-analyzer.json --input tests/runtime/heap_overflow.c --report-file '${ctrace_runtime_related_sarif}' > /dev/null 2>&1; grep -q '\"allocated here\"' '${ctrace_runtime_related_sarif}' && grep -q '(\\[allocated here\\](0))' '${ctrace_runtime_related_sarif}' && grep -q '\"confidence\": \"high\"' '${ctrace_runtime_related_sarif}'" + WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}) file(WRITE ${CMAKE_BINARY_DIR}/fake-runtime-no-location.json "{\"schema_version\": 1, \"tools\": {\"coretrace-runtime-analyzer\": {\"path\": \"${CMAKE_SOURCE_DIR}/tests/fake-runtime-analyzer.sh\", \"args\": [\"--fake-no-location\"]}}}\n") set(ctrace_no_location_sarif ${CMAKE_BINARY_DIR}/no-location.sarif) diff --git a/README.md b/README.md index f147b75..46f7bb2 100644 --- a/README.md +++ b/README.md @@ -362,6 +362,13 @@ runtime analyzers do not describe their rules yet [coretrace-runtime-analyzer#29](https://github.com/CoreTrace/coretrace-runtime-analyzer/issues/29)): their rules keep their id alone. +A finding keeps the other places it involves and how sure its tool is, whichever tool reports +it: the related locations (`relatedLocations`, such as the other access of a data race or the +allocation of a heap overflow), which the message links to so that code scanning shows them as +links, and the confidence (`properties.confidence`, `low`, `medium` or `high`). The stack +analyzer's confidence, a number, is not carried yet +([coretrace-stack-analyzer#244](https://github.com/CoreTrace/coretrace-stack-analyzer/issues/244)). + A weakness reported by several tools, same file, same line and same CWE, is one result: the most severe report is kept in its tool's run, and its `alsoReportedBy` property names the other tools and rules. Reports without a CWE cannot be matched and are all kept; the console output diff --git a/include/Process/Tools/InputPaths.hpp b/include/Process/Tools/InputPaths.hpp index f81d837..a7d5e9d 100644 --- a/include/Process/Tools/InputPaths.hpp +++ b/include/Process/Tools/InputPaths.hpp @@ -87,9 +87,10 @@ namespace ctrace std::filesystem::path workingDirectory_; }; - /// Spells the file of every finding as InputPaths::display() does, for a tool that reports - /// the files it was given its own way: cppcheck and tscancode rewrite the paths of their file - /// list, the runtime analyzer names its source. Resolves nothing when there is no finding. + /// Spells the file of every finding, and of each place it involves, as InputPaths::display() + /// does, for a tool that reports the files it was given its own way: cppcheck and tscancode + /// rewrite the paths of their file list, the runtime analyzer names its source. Resolves + /// nothing when there is no finding. inline void respellFindings(std::vector& diagnostics, const std::vector& inputs) { @@ -101,6 +102,13 @@ namespace ctrace for (Diagnostic& diagnostic : diagnostics) { diagnostic.file = paths.display(diagnostic.file); + for (RelatedLocation& related : diagnostic.relatedLocations) + { + if (!related.file.empty()) + { + related.file = paths.display(related.file); + } + } } } } // namespace ctrace diff --git a/src/Process/Tools/PythonAnalyzerToolImplementation.cpp b/src/Process/Tools/PythonAnalyzerToolImplementation.cpp index 029c552..f13eaa2 100644 --- a/src/Process/Tools/PythonAnalyzerToolImplementation.cpp +++ b/src/Process/Tools/PythonAnalyzerToolImplementation.cpp @@ -66,6 +66,10 @@ namespace ctrace return diagnostics; } const InputPaths paths(inputs); + // The analyzer locates findings, and the places they involve, relative to the project + // root it was given. + const auto locate = [&root](const std::string& file) + { return fs::path(file).is_relative() ? root / file : fs::path(file); }; std::vector kept; for (Diagnostic& diagnostic : *diagnostics) { @@ -73,10 +77,14 @@ namespace ctrace { continue; } - // The analyzer locates findings relative to the project root it was given. - const fs::path located = fs::path(diagnostic.file).is_relative() - ? root / diagnostic.file - : fs::path(diagnostic.file); + for (RelatedLocation& related : diagnostic.relatedLocations) + { + if (!related.file.empty()) + { + related.file = paths.display(locate(related.file)); + } + } + const fs::path located = locate(diagnostic.file); if (auto input = paths.asInput(located)) { diagnostic.file = *input; diff --git a/src/Process/Tools/Sarif.cpp b/src/Process/Tools/Sarif.cpp index 3f05150..86eee48 100644 --- a/src/Process/Tools/Sarif.cpp +++ b/src/Process/Tools/Sarif.cpp @@ -4,6 +4,7 @@ #include #include +#include #include #include #include @@ -131,6 +132,95 @@ namespace ctrace return described; } + /// The text of a SARIF message without its embedded links (SARIF 2.1.0 §3.11.6): each + /// `[text](id)` becomes its text, and the brackets and backslashes escaped around them + /// become themselves. The merged log links the message to its related locations again. + [[nodiscard]] std::string plainText(std::string_view text) + { + std::string plain; + for (std::size_t at = 0; at < text.size(); ++at) + { + const char c = text[at]; + if (c == '\\' && at + 1 < text.size() && + (text[at + 1] == '\\' || text[at + 1] == '[' || text[at + 1] == ']')) + { + plain += text[++at]; + continue; + } + if (c == '[') + { + // An embedded link: an unescaped ']' right before '(' and a closing ')'. + std::size_t close = at + 1; + while (close < text.size() && text[close] != ']') + close += text[close] == '\\' ? 2 : 1; + const std::size_t end = close + 1 < text.size() && text[close + 1] == '(' + ? text.find(')', close + 2) + : std::string_view::npos; + if (close < text.size() && end != std::string_view::npos) + { + plain += plainText(text.substr(at + 1, close - at - 1)); + at = end; + continue; + } + } + plain += c; + } + return plain; + } + + /// The confidence a result's properties give, `low`, `medium` or `high` in any case; + /// empty for another value or none. + [[nodiscard]] std::string confidenceOf(const nlohmann::json& result) + { + const auto properties = result.find("properties"); + if (properties == result.end() || !properties->is_object()) + return {}; + const auto confidence = properties->find("confidence"); + if (confidence == properties->end() || !confidence->is_string()) + return {}; + std::string level = confidence->get(); + std::transform(level.begin(), level.end(), level.begin(), + [](unsigned char ch) { return static_cast(std::tolower(ch)); }); + return level == "low" || level == "medium" || level == "high" ? level : std::string(); + } + + /// The related locations of a result: each place, when it has one, and its message. + /// An entry that is not an object is skipped. + [[nodiscard]] std::vector relatedLocationsOf(const nlohmann::json& result) + { + std::vector related; + const auto locations = result.find("relatedLocations"); + if (locations == result.end() || !locations->is_array()) + return related; + for (const nlohmann::json& location : *locations) + { + if (!location.is_object()) + continue; + RelatedLocation place; + const nlohmann::json& physical = + location.value("physicalLocation", nlohmann::json::object()); + if (physical.is_object()) + { + const nlohmann::json& artifact = + physical.value("artifactLocation", nlohmann::json::object()); + const nlohmann::json& region = + physical.value("region", nlohmann::json::object()); + if (artifact.is_object()) + place.file = artifact.value("uri", ""); + if (region.is_object()) + { + place.line = region.value("startLine", 0U); + place.column = region.value("startColumn", 0U); + } + } + const nlohmann::json& message = location.value("message", nlohmann::json::object()); + if (message.is_object()) + place.message = plainText(message.value("text", "")); + related.push_back(std::move(place)); + } + return related; + } + void readLocation(const nlohmann::json& result, Diagnostic& diagnostic) { const auto locations = result.find("locations"); @@ -179,8 +269,10 @@ namespace ctrace } diagnostic.severity = severityFromLevel(result.value("level", "warning")); diagnostic.message = - result.value("message", nlohmann::json::object()).value("text", ""); + plainText(result.value("message", nlohmann::json::object()).value("text", "")); diagnostic.cwe = cweOf(result, diagnostic.message); + diagnostic.confidence = confidenceOf(result); + diagnostic.relatedLocations = relatedLocationsOf(result); readLocation(result, diagnostic); diagnostics.push_back(std::move(diagnostic)); } @@ -253,6 +345,36 @@ namespace ctrace return physical; } + /// `text` as literal SARIF message text, beside embedded links (SARIF 2.1.0 §3.11.6). + [[nodiscard]] std::string escaped(std::string_view text) + { + std::string out; + for (const char c : text) + { + if (c == '\\' || c == '[' || c == ']') + out += '\\'; + out += c; + } + return out; + } + + /// The message of a result, ending with an embedded link to each related location, which + /// GitHub code scanning shows as a link to the place; unchanged without any. + [[nodiscard]] std::string linkedMessage(const Diagnostic& diagnostic) + { + if (diagnostic.relatedLocations.empty()) + return diagnostic.message; + std::string text = escaped(diagnostic.message) + " ("; + for (std::size_t index = 0; index < diagnostic.relatedLocations.size(); ++index) + { + if (index != 0) + text += ", "; + text += "[" + escaped(diagnostic.relatedLocations[index].message) + "](" + + std::to_string(index) + ")"; + } + return text + ")"; + } + [[nodiscard]] nlohmann::json resultOf(const Diagnostic& diagnostic) { nlohmann::json result; @@ -261,16 +383,18 @@ namespace ctrace result["ruleId"] = diagnostic.ruleId; } result["level"] = levelOf(diagnostic.severity); - result["message"]["text"] = diagnostic.message; + result["message"]["text"] = linkedMessage(diagnostic); if (!diagnostic.file.empty()) { result["locations"] = nlohmann::json::array( {{{"physicalLocation", physicalLocationOf(diagnostic.file, diagnostic.line, diagnostic.column)}}}); } - for (const RelatedLocation& related : diagnostic.relatedLocations) + for (std::size_t index = 0; index < diagnostic.relatedLocations.size(); ++index) { + const RelatedLocation& related = diagnostic.relatedLocations[index]; nlohmann::json location; + location["id"] = index; if (!related.file.empty()) { location["physicalLocation"] = diff --git a/tests/fake-runtime-analyzer.sh b/tests/fake-runtime-analyzer.sh index 54bdbb7..10f8e7c 100755 --- a/tests/fake-runtime-analyzer.sh +++ b/tests/fake-runtime-analyzer.sh @@ -4,7 +4,7 @@ # --format sarif --timeout -o / [args] -- [compiler args] # with the run directory existing and the source last, not starting with `-`, and exits 64 # otherwise. Then it prints the SARIF the real tool prints for the fixtures, with the source -# spelled as it was given: a heap overflow for heap_overflow.c, a double free on line 12 for double_free.c, nothing for +# spelled as it was given, with its confidence: a heap overflow for heap_overflow.c, allocated on line 6, a double free on line 12 for double_free.c, nothing for # others. --fake-exit= ends with that code instead (2: could not build or run), # --fake-hang never ends, --fake-no-location reports a leak that has no location. When # CTRACE_FAKE_RUNTIME_LOG is set, the -o path is appended to it. @@ -46,7 +46,8 @@ if [ "$location" = no ]; then fi case "$(basename "$source")" in heap_overflow.c) rule=heap-buffer-overflow cwe=CWE-122 line=7 column=15 - message="heap-buffer-overflow WRITE of size 4" ;; + message="heap-buffer-overflow WRITE of size 4" + related=', "relatedLocations": [{"id": 0, "message": {"text": "allocated here"}, "physicalLocation": {"artifactLocation": {"uri": "'"$source"'"}, "region": {"startLine": 6, "startColumn": 19}}}]' ;; double_free.c) rule=double-free cwe=CWE-415 line=12 column=5 message="double free" ;; *) rule= ;; esac @@ -63,10 +64,10 @@ cat < items = + diagnosticsFromSarif(log, "tool").value_or(std::vector{}); + report.expect(items.size() == 4 && items[0].relatedLocations.size() == 1 && + items[0].relatedLocations[0].file == "a.c" && + items[0].relatedLocations[0].line == 6 && + items[0].relatedLocations[0].column == 19 && + items[0].relatedLocations[0].message == "allocated here", + "sarif: a result's related locations reach the model"); + report.expect(items.size() == 4 && items[0].message == "a[0] overflows (allocated here)", + "sarif: the message drops its embedded links and escapes"); + report.expect(items.size() == 4 && items[0].confidence == "high" && + items[1].confidence == "medium" && items[2].confidence.empty(), + "sarif: the confidence is read in any case; another value is ignored"); + report.expect(items.size() == 4 && items[3].relatedLocations.size() == 1 && + items[3].relatedLocations[0].file.empty(), + "sarif: a related location that is not an object is skipped, one " + "without a place kept without it"); + } + // A rule a tool describes in its own SARIF keeps its description through the model, as // flawfinder writes it (a title and a link, no long description). A text that only repeats // the rule's id, as the Python analyzer writes it, says nothing and is dropped; a rule the @@ -402,6 +445,34 @@ int main(int argc, char** argv) "renderSarif: no related location and no confidence are left out"); } + // GitHub code scanning links a result to its related locations when the message names them: + // each gets its id, and the message ends with an embedded link to each, its own brackets + // and backslashes escaped. A result without related locations keeps its message. + { + Diagnostic overflow{"coretrace-runtime-analyzer", + "heap-buffer-overflow", + "a.c", + 7, + 15, + Severity::Error, + "values[4] overflows", + "CWE-122"}; + const nlohmann::json bare = renderSarif({overflow})["runs"][0]["results"][0]; + overflow.relatedLocations = {{"a.c", 6, 19, "allocated here"}, + {"a.c", 8, 5, "freed [here]"}}; + nlohmann::json linked = renderSarif({overflow})["runs"][0]["results"][0]; + report.expect(linked["relatedLocations"][0]["id"] == 0 && + linked["relatedLocations"][1]["id"] == 1, + "renderSarif: each related location has its id"); + report.expect(linked["message"]["text"] == + "values\\[4\\] overflows ([allocated here](0), [freed \\[here\\]](1))", + "renderSarif: the message links to each related location"); + report.expect(bare["message"]["text"] == "values[4] overflows", + "renderSarif: a message without related locations is left as it is"); + report.expect(linked["partialFingerprints"] == bare["partialFingerprints"], + "renderSarif: the links change no fingerprint"); + } + // A rule its tool describes is described in the log, so that a SARIF consumer (GitHub code // scanning) can explain the finding; a rule without a description keeps its id alone. The // results, their fingerprints included, do not change. @@ -622,6 +693,53 @@ int main(int argc, char** argv) "spelled absolute"); } + // The places a finding involves are relative to the project root too: they are resolved + // from it and spelled as the main location is, or a link of the merged log would open + // another file (a project outside the working directory). + { + const std::string sarif = R"json({"version": "2.1.0", "runs": [{"results": [ + {"ruleId": "code-injection", "level": "error", "message": {"text": "flow"}, + "locations": [{"physicalLocation": { + "artifactLocation": {"uri": "app/main.py", "uriBaseId": "SRCROOT"}, + "region": {"startLine": 8, "startColumn": 5}}}], + "relatedLocations": [ + {"id": 0, "message": {"text": "sink"}, "physicalLocation": { + "artifactLocation": {"uri": "app/helpers.py", "uriBaseId": "SRCROOT"}, + "region": {"startLine": 2, "startColumn": 12}}}, + {"id": 1, "message": {"text": "again"}, "physicalLocation": { + "artifactLocation": {"uri": "app/main.py", "uriBaseId": "SRCROOT"}, + "region": {"startLine": 3, "startColumn": 1}}}]}]}]})json"; + const auto kept = PythonAnalyzerToolImplementation::parseDiagnostics( + sarif, "/elsewhere/proj", {"/elsewhere/proj/app/main.py"}); + const bool one = + kept.has_value() && kept->size() == 1 && (*kept)[0].relatedLocations.size() == 2; + report.expect(one && + (*kept)[0].relatedLocations[0].file == "/elsewhere/proj/app/helpers.py", + "coretrace-python-analyzer: a related location is resolved from the project " + "root"); + report.expect(one && (*kept)[0].relatedLocations[1].file == "/elsewhere/proj/app/main.py", + "coretrace-python-analyzer: a related location in an input is spelled as " + "the input"); + } + + // The tools that respell their findings as the inputs respell the places they involve too. + { + Diagnostic finding{"coretrace-runtime-analyzer", + "heap-buffer-overflow", + "-F.c", + 7, + 15, + Severity::Error, + "overflow", + ""}; + finding.relatedLocations = {{"-F.c", 6, 19, "allocated here"}}; + std::vector findings{finding}; + respellFindings(findings, {"./-F.c"}); + report.expect(findings[0].file == "./-F.c" && + findings[0].relatedLocations[0].file == "./-F.c", + "respellFindings: a related location is spelled as the input"); + } + // Through a stand-in executable with the real tool's contract (it only accepts a project // directory, like the real one in project mode). {