Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarify use case for last_affected / Should last_affected be used for unpatched vulnerabilities? #150

Closed
Marcono1234 opened this issue Apr 4, 2023 · 7 comments

Comments

@Marcono1234
Copy link

Could you please clarify the expected use case for last_affected?

Currently the GitHub advisory database seems to use it (in some cases?) for unpatched vulnerabilities to describe the currently latest version when the vulnerability was discovered. Is that the intended use case, or was it rather intended for situations where the exact fix version is unknown but the vulnerability cannot be reproduced starting with a certain version, respectively can only be reproduced up to version last_affected?

My concern is that, especially when advisories are created without proper communication with the maintainers (which sadly still happens), new versions after last_affected are released which still contain the vulnerability but the advisory is not adjusted. In that case users believe they are safe even though they are not.

Examples for this usage of last_affected might be GHSA-22wj-vf5f-wrvj (and corresponding discussion in h2database/h2database#3686) and GHSA-p8p7-x288-28g6. Both seem to just contain the back then latest version as last_affected. There are probably more cases of this, maybe even some where indeed the version after last_affected still contained the vulnerability, but finding such advisories might be tedious.

@andrewpollock
Copy link
Collaborator

Hi,

We're going to review the clarity of the text at https://ossf.github.io/osv-schema/#affectedrangesevents-fields

In a nutshell, last_affected sets a ceiling on known to be vulnerable versions, i.e. versions "less than or equal to" this version are to be considered vulnerable and everything after this version is not vulnerable.

We've flagged this issue with GitHub to bring it to their attention. It's quite possible they're erring on the side of false negatives in favour of false positives.

@rhalar
Copy link

rhalar commented Jun 13, 2023

I'm not sure I quite follow how this could be claimed, but perhaps I am not up to date in how this things get reported? In my understanding there is the situation where a vulnerability was found and the maintainer of the package issues a fix which in not part of any release (yet), but will be in the next one. In which case this is correct, the last version affected version will be the one noted in last_affected.

But I don't believe the assumption that the next release will necessarily contain the fix holds? How then does one mark a vulnerability which has no known fix, where each subsequent release should be vulnerable?
This would imply that advisories are kept up to date as new release come out, I didn't quite get the impression they are that well maintained though (understandably so I should add, this also depends on the transparency of the maintainer).

@chrisbloom7
Copy link
Collaborator

chrisbloom7 commented Jun 22, 2023

Hello! I work on that team at GitHub that owns the vulnerability publication work flow, including encoding to OSV, and I have contributed to the OSV spec as part of that work. I wanted to respond about how GitHub uses this attribute when publishing OSV data to our Advisory Database repo.

I am seeing two questions here:

  1. Are GHSAs using that field correctly?
  2. What does that field mean when new versions have been released?

Regarding the first question, we use last_affected when an affected product's range specification includes the <= operator AND there is no known fix. This aligns with the reasoning for its introduction that was outlined in #35. Before that event existed the only way to encode those types of version ranges was with "introduced": 0 combined with a database_specific field that recorded the upper limit for our internal use, but that meant that any automation following the OSV spec would read it as every version was impacted even if a fixed version was eventually released. In order to avoid this scenario last_affected was proposed as a way to encode any known upper limit at the time of publication in the absence of a known fixed version, which we felt was preferable to an unbounded upper limit. Both of the advisories noted in the description above use a <= operator and a version based on the vulnerability as it was described at the time of the import from NVD, neither of which had a known fixed version at the time.

As to the second question, I verified with our curation team that a general assumption is that once a vulnerability has been published it would be unlikely that future versions would continue to be released which still include the vulnerability except in rare circumstances, such as where the advisory is disputed (in which case if wouldn't matter how it was encoded in OSV) or the project was being maintained by a bad actor (in which case there are other steps to follow to remove the package altogether). Our standing is that it is better to flag the versions we know about rather than the ones we don't and then rely on updates from the source (e.g. NVD) or community feedback via our improvement suggestion form if new data becomes available.

I hope this provides some clarification to your questions. Thanks for bringing up your concerns!

@rhalar
Copy link

rhalar commented Jun 22, 2023

Hey, thanks for the clarification! Your answer does define and answer this quite nicely, and it verifies what at least I thought was happening.
I am curious about

...a general assumption is that once a vulnerability has been published it would be unlikely that future versions would continue to be released which still include the vulnerability except in rare circumstances

as not all vulnerabilities are high severity and not all packages are that security focused. I wouldn't be surprised if a not insignificant number of them choose not to address a specific vulnerability until a later release if it is deemed not critical. This is just my hunch though, but I'll will try and dig up some examples.
Once a fixed version does get assigned having both the last_affected and fixed entry could lead to some confusion in the presumably rare case when they are not sequential. At that point the last_affected is better assumed to relate to when the issue was discovered, as I see it.

@Marcono1234
Copy link
Author

Thanks for the clarification!

Would it then make sense to adjust the OSV schema documentation to say something like the following for last_affected:

last_affected is usually set to the latest affected version when an advisory is published and no fixed version exists yet. However, unlike the fixed field it is possible that versions newer than last_affected are actually affected by the vulnerability as well.

And maybe also add a comment in the evaluation pseudocode which currently sets vulnerable = false for v > evt.last_affected, to mention that it might be possible that the application is vulnerable nonetheless?

oliverchang pushed a commit that referenced this issue Jun 23, 2023
This is a first pass at further clarifying the `"last_affected"` field
and addressing #146 and #150.

Preview is available
[here](https://hayleycd.github.io/osv-schema/#requirements)

---------

Signed-off-by: Hayley Denbraver <denbraver@google.com>
Co-authored-by: Chris Bloom <chrisbloom7@github.com>
@chrisbloom7
Copy link
Collaborator

@Marcono1234, yup agreed. Hat tip to @hayleycd for getting the documentation part started and merged in #159. If you'd like to update the pseudocode, that would be a helpful contribution.

oliverchang pushed a commit that referenced this issue Jul 14, 2023
Hopefully wraps up #150 and #146 

View rendered example
[here](https://hayleycd.github.io/osv-schema/#last_affected-vs-fixed-example).

Changes were also made to the [affected.ranges.events
fields](https://hayleycd.github.io/osv-schema/#affectedrangesevents-fields)
to bring the formatting into line with the rest of the document. Fields
were being rendered like this: `"last_affected"` where `last_affected`
is preferred.

---------

Signed-off-by: Hayley Denbraver <denbraver@google.com>
@hayleycd
Copy link
Contributor

I think we can close this given #174 and #159

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants