This repository has been archived by the owner. It is now read-only.

preliminary write control only sandbox #38361

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@xu-cheng
Copy link
Contributor

xu-cheng commented Apr 4, 2015

Ref #37552

So I finally find some free time during the Easter holiday to implement this. This is a write control only sandbox which only effects build. It doesn't improve security in any appreciable way but serves as policy violations detector. However, this is the first step to any security improvement.

More test required. Maybe I will write a test case for brew tests later. To enable it, run brew install <formula> --sandbox.

cc @Homebrew/owners, @DomT4

Many works still wait to be done.

def initialize(formula=nil)
@profile = SandboxProfile.new
unless formula.nil?
allow_write "/private/tmp", :type => :subpath

This comment has been minimized.

@DomT4

DomT4 Apr 4, 2015

Contributor

This might not always be true. I think we allow TMPDIR to be set as an env?

This comment has been minimized.

@xu-cheng

xu-cheng Apr 4, 2015

Contributor

TMPDIR is inside /private/var/folders in below.

This comment has been minimized.

@DomT4

DomT4 Apr 4, 2015

Contributor

Apologies, I meant HOMEBREW_TEMP rather than TMPDIR.

This comment has been minimized.

@xu-cheng

xu-cheng Apr 4, 2015

Contributor

I guess I need to append HOMEBREW_TEMP as /private/tmp may be used by downstream.

This comment has been minimized.

@jacknagel

jacknagel Apr 4, 2015

Contributor

HOMEBREW_TEMP, it can be any arbitrary directory.

Edit: sorry, didn't refresh first.

This comment has been minimized.

@xu-cheng

xu-cheng Apr 4, 2015

Contributor

Already added. In last comment, I mean we should add both HOMEBREW_TEMP and /private/tmp.

@DomT4

This comment has been minimized.

Copy link
Contributor

DomT4 commented Apr 4, 2015

You're all much better at Ruby than I am, but this looks nice 👍. Linux has a few sandbox possibilities as well, but I guess we don't want to start writing significant chunks of Linux-only code into Homebrew core. I'd quite like to chase around this idea a little at some point, but I don't know how fail-safe that would be to implement and use.

@xu-cheng

This comment has been minimized.

Copy link
Contributor

xu-cheng commented Apr 4, 2015

As I work on the test case, I need a few help:

  • Why the test are run randomly? Only partial of tests are run each time. How to force it to run all of tests?
  • How to catch exception caused by Kernel.exec? OK, it turns out I have to put it into a fork block.
  • What's best way to get realpath? File.realpath doesn't exist on ruby 1.8. Pathname#realpath exists.
  • How to skip the entire test case? (need to skip sandbox test for linuxbrew) Skip using exit 0.
raise unless absolute?
exist? ? realpath : parent.expand_realpath+basename
end

This comment has been minimized.

@xu-cheng

xu-cheng Apr 5, 2015

Contributor

Sandbox requires realpath so I add this method.

This comment has been minimized.

@jacknagel

jacknagel Apr 5, 2015

Contributor

I'd prefer a private method on the sandbox class that accepts an argument over adding another method to pathname (which becomes public api).

foo = testpath/"foo"
s.allow_write "#{testpath}", :type => :subpath
s.exec "touch", foo
assert foo.exist?

This comment has been minimized.

@jacknagel

jacknagel Apr 5, 2015

Contributor
assert_predicate foo, :exist?
shutup do
assert_raises(RuntimeError) { s.exec "touch", bar }
end
assert !bar.exist?

This comment has been minimized.

@jacknagel

jacknagel Apr 5, 2015

Contributor
refute_predicate bar, :exist?

exit 0 unless Sandbox.available?

class SandboxTest < Homebrew::TestCase

This comment has been minimized.

@jacknagel

jacknagel Apr 5, 2015

Contributor

Can replace the exit with:

  def setup
    skip "sandbox not implemented" unless Sandbox.available?
  end
testpath = Pathname.new(TEST_TMPDIR)
foo = testpath/"foo"
s.allow_write "#{testpath}", :type => :subpath
s.exec "touch", foo

This comment has been minimized.

@jacknagel

jacknagel Apr 5, 2015

Contributor

Tests that mutate the filesystem should roll back the changes (this prevents order dependencies among tests)

@jacknagel

This comment has been minimized.

Copy link
Contributor

jacknagel commented Apr 5, 2015

Why the test are run randomly? Only partial of tests are run each time. How to force it to run all of tests?

How are you invoking the tests? rake or rake test runs all the tests.

@xu-cheng

This comment has been minimized.

Copy link
Contributor

xu-cheng commented Apr 5, 2015

How are you invoking the tests? rake or rake test runs all the tests.

brew tests

@jacknagel

This comment has been minimized.

Copy link
Contributor

jacknagel commented Apr 5, 2015

I never run them that way, so I dunno. What results are you seeing?

@xu-cheng

This comment has been minimized.

Copy link
Contributor

xu-cheng commented Apr 5, 2015

What results are you seeing?

En, I just find it's the same as bundle exec rake. Maybe It's just running in random order. And I was mislead to think it's running partially as it used to fail at random place.

@jacknagel

This comment has been minimized.

Copy link
Contributor

jacknagel commented Apr 5, 2015

Yeah, the order is random.

@xu-cheng

This comment has been minimized.

Copy link
Contributor

xu-cheng commented Apr 5, 2015

Updated. Thank for your advice, @jacknagel. I will update this again to remove the fork if we accept #38382.

@xu-cheng

This comment has been minimized.

Copy link
Contributor

xu-cheng commented Apr 7, 2015

I think this PR is ready. Hope to get comments from other maintainers.

Another question is how do we enable such feature for now. Enable by default or only enable for brew test-bot to evaluate the outcome for some time. Should we add a new environment flag, like HOMEBREW_SANDBOX or HOMEBREW_NO_SANDBOX?

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Apr 7, 2015

I'd add HOMEBREW_SANDBOX for now; I don't think I'd like to enable this by default for a while yet. I'd say we want to get it merged (after @jacknagel's comment is addressed) and you and any other volunteers have HOMEBREW_SANDBOX set locally for a few weeks. After that we can enable it if HOMEBREW_DEVELOPER is set (and this will enable it on the bots too) and then roll it out to users when we're happy all the above has worked out OK.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Apr 7, 2015

That said, though, this is really great stuff 👍

@xu-cheng

This comment has been minimized.

Copy link
Contributor

xu-cheng commented Apr 7, 2015

Updated to include HOMEBREW_SANDBOX. @jacknagel's comment is already addressed. I guess it's ready to merge?

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Apr 7, 2015

@xu-cheng Let's let @Homebrew/owners and @jacknagel take a final look and then I'm happy if we get another 👍

@jacknagel

This comment has been minimized.

Copy link
Contributor

jacknagel commented Apr 8, 2015

Since this is behind a feature flag I think it's OK to merge.

@jacknagel

This comment has been minimized.

Copy link
Contributor

jacknagel commented Apr 8, 2015

(Presumably we want #38434 first.)

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Apr 8, 2015

👍 from me too then

@xu-cheng

This comment has been minimized.

Copy link
Contributor

xu-cheng commented Apr 9, 2015

I refactored a few code since #38434 is merged. Final round review and I will merge this.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Apr 9, 2015

Looks good to me. Does this affect postinstall too or just install?

@xu-cheng

This comment has been minimized.

Copy link
Contributor

xu-cheng commented Apr 9, 2015

Does this affect postinstall too or just install?

It doesn't affect postinstall. My next step is to migrate postinstall and test into separate processes so we can sandbox them as well.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Apr 9, 2015

Ok, cool. I think postinstall may be tricker as we may need to allow writing to more locations e.g. node writes outside the cellar into HOMEBREW_PREFIX and test should probably only write to testpath: but 👍, sounds cool.

@xu-cheng

This comment has been minimized.

Copy link
Contributor

xu-cheng commented Apr 9, 2015

we may need to allow writing to more locations

I already did this in the build sandbox. (Some formulae write into these places in install rather than postinstall).

allow_write HOMEBREW_PREFIX/"etc", :type => :subpath
allow_write HOMEBREW_PREFIX/"var", :type => :subpath
allow_write HOMEBREW_CACHE, :type => :subpath
allow_write formula.rack, :type => :subpath
allow_write HOMEBREW_PREFIX/"etc", :type => :subpath
allow_write HOMEBREW_PREFIX/"var", :type => :subpath

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Apr 9, 2015

Member

Actually, it'd be good if this used the etc value from Formula so that files written to etc can be correctly bottled: https://github.com/Homebrew/homebrew/blob/master/Library/Homebrew/formula.rb#L301

We perhaps also want similar logic for var in Formula.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Apr 9, 2015

I already did this in the build sandbox. (Some formulae write into these places in install rather than postinstall).

postinstall will sometimes write into e.g. HOMEBREW_PREFIX/lib which will need handled later. Not for this PR but just for future. node and python will be good formulae to test there.

@xu-cheng

This comment has been minimized.

Copy link
Contributor

xu-cheng commented Apr 9, 2015

Updated to use formula.etc and formula.var. And thanks for the advice for future testing.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Apr 9, 2015

@xu-cheng 👍

@xu-cheng xu-cheng closed this in e82c3ce Apr 9, 2015

@xu-cheng xu-cheng deleted the xu-cheng:sandbox branch Apr 9, 2015

@Homebrew Homebrew locked and limited conversation to collaborators Jul 10, 2016

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.