sandbox: deny read access to HOMEBREW_PREFIX/{bin,include,lib} #2970

ilovezfs · 2017-07-29T19:05:11Z

Have you followed the guidelines in our Contributing document?
Have you checked to ensure there aren't other open Pull Requests for the same change?
Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your changes? Here's an example.
Have you successfully run brew tests with your changes locally?

See discussion in #2173.

ilovezfs · 2017-07-29T19:05:28Z

CC @zmwangx @MikeMcQuaid

zmwangx · 2017-07-29T19:20:08Z

LGTM, but you probably won't find the breakages until you rebuild everything.

zmwangx · 2017-07-29T19:29:55Z

Tried a few random things. First breakage I caught:

$ brew postinstall --debug python
/usr/local/Homebrew/Library/Homebrew/brew.rb (Formulary::FormulaLoader): loading /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/python.rb
==> Postinstalling python
==> Using the sandbox
/usr/bin/sandbox-exec -f /tmp/homebrew20170729-5335-1qaxxnw.sb nice /System/Library/Frameworks/Ruby.framework/Versions/2.0/usr/bin/ruby -W0 -I /usr/local/Homebrew/Library/Homebrew -- /usr/local/Homebrew/Library/Homebrew/postinstall.rb /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/python.rb --debug
/usr/local/Homebrew/Library/Homebrew/postinstall.rb (Formulary::FromPathLoader): loading /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/python.rb
Error: File exists - /usr/local/lib
/System/Library/Frameworks/Ruby.framework/Versions/2.0/usr/lib/ruby/2.0.0/fileutils.rb:245:in `mkdir'
/System/Library/Frameworks/Ruby.framework/Versions/2.0/usr/lib/ruby/2.0.0/fileutils.rb:245:in `fu_mkdir'
/System/Library/Frameworks/Ruby.framework/Versions/2.0/usr/lib/ruby/2.0.0/fileutils.rb:219:in `block (2 levels) in mkdir_p'
/System/Library/Frameworks/Ruby.framework/Versions/2.0/usr/lib/ruby/2.0.0/fileutils.rb:217:in `reverse_each'
/System/Library/Frameworks/Ruby.framework/Versions/2.0/usr/lib/ruby/2.0.0/fileutils.rb:217:in `block in mkdir_p'
/System/Library/Frameworks/Ruby.framework/Versions/2.0/usr/lib/ruby/2.0.0/fileutils.rb:203:in `each'
/System/Library/Frameworks/Ruby.framework/Versions/2.0/usr/lib/ruby/2.0.0/fileutils.rb:203:in `mkdir_p'
/System/Library/Frameworks/Ruby.framework/Versions/2.0/usr/lib/ruby/2.0.0/pathname.rb:559:in `mkpath'
/usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/python.rb:243:in `post_install'
/usr/local/Homebrew/Library/Homebrew/formula.rb:974:in `block (2 levels) in run_post_install'
/usr/local/Homebrew/Library/Homebrew/formula.rb:832:in `with_logging'
/usr/local/Homebrew/Library/Homebrew/formula.rb:973:in `block in run_post_install'
/usr/local/Homebrew/Library/Homebrew/utils.rb:554:in `with_env'
/usr/local/Homebrew/Library/Homebrew/formula.rb:965:in `run_post_install'
/usr/local/Homebrew/Library/Homebrew/postinstall.rb:16:in `<main>'

ilovezfs · 2017-07-29T19:36:45Z

Also, it somehow needs to exempt stdenv I think.

zmwangx · 2017-07-29T19:46:28Z

stdenv is still a thing? Wow.

ilovezfs · 2017-07-29T19:52:35Z

@zmwangx yeah, the test blocks and anything depending on scons.

DomT4 · 2017-07-29T23:26:15Z

Isn't the superenv change enough here without relying on the sandbox mechanism to enforce it?

ilovezfs · 2017-07-30T05:44:36Z

@DomT4 it is when it works. Not so much, when it doesn't. See Homebrew/homebrew-core#15910, and #2173 as already mentioned.

MikeMcQuaid

This is super awesome 👏 👍 🎉

MikeMcQuaid · 2017-07-30T15:18:12Z

Library/Homebrew/sandbox.rb

@@ -162,6 +162,11 @@ class SandboxProfile
          (regex #"^/dev/ttys?[0-9]*$")
          )
      (deny file-write*) ; deny non-whitelist file write operations
+      (deny file-read*


Probably want this in a def deny_write_homebrew_something method and then make it conditional on an environment variable being set.

Could also use HOMEBREW_PREFIX/"lib", etc in that method rather than hardcoding /usr/local, if desired.

MikeMcQuaid · 2017-07-30T15:18:38Z

Library/Homebrew/extend/ENV/super.rb

@@ -167,7 +167,7 @@ def determine_isystem_paths
  end

  def determine_include_paths
-    PATH.new(keg_only_deps.map(&:opt_include)).existing
+    PATH.new(deps.map(&:opt_include)).existing


Probably want this conditional on an environment variable being set

the same environment variable as in #2970 (comment)?

MikeMcQuaid · 2017-07-30T15:18:41Z

Library/Homebrew/extend/ENV/super.rb

@@ -176,8 +176,7 @@ def homebrew_extra_library_paths

  def determine_library_paths
    PATH.new(
-      keg_only_deps.map(&:opt_lib),
-      HOMEBREW_PREFIX/"lib",
+      deps.map(&:opt_lib),


Probably want this conditional on an environment variable being set

MikeMcQuaid · 2017-07-30T15:19:03Z

Library/Homebrew/sandbox.rb

@@ -162,6 +162,11 @@ class SandboxProfile
          (regex #"^/dev/ttys?[0-9]*$")
          )
      (deny file-write*) ; deny non-whitelist file write operations
+      (deny file-read*
+          (regex #"^/usr/local/bin.*$")


Worth having a trailing / before .*$? Worth handling sbin?

Trailing / still lets you see in the directory.

sbin

probably

DomT4 · 2017-07-30T15:23:37Z

@ilovezfs Fair enough. Just seemed a strange mix of tools to achieve the goal, but I'm rarely one to object to expanding a sandbox 😉. You've already fixed something I was going to point out & Mike has suggested a couple others, so, I'll be quiet for now 😄.

RandomDSdevel · 2017-11-11T20:18:41Z

Rises from lurking to ask @ilovezfs: What was the rationale for closing this? I don't see any mention of taking another stab at the part of #2173 that's relevant to this PR here as of late.

MikeMcQuaid · 2017-11-17T18:24:40Z

@RandomDSdevel Basically: unfortunately this PR isn't going to be something that we're ever going to be able to roll out to users.

RandomDSdevel · 2017-11-22T18:46:50Z

@MikeMcQuaid: OK, but can you refresh my memory on where that was pointed out? I can't seem to find why at the moment, at least in this discussion thread…though maybe I'm overlooking something obvious…? 🤷‍♂️

MikeMcQuaid · 2017-11-22T21:15:17Z

@RandomDSdevel It was not a public conversation I'm afraid.

RandomDSdevel · 2017-11-22T21:26:28Z

@MikeMcQuaid: Oh. Sorry to bother you about it, then.

sandbox: deny read access to HOMEBREW_PREFIX/{bin,include,lib}

21c1d44

ilovezfs force-pushed the sandbox-all-the-things branch from a08e9dc to 21c1d44 Compare July 29, 2017 19:14

ilovezfs added the do not merge label Jul 29, 2017

MikeMcQuaid requested changes Jul 30, 2017

View reviewed changes

Homebrew deleted a comment from Ellorah Jul 30, 2017

stale bot added the stale No recent activity label Aug 20, 2017

ilovezfs added the in progress Maintainers are working on this label Aug 26, 2017

stale bot removed the stale No recent activity label Aug 26, 2017

Homebrew deleted a comment from stale bot Aug 26, 2017

ilovezfs closed this Nov 10, 2017

Homebrew locked and limited conversation to collaborators May 4, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sandbox: deny read access to HOMEBREW_PREFIX/{bin,include,lib} #2970

sandbox: deny read access to HOMEBREW_PREFIX/{bin,include,lib} #2970

ilovezfs commented Jul 29, 2017

ilovezfs commented Jul 29, 2017

zmwangx commented Jul 29, 2017

zmwangx commented Jul 29, 2017 •

edited

ilovezfs commented Jul 29, 2017

zmwangx commented Jul 29, 2017

ilovezfs commented Jul 29, 2017

DomT4 commented Jul 29, 2017

ilovezfs commented Jul 30, 2017

MikeMcQuaid left a comment

MikeMcQuaid Jul 30, 2017

DomT4 Jul 30, 2017

MikeMcQuaid Jul 30, 2017

ilovezfs Jul 30, 2017

MikeMcQuaid Jul 30, 2017

MikeMcQuaid Jul 30, 2017

ilovezfs Jul 30, 2017

MikeMcQuaid Jul 30, 2017

MikeMcQuaid Jul 30, 2017

ilovezfs Jul 30, 2017

DomT4 commented Jul 30, 2017

RandomDSdevel commented Nov 11, 2017 •

edited

MikeMcQuaid commented Nov 17, 2017

RandomDSdevel commented Nov 22, 2017

MikeMcQuaid commented Nov 22, 2017

RandomDSdevel commented Nov 22, 2017

sandbox: deny read access to HOMEBREW_PREFIX/{bin,include,lib} #2970

sandbox: deny read access to HOMEBREW_PREFIX/{bin,include,lib} #2970

Conversation

ilovezfs commented Jul 29, 2017

ilovezfs commented Jul 29, 2017

zmwangx commented Jul 29, 2017

zmwangx commented Jul 29, 2017 • edited

ilovezfs commented Jul 29, 2017

zmwangx commented Jul 29, 2017

ilovezfs commented Jul 29, 2017

DomT4 commented Jul 29, 2017

ilovezfs commented Jul 30, 2017

MikeMcQuaid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DomT4 commented Jul 30, 2017

RandomDSdevel commented Nov 11, 2017 • edited

MikeMcQuaid commented Nov 17, 2017

RandomDSdevel commented Nov 22, 2017

MikeMcQuaid commented Nov 22, 2017

RandomDSdevel commented Nov 22, 2017

zmwangx commented Jul 29, 2017 •

edited

RandomDSdevel commented Nov 11, 2017 •

edited