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

macvim: Add sandboxProfile #69576

Merged
merged 1 commit into from Oct 9, 2019
Merged

Conversation

lilyball
Copy link
Member

@lilyball lilyball commented Sep 26, 2019

This allows full filesystem access when the sandbox is in relaxed mode.

Motivation for this change

The macvim package won't build in ofBorg due to sandboxing. I assume it still won't build with this change, but this should be friendlier to users who want sandboxing.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@@ -133,6 +133,9 @@ stdenv.mkDerivation {
find $out/share/man \( -name eVim.1 -or -name xxd.1 \) -delete
'';

# We rely on the user's Xcode install to build
__noChroot = true;
Copy link
Member

Choose a reason for hiding this comment

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

I personally don't like the idea of using __noChroot inside of nixpkgs. Have you tried to add the required paths like "/usr/bin/ibtool" to impureHostDeps instead? That would continue to avoid purity issues with eg. homebrew packages as well as non-obvious sandbox holes.

macvim.overrideAttrs (drv: {
  patches = [ ./upload-ssh-keys.patch ];
})

Copy link
Member

Choose a reason for hiding this comment

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

I mean additional sandbox definitions, impureHostDeps are for the pure mode only.

  sandboxProfile = ''
    (allow file-read* process-exec (literal "/usr/bin/ibtool"))
  '';

Copy link
Member Author

Choose a reason for hiding this comment

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

AIUI __impureHostDeps only works if the path exists within a directory defined using the allowed-impure-host-deps setting, and I'm rather skeptical that anyone is going to add all of Xcode to that, plus I don't even know if Xcode relies on any system-level binaries that would also need holes punched through (it already seems to require /usr/bin/tiffutil to be in the PATH for some reason, so I don't know what else is required).

That also wouldn't work if I configure my system such that the developer dir is something other than /Applications/Xcode.app either. I suppose I could explicitly set DEVELOPER_DIR to /Library/Developer/CommandLineTools which is the installation path for the command-line tools version of the build tooling, but I don't know how xcrun behaves if that hasn't been installed yet (does it fall back to /Applications/Xcode.app or does it prompt the user to install the tools?)

And even if I do manage to find a complete list of holes to request, and expect people to actually modify allowed-impure-host-deps to include them, there's no guarantee that Xcode updates won't change the paths that it needs access to.

Copy link
Member Author

Choose a reason for hiding this comment

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

In any case, most people on darwin aren't using sandboxing anyway, and punching holes won't allow ofBorg to build it because there's no way ofBorg will allow that (even if it has Xcode installed, which I wouldn't assume).

Copy link
Member Author

Choose a reason for hiding this comment

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

Just saw your update. I don't know how sandboxProfile works; does it require additional settings to enable its use like __impureHostDeps does?

Anyway, the commentary about xcode-select still holds, as does the questions of whether Xcode relies on other system-provided stuff. That said, if sandboxProfile can be used without settings changes, that at least makes doing the exploration of what's required a little more compelling. Though I'm still assuming ofBorg's builders don't have Xcode installed.

I wonder if I could do a sandbox profile that allows all filesystem access but still rejects network access? That would be better than nothing.

Copy link
Member

@LnL7 LnL7 Sep 26, 2019

Choose a reason for hiding this comment

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

It's similar to __noChroot since it allows defining arbitrary sandbox rules, but enables only opening only filesystem or a more limited subset of paths. This is the starting point that's used for the build sandbox. https://github.com/NixOS/nix/blob/5038e1bec43a71c97ae7f8be07218a8a2edbb6a1/src/libstore/sandbox-defaults.sb

eg.

(allow file-read* process-exec (subpath "/usr/bin")
                               (subpath "/Applications")
                               (subpath "/Library/Developer"))

or

(allow file-read* process-exec (subpath "/"))
; block homebrew dependencies
(deny file-read* process-exec (subpath "/usr/local"))

Copy link
Member Author

Choose a reason for hiding this comment

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

In an initial test, allowing full filesystem access as well as mach-lookup allows it to build. I will continue exploring this tonight as to what I can reasonably lock down (e.g. blocking homebrew deps seems like a good idea).

The Console does show a few blocked calls to fsctl and iokit-open. I don't know if I should just ignore those or allow them, since I don't know what's going on (the iokit-open calls have to do with nibs, I'm assuming it's trying to use the GPU for something; no idea about fsctl, the target there is 0x682f but I can't figure out what that corresponds to; the 0x68 prefix at least used to apply to HFS volumes but I can't find anything for 0x47 and most of the 0x68 definitions disappeared in a more recent version of xnu.). I'm tempted to go ahead and allow them since I don't think they're doing anything harmful, but the build does seem to work with them blocked.

@lilyball
Copy link
Member Author

I've pushed a new commit that uses sandboxProfile, but right now it's allowing complete filesystem access. I'll look into locking that down a bit later.

@lilyball lilyball changed the title macvim: Add __noChroot attribute macvim: Add sandboxProfile Sep 26, 2019
@lilyball
Copy link
Member Author

lilyball commented Sep 27, 2019

Updated. It still allows full filesystem access because we don't know where Xcode is located, but we're now blocking /usr/local. I went ahead and allowed fsctl and iokit-open because it seems harmless relative to full filesystem access.

@lilyball
Copy link
Member Author

Actually I take it back, I'm going to remove fsctl and iokit-open. Just because ibtool wants it doesn't mean we need to hand it over; the build seems to work perfectly fine without these.

Copy link
Member

@LnL7 LnL7 left a comment

Choose a reason for hiding this comment

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

I would prefer a whitelist instead, but this addresses the main concerns I had before.

@lilyball
Copy link
Member Author

It occurs to me I should add (with no-log) to the homebrew deny rule to get rid of a bunch of console spam

This allows full filesystem access except for Homebrew. This is because
we don't know where Xcode will be installed so we can't just whitelist
it and its dependencies.
@lilyball
Copy link
Member Author

Updated accordingly. Here's the trivial diff:

diff --git a/pkgs/applications/editors/vim/macvim.nix b/pkgs/applications/editors/vim/macvim.nix
index 0951c226bb8..b639ab61784 100644
--- a/pkgs/applications/editors/vim/macvim.nix
+++ b/pkgs/applications/editors/vim/macvim.nix
@@ -139,7 +139,7 @@ stdenv.mkDerivation {
   sandboxProfile = ''
     (allow file-read* file-write* process-exec mach-lookup)
     ; block homebrew dependencies
-    (deny file-read* file-write* process-exec mach-lookup (subpath "/usr/local"))
+    (deny file-read* file-write* process-exec mach-lookup (subpath "/usr/local") (with no-log))
   '';
 
   meta = with stdenv.lib; {

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-already-reviewed/2617/60

@worldofpeace worldofpeace merged commit 5862082 into NixOS:master Oct 9, 2019
@lilyball lilyball deleted the macvim-no-chroot branch October 10, 2019 00:05
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

4 participants