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

capabilities: raise ambient capabilities #2332

Merged
merged 1 commit into from May 16, 2018

Conversation

brauner
Copy link
Member

@brauner brauner commented May 15, 2018

Signed-off-by: Christian Brauner christian.brauner@ubuntu.com
Suggested-by: Jonathan Calmels jcalmels@nvidia.com

@brauner
Copy link
Member Author

brauner commented May 15, 2018

//cc @3XX0, you definitely need a custom mount binary though. :) But I could get the following hook to run successfully with this patch:

lxc.hook.mount =
lxc.hook.mount = /home/chb/files/Cground/test

where /home/chb/files/Cground/test.c contains:

int main()
{
        int ret = mount("/dev/console", "/usr/lib/x86_64-linux-gnu/lxc/home/ubuntu/.profile", NULL, MS_BIND, NULL);
        if (ret < 0) {
                fprintf(stderr, "%s\n", strerror(errno));
                exit(EXIT_FAILURE);
        }


        exit(EXIT_SUCCESS);
}

@brauner brauner force-pushed the 2018-05-16/use_ambient_capabilities branch from 6938358 to ad2ce6b Compare May 15, 2018 22:33
@brauner brauner requested a review from hallyn May 15, 2018 22:45
@3XX0
Copy link
Contributor

3XX0 commented May 16, 2018

Seems to work fine with the nvidia hook after I relaxed our checks on uid 0 🎉
We should probably restore the ambient caps after the start hook no? Otherwise an unprivileged container process will keep all capabilities (see below).

% lxc-execute cuda bash

I have no name!@cuda:/$ nvidia-smi -L          
GPU 0: GeForce GTX 1080 (UUID: GPU-4265621a-3886-6d0e-bcec-d99039a76278)
GPU 1: GeForce GTX 970 (UUID: GPU-c0ddf4f7-2a0a-718b-61a4-d26fe655b826)

I have no name!@cuda:/$ cat /proc/self/uid_map 
      1000       1000          1

I have no name!@cuda:/$ cat /proc/self/status | grep '^Cap'
CapInh: 0000003fffffffff
CapPrm: 0000003fffffffff
CapEff: 0000003fffffffff
CapBnd: 0000003fffffffff
CapAmb: 0000003fffffffff

But yes, LXCFS on the other hand will need a helper binary since libmount checks for root privileges.

@brauner brauner force-pushed the 2018-05-16/use_ambient_capabilities branch from ad2ce6b to beaa6cc Compare May 16, 2018 11:38
@brauner
Copy link
Member Author

brauner commented May 16, 2018

@3XX0, caps should now be lowered. :) Please test. :)

ubuntu@xenial:/$ cat /proc/self/status | grep '^Cap'
CapInh: 0000000000000000
CapPrm: 0000000000000000
CapEff: 0000000000000000
CapBnd: 0000003fffffffff
CapAmb: 0000000000000000

I'm wiping both ambient and effective.

@brauner brauner force-pushed the 2018-05-16/use_ambient_capabilities branch from beaa6cc to 2e4b638 Compare May 16, 2018 12:12
@brauner
Copy link
Member Author

brauner commented May 16, 2018

If you want more fancy useages of the mount command itself it might be worth to strip:
https://github.com/karelzak/util-linux/blob/master/sys-utils/mount.c
clean of any root checks and unneeded options such as --no-mtab and add a minimal lxc-mount that ships with the lxc shared library. It should really only support mounting, no printing of stuff, nothing fancy.

Copy link
Member

@hallyn hallyn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you think of any case where a site would want to run with non-root uid, but want hooks to be run without privilege? I can't, but if there is any such use case then we need to clearly document that they will have to somehow drop the caps. If not then no problem.


out:

cap_free(cap_names);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't this just be free(cap_names) ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the manpage you're supposed to use cap_free():

 cap_to_text()  converts the capability state in working storage identified
       by cap_p into a nul-terminated human-readable string.  This function allo‐
       cates any memory necessary to contain the string, and returns a pointer to
       the string.  If the pointer len_p is not NULL,  the  function  shall  also
       return the full length of the string (not including the nul terminator) in
       the location pointed to by len_p.  The capability state in  working  stor‐
       age,  identified  by  cap_p,  is  completely  represented in the character
       string.  When the  capability  state  in  working  storage  is  no  longer
       required,  the  caller  should  free  any  releasable  memory  by  calling
       cap_free() with the returned string pointer as an argument.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hah - good call, thanks.

src/lxc/caps.c Outdated
ret = cap_get_flag(caps, cap, CAP_PERMITTED, &flag);
if (ret < 0) {
if (errno == EINVAL) {
INFO("Last supported cap was %d", cap - 1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't you want to store the fact that you found the last supported cap here?

Otherwise if lxc was compiled with a CAP_LAST_CAP > the kernel was, then you'll get EPERM below when you try to raise the ambient bit.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, excellent, missed that. Thanks, Serge!

Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
Suggested-by: Jonathan Calmels <jcalmels@nvidia.com>
@brauner brauner force-pushed the 2018-05-16/use_ambient_capabilities branch from 2e4b638 to 611ddd3 Compare May 16, 2018 13:57
@brauner
Copy link
Member Author

brauner commented May 16, 2018

Can you think of any case where a site would want to run with non-root uid, but want hooks to be run without privilege? I can't, but if there is any such use case then we need to clearly document that they will have to somehow drop the caps. If not then no problem.

Not really. :)

@hallyn hallyn merged commit 23cf184 into lxc:master May 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants