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)