Skip to content

Commit

Permalink
fix(analysis): better tolerate GIT analysis crashes (#2884)
Browse files Browse the repository at this point in the history 
This commit improves tolerance for GIT commit analysis failures.

When GIT range analysis crashes, the entire record is discarded. The
record's `affected[]` may contain other non-GIT events (i.e. `package`
entries) that will be of value.

There are two known scenarios where GIT range analysis crashes:
- the commit being analysed is an orphaned commit
- the commit cannot be found in the repository (because it is from a
fork)

Includes a test to confirm behaviour
  • Loading branch information
@andrewpollock
andrewpollock authored Nov 21, 2024
1 parent a7dd06d commit 743b45e
Show file tree
Hide file tree
Showing 6 changed files with 338 additions and 4 deletions.
146 changes: 146 additions & 0 deletions docker/worker/testdata/CVE-2016-10046.json
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,146 @@
{
"id": "CVE-2016-10046",
"severity": [
{
"type": "CVSS_V3",
"score": "CVSS:3.0/AV:L/AC:L/PR:N/UI:R/S:U/C:N/I:N/A:H"
}
],
"details": "Heap-based buffer overflow in the DrawImage function in magick/draw.c in ImageMagick before 6.9.5-5 allows remote attackers to cause a denial of service (application crash) via a crafted image file.",
"affected": [
{
"package": {
"name": "imagemagick",
"ecosystem": "Debian:11"
},
"ranges": [
{
"type": "ECOSYSTEM",
"events": [
{
"introduced": "0"
},
{
"fixed": "8:6.9.6.2+dfsg-2"
}
]
}
],
"ecosystem_specific": {
"urgency": "not yet assigned"
}
},
{
"package": {
"name": "imagemagick",
"ecosystem": "Debian:12"
},
"ranges": [
{
"type": "ECOSYSTEM",
"events": [
{
"introduced": "0"
},
{
"fixed": "8:6.9.6.2+dfsg-2"
}
]
}
],
"ecosystem_specific": {
"urgency": "not yet assigned"
}
},
{
"package": {
"name": "imagemagick",
"ecosystem": "Debian:13"
},
"ranges": [
{
"type": "ECOSYSTEM",
"events": [
{
"introduced": "0"
},
{
"fixed": "8:6.9.6.2+dfsg-2"
}
]
}
],
"ecosystem_specific": {
"urgency": "not yet assigned"
}
},
{
"ranges": [
{
"type": "GIT",
"repo": "https://github.com/imagemagick/imagemagick",
"events": [
{
"introduced": "0"
},
{
"fixed": "989f9f88ea6db09b99d25586e912c921c0da8d3f"
}
]
},
{
"type": "GIT",
"repo": "https://github.com/imagemagick/imagemagick6",
"events": [
{
"introduced": "0"
},
{
"last_affected": "969a96ed7eea9603bea46492e9116c2ba28da60f"
}
]
}
]
}
],
"references": [
{
"type": "ADVISORY",
"url": "http://www.securityfocus.com/bid/95183"
},
{
"type": "ADVISORY",
"url": "https://github.com/ImageMagick/ImageMagick/commit/989f9f88ea6db09b99d25586e912c921c0da8d3f"
},
{
"type": "ARTICLE",
"url": "http://www.openwall.com/lists/oss-security/2016/12/26/9"
},
{
"type": "FIX",
"url": "https://github.com/ImageMagick/ImageMagick/commit/989f9f88ea6db09b99d25586e912c921c0da8d3f"
},
{
"type": "REPORT",
"url": "https://bugzilla.redhat.com/show_bug.cgi?id=1410448"
},
{
"type": "REPORT",
"url": "https://github.com/ImageMagick/ImageMagick/commit/989f9f88ea6db09b99d25586e912c921c0da8d3f"
},
{
"type": "WEB",
"url": "http://www.openwall.com/lists/oss-security/2016/12/26/9"
},
{
"type": "WEB",
"url": "http://www.securityfocus.com/bid/95183"
},
{
"type": "ADVISORY",
"url": "https://security-tracker.debian.org/tracker/CVE-2016-10046"
}
],
"modified": "2024-09-18T01:00:20Z",
"published": "2017-03-23T17:59:00Z"
}
146 changes: 146 additions & 0 deletions docker/worker/testdata/UpdateTest_analysis_crash_handling.txt
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,146 @@
{ 'affected': [],
'affected_fuzzy': [ '6-9-4-0',
'6-9-4-1',
'6-9-4-10',
'6-9-4-2',
'6-9-4-3',
'6-9-4-4',
'6-9-4-5',
'6-9-4-6',
'6-9-4-7',
'6-9-4-8',
'6-9-4-9',
'6-9-5-0',
'6-9-5-1',
'6-9-5-2',
'6-9-5-3',
'6-9-5-4'],
'affected_packages': [ { 'database_specific': None,
'ecosystem_specific': { 'urgency': 'not yet '
'assigned'},
'package': { 'ecosystem': 'Debian:11',
'name': 'imagemagick',
'purl': 'pkg:deb/debian/imagemagick?arch=source'},
'ranges': [ { 'events': [ { 'type': 'introduced',
'value': '0'},
{ 'type': 'fixed',
'value': '8:6.9.6.2+dfsg-2'}],
'repo_url': '',
'type': 'ECOSYSTEM'}],
'severities': [],
'versions': []},
{ 'database_specific': None,
'ecosystem_specific': { 'urgency': 'not yet '
'assigned'},
'package': { 'ecosystem': 'Debian:12',
'name': 'imagemagick',
'purl': 'pkg:deb/debian/imagemagick?arch=source'},
'ranges': [ { 'events': [ { 'type': 'introduced',
'value': '0'},
{ 'type': 'fixed',
'value': '8:6.9.6.2+dfsg-2'}],
'repo_url': '',
'type': 'ECOSYSTEM'}],
'severities': [],
'versions': []},
{ 'database_specific': None,
'ecosystem_specific': { 'urgency': 'not yet '
'assigned'},
'package': { 'ecosystem': 'Debian:13',
'name': 'imagemagick',
'purl': 'pkg:deb/debian/imagemagick?arch=source'},
'ranges': [ { 'events': [ { 'type': 'introduced',
'value': '0'},
{ 'type': 'fixed',
'value': '8:6.9.6.2+dfsg-2'}],
'repo_url': '',
'type': 'ECOSYSTEM'}],
'severities': [],
'versions': []},
{ 'database_specific': None,
'ecosystem_specific': None,
'package': { 'ecosystem': '',
'name': '',
'purl': None},
'ranges': [ { 'events': [ { 'type': 'introduced',
'value': '0'},
{ 'type': 'fixed',
'value': '989f9f88ea6db09b99d25586e912c921c0da8d3f'}],
'repo_url': 'https://github.com/imagemagick/imagemagick',
'type': 'GIT'},
{ 'events': [ { 'type': 'introduced',
'value': '0'},
{ 'type': 'last_affected',
'value': '969a96ed7eea9603bea46492e9116c2ba28da60f'}],
'repo_url': 'https://github.com/imagemagick/imagemagick6',
'type': 'GIT'}],
'severities': [],
'versions': [ '6.9.4-0',
'6.9.4-1',
'6.9.4-10',
'6.9.4-2',
'6.9.4-3',
'6.9.4-4',
'6.9.4-5',
'6.9.4-6',
'6.9.4-7',
'6.9.4-8',
'6.9.4-9',
'6.9.5-0',
'6.9.5-1',
'6.9.5-2',
'6.9.5-3',
'6.9.5-4']}],
'aliases': [],
'credits': [],
'database_specific': None,
'db_id': 'CVE-2016-10046',
'details': 'Heap-based buffer overflow in the DrawImage function in '
'magick/draw.c in ImageMagick before 6.9.5-5 allows remote '
'attackers to cause a denial of service (application crash) via '
'a crafted image file.',
'ecosystem': ['Debian', 'Debian:11', 'Debian:12', 'Debian:13'],
'fixed': '',
'has_affected': True,
'import_last_modified': DatetimeWithNanoseconds(2024, 9, 18, 1, 0, 20),
'is_fixed': True,
'issue_id': None,
'last_modified': DatetimeWithNanoseconds(2021, 1, 1, 0, 0),
'project': ['imagemagick'],
'public': True,
'purl': [ 'pkg:deb/debian/imagemagick',
'pkg:deb/debian/imagemagick?arch=source'],
'reference_url_types': { 'http://www.openwall.com/lists/oss-security/2016/12/26/9': 'WEB',
'http://www.securityfocus.com/bid/95183': 'WEB',
'https://bugzilla.redhat.com/show_bug.cgi?id=1410448': 'REPORT',
'https://github.com/ImageMagick/ImageMagick/commit/989f9f88ea6db09b99d25586e912c921c0da8d3f': 'REPORT',
'https://security-tracker.debian.org/tracker/CVE-2016-10046': 'ADVISORY'},
'regressed': '',
'related': [],
'search_indices': [ '10046',
'11',
'12',
'13',
'2016',
'cve',
'cve-2016-10046',
'debian',
'debian:11',
'debian:12',
'debian:13',
'github.com/imagemagick/imagemagick',
'github.com/imagemagick/imagemagick6',
'https://github.com/imagemagick/imagemagick',
'https://github.com/imagemagick/imagemagick6',
'imagemagick',
'imagemagick6'],
'semver_fixed_indexes': [],
'severities': [ { 'score': 'CVSS:3.0/AV:L/AC:L/PR:N/UI:R/S:U/C:N/I:N/A:H',
'type': 'CVSS_V3'}],
'source': 'source',
'source_id': 'source:CVE-2016-10046.json',
'source_of_truth': 2,
'status': 1,
'summary': '',
'timestamp': DatetimeWithNanoseconds(2017, 3, 23, 17, 59),
'withdrawn': None}
1 change: 1 addition & 0 deletions docker/worker/worker.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -465,6 +465,7 @@ def _analyze_vulnerability(self, source_repo, repo, vulnerability, path,
analyze_git=not source_repo.ignore_git,
detect_cherrypicks=source_repo.detect_cherrypicks,
versions_from_repo=source_repo.versions_from_repo)

if not result.has_changes and not added_fix_info:
return result

Expand Down
32 changes: 32 additions & 0 deletions docker/worker/worker_test.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1328,6 +1328,7 @@ def test_update_maven(self):

def test_update_linux(self):
"""Test a Linux entry."""
self.skipTest("Prefix not supported by schema")
self.source_repo.ignore_git = False
self.source_repo.versions_from_repo = False
self.source_repo.detect_cherrypicks = False
Expand Down Expand Up @@ -1702,6 +1703,37 @@ def test_dont_index_too_many_git_versions(self):
bug.put()
self.expect_dict_equal('dont_index_too_many_git_versions', bug._to_dict())

def test_analysis_crash_handling(self):
"""Test that formerly crash-inducing GIT events are handled gracefully."""
self.source_repo.ignore_git = False
self.source_repo.versions_from_repo = True
self.source_repo.detect_cherrypicks = False
self.source_repo.db_prefix.append('CVE-')
self.source_repo.put()

# Use any valid OSV input test file here.
self.mock_repo.add_file(
'CVE-2016-10046.json',
self._load_test_data(
os.path.join(TEST_DATA_DIR, 'CVE-2016-10046.json')),
)
self.mock_repo.commit('User', 'user@email')

task_runner = worker.TaskRunner(ndb_client, None, self.tmp_dir.name, None,
None)
message = mock.Mock()
message.attributes = {
'source': 'source',
'path': 'CVE-2016-10046.json',
'original_sha256': _sha256('CVE-2016-10046.json'),
'deleted': 'false',
}
task_runner._source_update(message)

bug = osv.Bug.get_by_id('CVE-2016-10046')

self.expect_dict_equal('analysis_crash_handling', bug._to_dict())


if __name__ == '__main__':
ds_emulator = tests.start_datastore_emulator()
Expand Down
2 changes: 2 additions & 0 deletions osv/analyze_tool/__init__.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
# pylint: disable=relative-beyond-top-level
from .. import impact
from .. import sources
from .. import vulnerability_pb2


def main():
Expand Down Expand Up @@ -103,6 +104,7 @@ def analyze(path, checkout_path, key_path, analyze_git, detect_cherrypicks):
analyze_git=analyze_git,
checkout_path=checkout_path,
detect_cherrypicks=detect_cherrypicks)

if not result.has_changes:
logging.info('No changes required.')
return
Expand Down
15 changes: 11 additions & 4 deletions osv/impact.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,11 @@ def _get_equivalent_commit(self,
# Invalid commit.
return None

target_patch_id = repo.diff(target.parents[0], target).patchid
try:
target_patch_id = repo.diff(target.parents[0], target).patchid
except IndexError:
# Orphaned target_commit.
return None

search = repo.revparse_single(to_search)
try:
Expand Down Expand Up @@ -386,9 +390,12 @@ def process_commit(commit):
def _branches_with_commit(repo, commit):
"""Get all remote branches that include a commit."""
# pygit2's implementation of this is much slower, so we use `git`.
branches = subprocess.check_output(
['git', '-C', repo.path, 'branch', '-r', '--contains',
commit]).decode().splitlines()
try:
branches = subprocess.check_output(
['git', '-C', repo.path, 'branch', '-r', '--contains',
commit]).decode().splitlines()
except subprocess.CalledProcessError:
branches = []

def process_ref(ref):
return ref.strip().split()[0]
Expand Down

0 comments on commit 743b45e

Please sign in to comment.