From 05eb0af66631ffdfc4cd6e0401ee00c16a07221d Mon Sep 17 00:00:00 2001 From: jonathanmetzman <31354670+jonathanmetzman@users.noreply.github.com> Date: Tue, 2 May 2023 13:14:09 -0400 Subject: [PATCH] Improve sarif (#10211) Improve docs, fix bugs, add tests. --- .../getting-started/continuous_integration.md | 19 ++++- infra/cifuzz/affected_fuzz_targets_test.py | 4 +- infra/cifuzz/fuzz_target.py | 3 +- infra/cifuzz/run_fuzzers.py | 6 +- infra/cifuzz/sarif_utils.py | 20 +++-- infra/cifuzz/sarif_utils_test.py | 78 +++++++++++++++++++ 6 files changed, 114 insertions(+), 16 deletions(-) create mode 100644 infra/cifuzz/sarif_utils_test.py diff --git a/docs/getting-started/continuous_integration.md b/docs/getting-started/continuous_integration.md index 3acf599c8..1a3fb9e3f 100644 --- a/docs/getting-started/continuous_integration.md +++ b/docs/getting-started/continuous_integration.md @@ -23,7 +23,8 @@ If CIFuzz finds a crash, CIFuzz reports the stacktrace, makes the crashing input available for download and the CI test fails (red X). If CIFuzz doesn't find a crash during the allotted time, the CI test passes -(green check). If CIFuzz finds a crash, it reports the crash only if both of following are true: +(green check). If CIFuzz finds a crash, it reports the crash only if both of +following are true: * The crash is reproducible (on the PR/commit build). * The crash does not occur on older OSS-Fuzz builds. (If the crash does occur on older builds, then it was not introduced by the PR/commit @@ -94,7 +95,7 @@ jobs: path: ./out/artifacts # Uncomment this to get results in the GitHub security dashboard. # - name: Upload Sarif - # if: failure() && steps.build.outcome == 'success' + # if: always() && steps.build.outcome == 'success' # uses: github/codeql-action/upload-sarif@v2 # with: # # Path to SARIF file relative to the root of the repository @@ -159,12 +160,22 @@ jobs: language: c++ fuzz-seconds: 600 sanitizer: ${{ matrix.sanitizer }} + # Uncomment this to get results in the GitHub security dashboard. + # output-sarif: true - name: Upload Crash - uses: actions/upload-artifact@v1 - if: failure() && steps.build.outcome == 'success' + uses: actions/upload-artifact@v3 + if: steps.build.outcome == 'success' with: name: ${{ matrix.sanitizer }}-artifacts path: ./out/artifacts + # Uncomment this to get results in the GitHub security dashboard. + # - name: Upload Sarif + # if: always() && steps.build.outcome == 'success' + # uses: github/codeql-action/upload-sarif@v2 + # with: + # # Path to SARIF file relative to the root of the repository + # sarif_file: cifuzz-sarif/results.sarif + # checkout_path: cifuzz-sarif {% endraw %} ``` diff --git a/infra/cifuzz/affected_fuzz_targets_test.py b/infra/cifuzz/affected_fuzz_targets_test.py index 36bc5f893..3197ebbf1 100644 --- a/infra/cifuzz/affected_fuzz_targets_test.py +++ b/infra/cifuzz/affected_fuzz_targets_test.py @@ -38,7 +38,7 @@ TEST_DATA_OUT_PATH = os.path.join(os.path.dirname(os.path.abspath(__file__)), 'test_data', 'build-out') -class RemoveUnaffectedFuzzTargets(unittest.TestCase): +class RemoveUnaffectedFuzzTargetsTest(unittest.TestCase): """Tests remove_unaffected_fuzzers.""" TEST_FUZZER_1 = os.path.join(TEST_DATA_OUT_PATH, 'example_crash_fuzzer') @@ -85,7 +85,7 @@ class RemoveUnaffectedFuzzTargets(unittest.TestCase): self.assertEqual(expected_dir_len, len(os.listdir(tmp_dir))) -class IsFuzzTargetAffected(unittest.TestCase): +class IsFuzzTargetAffectedTest(unittest.TestCase): """Tests for is_fuzz_target_affected.""" def setUp(self): diff --git a/infra/cifuzz/fuzz_target.py b/infra/cifuzz/fuzz_target.py index 345852f2d..2342c22bc 100644 --- a/infra/cifuzz/fuzz_target.py +++ b/infra/cifuzz/fuzz_target.py @@ -19,6 +19,7 @@ import os import shutil import stat import tempfile +from typing import Optional import clusterfuzz.environment import clusterfuzz.fuzz @@ -159,7 +160,7 @@ class FuzzTarget: # pylint: disable=too-many-instance-attributes print(result.logs) return FuzzResult(None, result.logs, self.pruned_corpus_path) - def fuzz(self, batch=False): + def fuzz(self, batch=False) -> Optional[FuzzResult]: """Starts the fuzz target run for the length of time specified by duration. Returns: diff --git a/infra/cifuzz/run_fuzzers.py b/infra/cifuzz/run_fuzzers.py index 19ae04a38..88b3265f5 100644 --- a/infra/cifuzz/run_fuzzers.py +++ b/infra/cifuzz/run_fuzzers.py @@ -149,18 +149,18 @@ class BaseFuzzTargetRunner: # pylint: disable=undefined-loop-variable if not target_path: logging.error('Ran no fuzz targets.') - elif bug_found and self.config.output_sarif: + elif self.config.output_sarif: # TODO(metzman): Handle multiple crashes. write_fuzz_result_to_sarif(result, target_path, self.workspace) self.clusterfuzz_deployment.upload_crashes() return bug_found -def write_fuzz_result_to_sarif(result, target_path, workspace): +def write_fuzz_result_to_sarif(fuzz_result, target_path, workspace): """Write results of fuzzing to SARIF.""" logging.info('Writing sarif results.') workspace.make_repo_for_sarif() - sarif_utils.write_stacktrace_to_sarif(result.stacktrace, target_path, + sarif_utils.write_stacktrace_to_sarif(fuzz_result.stacktrace, target_path, workspace) diff --git a/infra/cifuzz/sarif_utils.py b/infra/cifuzz/sarif_utils.py index 17619f472..2575adaf8 100644 --- a/infra/cifuzz/sarif_utils.py +++ b/infra/cifuzz/sarif_utils.py @@ -13,8 +13,9 @@ # limitations under the License. """Module for outputting SARIF data.""" import copy -import os import json +import logging +import os from clusterfuzz import stacktraces @@ -174,15 +175,17 @@ def get_error_source_info(crash_info): frame = get_error_frame(crash_info) if not frame: return (None, 1) - return frame.filename, int(frame.fileline or 1) + return redact_src_path(frame.filename), int(frame.fileline or 1) def get_rule_index(crash_type): """Returns the rule index describe the rule that |crash_type| ran afoul of.""" # Don't include "READ" or "WRITE" or number of bytes. crash_type = crash_type.split(' ')[0].lower() + logging.info('crash_type: %s.', crash_type) for idx, rule in enumerate(SARIF_RULES): if rule['id'] == crash_type: + logging.info('Rule index: %d.', idx) return idx return get_rule_index('no-crashes') @@ -190,6 +193,10 @@ def get_rule_index(crash_type): def get_sarif_data(stacktrace, target_path): """Returns a description of the crash in SARIF.""" + data = copy.deepcopy(SARIF_DATA) + if stacktrace is None: + return data + fuzz_target = os.path.basename(target_path) stack_parser = stacktraces.StackParser(fuzz_target=fuzz_target, symbolized=True, @@ -197,7 +204,9 @@ def get_sarif_data(stacktrace, target_path): include_ubsan=True) crash_info = stack_parser.parse(stacktrace) error_source_info = get_error_source_info(crash_info) - uri = redact_src_path(error_source_info[0]) + uri = error_source_info[0] + rule_idx = get_rule_index(crash_info.crash_type) + rule_id = SARIF_RULES[rule_idx]['id'] result = { 'level': 'error', @@ -217,10 +226,9 @@ def get_sarif_data(stacktrace, target_path): } } }], - 'ruleId': 'no-crashes', - 'ruleIndex': get_rule_index(crash_info.crash_type) + 'ruleId': rule_id, + 'ruleIndex': rule_idx } - data = copy.deepcopy(SARIF_DATA) data['runs'][0]['results'].append(result) return data diff --git a/infra/cifuzz/sarif_utils_test.py b/infra/cifuzz/sarif_utils_test.py new file mode 100644 index 000000000..b250110a6 --- /dev/null +++ b/infra/cifuzz/sarif_utils_test.py @@ -0,0 +1,78 @@ +# Copyright 2023 Google LLC +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +"""Tests for sarif_utils.py""" +import unittest +from unittest import mock + +import sarif_utils + +CRASH_INFO_FILELINE = 403 + + +class GetSarifDataTest(unittest.TestCase): + """Tests for get_sarif_data.""" + + def setUp(self): + self.maxDiff = None # pylint: disable=invalid-name + + def test_get_sarif_data_none(self): + """Tests get_sarif_data when there was no crash.""" + self.assertEqual(sarif_utils.get_sarif_data(None, '/root/target'), + sarif_utils.SARIF_DATA) + + +class RedactSrcPathTest(unittest.TestCase): + """Tests for redact_src_path.""" + + def test_redact_src_path(self): + """Tests redact_src_path.""" + path = '/src/src-repo/subdir/file' + self.assertEqual(sarif_utils.redact_src_path(path), 'subdir/file') + + +def _get_mock_crash_info(): + """Returns a mock crash_info to be used in tests.""" + stack_frame = mock.MagicMock() + stack_frame.filename = '/src/repo-dir/sub/vuln.cc' + stack_frame.function_name = 'vuln_func' + stack_frame.fileline = CRASH_INFO_FILELINE + crash1_frames = [stack_frame, stack_frame] + frames = [crash1_frames] + crash_info = mock.MagicMock() + crash_info.frames = frames + crash_info.crash_state = 'vuln_func\nvuln_func0\nvuln_func1' + return crash_info + + +class GetErrorSourceInfoTest(unittest.TestCase): + """Tests for get_error_source_info.""" + + def test_redact_src_path(self): + """Tests that get_error_source_info finds the right source info.""" + crash_info = _get_mock_crash_info() + source_info = sarif_utils.get_error_source_info(crash_info) + expected_source_info = ('sub/vuln.cc', CRASH_INFO_FILELINE) + self.assertEqual(source_info, expected_source_info) + + +class GetRuleIndexTest(unittest.TestCase): + """Tests for get_rule_index.""" + CRASH_INFO_CRASH_TYPE = 'Heap-use-after-free READ 8' + + def test_get_rule_index(self): + """Tests that get_rule_index finds the right rule index.""" + index = sarif_utils.get_rule_index(self.CRASH_INFO_CRASH_TYPE) + self.assertEqual(sarif_utils.SARIF_RULES[index]['id'], + 'heap-use-after-free') + self.assertEqual(sarif_utils.get_rule_index('no-crashes'), 0)