Drop root privileges before forking #333

teto519f · 2024-05-24T11:53:11Z

Implemented a way for lo2s to detect a call with sudo and then drop its privileges before launching any process. Different methods of gaining root aren't taken into account (yet).

This refers to #301

To Do:

test functionality in real use case
ensure compatibility with lo2s -a
look into doas support (then update manpage)

teto519f · 2024-05-27T08:02:30Z

With current changes you cannot call on any process, that needs sudo through lo2s.
For example lo2s -- sudo apt update does not work, since lo2s can't call on sudo without root privileges.
sudo lo2s -- sudo apt update should be something you're able to do (imo), but this doesn't work when dropping privileges.

TL;DR I would like to add an option, that disables the sudo/doas check. Is this valid?

cvonelm · 2024-05-27T08:57:54Z

TL;DR I would like to add an option, that disables the sudo/doas check. Is this valid?

I would do it the other way around, enabling this getuid functionality only if some option is given. This feels like something that might lead to weird-to-detect errors if lo2s would just do privilege dropping automatically

Ideally, this option would also accept some username, so that you could specify that you want to switch to that specific user.

You can get the UID/GID of some username by using getpwnam (https://linux.die.net/man/3/getpwnam)

teto519f · 2024-05-27T09:07:53Z

This way a user could forget to add the option. Question is which case is worse to run into: accidentally launching a process with root or potentially having some weird errors.

And regarding the username-thing: I have a weird feeling about giving a user the ability to act as every other user. Nothing would stop them from switching to some daemon or a nologin user.

cvonelm · 2024-05-29T10:52:41Z

src/monitor/process_monitor_main.cpp

+    else
+    {
+        // sudo is garanteed in this case, see check in config.hpp
+        original_uid = atoi(std::getenv("SUDO_UID"));


Use stoi() instead of atoi()

Print an error message if stoi can not parse SUDO_UID to a number

cvonelm · 2024-05-29T10:54:21Z

src/monitor/process_monitor_main.cpp

+        auto orig_user = getpwnam(config().user.c_str());
+        if (orig_user == nullptr)
+        {
+            Log::error() << "No user found with specified username.";


print which user failed here

cvonelm · 2024-05-29T10:58:23Z

man/lo2s.1.pod

@@ -154,6 +154,14 @@ determine the trace path instead, including variable substitution.
 Attach to a running process with process ID I<PID> instead of launching
 I<COMMAND>.

+=item B<-u>, B<--as-current-user>


I personally dont like "current" in the option and in the text wording, maybe --as-pre-sudo-user?

cvonelm · 2024-05-29T11:11:43Z

man/lo2s.1.pod

+
+=item B<-U>, B<--as-user> I<USERNAME>
+
+Launch I<COMMAND> as the specified user. Implies B<-u> but does not require sudo, but the current user has to have setuid() privileges. Will take priority over B<-u> if specified together.


user I instead of "the specified user" . I think the part about "implies" in its current implementation confuses more than it helps

cvonelm · 2024-05-29T11:25:57Z

src/config.cpp

+        .toggle("as-current-user", "Drop root privileges to current user before launching COMMAND.")
+        .short_name("u");
+
+    general_options.option("as-user", "Specify a user to drop privileges to.")


change wording in accordance with man page comment above.

cvonelm · 2024-05-29T11:26:17Z

src/config.cpp

@@ -421,6 +431,18 @@ void parse_program_options(int argc, const char** argv)
        }
    }

+    if (arguments.given("as-current-user") && std::getenv("SUDO_UID") == NULL)
+    {
+        Log::error() << "-u was specified but no sudo was used.";


used -> detected

cvonelm · 2024-05-30T07:58:50Z

man/lo2s.1.pod


 =item B<-U>, B<--as-user> I<USERNAME>

-Launch I<COMMAND> as the specified user. Implies B<-u> but does not require sudo, but the current user has to have setuid() privileges. Will take priority over B<-u> if specified together.
+Launch I<COMMAND> as I<USERNAME>. I<USERNAME> has to have setuid() privileges. Will take priority over B<-u> if specified together.


USERNAME is not the user that needs to be able to do setuid. The user we are before we use setuid() to switch to USERNAME needs to be able to do setuid().

Do you think "the caller" or "the calling user" is a good description for this user? Just wanted to ask before I commit again.

"the caller needs to have CAP_SETUID" is the most correct way to write it me thinks

cvonelm · 2024-05-30T10:51:39Z

I have tested this now with our main use-case (which is running the IOR benchmark with mpirun, while lo2s runs as root) which seems to run fine, so we can check of testing

bmario · 2024-06-03T21:11:22Z

src/config.cpp

@@ -185,6 +185,14 @@ void parse_program_options(int argc, const char** argv)
        .metavar("PID")
        .optional();

+    general_options.toggle("as-pre-sudo-user", "Drop root privileges before launching COMMAND.")


The internal config option is called drop_root, the description is "Drop root privileges ...", but the thing is called --as-pre-sudo-user. I think that is confusing. Don't differ between "internal" and "external" naming of features. I think --drop-root(-privileges) would have been a better name.

bmario · 2024-06-03T21:18:27Z

src/config.cpp

+    general_options.toggle("as-pre-sudo-user", "Drop root privileges before launching COMMAND.")
+        .short_name("u");
+
+    general_options.option("as-user", "User to drop privileges to before launching COMMAND.")


I think this is easier to understand:
"Launch the COMMAND as the user USERNAME"

bmario · 2024-06-03T21:22:10Z

src/config.cpp

@@ -421,6 +430,18 @@ void parse_program_options(int argc, const char** argv)
        }
    }

+    if (arguments.given("as-pre-sudo-user") && std::getenv("SUDO_UID") == NULL)


This is not the behavior I would expect.
While lo2s -a -A works fine, lo2s -u -U $USER complains about -u was specified but no sudo was detected.

Or in other words, -U should imply -u.

Also, in C++, we don't use NULL. Use nullptr instead.

bmario · 2024-06-03T21:26:39Z

src/monitor/process_monitor_main.cpp

+    }
+    else
+    {
+        // sudo is garanteed in this case, see check in config.hpp


Don't only put your assumptions into comments. Put them in assert()s.

bmario · 2024-06-03T21:33:48Z

src/monitor/process_monitor_main.cpp

+        }
+        catch (std::invalid_argument const& e)
+        {
+            Log::error() << "Cannot parse SUDO_UID/SUDO_GID into int.";


This will be a user-facing message. Users don't know what an int is or that SUDO_UID/SUDO_GID are environment variables.

Cannot parse the environment variables SUDO_UID and/or SUDO_GID.

bmario · 2024-06-03T21:36:01Z

src/monitor/process_monitor_main.cpp

+        catch (std::invalid_argument const& e)
+        {
+            Log::error() << "Cannot parse SUDO_UID/SUDO_GID into int.";
+            throw_errno();


std::stoi raises an exception, but does not set errno. throw_errno() would need that.

Replace with throw;, which rethrows the current exception.

bmario · 2024-06-03T21:38:25Z

src/monitor/process_monitor_main.cpp

+            Log::error() << "Cannot parse SUDO_UID/SUDO_GID into int.";
+            throw_errno();
+        }
+        catch (std::out_of_range const& e)


I don't think the distinction between both exceptions types adds any value here. I'd rather have one "parsing failed" message.

Would you remove the second catch statement and catch general exceptions or duplicate the error handling for both types?
I'd go for the second option, but redundant code is wack and a function would be overkill.

I'd go with one handler catch(const std::exception& e). As users typically do not set these environment variables themselves, I don't think any other message than "failed to parse ..." is necessary, but rather confusing.

bmario · 2024-06-03T21:38:50Z

src/monitor/process_monitor_main.cpp

+        catch (std::out_of_range const& e)
+        {
+            Log::error() << "SUDO_UID/SUDO_GID out of range.";
+            throw_errno();


throw_errno() doesn't work here, but remove it completely.

Edit setuid() options as requested in PR #333

teto519f force-pushed the issue-301-setuid-sudo branch from b4de0bf to 3ca8f2b Compare May 27, 2024 07:36

teto519f force-pushed the issue-301-setuid-sudo branch from 33385d9 to 28f1c45 Compare May 28, 2024 06:24

teto519f changed the title ~~WIP: Drop root privileges before forking~~ Drop root privileges before forking May 28, 2024

teto519f marked this pull request as ready for review May 28, 2024 06:40

teto519f marked this pull request as draft May 28, 2024 08:53

teto519f marked this pull request as ready for review May 28, 2024 10:05

cvonelm reviewed May 29, 2024

View reviewed changes

cvonelm reviewed May 30, 2024

View reviewed changes

cvonelm approved these changes May 30, 2024

View reviewed changes

Add -u and -U option

8722131

teto519f force-pushed the issue-301-setuid-sudo branch from 655fa5b to 8722131 Compare May 30, 2024 09:09

cvonelm merged commit 8e13667 into tud-zih-energy:master May 30, 2024
21 checks passed

bmario reviewed Jun 3, 2024

View reviewed changes

teto519f pushed a commit to teto519f/lo2s that referenced this pull request Jun 4, 2024

include requested changes from PR tud-zih-energy#333

aa0ef8f

teto519f mentioned this pull request Jun 4, 2024

Edit setuid() options as requested in PR #333 #335

Merged

bmario added a commit that referenced this pull request Jul 11, 2024

Merge pull request #335 from teto519f/setuid_changes

53117b2

Edit setuid() options as requested in PR #333

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drop root privileges before forking #333

Drop root privileges before forking #333

teto519f commented May 24, 2024 •

edited by cvonelm

Loading

teto519f commented May 27, 2024

cvonelm commented May 27, 2024

teto519f commented May 27, 2024

cvonelm May 29, 2024

cvonelm May 29, 2024

cvonelm May 29, 2024

cvonelm May 29, 2024

cvonelm May 29, 2024

cvonelm May 29, 2024

cvonelm May 30, 2024

teto519f May 30, 2024

cvonelm May 30, 2024

cvonelm commented May 30, 2024

bmario Jun 3, 2024

bmario Jun 3, 2024

bmario Jun 3, 2024

bmario Jun 3, 2024

bmario Jun 3, 2024

bmario Jun 3, 2024

bmario Jun 3, 2024

bmario Jun 3, 2024

teto519f Jun 4, 2024

bmario Jun 4, 2024

bmario Jun 3, 2024


		=item B<-U>, B<--as-user> I<USERNAME>

		Launch I<COMMAND> as the specified user. Implies B<-u> but does not require sudo, but the current user has to have setuid() privileges. Will take priority over B<-u> if specified together.

Drop root privileges before forking #333

Drop root privileges before forking #333

Conversation

teto519f commented May 24, 2024 • edited by cvonelm Loading

teto519f commented May 27, 2024

cvonelm commented May 27, 2024

teto519f commented May 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cvonelm commented May 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teto519f commented May 24, 2024 •

edited by cvonelm

Loading