From 18020c308238849509207852c7a3b5d18f894012 Mon Sep 17 00:00:00 2001 From: Arthur Sonzogni Date: Wed, 10 Sep 2025 14:49:01 +0000 Subject: [PATCH] Support distinct handling and configuration for DCHECK failures Separates `DCHECK` failures from standard `CHECK` failures to enable granular severity assessment and issue tracking policies. In Chromium, `DCHECK` failures often carry different security and priority implications than production `CHECK` failures. While they may not always be treated as immediate security vulnerabilities, they present information disclosure risks if filed publicly. Current logic groups them together, preventing distinct visibility rules. Detailed changes: - **Stack Parsing:** Updates `stacktraces` regex constants to explicitly distinguish "DCHECK failed" from "Check failed/NOTREACHED", assigning the distinct crash type `DCHECK failure`. - **Security Implications:** Introduces the `DCHECKS_HAVE_SECURITY_IMPLICATION` environment variable to control whether DCHECKs are flagged as security issues per-fuzzer. - **Policy Engine:** Refactors `IssueTrackerPolicy` to support recursive configuration application. This allows nested conditions (e.g., `all` -> `non_security` -> `dcheck`) to apply specific labels, access limits, or priority levels based on the intersection of crash traits. This decouple the configuration depth from the code, enabling arbitrary nesting or rules and simplifying the addition of future condition types. Bug: https://issues.chromium.org/issues/406667202 --- .../crash_analysis/crash_analyzer.py | 13 +- .../_internal/issue_management/issue_filer.py | 4 +- .../issue_management/issue_tracker_policy.py | 80 ++++--- .../tests/appengine/libs/issue_filer_test.py | 216 ++++++++++++++---- .../stack_parsing/stack_analyzer_test.py | 11 +- src/clusterfuzz/stacktraces/__init__.py | 6 + src/clusterfuzz/stacktraces/constants.py | 5 +- 7 files changed, 251 insertions(+), 84 deletions(-) diff --git a/src/clusterfuzz/_internal/crash_analysis/crash_analyzer.py b/src/clusterfuzz/_internal/crash_analysis/crash_analyzer.py index d1573c295a4..bef0cd92959 100644 --- a/src/clusterfuzz/_internal/crash_analysis/crash_analyzer.py +++ b/src/clusterfuzz/_internal/crash_analysis/crash_analyzer.py @@ -320,16 +320,21 @@ def is_security_issue(crash_stacktrace, crash_type, crash_address): return True if crash_type == 'CHECK failure': - # TODO(ochang): Remove this once we pick up newer builds that distinguish - # DCHECKs from CHECKs. checks_have_security_implication = environment.get_value( 'CHECKS_HAVE_SECURITY_IMPLICATION', False) return checks_have_security_implication - # Debug CHECK failure should be marked with security implications. - if crash_type in ('Security DCHECK failure', 'DCHECK failure'): + # Security DCHECKs are always security issues. + if crash_type == 'Security DCHECK failure': return True + # For regular DCHECKs, projects can choose whether or not a particular fuzzer + # job treats them as security issues. + if crash_type == 'DCHECK failure': + dchecks_have_security_implication = environment.get_value( + 'DCHECKS_HAVE_SECURITY_IMPLICATION', True) + return dchecks_have_security_implication + # Hard crash, explicitly enforced in code. if (crash_type == 'Fatal error' or crash_type == 'Unreachable code' or crash_type.endswith('Exception') or crash_type.endswith('CHECK failure')): diff --git a/src/clusterfuzz/_internal/issue_management/issue_filer.py b/src/clusterfuzz/_internal/issue_management/issue_filer.py index 5001a338e09..05783ef08c7 100644 --- a/src/clusterfuzz/_internal/issue_management/issue_filer.py +++ b/src/clusterfuzz/_internal/issue_management/issue_filer.py @@ -323,7 +323,9 @@ def file_issue(testcase, is_crash = not utils.sub_string_exists_in(NON_CRASH_TYPES, testcase.crash_type) properties = policy.get_new_issue_properties( - is_security=testcase.security_flag, is_crash=is_crash) + is_security=testcase.security_flag, + is_crash=is_crash, + crash_type=testcase.crash_type) issue = issue_tracker.new_issue() issue.title = data_handler.get_issue_summary(testcase) diff --git a/src/clusterfuzz/_internal/issue_management/issue_tracker_policy.py b/src/clusterfuzz/_internal/issue_management/issue_tracker_policy.py index 0210b4c929a..dca0849e4a6 100644 --- a/src/clusterfuzz/_internal/issue_management/issue_tracker_policy.py +++ b/src/clusterfuzz/_internal/issue_management/issue_tracker_policy.py @@ -146,62 +146,80 @@ def fallback_policy_message(self): """Get the fallback policy message, if it exists.""" return self._data.get('fallback_policy_message') - def get_new_issue_properties(self, is_security, is_crash): + def get_new_issue_properties(self, is_security, is_crash, crash_type): """Get the properties to apply to a new issue.""" policy = NewIssuePolicy() - if 'all' in self._data: - self._apply_new_issue_properties(policy, self._data['all'], is_crash) + properties = { + 'is_security': is_security, + 'is_crash': is_crash, + 'is_dcheck': 'DCHECK failure' in crash_type + } + self._apply_new_issue_properties(policy, self._data.get('all'), + **properties) + + # Legacy support: support top-level 'security' and 'non_security' keys. New + # configs should use the 'all' section and add 'security' and + # 'non_security' subsections within that. if is_security: - if 'security' in self._data: - self._apply_new_issue_properties(policy, self._data['security'], - is_crash) + self._apply_new_issue_properties(policy, self._data.get('security'), + **properties) else: - if 'non_security' in self._data: - self._apply_new_issue_properties(policy, self._data['non_security'], - is_crash) + self._apply_new_issue_properties(policy, self._data.get('non_security'), + **properties) return policy - def _apply_new_issue_properties(self, policy, issue_type, is_crash): + def _apply_new_issue_properties(self, policy, data, **properties): """Apply issue policies.""" - if not issue_type: + if not data: return - if 'status' in issue_type: - policy.status = self._data['status'][issue_type['status']] + if 'status' in data: + policy.status = self._data['status'][data['status']] - if 'ccs' in issue_type: - policy.ccs.extend(issue_type['ccs']) + if 'ccs' in data: + policy.ccs.extend(data['ccs']) - labels = issue_type.get('labels') + labels = data.get('labels') if labels: policy.labels.extend(_to_str_list(labels)) - issue_body_footer = issue_type.get('issue_body_footer') + issue_body_footer = data.get('issue_body_footer') if issue_body_footer: policy.issue_body_footer = issue_body_footer - if is_crash: - crash_labels = issue_type.get('crash_labels') - if crash_labels: - policy.labels.extend(_to_str_list(crash_labels)) - else: - non_crash_labels = issue_type.get('non_crash_labels') - if non_crash_labels: - policy.labels.extend(_to_str_list(non_crash_labels)) - - for k, v in self.get_extension_fields(issue_type).items(): + for k, v in self.get_extension_fields(data).items(): policy.extension_fields[k] = v + # Handle conditions recursively: + children_conditions = { + 'security': properties.get('is_security', None) is True, + 'non_security': properties.get('is_security', None) is False, + 'crash': properties.get('is_crash', None) is True, + 'non_crash': properties.get('is_crash', None) is False, + 'dcheck': properties.get('is_dcheck', None) is True, + 'non_dcheck': properties.get('is_dcheck', None) is False, + } + for child, condition in children_conditions.items(): + if condition: + self._apply_new_issue_properties(policy, data.get(child), **properties) + + # Legacy support: support "crash_label" and "non_crash_label" keys. + # New configs should use the "crash" and "non_crash" sections instead and + # a "labels" list within those sections. + if properties.get('is_crash', None) is False and 'crash_label' in data: + policy.labels.append(str(data['crash_label'])) + if properties.get('is_crash', None) is True and 'non_crash_label' in data: + policy.labels.append(str(data['non_crash_label'])) + def get_existing_issue_properties(self): """Get the properties to apply to a new issue.""" policy = NewIssuePolicy() - - if 'existing' in self._data: - self._apply_new_issue_properties(policy, self._data['existing'], False) - + properties = {} # No recursive properties for existing issues. + self._apply_new_issue_properties(policy, self._data['existing'], + **properties) return policy diff --git a/src/clusterfuzz/_internal/tests/appengine/libs/issue_filer_test.py b/src/clusterfuzz/_internal/tests/appengine/libs/issue_filer_test.py index 365d32f5e5d..ff15e105d6f 100644 --- a/src/clusterfuzz/_internal/tests/appengine/libs/issue_filer_test.py +++ b/src/clusterfuzz/_internal/tests/appengine/libs/issue_filer_test.py @@ -49,12 +49,28 @@ }, 'all': { 'status': 'new', - 'labels': ['ClusterFuzz', 'Stability-%SANITIZER%'] - }, - 'non_security': { - 'labels': ['Type-Bug'], - 'crash_labels': ['Stability-Crash', 'Pri-1'], - 'non_crash_labels': ['Pri-2'] + 'labels': ['ClusterFuzz', 'Stability-%SANITIZER%'], + 'non_security': { + 'labels': ['Type-Bug'], + 'non_crash': { + 'labels': ['Pri-2'], + }, + 'crash': { + 'labels': ['Stability-Crash', 'Pri-1'], + 'dcheck': { + '_ext_issue_access_limit': { + 'access_limit': IssueAccessLevel.LIMIT_VIEW_TRUSTED + } + }, + }, + }, + 'security': { + 'labels': ['Type-Bug-Security'], + '_ext_collaborators': ['superman@krypton.com', 'batman@gotham.com'], + '_ext_issue_access_limit': { + 'access_limit': IssueAccessLevel.LIMIT_VIEW + } + }, }, 'labels': { 'ignore': 'ClusterFuzz-Ignore', @@ -71,13 +87,6 @@ 'unreproducible': 'Unreproducible', 'restrict_view': 'Restrict-View-SecurityTeam' }, - 'security': { - 'labels': ['Type-Bug-Security'], - '_ext_collaborators': ['superman@krypton.com', 'batman@gotham.com'], - '_ext_issue_access_limit': { - 'access_limit': IssueAccessLevel.LIMIT_VIEW - } - }, 'existing': { 'labels': ['Stability-%SANITIZER%'] }, @@ -95,12 +104,19 @@ }, 'all': { 'status': 'new', - 'labels': ['ClusterFuzz', 'Stability-%SANITIZER%'] - }, - 'non_security': { - 'labels': ['Type-Bug'], - 'crash_labels': ['Stability-Crash', 'Pri-1'], - 'non_crash_labels': ['Pri-2'] + 'labels': ['ClusterFuzz', 'Stability-%SANITIZER%'], + 'non_security': { + 'labels': ['Type-Bug'], + 'crash_labels': ['Stability-Crash', 'Pri-1'], + 'non_crash_labels': ['Pri-2'] + }, + 'security': { + 'labels': ['Type-Bug-Security'], + '_ext_collaborators': ['superman@krypton.com', 'batman@gotham.com'], + '_ext_issue_access_limit': { + 'access_limit': IssueAccessLevel.LIMIT_VIEW + } + }, }, 'labels': { 'ignore': 'ClusterFuzz-Ignore', @@ -117,13 +133,6 @@ 'unreproducible': 'Unreproducible', 'restrict_view': 'Restrict-View-SecurityTeam' }, - 'security': { - 'labels': ['Type-Bug-Security'], - '_ext_collaborators': ['superman@krypton.com', 'batman@gotham.com'], - '_ext_issue_access_limit': { - 'access_limit': IssueAccessLevel.LIMIT_VIEW - } - }, 'existing': { 'labels': ['Stability-%SANITIZER%'] }, @@ -145,12 +154,23 @@ }, 'all': { 'status': 'new', - 'labels': ['1679217', 'Stability-%SANITIZER%'] - }, - 'non_security': { - 'labels': ['Type-Bug'], - 'crash_labels': ['Stability-Crash', 'Pri-1'], - 'non_crash_labels': ['Pri-2'] + 'labels': ['1679217', 'Stability-%SANITIZER%'], + 'non_security': { + 'labels': ['Type-Bug'], + 'non_crash': { + 'labels': ['Pri-2'], + }, + 'crash': { + 'labels': ['Stability-Crash', 'Pri-1'], + } + }, + 'security': { + 'labels': ['Type-Bug-Security'], + '_ext_collaborators': ['superman@krypton.com', 'batman@gotham.com'], + '_ext_issue_access_limit': { + 'access_limit': IssueAccessLevel.LIMIT_VIEW + } + }, }, 'labels': { 'ignore': '1', @@ -167,13 +187,6 @@ 'unreproducible': '11', 'restrict_view': '12' }, - 'security': { - 'labels': ['Type-Bug-Security'], - '_ext_collaborators': ['superman@krypton.com', 'batman@gotham.com'], - '_ext_issue_access_limit': { - 'access_limit': IssueAccessLevel.LIMIT_VIEW - } - }, 'existing': { 'labels': ['Stability-%SANITIZER%'], 'ext_collaborators': ['user1@google.com'] @@ -315,10 +328,13 @@ 'This information can help downstream consumers.\n\n' 'If you need to contact the OSS-Fuzz team with a question, ' 'concern, or any other feedback, please file an issue at ' - 'https://github.com/google/oss-fuzz/issues.' - }, - 'non_security': { - 'labels': ['Type-Bug'] + 'https://github.com/google/oss-fuzz/issues.', + 'non_security': { + 'labels': ['Type-Bug'] + }, + 'security': { + 'labels': ['Type-Bug-Security'] + }, }, 'labels': { 'ignore': 'ClusterFuzz-Ignore', @@ -335,9 +351,6 @@ 'unreproducible': 'Unreproducible', 'restrict_view': 'Restrict-View-Commit' }, - 'security': { - 'labels': ['Type-Bug-Security'] - }, 'deadline_policy_message': 'This bug is subject to a 90 day disclosure deadline. If 90 days ' 'elapse\n' @@ -964,6 +977,117 @@ def test_additional_crash_categories(self): self.assertNotIn('Resource Exhaustion', issue_tracker._itm.last_issue.labels) + def test_filed_issues_nesting(self): + """Tests issue filing for dchecks.""" + self.mock.get.return_value = issue_tracker_policy.IssueTrackerPolicy({ + 'all': { + 'status': 'new', + 'labels': ['ClusterFuzz'], + 'dcheck': { + 'labels': ['Is-DCHECK'], + 'security': { + 'labels': ['Is-Security-DCHECK'], + }, + 'non_security': { + 'labels': ['Is-Non-Security-DCHECK'], + } + }, + 'non_dcheck': { + 'labels': ['Is-Non-DCHECK'], + 'security': { + 'labels': ['Is-Security-Non-DCHECK'], + }, + 'non_security': { + 'labels': ['Is-Non-Security-Non-DCHECK'], + } + } + }, + 'status': { + 'assigned': 'Assigned', + 'duplicate': 'Duplicate', + 'verified': 'Verified', + 'new': 'Untriaged', + 'wontfix': 'WontFix', + 'fixed': 'Fixed' + }, + 'labels': { + 'ignore': 'ClusterFuzz-Ignore', + 'verified': 'ClusterFuzz-Verified', + 'security_severity': 'Security_Severity-%SEVERITY%', + 'needs_feedback': 'Needs-Feedback', + 'invalid_fuzzer': 'ClusterFuzz-Invalid-Fuzzer', + 'reported': None, + 'wrong': 'ClusterFuzz-Wrong', + 'fuzz_blocker': 'Fuzz-Blocker', + 'reproducible': 'Reproducible', + 'auto_cc_from_owners': 'ClusterFuzz-Auto-CC', + 'os': 'OS-%PLATFORM%', + 'unreproducible': 'Unreproducible', + 'restrict_view': 'Restrict-View-SecurityTeam' + }, + }) + issue_tracker = monorail.IssueTracker(IssueTrackerManager('chromium')) + + # +DCHECK +SECURITY + self.testcase1.crash_type = 'DCHECK failure' + self.testcase1.security_flag = True + self.testcase1.put() + issue_filer.file_issue(self.testcase1, issue_tracker) + self.assertIn('Is-DCHECK', issue_tracker._itm.last_issue.labels) + self.assertNotIn('Is-Non-DCHECK', issue_tracker._itm.last_issue.labels) + self.assertIn('Is-Security-DCHECK', issue_tracker._itm.last_issue.labels) + self.assertNotIn('Is-Security-Non-DCHECK', + issue_tracker._itm.last_issue.labels) + self.assertNotIn('Is-Non-Security-DCHECK', + issue_tracker._itm.last_issue.labels) + self.assertNotIn('Is-Non-Security-Non-DCHECK', + issue_tracker._itm.last_issue.labels) + + # +DCHECK -SECURITY + self.testcase1.crash_type = 'DCHECK failure' + self.testcase1.security_flag = False + self.testcase1.put() + issue_filer.file_issue(self.testcase1, issue_tracker) + self.assertIn('Is-DCHECK', issue_tracker._itm.last_issue.labels) + self.assertNotIn('Is-Non-DCHECK', issue_tracker._itm.last_issue.labels) + self.assertNotIn('Is-Security-DCHECK', issue_tracker._itm.last_issue.labels) + self.assertNotIn('Is-Security-Non-DCHECK', + issue_tracker._itm.last_issue.labels) + self.assertIn('Is-Non-Security-DCHECK', + issue_tracker._itm.last_issue.labels) + self.assertNotIn('Is-Non-Security-Non-DCHECK', + issue_tracker._itm.last_issue.labels) + + # -DCHECK +SECURITY + self.testcase1.crash_type = 'Heap-use-after-free' + self.testcase1.security_flag = True + self.testcase1.put() + issue_filer.file_issue(self.testcase1, issue_tracker) + self.assertNotIn('Is-DCHECK', issue_tracker._itm.last_issue.labels) + self.assertIn('Is-Non-DCHECK', issue_tracker._itm.last_issue.labels) + self.assertIn('Is-Security-Non-DCHECK', + issue_tracker._itm.last_issue.labels) + self.assertNotIn('Is-Security-DCHECK', issue_tracker._itm.last_issue.labels) + self.assertNotIn('Is-Non-Security-DCHECK', + issue_tracker._itm.last_issue.labels) + self.assertNotIn('Is-Non-Security-Non-DCHECK', + issue_tracker._itm.last_issue.labels) + + # -DCHECK -SECURITY + self.testcase1.crash_type = 'Heap-use-after-free' + self.testcase1.security_flag = False + self.testcase1.put() + issue_filer.file_issue(self.testcase1, issue_tracker) + self.assertNotIn('Is-DCHECK', issue_tracker._itm.last_issue.labels) + self.assertIn('Is-Non-DCHECK', issue_tracker._itm.last_issue.labels) + self.assertNotIn('Is-Security-DCHECK', issue_tracker._itm.last_issue.labels) + self.assertNotIn('Is-Security-Non-DCHECK', + issue_tracker._itm.last_issue.labels) + self.assertIn('Is-Non-Security-Non-DCHECK', + issue_tracker._itm.last_issue.labels) + self.assertNotIn('Is-Non-Security-DCHECK', + issue_tracker._itm.last_issue.labels) + class MemoryToolLabelsTest(unittest.TestCase): """Memory tool labels tests.""" diff --git a/src/clusterfuzz/_internal/tests/core/crash_analysis/stack_parsing/stack_analyzer_test.py b/src/clusterfuzz/_internal/tests/core/crash_analysis/stack_parsing/stack_analyzer_test.py index 0b31ee12a5e..df0267a82a8 100644 --- a/src/clusterfuzz/_internal/tests/core/crash_analysis/stack_parsing/stack_analyzer_test.py +++ b/src/clusterfuzz/_internal/tests/core/crash_analysis/stack_parsing/stack_analyzer_test.py @@ -2325,14 +2325,23 @@ def test_check_failure_chrome(self): def test_dcheck_failure_chrome(self): """Test a DCHECK failure with a Chrome symbolized stacktrace.""" data = self._read_test_data('dcheck_failure_chrome.txt') - expected_type = 'CHECK failure' + expected_type = 'DCHECK failure' expected_address = '' expected_state = ('!terminated_ in latency_info.cc\n' 'ui::LatencyInfo::AddLatencyNumberWithTimestampImpl\n' 'ui::LatencyInfo::AddLatencyNumberWithTimestamp\n') expected_stacktrace = data + + # Test with DCHECKS_HAVE_SECURITY_IMPLICATION unset. + environment.set_value('DCHECKS_HAVE_SECURITY_IMPLICATION', False) expected_security_flag = False + self._validate_get_crash_data(data, expected_type, expected_address, + expected_state, expected_stacktrace, + expected_security_flag) + # Test with DCHECKS_HAVE_SECURITY_IMPLICATION set. + environment.set_value('DCHECKS_HAVE_SECURITY_IMPLICATION', True) + expected_security_flag = True self._validate_get_crash_data(data, expected_type, expected_address, expected_state, expected_stacktrace, expected_security_flag) diff --git a/src/clusterfuzz/stacktraces/__init__.py b/src/clusterfuzz/stacktraces/__init__.py index 2924f117f0b..d1cf55b0001 100644 --- a/src/clusterfuzz/stacktraces/__init__.py +++ b/src/clusterfuzz/stacktraces/__init__.py @@ -1033,8 +1033,14 @@ def parse(self, stacktrace: str) -> CrashInfo: # Check failures. self.update_state_on_check_failure(state, line, GOOGLE_LOG_FATAL_REGEX, 'Fatal error') + + # Chrome self.update_state_on_check_failure( state, line, CHROME_CHECK_FAILURE_REGEX, 'CHECK failure') + self.update_state_on_check_failure( + state, line, CHROME_DCHECK_FAILURE_REGEX, 'DCHECK failure') + + # Google self.update_state_on_check_failure( state, line, GOOGLE_CHECK_FAILURE_REGEX, 'CHECK failure') diff --git a/src/clusterfuzz/stacktraces/constants.py b/src/clusterfuzz/stacktraces/constants.py index 05275a71e1a..b382c894716 100644 --- a/src/clusterfuzz/stacktraces/constants.py +++ b/src/clusterfuzz/stacktraces/constants.py @@ -87,7 +87,10 @@ CFI_NODEBUG_ERROR_MARKER_REGEX = re.compile( r'CFI: Most likely a control flow integrity violation;.*') CHROME_CHECK_FAILURE_REGEX = re.compile( - r'\s*\[[^\]]*[:]([^\](]*\([0-9]+\)|[^\]:]*[:][0-9]+).*\].*(?:Check failed:|DCHECK failed:|NOTREACHED hit.)\s*(.*)' # pylint: disable=line-too-long + r'\s*\[[^\]]*[:]([^\](]*\([0-9]+\)|[^\]:]*[:][0-9]+).*\].*(?:Check failed:|NOTREACHED hit.)\s*(.*)' # pylint: disable=line-too-long +) +CHROME_DCHECK_FAILURE_REGEX = re.compile( + r'\s*\[[^\]]*[:]([^\](]*\([0-9]+\)|[^\]:]*[:][0-9]+).*\].*(?:DCHECK failed:)\s*(.*)' # pylint: disable=line-too-long ) CHROME_STACK_FRAME_REGEX = re.compile( r'[ ]*(#(?P[0-9]+)[ ]' # frame id (2)