Gentoo Websites Logo
Go to: Gentoo Home Documentation Forums Lists Bugs Planet Store Wiki Get Gentoo!
Bug 161045 - [PATCH] RESTRICT=sandbox doesn't work
Summary: [PATCH] RESTRICT=sandbox doesn't work
Status: RESOLVED WONTFIX
Alias: None
Product: Portage Development
Classification: Unclassified
Component: Sandbox (show other bugs)
Hardware: All Linux
: High normal (vote)
Assignee: Portage team
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 161041 321017
  Show dependency tree
 
Reported: 2007-01-09 00:58 UTC by Ed Catmur
Modified: 2017-09-08 20:02 UTC (History)
3 users (show)

See Also:
Package list:
Runtime testing required: ---


Attachments
respect-restrict-sandbox.patch (respect-restrict-sandbox.patch,430 bytes, patch)
2007-01-09 01:08 UTC, Ed Catmur
Details | Diff
respect-restrict-sandbox.patch (respect-restrict-sandbox.patch,2.51 KB, patch)
2007-01-09 02:24 UTC, Ed Catmur
Details | Diff
alternate-respect-restrict-sandbox.patch (alternate-respect-restrict-sandbox.patch,2.55 KB, patch)
2007-01-09 03:12 UTC, Ed Catmur
Details | Diff
ebuild.5-no-restrict-sandbox.patch (ebuild.5-no-restrict-sandbox.patch,365 bytes, text/plain)
2007-01-10 08:50 UTC, Ed Catmur
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Ed Catmur 2007-01-09 00:58:52 UTC
See bug 161041, http://marc.theaimsgroup.com/?l=gentoo-user-de&m=115148403700916&w=2

ebuild(5) claims:
       RESTRICT = [strip,mirror,fetch,userpriv]
...
              sandbox
                     disable sandbox (do not use without very good reason).

However a quick glance at portage.py shows RESTRICT=sandbox is never checked.

Which way are we going to let this fall - change the docs, or change portage.py?
Comment 1 Ed Catmur 2007-01-09 01:08:53 UTC
Created attachment 106133 [details, diff]
respect-restrict-sandbox.patch

Well, this is what I think.
Comment 2 Ed Catmur 2007-01-09 02:10:17 UTC
OK, that works, but bug 161041 will still exist because actionmap has

        actionmap = {
"depend": {"cmd":ebuild_sh, "args":{"droppriv":1, "free":0,         "sesandbox":
0}},
"setup":  {"cmd":ebuild_sh, "args":{"droppriv":0, "free":1,         "sesandbox":
0}},
"unpack": {"cmd":ebuild_sh, "args":{"droppriv":1, "free":0,         "sesandbox":
sesandbox}},
"compile":{"cmd":ebuild_sh, "args":{"droppriv":1, "free":nosandbox, "sesandbox":
sesandbox}},
"test":   {"cmd":ebuild_sh, "args":{"droppriv":1, "free":nosandbox, "sesandbox":
sesandbox}},
"install":{"cmd":ebuild_sh, "args":{"droppriv":0, "free":0,         "sesandbox":
sesandbox}},
"rpm":    {"cmd":misc_sh,   "args":{"droppriv":0, "free":0,         "sesandbox":
0}},
"package":{"cmd":misc_sh,   "args":{"droppriv":0, "free":0,         "sesandbox":
0}},
        }

For RESTRICT="sandbox" to do the Right Thing, it'll need to be able to disable sandbox in unpack and install phases.  Should we set "free":nosandbox for those phases, or introduce a "restrictsandbox" variable?

OK, nosandbox is currently about userpriv (and usersandbox).  Does userpriv turning off sandbox make sense in unpack and install?
Comment 3 Ed Catmur 2007-01-09 02:13:18 UTC
Answering my own question: yes for unpack, no for install; root can't drop to portage:portage for src_install as it needs to be able to create various-owned and setuid/setgid binaries. 

So it looks like we will need a restrictsandbox variable.
Comment 4 Ed Catmur 2007-01-09 02:24:01 UTC
Created attachment 106137 [details, diff]
respect-restrict-sandbox.patch

Updated patch.

Also killed some unreached code.

I'm not entirely happy with actionmap, though; there's some logic in there that could be more declarative.
Comment 5 Ed Catmur 2007-01-09 03:12:25 UTC
Created attachment 106141 [details, diff]
alternate-respect-restrict-sandbox.patch

More logic-based version.
Comment 6 SpanKY gentoo-dev 2007-01-09 13:16:57 UTC
you should also post a patch to repoman that prevents people from setting SANDBOX_ON (declare it a readonly var to check in repoman)
Comment 7 Zac Medico gentoo-dev 2007-01-09 19:39:44 UTC
I think we need to provide a way for the user to filter packages with sandbox restriction.  If they have sandbox enabled, then RESTRICT=sandbox shouldn't disable it without the user's permission.
Comment 8 SpanKY gentoo-dev 2007-01-09 19:49:38 UTC
i dont quite follow

everyone has FEATURES=sandbox as it's in all of our profiles

with that logic, users will be unable to emerge these two packages that are completely broken inside of the sandbox (gcl and emacs)
Comment 9 Zac Medico gentoo-dev 2007-01-09 21:01:05 UTC
(In reply to comment #8)
> with that logic, users will be unable to emerge these two packages that are
> completely broken inside of the sandbox (gcl and emacs)

They'll be able to emerge them if they manually disable sandbox or set some flag that indicates to emerge/portage that packages with RESTRICT=sandbox are okay.

For example, we could introduce a new variable called ACCEPT_RESTRICT.  If the user has ACCEPT_RESTRICT="sandbox", then packages with RESTRICT=sandbox will be silently installed even if the user has FEATURES=sandbox enabled.  ACCEPT_RESTRICT would also be useful for other restrictions such as ebuilds that require interactivity (bug #151113) or ebuilds that restrict userpriv (bug #159876).
Comment 10 Alec Warner archtester Gentoo Infrastructure gentoo-dev Security 2007-01-10 05:57:46 UTC
(In reply to comment #6)
> you should also post a patch to repoman that prevents people from setting
> SANDBOX_ON (declare it a readonly var to check in repoman)
> 

The primary purpose of repoman (IMHO) is to detect common mistakes.  I don't really see a check like this being very useful (although probably fairly trivial to implement).  How many poeple do this now?
Comment 11 Ed Catmur 2007-01-10 07:48:37 UTC
(In reply to comment #9)
> For example, we could introduce a new variable called ACCEPT_RESTRICT.  If the
> user has ACCEPT_RESTRICT="sandbox", then packages with RESTRICT=sandbox will be
> silently installed even if the user has FEATURES=sandbox enabled. 

Wouldn't this need to be settable per-package, though? Can we cope with an /etc/portage/package.accept_restrict?
Comment 12 Jakub Moc (RETIRED) gentoo-dev 2007-01-10 08:24:39 UTC
(In reply to comment #8)
> everyone has FEATURES=sandbox as it's in all of our profiles
> 
> with that logic, users will be unable to emerge these two packages that are
> completely broken inside of the sandbox (gcl and emacs)

What's the point of making a rather dangerous RESTRICT valid? For these two packages only? Nothing else broken in this way should ever be committed to the tree, and noone should encourage developers to fiddle with sandbox in as easy way as is RESTRICT=sandbox. SANDBOX_ON is an undocumented hack and should remain as such, we don't need RESTRICT=sandbox at all.
Comment 13 Ed Catmur 2007-01-10 08:50:19 UTC
Created attachment 106320 [details]
ebuild.5-no-restrict-sandbox.patch

Well, I thought it had been documented for a reason.  No problem to remove, though.
Comment 14 SpanKY gentoo-dev 2007-01-10 14:25:52 UTC
the primary purpose of repoman is to make sure bad QA doesnt make it into the tree

whether it's common is irrelevant (how can you justify labeling USE as a readonly var in that vein of thinking ?  how common was it for ebuilds to set that ?)
Comment 15 Marius Mauch (RETIRED) gentoo-dev 2007-01-11 06:59:09 UTC
Removed RESTRICT=sandbox from ebuild(5) until this bug is resolved.
Comment 16 Marius Mauch (RETIRED) gentoo-dev 2007-06-23 16:53:00 UTC
Anyone object if I close this?
Comment 17 Ulrich Müller gentoo-dev 2015-02-27 15:15:36 UTC
PMS specifies what the package manager must do when RESTRICT=sandbox is set:

   "The sandbox tool must not be used when building the package."

As I see it, we have the following possibilities here:

1. Make Portage compliant with what PMS specifies.
2. Remove RESTRICT=sandbox from PMS, and document the SANDBOX_ON variable.
Comment 18 SpanKY gentoo-dev 2015-02-27 19:51:55 UTC
(In reply to Ulrich Müller from comment #17)

imo we should drop RESTRICT=sandbox.  i haven't seen a compelling case yet where it should be supported for an entire package.  every case in the tree is a matter of limited surgery to deal with a specific problem in the package they're building.  there's no reason at all we should ever consider letting someone add an ebuild using RESTRICT=sandbox to the main tree.  yes, we could have repoman reject it every time out of hand, but i don't think the reasonable real world needs of packages even line up with RESTRICT.

here's a survey of the whole tree:
* app-editors/emacs: (limited to src_compile) see bug 131505; most likely due to the insane build system they have where they dump a memory image of itself during build time and then reload that at runtime
* app-editors/emacs-vcs: same as above
* dev-lisp/cmucl: (limited to src_compile) no idea why, but it's only in the old versions, so it'll get cleaned out
* virtualx.eclass: (limited to `virtualmake`) looks like a hack to spawn an Xvfb server for building purposes
* sys-libs/libunwind: the testsuite involves probing its own memory/backtraces

at least for ecmacs/libunwind, they don't want to turn off the sandbox because they need to violate paths.  they do it because the current sandbox implementation is a LD_PRELOAD library that is in the same memory space.  a transparent sandbox (like ptrace, or seccomp) would still be fine.  i can't speak to the exact needs of virtualx though.  if they truly wanted to violate permission restrictions, they could have used the already existing `addwrite /` escape hatch.

SANDBOX_ON is an ugly hack that is a pita to use (it's more a sandbox implementation detail leaking).  whether we standardize that exact env var or come up with another command to go alongside `addwrite` and friends, i have no opinion.  the backwards compat aspect would be trivial to deal with by adding a wrapper in eutils.eclass.
Comment 19 Ulrich Müller gentoo-dev 2015-03-07 18:46:45 UTC
I suggest to drop RESTRICT=sandbox from PMS. Retroactively, because it is neither used in the tree nor supported by Portage.

And for the next EAPI:
1. Add a sandboxoff command.
2. Limit the scope of all sandbox commands to the current phase.
3. Add a pair of sandboxsave and sandboxrestore commands that would save/restore the current state. This is to avoid such constructs as we have in cvs.eclass: http://sources.gentoo.org/cgi-bin/viewvc.cgi/gentoo-x86/eclass/cvs.eclass?revision=1.84&view=markup#l276
Alternatively, these could be sandboxpush/sandboxpop, but I'm not sure if nesting would be needed.
Comment 20 Zac Medico gentoo-dev 2015-03-07 19:03:39 UTC
(In reply to Ulrich Müller from comment #19)
> I suggest to drop RESTRICT=sandbox from PMS. Retroactively, because it is
> neither used in the tree nor supported by Portage.

Sounds good. Resolving as wontfix.

> And for the next EAPI:
> 1. Add a sandboxoff command.
> 2. Limit the scope of all sandbox commands to the current phase.
> 3. Add a pair of sandboxsave and sandboxrestore commands that would
> save/restore the current state. This is to avoid such constructs as we have
> in cvs.eclass:
> http://sources.gentoo.org/cgi-bin/viewvc.cgi/gentoo-x86/eclass/cvs.
> eclass?revision=1.84&view=markup#l276
> Alternatively, these could be sandboxpush/sandboxpop, but I'm not sure if
> nesting would be needed.

We might also consider adding a variable defined by PMS which refers to a persistent writable directory for vcs eclasses to use, with no need for sandbox interaction.
Comment 21 Ulrich Müller gentoo-dev 2015-03-16 09:21:45 UTC
RESTRICT=sandbox dropped from PMS:
http://gitweb.gentoo.org/proj/pms.git/commit/?id=4055e2091ab22d34b45790462513bbaf3d70a1bf