[lxc-devel] [PATCH] handle unprivileged user calls more gracefully

S.Çağlar Onur caglar at 10ur.org
Sat Jan 18 00:27:03 UTC 2014


Hey Stéphane,

On Fri, Jan 17, 2014 at 3:18 PM, Stéphane Graber <stgraber at ubuntu.com> wrote:
> On Fri, Jan 17, 2014 at 03:01:46PM -0500, S.Çağlar Onur wrote:
>> Return an error if the function is not supposed to be called by an unprivileged user.
>> Otherwise those calls fail in the middle of their execution with different reasons.
>>
>> Signed-off-by: S.Çağlar Onur <caglar at 10ur.org>
>
> Hmm, so I have mixed feelings about this. I certainly understand the
> reason for wanting this and I was actually considering a similar patch
> after seeing lxc-info complain quite a bit about unprivileged
> containers.
>
> However, I think API calls should print an error message indicating that
> the requested function isn't currently supported with unprivileged
> containers and the individual callers should themselves check whether
> they're running unprivileged and show something more relevant to the
> user.

This is what I'm doing in go bindings but we still need to handle
those cases in the C API itself as I believe there will be people
directly using it. I can definitely add some error messages to
unprivileged cases to make it clear to the caller why the call failed.

On the other hand, I see this patch as an intermediate step as I
believe some of those functions could be made functional with
unprivileged containers as well.

> Specifically, my concern with this is that lxc-ls and lxc-info will now
> report that a container doesn't have any interface or ip address, which
> is just wrong. Instead the user should be told that those are unknown as
> lxc-ls is currently doing.
>
> Also, the condition for am_unpriv seems a bit odd to me as it'd indicate
> that I can do:
>  - p1 = Container("p1")
>  - p1.start()
>  - p1.clear_config_item("lxc.id_map")
>  - p1.get_ips()
>
> And that'd workaround your test.
>
> Why can't it simply be: "return getuid() != 0;"?
>
> And if it ends up being just that, do we really need an am_unpriv(c)
> function when we can simply call "if (geteuid())"?

I guess we can just call geteuid as you suggested, that am_priv code
was already in the code so I just reused it :)

>> ---
>>  src/lxc/lxccontainer.c | 33 ++++++++++++++++++++++++++++-----
>>  1 file changed, 28 insertions(+), 5 deletions(-)
>>
>> diff --git a/src/lxc/lxccontainer.c b/src/lxc/lxccontainer.c
>> index 0bebdff..8d49e94 100644
>> --- a/src/lxc/lxccontainer.c
>> +++ b/src/lxc/lxccontainer.c
>> @@ -64,6 +64,10 @@
>>
>>  lxc_log_define(lxc_container, lxc);
>>
>> +static bool am_unpriv(struct lxc_container *c) {
>> +     return c->lxc_conf && geteuid() != 0 && !lxc_list_empty(&c->lxc_conf->id_map);
>> +}
>> +
>>  static bool file_exists(const char *f)
>>  {
>>       struct stat statbuf;
>> @@ -1489,6 +1493,9 @@ static char** lxcapi_get_interfaces(struct lxc_container *c)
>>       char **interfaces = NULL;
>>       int old_netns = -1, new_netns = -1;
>>
>> +     if (am_unpriv(c))
>> +             goto out;
>> +
>>       if (!enter_to_ns(c, &old_netns, &new_netns))
>>               goto out;
>>
>> @@ -1538,6 +1545,9 @@ static char** lxcapi_get_ips(struct lxc_container *c, const char* interface, con
>>       char *address = NULL;
>>       int old_netns = -1, new_netns = -1;
>>
>> +     if (am_unpriv(c))
>> +             goto out;
>> +
>>       if (!enter_to_ns(c, &old_netns, &new_netns))
>>               goto out;
>>
>> @@ -1818,7 +1828,7 @@ static int lxc_rmdir_onedev_wrapper(void *data)
>>  static bool lxcapi_destroy(struct lxc_container *c)
>>  {
>>       struct bdev *r = NULL;
>> -     bool bret = false, am_unpriv;
>> +     bool bret = false;
>>       int ret;
>>
>>       if (!c || !lxcapi_is_defined(c))
>> @@ -1833,14 +1843,12 @@ static bool lxcapi_destroy(struct lxc_container *c)
>>               goto out;
>>       }
>>
>> -     am_unpriv = c->lxc_conf && geteuid() != 0 && !lxc_list_empty(&c->lxc_conf->id_map);
>> -
>>       if (c->lxc_conf && has_snapshots(c)) {
>>               ERROR("container %s has dependent snapshots", c->name);
>>               goto out;
>>       }
>>
>> -     if (!am_unpriv && c->lxc_conf->rootfs.path && c->lxc_conf->rootfs.mount) {
>> +     if (!am_unpriv(c) && c->lxc_conf->rootfs.path && c->lxc_conf->rootfs.mount) {
>>               r = bdev_init(c->lxc_conf->rootfs.path, c->lxc_conf->rootfs.mount, NULL);
>>               if (r) {
>>                       if (r->ops->destroy(r) < 0) {
>> @@ -1857,7 +1865,7 @@ static bool lxcapi_destroy(struct lxc_container *c)
>>       const char *p1 = lxcapi_get_config_path(c);
>>       char *path = alloca(strlen(p1) + strlen(c->name) + 2);
>>       sprintf(path, "%s/%s", p1, c->name);
>> -     if (am_unpriv)
>> +     if (am_unpriv(c))
>>               ret = userns_exec_1(c->lxc_conf, lxc_rmdir_onedev_wrapper, path);
>>       else
>>               ret = lxc_rmdir_onedev(path);
>> @@ -2406,6 +2414,9 @@ static struct lxc_container *lxcapi_clone(struct lxc_container *c, const char *n
>>       if (!c || !c->is_defined(c))
>>               return NULL;
>>
>> +     if (am_unpriv(c))
>> +             return NULL;
>> +
>>       if (container_mem_lock(c))
>>               return NULL;
>>
>> @@ -2587,6 +2598,9 @@ static int lxcapi_snapshot(struct lxc_container *c, const char *commentfile)
>>       struct lxc_container *c2;
>>       char snappath[MAXPATHLEN], newname[20];
>>
>> +     if (am_unpriv(c))
>> +             return -1;
>> +
>>       // /var/lib/lxc -> /var/lib/lxcsnaps \0
>>       ret = snprintf(snappath, MAXPATHLEN, "%ssnaps/%s", c->config_path, c->name);
>>       if (ret < 0 || ret >= MAXPATHLEN)
>> @@ -2802,6 +2816,9 @@ static bool lxcapi_snapshot_restore(struct lxc_container *c, const char *snapnam
>>       if (!c || !c->name || !c->config_path)
>>               return false;
>>
>> +     if (am_unpriv(c))
>> +             return false;
>> +
>>       bdev = bdev_init(c->lxc_conf->rootfs.path, c->lxc_conf->rootfs.mount, NULL);
>>       if (!bdev) {
>>               ERROR("Failed to find original backing store type");
>> @@ -2851,6 +2868,9 @@ static bool lxcapi_snapshot_destroy(struct lxc_container *c, const char *snapnam
>>       if (!c || !c->name || !c->config_path)
>>               return false;
>>
>> +     if (am_unpriv(c))
>> +             return false;
>> +
>>       ret = snprintf(clonelxcpath, MAXPATHLEN, "%ssnaps/%s", c->config_path, c->name);
>>       if (ret < 0 || ret >= MAXPATHLEN)
>>               goto err;
>> @@ -2888,6 +2908,9 @@ static bool add_remove_device_node(struct lxc_container *c, const char *src_path
>>       char *directory_path = NULL;
>>       const char *p;
>>
>> +     if (am_unpriv(c))
>> +             goto out;
>> +
>>       /* make sure container is running */
>>       if (!c->is_running(c)) {
>>               ERROR("container is not running");
>> --
>> 1.8.3.2
>>
>> _______________________________________________
>> lxc-devel mailing list
>> lxc-devel at lists.linuxcontainers.org
>> http://lists.linuxcontainers.org/listinfo/lxc-devel
>
> --
> Stéphane Graber
> Ubuntu developer
> http://www.ubuntu.com
>
> _______________________________________________
> lxc-devel mailing list
> lxc-devel at lists.linuxcontainers.org
> http://lists.linuxcontainers.org/listinfo/lxc-devel
>



-- 
S.Çağlar Onur <caglar at 10ur.org>


More information about the lxc-devel mailing list