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

nixpkgs: allow packages to be marked insecure (reverted) #22890

Merged
merged 2 commits into from Feb 23, 2017

Conversation

grahamc
Copy link
Member

@grahamc grahamc commented Feb 17, 2017

If a package's meta has knownVulnerabilities, like so:

stdenv.mkDerivation {
  name = "foobar-1.2.3";

  ...

  meta.knownVulnerabilities = [
    "CVE-0000-00000: remote code execution"
    "CVE-0000-00001: local privilege escalation"
  ];
}

and a user attempts to install the package, they will be greeted with
a warning indicating that maybe they don't want to install it:

error: Package ‘foobar-1.2.3’ in ‘...default.nix:20’ is marked as insecure, refusing to evaluate.

Known issues:

 - CVE-0000-00000: remote code execution
 - CVE-0000-00001: local privilege escalation

You can install it anyway by whitelisting this package, using the
following methods:

a) for `nixos-rebuild` you can add ‘foobar-1.2.3’ to
   `nixpkgs.config.permittedInsecurePackages` in the configuration.nix,
   like so:

     {
       nixpkgs.config.permittedInsecurePackages = [
         "foobar-1.2.3"
       ];
     }

b) For `nix-env`, `nix-build`, `nix-shell` or any other Nix command you can add
‘foobar-1.2.3’ to `permittedInsecurePackages` in
~/.config/nixpkgs/config.nix, like so:

     {
       permittedInsecurePackages = [
         "foobar-1.2.3"
       ];
     }

Adding either of these configurations will permit this specific
version to be installed. A third option also exists:

NIXPKGS_ALLOW_INSECURE=1 nix-build ...

though I specifically avoided having a global file-based toggle to
disable this check. This way, users don't disable it once in order to
get a single package, and then don't realize future packages are
insecure.

Motivation for this change
Things done
  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux

cc @globin @domenkozar @fpletz @edolstra

@mention-bot
Copy link

@grahamc, thanks for your PR! By analyzing the history of the files in this pull request, we identified @edolstra, @codyopel and @svanderburg to be potential reviewers.

@grahamc
Copy link
Member Author

grahamc commented Feb 17, 2017

This is my first time hacking around this close to the core. Please review carefully and leave liberal amounts of feedback. This does work. I'm working on documentation, but wanted to get something up for review sooner than later.

@@ -75,6 +75,12 @@ let
isUnfree (lib.lists.toList attrs.meta.license) &&
!allowUnfreePredicate attrs;

allowInsecurePredicate = x: builtins.elem x.name (config.permittedInsecurePackages or []);
Copy link
Member

Choose a reason for hiding this comment

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

Note that for allowUnfreePredicate the whole point is to allow user to define a more complicated policy (allow all flashplayer but nothing else). It is not described in the error message to avoid confusing users, though.

So I would prefer the same approach here:

  allowInsecurePredicate = x: ((config.allowInsecurePredicate or y: false) x) ||
    (builtins.elem x.name (config.permittedInsecurePackages or []));

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 fixed

@danbst
Copy link
Contributor

danbst commented Feb 17, 2017

Will I get this prompt if I apply fix patch manually (via .override*)?


b) For `nix-env`, `nix-build`, `nix-shell` or any other Nix command you can add
‘${attrs.name or "«name-missing»"}’ to `permittedInsecurePackages` in
~/.nixpkgs/config.nix, like so:
Copy link
Contributor

Choose a reason for hiding this comment

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

since 9d6a55a you can use ~/.config/nixpkgs/config.nix

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 fixed

@grahamc
Copy link
Member Author

grahamc commented Feb 17, 2017

It appears that this code requires you add the exception and then override:

{
  permittedInsecurePackages = [
    "foobar-v0.5"
  ];

  packageOverrides = pkgs: rec {
    foobar2 = pkgs.foobar.overrideAttrs (x: {
      name = "foobar-v2";
      meta.knownVulnerabilities = [];
    });
  };
}

but I can then install foobar2 without a warning.

If a package's meta has `knownVulnerabilities`, like so:

    stdenv.mkDerivation {
      name = "foobar-1.2.3";

      ...

      meta.knownVulnerabilities = [
        "CVE-0000-00000: remote code execution"
        "CVE-0000-00001: local privilege escalation"
      ];
    }

and a user attempts to install the package, they will be greeted with
a warning indicating that maybe they don't want to install it:

    error: Package ‘foobar-1.2.3’ in ‘...default.nix:20’ is marked as insecure, refusing to evaluate.

    Known issues:

     - CVE-0000-00000: remote code execution
     - CVE-0000-00001: local privilege escalation

    You can install it anyway by whitelisting this package, using the
    following methods:

    a) for `nixos-rebuild` you can add ‘foobar-1.2.3’ to
       `nixpkgs.config.permittedInsecurePackages` in the configuration.nix,
       like so:

         {
           nixpkgs.config.permittedInsecurePackages = [
             "foobar-1.2.3"
           ];
         }

    b) For `nix-env`, `nix-build`, `nix-shell` or any other Nix command you can add
    ‘foobar-1.2.3’ to `permittedInsecurePackages` in
    ~/.config/nixpkgs/config.nix, like so:

         {
           permittedInsecurePackages = [
             "foobar-1.2.3"
           ];
         }

Adding either of these configurations will permit this specific
version to be installed. A third option also exists:

  NIXPKGS_ALLOW_INSECURE=1 nix-build ...

though I specifically avoided having a global file-based toggle to
disable this check. This way, users don't disable it once in order to
get a single package, and then don't realize future packages are
insecure.
@grahamc
Copy link
Member Author

grahamc commented Feb 18, 2017

Should we wait for a 👍 from @edolstra prior to merging?

Patches currently available don't seem to apply.
@grahamc
Copy link
Member Author

grahamc commented Feb 23, 2017

Ping again: should we wait for @edolstra? I just used it for the first time, marking libplist insecure.

@FRidh
Copy link
Member

FRidh commented Feb 23, 2017

you might want to change the base to staging

@grahamc
Copy link
Member Author

grahamc commented Feb 23, 2017

@FRidh this PR doesn't cause anything to rebuild, is there another reason to do that?

@FRidh
Copy link
Member

FRidh commented Feb 23, 2017

oh, I thought it would. Nevermind then.

@grahamc
Copy link
Member Author

grahamc commented Feb 23, 2017

👍 thanks!

@grahamc grahamc merged commit 037c489 into NixOS:master Feb 23, 2017
@grahamc grahamc deleted the mark-as-insecure branch February 23, 2017 12:13
@jgeerds
Copy link
Member

jgeerds commented Feb 23, 2017

Shouldn't we document this feature? (changelog/manual)

@grahamc
Copy link
Member Author

grahamc commented Feb 23, 2017

Yes we should, I evidently forgot that part. I'm reverting this merge until docs are here.

@grahamc
Copy link
Member Author

grahamc commented Feb 23, 2017

Thank you for the bump on that, @jgeerds.

@globin
Copy link
Member

globin commented Feb 23, 2017

You didn't have to revert, writing changelog/docs before the branch-off on 27.02. would've been enough IMHO.

@vcunat
Copy link
Member

vcunat commented Feb 23, 2017

There will be nontrivial conflicts with #22277, but never mind that. I need to find time to fix the performance regression in there first.

Alternatively, I could split up a move-code-out part and merge that – that's what conflicts with this and it has no performance impact.

@grahamc
Copy link
Member Author

grahamc commented Feb 24, 2017

I'll have docs ready by morning. My reasoning for reverting was so that I was highly incentivized to write the docs, and didn't just let them go.

@grahamc grahamc removed the weekly label Feb 24, 2017
@grahamc grahamc changed the title nixpkgs: allow packages to be marked insecure nixpkgs: allow packages to be marked insecure (reverted) Feb 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants