aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Zhitao Li <zhitaoli...@gmail.com>
Subject Re: Review Request 51564: Allow E_NAME_IN_USE in useradd/groupadd.
Date Thu, 01 Sep 2016 17:33:13 GMT


> On Aug. 31, 2016, 10:17 p.m., Stephan Erb wrote:
> > src/main/python/apache/aurora/executor/common/sandbox.py, line 239
> > <https://reviews.apache.org/r/51564/diff/3/?file=1489394#file1489394line239>
> >
> >     This changes seems to come with a severe security risk. As an normal user, I
can now gain root on any agent:
> >     
> >     * Prepare a docker/appc container with a manually crafted user with UID 0 but
with my role name.
> >     * Launch the container with said role name.
> >     * The sandbox code will bail out early here and don't proceed to create an unpriviledged
user
> >     * Setuid will switch from root to my prepare custom user with root permissions
> >     * Game over  
> >     
> >     Unless someone can correct me here, that would be a -1 from my end.
> 
> Joshua Cohen wrote:
>     I'm not sure about step 4 above. Are you referring to the [setuid in process.py](https://github.com/apache/aurora/blob/master/src/main/python/apache/thermos/core/process.py#L369-L380)?
If so, that setuid shouldn't be switching to root, it will be switching to the user matching
the role name on the host system, the uid set in your docker/appc image wouldn't have any
impact on that. Am I missing something?
> 
> John Sirois wrote:
>     Joshua mentioned this in Slack/IRC, but I do think we need to ensure the uid/uname
and gid/gname pairs in the chroot match those of the host system when we hit an exists condition
in either direction.
>     
>     Given:
>     Job author only specifies a role name, in this example `jsirois`
>     
>     Scenarios:
>     1. host (uid=1000, uname=jsirois) chroot (uid=500, uname=jsirois)
>     2. host (uid=1000, uname=jsirois) chroot (uid=1000, uname=fred)
>     3. host (uid=1000, uname=jsirois) chroot (uid=1000, uname=jsirois)
>      
>     A Job author can have task code that references the role name, for example it might
shell out a call to `id -g jsirois` where the role name is `jsirois` to find the primary group
id for the current role.  It seems then that we must ensure the chroot has the role name available,
and fwict, besides the special case of uid 0, we don't really care what the uid is.  If it
matches that's fine, but since the chroot environment will share nothing with the host, ids
need not match (IIUC).
>     
>     So it seems to me scenarios 1 and 3 are OK - the sandbox can move along.  Scenario
2 though should fail (we currently pass).
> 
> John Sirois wrote:
>     > Joshua mentioned this in Slack/IRC, but I do think we need to ensure the uid/uname
and gid/gname pairs in the chroot match those of the host system when we hit an exists condition
in either direction.
>     
>     Obviously I changed my thinking as I composed the Given/Scenarios/Conclusion below
that statement!
> 
> Stephan Erb wrote:
>     Excellent idea to spell out the scenarios explicitly.
>     
>         [W]e don't really care what the uid is.  If it matches that's fine, but since
the chroot environment will share nothing with the host, ids need not match (IIUC).
>         
>     I would disagree here. With container mounts, the host and a container can share
parts of the filesystem. As filesystem permissions are based on IDs, we have to make sure
those match inside and outside of the container.
>     
>     This implies that 'Scenario 3' would be the only acceptable one. We have to abort
the container launch in Scenario 1 and 2.
> 
> John Sirois wrote:
>     I concur.  If we'll (Aurora or Mesos) supply mounts into the chroot then 3 is the
only acceptable scenario.

tl'dr +1 for checking uid/gid matches between the image filesystem and host for now.

This is a quite complex problem, and IMO one of the aspects that Linux containerization and
cluster management layer generally haven't really solved well.

1. In our private use case, because we control all images running, we actually made special
arrangements to make sure relevent users have the same uid/gid between image and host system;
2. The above solution clearly doesn't scale for people who don't 100% control their build.
user namespace support should the savior here, but unfortunately it often conflicts with network
namespace, therefore neither docker daemon nor mesos really support it well;
3. For your quote of `With container mounts, the host and a container can share parts of the
filesystem. ` However, only certain parts we want the container to share with host system
(pretty much everything mounted into the container) will indeed be shared. This also depending
on how we want volume support for Mesos containerizer be.
4. Also, I think the risk you mentioned right now is also possible if people use DockerContainerizer
and specify a malicious `--volume` argument value.


- Zhitao


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51564/#review147497
-----------------------------------------------------------


On Aug. 31, 2016, 8:56 p.m., Zhitao Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51564/
> -----------------------------------------------------------
> 
> (Updated Aug. 31, 2016, 8:56 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, John Sirois, and Zameer Manji.
> 
> 
> Bugs: AURORA-1761
>     https://issues.apache.org/jira/browse/AURORA-1761
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Allow E_NAME_IN_USE in useradd/groupadd.
> 
> 
> Diffs
> -----
> 
>   src/main/python/apache/aurora/executor/common/sandbox.py a172691e164cf64792f65f049d698f9758336542

>   src/test/python/apache/aurora/executor/common/test_sandbox.py 57ab39e2444100c3a689bb0ff745c62f7bc2f1a6

> 
> Diff: https://reviews.apache.org/r/51564/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message