Improve sarif (#10211)

Improve docs, fix bugs, add tests.

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 %}
 ```
 

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):

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:

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)
 
 

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
 

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)