> 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?

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


-----------------------------------------------------------
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