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

restrict-eval and readFile on builtins.path #3234

Closed
nmattia opened this issue Nov 21, 2019 · 8 comments
Closed

restrict-eval and readFile on builtins.path #3234

nmattia opened this issue Nov 21, 2019 · 8 comments

Comments

@nmattia
Copy link
Contributor

nmattia commented Nov 21, 2019

There is something I do not understand about restrict-eval. My assumption was that it disallowed any readFile et al. from derivations, but that appears not to be the case. I put together this Nix snippet, of which some targets fail to evaluate, which I cannot explain:

let
  pkgs = import <nixpkgs> {};
  drv = pkgs.runCommand "foo" {} "cp -r '${./.}' $out";
  drv2 = pkgs.runCommand "foo" {} "cp -r '${builtins.path { path = ./.; }}' $out";
  ps =
    { # works
      p1 = ./file;

      # works
      p2 = ./. + "/file" ;

      # access to path /nix/store/... is forbidden in restricted mode
      p3 = "${./. + "/file"}" ;

      # access to path /nix/store/.../file is forbidden in restricted mode
      p4 = "${./.}" + "/file" ;

      # works
      p5 = drv + "/file" ;

      # access to path /nix/store/.../file is forbidden in restricted mode
      p6 = pkgs.lib.cleanSource ./. + "/file" ;

      # access to path /nix/store/.../file is forbidden in restricted mode
      p7 = builtins.path { path =  ./.; } + "/file" ;

      # works
      p8 = drv2 + "/file" ;
    };
in
  pkgs.lib.mapAttrs (
    n: p:
      pkgs.runCommand "build-${n}" {} "echo '${builtins.readFile p}' > $out"
    ) ps

It is basically different ways of reading a file, which may or may not come from a derivation. Run with the following command:

for i in seq 1 8; do echo -n "p$i:"; NIX_PATH=src=.:nixpkgs=/path/to/nixpkgs/ nix-build --option restrict-eval true -A p$i || echo "failed: p$i"; done

  • p1 working makes sense.
  • p2 working makes sense as well, since we create a path from an existing path.
  • p3 failing fits with my understanding of restrict-eval preventing reading from derivation, as well as p4, p6 and p7 failing.
  • p5 working fits my new understanding that restrict-eval does not prevent reading from derivation. This however contradicts the point above.
  • p8 is a complete mystery, unless builtins.path itself creates some issue that is then resolved by wrapping everything in a (non-builtins.path) derivation.

The question is: is this a bug in builtins.path? if not, what is the benefit of preventing access to builtins.path derivations (p7), but not to derivations importing builtins.path (p8)?

EDIT: I just discovered the option allow-import-from-derivation. When set to false, p5 and p8 fail as well, which makes sense. I then do believe that the issue here is a bug in builtins.path.

@edolstra
Copy link
Member

restrict-eval originally allowed access to files in NIX_PATH or the Nix store. This was later tightened to only allow access to files in NIX_PATH or paths in the Nix store instantiated by the evaluation (d4dcffd). It's possible that the latter misses some cases (e.g. maybe builtins.path needs to add some paths to allowedPaths).

@edolstra
Copy link
Member

Indeed prim_path doesn't update allowedPaths at the moment.

@zimbatm zimbatm added the bug label Nov 22, 2019
@stale
Copy link

stale bot commented Feb 16, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the stale label Feb 16, 2021
@adrian-gierakowski
Copy link

Indeed prim_path doesn't update allowedPaths at the moment.

has this been resolved?

@stale stale bot removed the stale label Jul 15, 2021
@tomberek
Copy link
Contributor

Perhaps #5163 ?

@adrian-gierakowski
Copy link

@edolstra do you think #5163 is a valid fix? Thanks!

@stale
Copy link

stale bot commented Apr 16, 2022

I marked this as stale due to inactivity. → More info

@stale stale bot added the stale label Apr 16, 2022
@tomberek
Copy link
Contributor

I believe this is solved. Re-open if needed.

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

5 participants