bootctl: allow non-root user to run `bootctl status` #5964

yuwata · 2017-05-15T01:40:31Z

The command bootctl status does not make any change to the system. Lets's allow non-root user to run the command. However, for non-root user, I have no idea to get the partition UUID of detected ESP. So, currently only root user can show the UUID.

tixxdz · 2017-05-30T08:19:32Z

@yuwata thanks for the patch.

Are you sure that what bootctl status will show will not break some privacy here ? detect_container and must_be_root() are explicit function names here ?

Also could you please improve that must_be_root() and geteuid() shortcut with something like:

If we are not in a new user namespace. IOW only the init user namspace is valid, all others are not. (geteuid() will return 0 if it is mapped in new usernamespaces).
If we do not have the needed capabilities there otherwise geteuid() != 0 is fine too, it is easy so maybe just keep it. I would have replaced also geteuid() with getuid() but that's unrelated, so it is fine. But change in 1) I think is necessary.

yuwata · 2017-06-05T11:43:07Z

@tixxdz Thank you for your comment and sorry for late reply. I've force pushed the updated version. Please take a look.

Are you sure that what bootctl status will show will not break some privacy here ?

I think it does not. bootctl status just reads and shows the data in /sys/firmware/efi/efivars. The command invoked by non-root users shows the information only if the directory is mounted and readable by non-root users, and in that case, even though the command does not support non-root users, we can get the information by e.g. cat /sys/firmware/efi/efivars/LoaderDevicePartUUID-*.

Also could you please improve that must_be_root() and geteuid() shortcut with something like:

If we are not in a new user namespace. IOW only the init user namspace is valid, all others are not. (geteuid() will return 0 if it is mapped in new usernamespaces).

If we do not have the needed capabilities there otherwise geteuid() != 0 is fine too, it is easy so maybe just keep it. I would have replaced also geteuid() with getuid() but that's unrelated, so it is fine. But change in 1) I think is necessary.

Unfortunately, I do not have enough knowledge about user namespace and capabilities, so I may not correctly understand your comments. Anyway, in the updated version, I use getuid() instead of geteuid().

keszybz · 2017-07-09T17:05:14Z

Hi, sorry for the delay. My UEFI vm suffered a catastrophic failure a while back (when testing the systemd-tmpfiles-wipes-rootfs bug ;)), and it took me a while to get around to testing this.

Patch 1/2 seems like the right thing to do. Nevertheless, I get the following warning:
Failed to determine block device node of parent of "/boot/efi": Permission denied
This error does not seem to cause any further issues, but it's ugly. Can we silence it?

I don't understand patch 2/2. I don't see why bootctl would be running with uid != euid, so I don't think it makes much difference in practice, but euid seems like the right thing to check in general...

@tixxdz this code is all unprivileged, so the checks are only for convenience. I don't think namespaces are relevant here. Also, can you explain why you think getuid() is better than geteuid()?

keszybz · 2017-07-09T20:57:22Z

src/boot/bootctl.c

-        if (r < 0)
-                return r;
-
        r = find_esp(NULL, NULL, NULL, &uuid);


The value of r is never checked.

keszybz · 2017-07-09T21:00:44Z

src/boot/bootctl.c

-        if (r < 0)
-                return r;
+        if (arg_path)
+                r = status_binaries(arg_path, uuid);


So you stop checking r and continue, even if there are errors. I think this is OK, because it is better to try to print as much information as possible, even if something fails early in the process. But at the end, failure should be returned in any of the parts failed. So I think this should be changed to:

r = find_esp(...); if (arg_path) r2 = status_binaries(...); if (is_efi_boot()) r3 = status_variables(); return r < 0 ? r : r2 < 0 ? r2 : r3;

yuwata · 2017-07-10T02:17:09Z

@keszybz Thank you for your comments. I've force pushed the updated version. The changes in the updated version are

Make the error messages in verify_esp() as you saw be silent.
The return values in verb_status() are now referenced. But only the last error code is returned.
1/2 commit uses geteuid(). I cannot judge which function geteuid() or getuid() is suitable. So, I replace geteuid() to getuid() in the 2/2 commit. Please cherry-pick the 1/2 commit if geteuid() should be used.

keszybz · 2017-07-10T15:48:58Z

src/boot/bootctl.c

                        return -ENOENT;

-                return log_error_errno(errno, "Failed to check file system type of \"%s\": %m", p);
+                return quiet ? -errno : log_error_errno(errno, "Failed to check file system type of \"%s\": %m", p);


It's usually better to log at debug level (it makes any debugging much easier). And the condition could be made much stricter. Why not something like this:

return log_full_errno(errno, quiet && errno == EPERM ? LOG_DEBUG : LOG_ERROR, ....);

keszybz · 2017-07-10T15:50:17Z

src/boot/bootctl.c

                        return -EADDRNOTAVAIL;
-
-                log_error("File system \"%s\" is not a FAT EFI System Partition (ESP) file system.", p);
+                if (!quiet)


If statfs failed, we exit above. So this error should not be silenced, I think.

keszybz · 2017-07-10T15:51:09Z

src/boot/bootctl.c


        if (stat(p, &st) < 0)
-                return log_error_errno(errno, "Failed to determine block device node of \"%s\": %m", p);
+                return quiet ? -errno : log_error_errno(errno, "Failed to determine block device node of \"%s\": %m", p);


Again, log_full_errno(errno, quiet && errno == EPERM ? LOG_DEBUG : LOG_ERROR, ...);

keszybz · 2017-07-10T15:53:29Z

src/boot/bootctl.c


        if (major(st.st_dev) == 0) {
-                log_error("Block device node of %p is invalid.", p);
+                if (!quiet)


Similarly, I don't think this conditional should be added here.

keszybz · 2017-07-10T15:53:45Z

src/boot/bootctl.c

        r = stat(t2, &st2);
        if (r < 0)
-                return log_error_errno(errno, "Failed to determine block device node of parent of \"%s\": %m", p);
+                return quiet ? -errno : log_error_errno(errno, "Failed to determine block device node of parent of \"%s\": %m", p);


keszybz · 2017-07-10T15:53:56Z

src/boot/bootctl.c


        if (st.st_dev == st2.st_dev) {
-                log_error("Directory \"%s\" is not the root of the EFI System Partition (ESP) file system.", p);
+                if (!quiet)


poettering · 2017-07-10T20:01:35Z

not grokking the getuid()/geteuid() change. Can you elaborate?

yuwata · 2017-07-11T01:50:03Z

@keszybz and @poettering Thank you for your comments. Updated.

poettering · 2017-07-11T07:49:07Z

lgtm, but @keszybz has an outstanding review left, so I'll leave it for him to merge.

(i still wonder about the getuid()/geteuid() thing you now dropped, what was the rationale there? i am honestly interested!)

yuwata · 2017-07-11T13:03:27Z

@poettering As I wrote in the above, unfortunately I do not have enough knowledge about user namespace. So, I cannot judge which function is suitable here, and now I just omit the geteuid()/getuid() change. If all you judge that getuid() instead of geteuid() is suitable, then the change is trivial so I will add the commit again. Thanks.

keszybz · 2017-07-11T15:14:38Z

@poettering I was proposed by @tixxdz (#5964 (comment)) and we have all been clamouring for justification since ;)

keszybz · 2017-07-11T15:30:11Z

Looks good. Thanks.

tixxdz · 2017-07-21T13:06:38Z

@poettering @keszybz oups sorry :-) , totally forget about this question getuid()/geteuid() , actually it is just a security measure, since I saw some users or vendors setting the setuid bit on some systemd tools and IIRC even PRs here suggest that some users do this or set file capabilities, and since this deals with boot stuff I just suggested it would be at least safe to use getuid() to ignore the setuid bits and avoid damage since that checks seems to be used by other write operations... anyway this is not really a big issue and users can do whatever they want :-) !

yuwata force-pushed the bootctl-status branch from 7f0b2bc to 36b85bc Compare May 15, 2017 03:27

yuwata force-pushed the bootctl-status branch from 36b85bc to a9f80f5 Compare June 5, 2017 11:20

keszybz reviewed Jul 9, 2017

View reviewed changes

yuwata force-pushed the bootctl-status branch 2 times, most recently from b2196aa to 6492371 Compare July 10, 2017 02:08

keszybz requested changes Jul 10, 2017

View reviewed changes

bootctl: allow non-root user to run bootctl status

2beeeb0

yuwata force-pushed the bootctl-status branch from 6492371 to 2beeeb0 Compare July 11, 2017 01:49

keszybz approved these changes Jul 11, 2017

View reviewed changes

keszybz merged commit cd2d4c7 into systemd:master Jul 11, 2017

yuwata mentioned this pull request Aug 10, 2017

add ! and !! ExecStart= flags to make ambient caps useful #6577

Merged

yuwata deleted the bootctl-status branch November 12, 2017 05:44

Uh oh!

bootctl: allow non-root user to run bootctl status #5964

bootctl: allow non-root user to run bootctl status #5964

Uh oh!

Conversation

yuwata commented May 15, 2017

Uh oh!

tixxdz commented May 30, 2017

Uh oh!

yuwata commented Jun 5, 2017

Uh oh!

keszybz commented Jul 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yuwata commented Jul 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

poettering commented Jul 10, 2017

Uh oh!

yuwata commented Jul 11, 2017

Uh oh!

poettering commented Jul 11, 2017

Uh oh!

yuwata commented Jul 11, 2017

Uh oh!

keszybz commented Jul 11, 2017

Uh oh!

keszybz commented Jul 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tixxdz commented Jul 21, 2017

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

bootctl: allow non-root user to run `bootctl status` #5964

bootctl: allow non-root user to run `bootctl status` #5964

keszybz commented Jul 11, 2017 •

edited

Loading