Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

idmapping: disallow hostids intersecting subids #3807

Merged
merged 2 commits into from Sep 19, 2017

Conversation

@brauner
Copy link
Member

brauner commented Sep 17, 2017

Our code has explicitly disallowed this for a long time but somehow we have
never correctly errored out in these cases like we should.
After having discussed this with Eric at Plumbers I think this should be
considered a security liability. The problem I see with this is that it
accidently allows users to escalate a container that is unprivileged to a
privileged container without their knowledge. The most obvious case is when
the allocated range of subids is accidently 0-65536 which is rare but basically
runs afoul of creating an unprivileged container but there are more subtle
problems when using "{uid,gid,both} <id> <id>" directly so I prefer to disallow
this for now. Especially since this feature is currently not very useful since
the maximum number of allowed idmappings is currently capped at five so this
would overflow quite easy.

Closes #3416.

Signed-off-by: Christian Brauner christian.brauner@ubuntu.com

@brauner brauner force-pushed the brauner:2017-09-17/disallow_hostids_in_subids branch from 6ce5ede to d7517b6 Sep 17, 2017
echo "lxd:"$UID_BASE":$UIDs" > /etc/subuid
echo "lxd:"$UID_BASE":$GIDs" > /etc/subgid
echo "root:"$GID_BASE":$UIDs" > /etc/subuid
echo "root:"$GID_BASE":$GIDs" > /etc/subgid

This comment has been minimized.

Copy link
@stgraber

stgraber Sep 17, 2017

Member

We absolutely can't do that kind of stuff in the testsuite, a failure in the middle of this code would break the system and when running multiple testsuites in parallel, you'll end up with undefined results.

We can't have the testsuite modify system files like that.

This comment has been minimized.

Copy link
@brauner

brauner Sep 17, 2017

Author Member

ok

@stgraber

This comment has been minimized.

Copy link
Member

stgraber commented Sep 17, 2017

I'll have to think through this some more tomorrow. I do seem to remember there being very much legitimate use cases for the original bug report and so just denying it doesn't feel lite the right option, but I need to dig into this some more myself.

@brauner

This comment has been minimized.

Copy link
Member Author

brauner commented Sep 17, 2017

Yeah, I'm not denying that there are legitimate use cases I'm just concerned about edge cases and furthermore I'm unsure whether the benefits of allowing this licenses the increased complexity of the algorithm to handle these cases at this point in time when we are limited wrt to the useable mappings. The last two days I tried to handle cases like:

uid 1000 1000
uid  883 1001

which e.g. would (given a range of e.g. 999-1000000000) translate to roughly something like

lxc.idmap = u     0  999   1
lxc.idmap = u  1000 1000   1
lxc.idmap = u   883 1001   1
lxc.idmap = u     1 1002 883
lxc.idmap = u   884 1885 116
lxc.idmap = u  1001 2001 end

uid 1000 1000
uid 1001 1001

which makes a hole-punching algorithm a real pain. :)

@brauner brauner force-pushed the brauner:2017-09-17/disallow_hostids_in_subids branch from d7517b6 to 1bcc41a Sep 17, 2017
if v.Nsid == v.Hostid {
return nil, 0, idmap.ErrHostIdIsSubId
}
}

This comment has been minimized.

Copy link
@stgraber

stgraber Sep 19, 2017

Member

Per our call, we should drop that.

Our code has explicitly disallowed this for a long time but somehow we have
never correctly errored out in these cases like we should.
After having discussed this with Eric at Plumbers I think this should be
considered a security liability. The problem I see with this is that it
accidently allows users to escalate a container that is unprivileged to a
privileged container without their knowledge. The most obvious case is when
the allocated range of subids is accidently 0-65536 which is rare but basically
runs afoul of creating an unprivileged container but there are more subtle
problems when using "{uid,gid,both} <id> <id>" directly so I prefer to disallow
this for now. Especially since this feature is currently not very useful since
the maximum number of allowed idmappings is currently capped at five so this
would overflow quite easy.

Closes #3416.

Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
@brauner brauner force-pushed the brauner:2017-09-17/disallow_hostids_in_subids branch from 1bcc41a to 295b479 Sep 19, 2017
Signed-off-by: Stéphane Graber <stgraber@ubuntu.com>
@brauner brauner force-pushed the brauner:2017-09-17/disallow_hostids_in_subids branch from ced324f to 7f7622c Sep 19, 2017
@stgraber

This comment has been minimized.

Copy link
Member

stgraber commented Sep 19, 2017

jenkins: test this please

@stgraber stgraber merged commit 53656a5 into lxc:master Sep 19, 2017
3 of 5 checks passed
3 of 5 checks passed
Testsuite Testsuite failed
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Branch target Branch target is correct
Details
Signed-off-by All commits signed-off
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@jfgibbins

This comment has been minimized.

Copy link

jfgibbins commented Sep 20, 2017

The disallow would only apply when the ID your mapping from the host, is one that would already be in the container mapping, ie container is 100000-165535, and someone wants to remap 101000 at a new location? As long as the mapped ID isn't already in the container to begin with, mapping will be allowed without incident?

@brauner

This comment has been minimized.

Copy link
Member Author

brauner commented Sep 20, 2017

@jfgibbins, correct. :)

@brauner

This comment has been minimized.

Copy link
Member Author

brauner commented Sep 20, 2017

We'll certainly will implement a proper algorithm handling that case as well but that will take more thought.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.