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

Semantically tag CVE patches #15660

Closed
domenkozar opened this issue May 24, 2016 · 19 comments
Closed

Semantically tag CVE patches #15660

domenkozar opened this issue May 24, 2016 · 19 comments

Comments

@domenkozar
Copy link
Member

domenkozar commented May 24, 2016

I propose we add cvePatches attribute to mkDerivation.

Motivation

Given recent announcement of vulnix, it's clear we need a way to identify if a specific version of software has patches applied. A lot of times we have to patch existing version, for example unzip has 7 CVE patches on 16.03.

Implementation

  • It should be a list of CVE patches that should be applied in patchPhase. The name of the patch denotes the CVE being patched
  • Stdenv builder would then create $out/nix-support/CVE-BLABLA for each of them. This way CVE scanning software can see what is already applied

Thoughts?

cc @peti @copumpkin @ctheune @edolstra

@vcunat
Copy link
Member

vcunat commented May 24, 2016

  • There were some cases when a CVE patch turns out not to fix the CVE completely, but that's probably rare enough to disregard for this purpose.
  • Speaking of vulnix, it seems to want to work with store paths and not expressions, which brings some additional complications. For example, we would probably have to store the list somewhere in each output.

@ctheune
Copy link
Contributor

ctheune commented May 24, 2016

Yes. We were planning on putting them into environment variables that are passed to the builder.
We'd love to work with expressions (or parse trees or something) but as we're focussing on looking at things installed on a machine we usually do not have access to the expressions.

@ctheune
Copy link
Contributor

ctheune commented May 24, 2016

one interesting note is that we look at the derivation files. those are accidentally parsable as Python (yes, haha) and we're able to introspect that. we could also look into the store paths into nix-support, too. that would actually be much more structured and less hacky than "eval" ing .drv files in Python :)

@vcunat
Copy link
Member

vcunat commented May 24, 2016

If you assume you have the derivation, then in its closure there's also the CVE-xxxx-xxxx.patch.drv or the patch directly (depending on how the expression obtained the patch).

@peti
Copy link
Member

peti commented May 24, 2016

Having the set of applied CVE patches available as a first-order value in the package set seems like overkill to me. Most CVEs are fixed by virtue of a version update, so there's no patch to begin with. And in those cases where a patch does exist, I'd just call the file "cve-2016-xxx.patch" or write the number in a comment behind the patch's file name.

@domenkozar
Copy link
Member Author

@peti but once you lose .drv you have no idea what CVEs were applied. And most of the time in real world, you don't keep .drv files. It's essential to know that at runtime. Creating empty files should add ~0 overhead and huge gain for accountability.

@peti
Copy link
Member

peti commented May 24, 2016 via email

@ctheune
Copy link
Contributor

ctheune commented May 24, 2016

Right. That's ultimately two different perspectives on the issue. However, using nix to its full extent does not promise you that you can point to a specific nixpkgs expression. I can also have something in the store that is reachable from an active root that came from a manual nix-build or other thing where we can't automatically determine the source.

@vcunat
Copy link
Member

vcunat commented May 24, 2016

I'd expect that in the cases you care about CVE a lot you typically also want "track" which nixpkgs revisions you use, as the knowledge is useful for other reasons as well. On the other hand, e.g. Debian always installs even a full changelog.

@peti
Copy link
Member

peti commented May 24, 2016

using nix to its full extent does not promise you that you can point to a specific nixpkgs expression.

I think it does and I think you can. I manage a set of machines that's very diverse, ranging from desktops to servers and laptops, yet I have no trouble telling for every package I've installed what its source is. Granted, you can install packages in a way that's not reproducible, but I sure hope security-conscious people don't do that much.

@ctheune
Copy link
Contributor

ctheune commented May 24, 2016

For me that's not about reproducibility. That's separate from knowing where something came from, right? We're providing a co-managed environment where we support our customers installing custom things in a structured way (yay, nix!). We can't guarantee to follow things in the store back to their expressions, though.

@copumpkin
Copy link
Member

If we can get deterministic (bit-for-bit) builds and NixOS/nix#709 merged in, we could actually tie all builds back to the source that produced them. That's a decent amount of work though 😄

@peti
Copy link
Member

peti commented May 24, 2016

We're providing a co-managed environment where we support our customers installing custom things in a structured way.

It seems to me like the target group for the extension you're suggesting is Nix users who are (a) security conscious and (b) install things into their system where they have no clue what the source code was. I don't know ... that seems like a small set of people. Just my 2 cents.

@Phreedom
Copy link
Member

Phreedom commented May 25, 2016 via email

@ctheune
Copy link
Contributor

ctheune commented May 25, 2016

Heh, let's rephrase, I did mean something different. My point is that technically there will easily be cases where someone did not leave the expression around after something was installed. I don't follow that just because someone is security conscious means that on the system where a package is installed the expression must remain present after it was installed.

@domenkozar
Copy link
Member Author

domenkozar commented May 29, 2016

I'd like to summarize observations so far:

  • CVE doesn't always map to a version bump or a patch

I'd like to see examples of this to really address it. Hopefully the upstream issues a new CVE if existing patch doesn't fix it, if not I don't really see a way to avoid this

  • in general (as Peter noted) we'd like to reason about security based on .drv files where this information is accessible already

In multi-user managed machine (remember, Nix promotes this as a feature, everyone can install any nix expression as a user), we currently don't have a way to track back all built derivations to their source.

Therefore having cvePatches generate $out/nix-support/CVE-XXX would barely add any overhead while giving tools like vulnix a chance to better reason about CVEs. I understand not everyone will benefit from that, but my argument is that there are no cons, just pros.

ckauhaus pushed a commit to nix-community/vulnix that referenced this issue Nov 27, 2017
Inspect the derivation and search for patches named `CVE-YYYY-NNNNN`.
vulnix assumes that these CVE have been fixed in the current version. If
a single patch fixes several CVEs at once, construct a file name that
names all of them.

Of course, patches can be removed if a new program version gets released
which is not known to be affected according to the current CVE database.
So we don't need to keep CVE patches forever. The whole mechanism is
only for fixes which don't change the version number. See `unzip` for
example.

Related to NixOS/nixpkgs#15660
@ckauhaus
Copy link
Contributor

I'd like to pick up the discussion. vulnix is now able to recognize CVE patches if they are named accordingly. I think this presents a practical solution for Domen's first point in the original description.

This means:

  • If a package has published vulnerabilities, but the derivation contains patches that have CVE identifiers in their file names, these vulns are filtered out. Example: unzip
  • If upstream releases a fixed version which is not listed in the CVE database and we update, everything is fine anyway. Example: postgresql
  • If a single patch fixes several CVEs, its file name should contain all of them.
  • If all of the above fails, we can still employ vulnix' whitelist feature NixOS-wide. Note that the whitelist feature is currently not ready for general use.

Comments?

@ckauhaus
Copy link
Contributor

See #39392 for related discussion. We should probably merge the two issues.

@ckauhaus
Copy link
Contributor

I'd propose to close this issue. Discussion on NixCon has shown that including CVE names in patches is a workable practice. This will not cover every case, so the points being discussed in #39392 are still relevant. But we should focus discussion there and not here.

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

No branches or pull requests

7 participants