Skip to content

A database of security advisories for Hackage #37

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

Conversation

david-christiansen
Copy link
Contributor

@david-christiansen david-christiansen commented Aug 3, 2022

@haskellfoundation/tech-proposals - here's the long-awaited advisory DB proposal.

I've talked to the committee about this before, and the pre-HFTP was posted some time ago. I think that this version takes the various feedback I've gotten so far into account.

@david-christiansen david-christiansen changed the title A proposal for a database of security advisories for Hackage A database of security advisories for Hackage Aug 3, 2022
@Ericson2314
Copy link
Contributor

Ericson2314 commented Aug 13, 2022

My concern expressed in the last meeting is with editing version bounds.

The concern

Suppose one has this today:

build-depends base >= 3 && < 4.18

if base gets security advisories in a bunch of versions (say 3.1.0.0, and 3.6.0.0), and modify the version bound accordingly, we'll end up with:

build-depends base (>= 3 && < 3.1.0.0) || (> 3.1.0.0 && < 3.6.0.0) && (> 3.6.0.0 && < 4.18)

I call bounds like this "Swiss cheese" bounds, because they have a number of "holes" corresponding to the removed versions.

I think editing cabal files like this isn't good --- it makes Cabal files much harder to read, and effectly is just redundantly encoding the vulnerability database in each cabal file.

Philosophically, I consider a security vulnerability a deviation from the intended behavior of the package. ("intended" is a fuzzy notion that is a mix of how authors intend, and how users would reasonably interpret the packages documentation. So no "author intends to pwn your machine" cop out.) Likewise bounds suppose indicate not literally which packages work, but which packages' intentions work. So 3.1.0.0 might have a vuln, but this is not the downstream package's "fault", what 3.1.0.0 was supposed to be is still a valid debt, and so the downstream package is within its rights to have it in the bound.

What about other languages?

The GitHub docs say that packages and lock files (to use the term for freeze files from elsewhere) can both be edited, but in practice (Rust, Javascript, Go), I've only seen Dependabot issues and pull requests for lock files. Lock files are supposed to pin specific versions, and so modifying them feels more appropriate (they are a post version solving source of truth), and also doesn't lead to bloat.

So I would be fine with doing issues/pull requests for our Freeze file, but not (at least by default) for regular cabal files.

What about when there are no freeze files?

Lock files are much more commonplace in other languages, however. So the above policy would mean that Haskell projects would be getting a lot fewer issues and pull requests. Is that good?

I think it is fine. As discussed it above, I think it should not be downstream package's responsibility to complicate their bounds when upstream packages deviate from their intent. They should be able to write the bounds assuming upstream packages didn't screw up.

Instead, it is the solver's responsibility to ensure that vulnerable or otherwise blacklisted packages are not chosen as part of the solved concrete plan.

We can implement this with the new "imported project files" feature. We can either generate a cabal project file blacklisting packages with vulns, or we can just teach cabal-install how to parse the vuln db directory, interpreting it as such an imported project file. Then, we can simply change cabal-install to do this by default (users can opt-out of course).

With that, no modifying bounds is necessary! The solver will still ensure bad package are not chosen whether the bounds are "turned into swiss cheese" or not.


Bottom line is I think this is a good idea and we should do it, but I want to start very conservative about what pull requests and even issue dependent opens.

Such automations establish norms, and I don't think a norm of everyone indivudally policing their bounds based on the same source of information is good. We can always change our mind and enable such issues / pull requests later without issue, but conversely stopping once the norm is establish will be much more fraught.

Let's establish a beachhead of having the vulnerability and linting freeze files --- which I don't think is controversial to anyone --- and only afterwords with this solid first step in place discuss if we want any additional linting.

@gbaz
Copy link
Collaborator

gbaz commented Aug 14, 2022

Here are two ideas that came up in addressing the "swiss cheese bounds" concern.

  1. A proposal I had was that the security advisory repo could also include auto-generated constraint files (perhaps at different sorts of threat models -- i.e. network or non-network, etc). These would just consist of cabal.project files which had constraints excluding library versions with security advisories filed against them. That way, end-users could have their cabal.project files include remote imports of these advisory files, to automatically pick up appropriate constraints (one way of preventing the "swiss cheese bounds" john describes above).

  2. Additionally, davean had kicked around something perhaps more sophisticated, making use of the CVSS classification attached to each advisory. I think we would want to make the CVSS field mandatory rather than optional (but noting that the advisory committee could help with it -- i.e. individual submitters who couldn't figure it out could ask for help in their draft PR). Then, somewhere (where?) a project could specify that only a certain filter on advisories should apply to it, and this could be made use of by dependabot (with some tweaking) and also by other sorts of integration, including native cabal integration, at some future point. To specify this I feel like we'd need A) a grammar for filters on cvss classifications and B) a proposal where to put such a filter (an x-field in a cabal file? a new file?), and of course to then build out appropriate support.

The first proposal would make avoiding advisories with bounds not pollute cabal files. The second would reduce the number of advisories to be avoided by narrowing in only on those applicable. The first of the two seems easier, and I think both can be sketched in the proposal but neither is necessarily a blocker (in terms of full spec) for passing the proposal. Though, regardless, I do think we should make CVSS non-optional.

@david-christiansen
Copy link
Contributor Author

Part of the process of accepting an advisory should include trying to deprecate the version in question, unless it's something that only affects certain use cases. That will at least reduce the "Swiss cheese" problem here.

@gbaz, are you aware of any other tooling or other communities that use CVSS in the way that we talked about? Do we have existing notations we can adopt that express what kinds of CVSS classifications are relevant to a given project?

@gbaz
Copy link
Collaborator

gbaz commented Aug 15, 2022

Nope, not aware of any other tooling. Note that a CVSS vector looks like this: AV:N/AC:H/PR:H/UI:R/S:U/C:N/I:N/A:N so we could just have a filter on components. I think adding the CVSS filter as just like an independent file is ideal, since this would then be a general-purpose contribution to dependabot with no haskell specificity.

I imagine we also might want to have simply a "base score" filter that would allow filtering for the overall score, ranging between 0 and 10.

@m4dc4p
Copy link

m4dc4p commented Aug 31, 2022

I found the background on freeze files / lock files (and discussion further on with recommendations) confusing. The proposal should limit itself to the database of vulnerabilites and the associated file format.

In particular, all I could think was "what about nix???", when reading the freeze file section(s). On re-reading I realized the proposal wasn't about those tools at all.

However, for future proposals, I think a common format representing dependencies should be defined, so tools can process them without needing to parse cabal, stack, or package.yaml files.

In any case this is very exciting! I hope to see it happen.

@gbaz
Copy link
Collaborator

gbaz commented Aug 31, 2022

I found the background on freeze files / lock files (and discussion further on with recommendations) confusing. The proposal should limit itself to the database of vulnerabilites and the associated file format.

I think we can't do that. We need to provide some recommendations as to practices for using the db. Otherwise the db is just a random entity with no particular intended purpose. The standards we have are precisely in cabal.project files, cabal freeze files, and stack files. Vis a vis nix, that turns into a general question for security advisories for nix, which is, thankfully, not our problem.

I think the question of a "common format representing dependencies" is out of scope. In a sense we do have an ultimate one, which is the cabal grammar. However, dependencies are always relative to some notion of a system -- on their own they aren't meaningful. As such they will always be presented within some given format.

@blackheaven
Copy link

In particular, all I could think was "what about nix???", when reading the freeze file section(s). On re-reading I realized the proposal wasn't about those tools at all.

I had the same thought but, as mentioned, we could either let nixpkgs contributors deal with it, or add an utility or an argument to generate a lock file too.

@silky
Copy link
Contributor

silky commented Sep 8, 2022

Though, regardless, I do think we should make CVSS non-optional.

I guess one thing that's a bit unfortunate about doing this is that it makes the barrier to reporting vulnerabilities higher; and depending on who's doing the reporting, might make it too high to even bother reporting.

Maybe this needs to go a bit hand-in-hand with the team of volunteers that, as you say, upon a vague PR describing a security bug, they could help investigate and help calculate a CVSS?

@@ -138,6 +138,8 @@ We recommend that consumers who do not use the Cabal API to process build and fr

We additionally recommend that tools prioritize `.cabal` files, followed by freeze files, followed by consulting `stack.yaml` to retrieve the constraints of a Stackage resolver. This recommendation is because virtually all Haskell projects have `.cabal` files (some of which are generated by tools such as `hpack`), and because Stackage sets can be parsed using the same tools as Cabal freeze files.

Tools that automatically update bounds to exclude packages that have advisories should only do so on freeze files. Otherwise, we are likely to end up with many holes in the allowed dependency versions of intermediate transitive dependencies, which could lead to fragmentation, difficult-to-read build configurations, and complicated dependency errors. Projects without freeze files will ideally be presented only with a warning, and left to react appropriately themselves.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏻 ; fwiw, I think dependabot needs more features along these lines anyway; I already get too many notifications from it. I.e. I agree this is basically an out-of-scope consideration that dependabot themselves need to think about.

@@ -146,6 +148,8 @@ To begin with, the HF executive team will assemble a group of five volunteers, w

We expect that the work done by this team will occur mostly asynchronously, but we plan to have meetings a few times per year in order to have discussions about how the group is working and how it can be improved.

Because CVSS is a bit complex, PRs without CVSS will be provisionally accepted, and the volunteers will work with reporters to help them fill out the field appropriately.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worth linking this here - https://nvd.nist.gov/vuln-metrics/cvss/v3-calculator

@david-christiansen
Copy link
Contributor Author

We held a vote on the mailing list for acceptance. The following members voted in favor:

@eviefp did not vote either way.

This is a clear majority, so the proposal is accepted and we can move on to implementation. Thank you all for the discussion and inputs!

@david-christiansen david-christiansen merged commit 4da5ac3 into haskellfoundation:main Sep 27, 2022
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

Successfully merging this pull request may close these issues.

6 participants