Impact analysis of fixed events - Discrepancy between specification and implementation

[RomainLefeuvre]

Describe the bug

The OSV.dev implementation of impact analysis diverges from the specification for fixed events. They seem to be treated like limit events, differently from the example in the specification.

To Reproduce

1. clone https://github.com/RomainLefeuvre/osv.dev
2. setup your python virtualenv as described in the osv documentation
3. from the root level of the repo, execute python -m osv.analyze_tool --format json "./osv/osv_bug/vuln.json"
4. it prints the vulnerable commits:

❯ python -m  osv.analyze_tool --format json  "./osv/osv_bug/vuln.json"
INFO:root:Analyzing ./osv/osv_bug/vuln.json
INFO:root:Cloning https://github.com/RomainLefeuvre/osv_issue_minimal_example.git to /tmp/tmpakfa5gj5
INFO:root:Finding equivalent regress commit to d241812d2722d573a7b096d44d139946d8dcb484 in refs/remotes/origin/main in https://github.com/RomainLefeuvre/osv_issue_minimal_example.git
INFO:root:Finding equivalent fix commit to 80f15009d903ac95ffc5a5a07a3a213e4980bb62 in refs/remotes/origin/main in https://github.com/RomainLefeuvre/osv_issue_minimal_example.git
INFO:root:Getting commits d241812d2722d573a7b096d44d139946d8dcb484..80f15009d903ac95ffc5a5a07a3a213e4980bb62 from https://github.com/RomainLefeuvre/osv_issue_minimal_example.git
AnalyzeResult(has_changes=False, commits={'868d891cffe96cd67b2abac82c62ade7219af9b5', 'd241812d2722d573a7b096d44d139946d8dcb484', 'e54222cfdedd86a37dc37d999ebc63dccf3fc9da', 'ec5e313170f68d3fc575d107a8b92d43ae140249'})
INFO:root:No changes required.

I.e.:

• 868d891cffe96cd67b2abac82c62ade7219af9b5 (C)
• d241812d2722d573a7b096d44d139946d8dcb484 (X)
• e54222cfdedd86a37dc37d999ebc63dccf3fc9da (A)
• ec5e313170f68d3fc575d107a8b92d43ae140249 (B)

Note that :

• The history of the repo at https://github.com/RomainLefeuvre/osv_issue_minimal_example matches the example in the specification:
  

• The toy vulnerability matches also the one used in the example :

{
  "id": "test",
  "details": "test",
  "modified": "2024-01-10T08:29:31.368677Z",
  "published": "2024-01-10T08:29:31.368677Z",
  "affected": [
    {
      "package": {
        "name": "test",
        "ecosystem": "PyPI",
        "purl": "test"
      },
      "ranges": [
        {
          "type": "GIT",
          "repo": "https://github.com/RomainLefeuvre/osv_issue_minimal_example.git",
          "events": [
            {
              "introduced": "d241812d2722d573a7b096d44d139946d8dcb484"
            },
            {
              "fixed": "80f15009d903ac95ffc5a5a07a3a213e4980bb62"
            }
          ]
        }
  ],
  "schema_version": "1.6.0"
}
  ]
}

Expected behaviour
D, F and E should also be reported as vulnerable as described:

Without an explicit limit, the list of computed affected commits will be X, A, B, C, D, E, F. This is the desired behaviour in most cases.

[oliverchang]

Thanks for reporting this!

You are right, and this is because of a TODO in our implementation here:

osv.dev/osv/impact.py

Line 180 in 6587285

# TODO(ochang): Remove this check. This behaviour should only be keyed

If you pass --detect_cherrypicks true, you should get the correct result.

The reasoning behind this is we wanted to be more cautious about surfacing potential false positive matches to our consumers. We keyed this behaviour on --detect_cherrypicks true to reduce the possiblity of false positives while we evaluate the quality of our existing sources by doing this best effort cherrypick detection before we explore all branches.

[RomainLefeuvre]

Thanks again for your really quick answer.

Thanks for this precision, I understand the reason behind the implementation choice and the need to reduce false positive that can be costly for the end user.

If you pass --detect_cherrypicks true, you should get the correct result.

Indeed, in this specific case with the cherry-picked option to true we obtained the correct result.

However, I noticed that for last_affected event the implementation is not satisfying the specification even with the cherry_picked option set to true.

The reason behind that is that for last_affected events, the cherry-pick mechanism is disabled resulting in the same case that I described previously described
6622506

To illustrate this example I created a third vulnerability in my fork, replacing the fixed event by a last_affected event pointing to the same commit.

https://github.com/RomainLefeuvre/osv.dev/blob/66225062b983dcfaa986620076064311548441db/osv/osv_bug/vuln_3.json#L1C1-L30C2

"events": [
            {
              "introduced": "d241812d2722d573a7b096d44d139946d8dcb484" [X]
            },
            {
              "last_affected": "80f15009d903ac95ffc5a5a07a3a213e4980bb62" [Y]
            }
          ]

Result

• 'd241812d2722d573a7b096d44d139946d8dcb484' [X]
• '868d891cffe96cd67b2abac82c62ade7219af9b5' [C]
• 'e54222cfdedd86a37dc37d999ebc63dccf3fc9da' [A]
• '80f15009d903ac95ffc5a5a07a3a213e4980bb62' [Y]
• 'ec5e313170f68d3fc575d107a8b92d43ae140249' [B]

Expected Results
The previous commit + D, F, E

[RomainLefeuvre]

If you need more details about how to reproduce the bug with the last_affected event I can create another issue with the different steps. Isn't this a different bug?

Thanks again.

Romain Lefeuvre

[RomainLefeuvre]

Hello,
I just checked and the --detect_cherrypicks true do not correct the problem related to multiple non-overalpping ranges on the same branch :
#1898

Thanks!
Romain Lefeuvre

[oliverchang]

Hello, I just checked and the --detect_cherrypicks true do not correct the problem related to multiple non-overalpping ranges on the same branch : #1898

Thanks! Romain Lefeuvre

Just to clarify: is this the same issue with last_affected, or is there something else?

[RomainLefeuvre]

I continued the discussion here as you suggest in #1898 but in my opinion, these are two different bugs.

[RomainLefeuvre]

Hi, Do you prefer that I open a dedicated issue for this second bug ?
Sincerly
Romain

[oliverchang]

Hi, Do you prefer that I open a dedicated issue for this second bug ? Sincerly Romain

Sorry for the slow reply! Please open another bug to make it clear what the second issue is.

[RomainLefeuvre]

I just created a new issue here : #1938

Thanks again