> On Aug. 31, 2016, 4: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.

I concur.  If we'll (Aurora or Mesos) supply mounts into the chroot then 3 is 
the only acceptable scenario.


- John


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


On Aug. 31, 2016, 2: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, 2: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
> 
>

Reply via email to