FlexMeter: Add FlexMeter functionality #1571

bogdanovs · 2024-12-17T13:30:35Z

FlexMeter provides functionality which will allow
users to make custom meters without need of rebuilding every time htop binary and adding source to the project. It can be used to print some device status, free disk space CPU or other specific temeraturer, fan RPM and many more. Everything that can be fetched from linux shell
with one line result can be printer. For fething information can be used anything from shell, python, precompiled binary or simply reading file located somewhere in file system.

New meter will appear uppon restart of htop in list with available meters.

Configuration folder location where metes should be placed:

/home/$USER/.config/htop/FlexMeter/ On start folder will be created if does not exist, together with template file .Template in same folder.
Note: Files starting with '.' (.Template for examlpe) are ignored

Meter Example:
File name : Template

name=
command=
type=<METER TYPE FOR NO ONLY "TEXT_METER">
caption="CAPTION TEXT SHOWN IN THE BEGGINING OF THE METER"

According to this implementation 30 Flex meter can be added Currently they have hardcoded limit of 30 meter in addition to all that already exist.

I am using this functionality for about an years maybe, so far had no issues. It might not be most optimal implementation by try to follow project stile while developed it. I am open for suggestion to improvements.

BenBE · 2024-12-17T15:31:17Z

Please have a very close look at your implementation again as I noticed several trivial buffer overflows in the file iteration/handling code. Furthermore I'd like to point you to our styleguide which gives additional guidance on how the code should be set up.

Also when integrating this meter, we have to take care of privilege escalations when running the specified commands. This is in particular true when running htop as root via sudo, when the home directory is still set to the logged-in user's HOME directory. In that situation a command in /home/user/.config/htop/FlexMeter/malicious would now potentially be run as root, instead of the user this meter belongs to. A minimum safety check would be limiting meter execution to files that belong to the current process's EUID IFF that config file is also chmod 644 or less. Also note, that htop tries to drop capabilities: With the FlexMeters I'd suggest to enforce this.

Explorer09 · 2024-12-18T11:01:16Z

Just wondering, why was this meter called FlexMeter? Was it a random name you thought of? pcp-htop has a PCPDynamicMeter. And it seems this PR is about a dynamic meter as well.

Also, I agree with @BenBE on the security issue here. The shell script to launch should have its owner same as the EUID or else htop should refuse to execute it.

bogdanovs · 2024-12-18T12:01:58Z

@BenBE I looking in codding stile which I might not follow strictly , and thanks for remark on overflow issue I am aware of it but totally forget to fix it

@Explorer09 FlexMeter was chosen because you can select name of the Meter created from configuration file, I thought it was good. I was looking for easy and simple way to extend my htop with some stats, which would require development of bunch of specific meters for simple one line shell for example.

Regarding PCPDynamicMeter - I was looking for something simpler. This was my idea. I was looking to report some custom stuff from my system like peripherals battery status or UPS work temperature.

pcp-htop is a version of htop built using the Performance Co-Pilot (PCP) Metrics API (see [PCPIntro(1)]
https://man.archlinux.org/man/PCPIntro.1.en), PMAPI(3)),
allowing to extend htop to display values from arbitrary metrics. See the section below titled CONFIG FILES for further details.

I will fix security issue too as far it is possible.

BenBE · 2024-12-18T12:48:05Z

@BenBE I looking in codding stile which I might not follow strictly , and thanks for remark on overflow issue I am aware of it but totally forget to fix it

The buffer overflow was just one thing in the implementation. What initially tipped me off was the extensive use of static buffers all over the place. htop has xMalloc and xCalloc available for strings and these should also be used. Also when using the standard C library string functions: If there is an error-checked version in XUtils.h that one should be used (or if missing created). Also, if you ensure, all invalid/unset pointers are NULL (by using xCalloc when requesting memory and assigning NULL after free), you can simply call free (we follow the C99 standard, which defines free(NULL); to be a no-op); checks for !=NULL before accessing should be placed though (unless marked by a function attribute).

Overall, memset/memmove should appear sparingly and memory buffers on the stack should be a rarity (even no VLA at all, anywhere). Usually instead of memcpy you usually want something like xStrdup or xSnprintf/xAsprintf instead.

@Explorer09 FlexMeter was chosen because you can select name of the Meter created from configuration file, I thought it was good. I was looking for easy and simple way to extend my htop with some stats, which would require development of bunch of specific meters for simple one line shell for example.

I think naming-wise I'm fine with both: FlexMeter or DynamicMeter. Depending on how much infrastructure can be shared with the PCP implementation, calling it DynamicMeter might be an option; but that might be a source of confusion.

Regarding PCPDynamicMeter - I was looking for something simpler. This was my idea. I was looking to report some custom stuff from my system like peripherals battery status or UPS work temperature.

AFAICS the current implementation only implements text mode? Maybe we should limit it to that too; thinking re #1387 …

pcp-htop is a version of htop built using the Performance Co-Pilot (PCP) Metrics API (see [PCPIntro(1)]
https://man.archlinux.org/man/PCPIntro.1.en), PMAPI(3)),
allowing to extend htop to display values from arbitrary metrics. See the section below titled CONFIG FILES for further details.

I will fix security issue too as far it is possible.

Both the buffer overflows (CWE-787, CWE-121, and CWE-122) and the privilege escalation (CWE-250, CWE-265, CWE-266,, CWE-269, CWE-270, and CWE-273,) are all security issues; the privilege escalation is just the more obvious architectural one, which needs some more thorough thoughts to mitigate.

A good rule of thumb is to assume that every bug your code has will wipe your system. Now write your code like (if) you value your data …

bogdanovs · 2024-12-29T21:37:48Z

I have been working to make FlexMeter more compliant with all comments above. Used xStrdup in place of previous implementation. I followed docs/styleguide.md with next iteration will clean some more leftover things.

If we agree lest stick with FlexMeter, since AFAIK it does not share functionality and implementation with PCP and might be confusing.

AFAICS the current implementation only implements text mode? Maybe we should limit it to that too; thinking re #1387 …

Yes it implements only text mode at the moment. I kept type in implementation since I thought it might be cool to implements and others. So far it look like it is not needed.

type=TEXT_METERMODE

Both the buffer overflows (CWE-787, CWE-121, and CWE-122) and the privilege escalation (CWE-250, CWE-265, CWE-266,, CWE-269, CWE-270, and CWE-273,) are all security issues; the privilege escalation is just the more obvious architectural one, which needs some more thorough thoughts to mitigate.

Most of this are addressed. I need to handle privileges for files located in ~/.config/htop/FlexMeter

FlexMeter.c

Explorer09 · 2024-12-30T01:07:24Z

FlexMeter.c

+   // path to home folder 1 for zero 1 for slash
+   char * home = (char * ) xCalloc(1,(strlen(homedir) + strlen(FLEX_CFG_FOLDER) + 2));
+
+   sprintf(home,"%s/%s",homedir,FLEX_CFG_FOLDER);


Use xSnprintf. sprintf is insecure.

I forgot to mention this: htop complies with XDG Base Directory specification and that means we fetch user's config directory with the variable XDG_CONFIG_HOME. Specifically the flex meter directories should go in $XDG_CONFIG_HOME/htop/FlexMeter. Don't hard-code the .config directory part and don't use $HOME/.config/htop/FlexMeter. Look in the Settings.c file in htop source code to see how we fetch the XDG_CONFIG_HOME variable and use the fallback default if it's unset.

I will check that, thanks

FlexMeter.c

Explorer09 · 2024-12-30T10:50:20Z

Before I review more about the code style and quality of the meter, I have some general comments:

How do we indicate the user has permissions and intent to execute the commands in the FlexMeter template?

Honestly, I don't like the current design of the template file format. It mixes static data and executable code, and that's a bad idea. Also, it does not allow me to indicate the shell to use (i.e. it contains no shebang). I know you are using popen(3) to launch commands in a shell, but I expect also that the shell to use be customizable.

In other words, I wish the FlexMeter template files be shell scripts themselves, with appropriate shebang and execute permission (mode) bits. This would help the security in the long run. (The last thing I wish to see is a systemd-style, INI-like format for managing executable sub-processes. I was referring to this "comparison of init systems" page.)

What to do if the command produces no output, or hangs?

AFAIK, the fgets(3) function can block if the file descriptor wasn't opened with O_NONBLOCK flag. If fgets blocks, then htop would look frozen, which is bad. Also I believe there should be a timeout of some sort for htop waiting for child process to output. We should probably handle both situations, where (a.) the child process outputs nothing or needs too much time to output (so that htop can skip it and go to the next one), and (b.) the child process outputs in a faster rate than htop can consume. In the latter situation I expect htop only displays the last line, rather than the first line waiting in the pipe, so that the data can be closer to real time when possible.

Explorer09 · 2024-12-30T11:09:05Z

In other words, I wish the FlexMeter template files be shell scripts themselves, with appropriate shebang and execute permission (mode) bits. This would help the security in the long run.

P.S. I have a rough logic of determining whether the user can or has intent to run the commands. But it needs execute permission on the template files themselves:

If the file's owner is the same as EUID of the running htop process:
   If the file does NOT have S_IXUSR permission:
      Reject. (This trumps other execute permission bits.)
   If the file has S_IXUSR permission:
      Accept.
Else, if the file belongs to a group of which the EUID is a member:
   If the file does NOT have S_IXGRP permission:
      Reject.
   If the file has S_IXGRP permission:
      Accept.
Else, if the file's owner is UID 0 (that is, root), and
   the file has S_IXOTH permission:
      Accept.
Else
   Reject.

I'm not sure if it's worth it to implements a "drop privilege" mechanism or switch back to UID on this. But if there is a way to drop privilege, then the logic of checking whether the commands should run with the UID would be same as when checking with the EUID.

BenBE · 2024-12-31T20:37:39Z

@Explorer09 What do you think about handling group and user sticky bits on the files? How to handle symlinks?

Explorer09 · 2024-12-31T22:24:12Z

@Explorer09 What do you think about handling group and user sticky bits on the files? How to handle symlinks?

Sticky bit for executables are largely irrelevant for our case. (Linux ignores it currently, and historically it controlled whether the executable's text segments should be unloaded from swap space after exit - nothing to do with permissions here.)
SUID and SGID - I would suggest rejecting files with SUID or SGID (or both) for now. I can't think of a good use case of starting a meter, executing a command/script, with a privilege of someone else. Until there is a good use case for that, we can change that position.

Note that my proposed logic only checks for execute permission of the meter (template) files themselves, and not on the executability of the commands. My imagined case is when the meter command was rm -rf /. rm(1) is world-executable or course, but it's the "command and arguments" combination that makes it dangerous to be run as root. So I go for checking the permissions on the meter files instead.

Symlinks - I don't think there can be a problem on this. Just follow the link and check for execute permission of the target. (We did this with htoprc already.)

Update: I've missed one thing about symlinks. We should only follow the symlink if it's created or owned by the user (here I mean EUID). It's unsafe to follow a link created by someone else. If a symlink is created by the user named "alice" but she runs htop as root, then the meter script should drop privileges and run as if EUID = Alice's UID.

BenBE · 2025-01-02T18:19:45Z

@Explorer09 Thanks for linking that old issue.

@bogdanovs What do you think of the ideas/protocol mentioned in that issue #526?

bogdanovs · 2025-01-02T21:30:00Z

@bogdanovs What do you think of the ideas/protocol mentioned in that issue #526?

This issue is similar to my proposal but more complex to be used in first glance. In next iteration of FlexMeter could be implemented different than TEXT_MODE, but this will require scripting on background to provide more specific data. This is not bad but more demanding from user.

FlexMeter is some how simple , not so tight to anything else. You can show whatever data you want. Issue described is more restrictive and might be harder for most average used to jump in directly.

In the proposal is mentioned just 4 additional meters which is really limiting. Currently with this implementation I have between 6 an 8 all the time. If it is custom , let it be custom.

I dont like part with open pipe all the time and somebody just feeding it.

bogdanovs · 2025-01-02T21:38:03Z

@BenBE @Explorer09

Regarding the permissions of the Meters - I think we can stick with permissions which used for SSH
For executable scripts instead of configuration files: this file could be changed and something malicious could be injected in it. Currently FlexMeter - just load the command and while htop is running we dont care for the templates. I personally dont like the idea. We could advice user for more complex operation to write script which will be called.

Explorer09 · 2025-01-03T19:24:26Z

Now is clean what you mean with 700. chmod u+x should act like sign that Meter is kind of "safe" or atleast user is aware of what meter is doing, kind of consent. In case mode is just 600 we will not list this Meter at all.

Correct. If the mode is 600, the user can still read and edit the file but would work as a sign that "this meter wasn't ready / it's a draft".

Meter configuration file will not be executed and it is not meant to be executed like standalone script, just loaded from htop at start.

I could make it work like a standalone script and use watch(1) command to monitor the same thing. watch -n 1 MyFlexMeter Does this example make sense? Same script, but monitor without htop.

bogdanovs · 2025-01-03T20:37:10Z

I could make it work like a standalone script and use watch(1) command to monitor the same thing. watch -n 1 MyFlexMeter Does this example make sense? Same script, but monitor without htop.

Yes it make sens to use watch but not very useful tbh, just to test MyFlexMeter maybe is fine.

But I still think #!/bin/sh is not needed, having script with which is not supposed to be executed execpt for testing is kind of confusing without further explanation.

Explorer09 · 2025-01-04T09:08:31Z

But I still think #!/bin/sh is not needed, having script with which is not supposed to be executed execpt for testing is kind of confusing without further explanation.

Your popen(3) call will launch the command under the /bin/sh shell anyway. I just make that fact explicit in the format.
I won't say we need to support other shebangs (Perl or Python?) but I think users would be able to workaround them quite easily. (Such as python -c my_command)

bogdanovs · 2025-01-04T09:16:39Z

Your popen(3) call will launch the command under the /bin/sh shell anyway. I just make that fact explicit in the format. I won't say we need to support other shebangs (Perl or Python?) but I think users would be able to workaround them quite easily. (Such as python -c my_command)

Correct popen is using /bin/sh -c <some_commands>. You can call whatever you want.

Well it should not affect FlexMeter configuration if there is other shebang, I can generally ignore it while loading and care only for whatever keys words I am supporting.

bogdanovs · 2025-01-04T09:36:23Z

Summary on latest update :

check ~/.config/htop/FlexMeter folder mod bits , if not 700 ignore it
check every meter under FlexMeter directory for access bits. if not 700 , meter is nor listed
Dry run every command on load. If fail for some reason fail meter is listed and when selected print '[ERR] Check command'

Explorer09 · 2025-01-04T10:27:55Z

Dry run every command on load. If fail for some reason fail meter is listed and when selected print '[ERR] Check command'

I would choose to run the meter command only when the meter is added. Not any earlier. Think of when the command is echo something >some_log_file and now the log file would be written when htop starts no matter you have the meter or not. (I can imagine some other more destructive commands here, but you should get the idea.)

I will have comments about the permission check code later.

BenBE

Possibly split the unrelated changes into a separate commit.

Also, the types within FlexMeter.c don't follow the usual naming convention?

Any reason for the hard limit on supported flex meters?

FlexMeter.c

BenBE · 2025-01-04T15:01:13Z

FlexMeter.c

+   struct passwd* pw = getpwuid(getuid());
+   const char* homedir = pw->pw_dir;


Note the comment by @Explorer09 regarding the XDG_HOME_DIR variable and fallbacks …

Is current XDG_CONFIG_HOME and fallbacks enough ?

getpwuid has some bad side effects (calls to libpam and potentially causes network traffic; thus can potentially hang). Furthermore, when building htop with --enable-static this may cause issues, thus call sites should be minimized.

Not to mention the call can fail and is lacking error checks.

Please compare with Settings_new in Settings.c. Possibly, we should refactor this code block to be a separate function. @Explorer09?

FlexMeter.c

bogdanovs · 2025-01-06T16:12:29Z

@BenBE Is it mandatory to user bool instead of int. If not I would prefer to keep function with int

BenBE · 2025-01-06T17:53:42Z

@BenBE Is it mandatory to user bool instead of int. If not I would prefer to keep function with int

If semantics are pure truth values: yes. If these functions provide full error reasons as their return value, int is fine.

If using int, use <0 (e.g. -errno) for error, and a sensible meaning for the success case.

bogdanovs · 2025-01-06T20:43:41Z

FYI: Just pushed latest version which is without some comments which are already under some kind of discussion.

FlexMeter.c

BenBE · 2025-01-07T08:23:57Z

FlexMeter.c

+            meter_list[meters_count].name = xStrdup(dir->d_name);
+            xAsprintf(&path, "%s/%s", home, dir->d_name);
+
+            if (access(path, R_OK | W_OK | X_OK) == 0) {


Do we need write access here? RX should be fine.

no we dont need write access , but this match what we discuss for 700

I think it'd be best to allow both 0500 and 0700 so the user can disable write access as that is not needed for the feature to work. The check should be for the minimum we require, with 0700 being the maximum we allow (technically even 0755 would still be fine).

(Sorry for the last comment about 0400 as I misread the code. I deleted that comment.)

By the way, do we have a better API than access(2) here? access(2) is vulnerable to TOCTOU and I wish to avoid it when possible.

An option may be to use open(2) with O_EXCL and fstat(2) the fd, before passing it on to fdopen(3) in load_config, which would need to receive the fd (or a FILE*, if we did the fdopen(3) in the caller).

There is even the possibility to use faccessat(2) with AT_EMPTY_FILE to emulate access(2) using an already open fd, but this is Linux-specific (read: non-portable) and requires a recent (>=5.8) kernel to work.

what about replacing access(2) with stat(2) like for folder ?

@BenBE @bogdanovs
How about doing it in the same way that we check for htoprc?
open(2) the file with the option (O_RDONLY | O_NOFOLLOW); after it succeeds, fstat(2) the file descriptor and check its execute bit (sb.st_mode & S_IXUSR) != 0; after both of these are done, fdopen(3).

The stat structure should give us enough information to check not only for modes but also owner ID and group ID. If you are able, you may try implementing the whole logic I made in a previous comment.

@BenBE @bogdanovs How about doing it in the same way that we check for htoprc? open(2) the file with the option (O_RDONLY | O_NOFOLLOW); after it succeeds, fstat(2) the file descriptor and check its execute bit (sb.st_mode & S_IXUSR) != 0; after both of these are done, fdopen(3).

The stat structure should give us enough information to check not only for modes but also owner ID and group ID. If you are able, you may try implementing the whole logic I made in a previous comment.

if we agree on stat approach I will implement it that way

@Explorer09 What's the difference in your suggestion compared to mine? Mine was mostly focused on the actual files, but would work similar for the directory too.

@BenBE Probably no difference, except that the O_EXCL flag you mentioned won't do what you want. (O_EXCL is for creating the file only - it forces open(2) to error out if a file with the same name exists).

BenBE · 2025-01-07T08:30:15Z

FlexMeter.c

+         FlexMeter_class[i].name = (const char*) xStrdup(meter_list[i].name);
+         FlexMeter_class[i].uiName = (const char*) xStrdup(meter_list[i].uiName);
+         FlexMeter_class[i].caption = (const char*) xStrdup(meter_list[i].caption);


Given the unrestricted nature of how the uiName and caption are set, a FlexMeter could just configure to use the same strings as another (built-in) meter and thus impersonate another (e.g. built-in) meter. This is not a security issue directly, but more a point for discussion, if we want to allow the user to create FlexMeters that are indistinguishable from built-in meters.

FlexMeter.c

Header.c

Explorer09 · 2025-01-10T21:58:45Z

FlexMeter.c

+   if (home != NULL) {
+      free(home);
+      home = NULL;
+   }


You can assume free(NULL) will do nothing. This behavior is required by ANSI C standard. (Operating systems that do not have this behavior are too old for htop to support.)

To be precise: A libc where free(NULL); is anything but a no-op does not comply to the ISO C99 standard (section 7.20.3.2) and is thus outside the scope of what we support.

Just to confirm which approach is preferred here with if (ptr != NULL) of without ?

Without …

My fault , initially I understood that this check is required.
I will remove them

bogdanovs · 2025-01-16T19:52:35Z

This is the format of my vision:
#!/bin/sh
#htop_meter_name=Meter name
#htop_meter_modes=text
#htop_meter_caption=Caption

# Commands follow...
uptime
That is ok like format, but still cant get the point of having #!/bin/bash and file have execute permissions if this it is not supposed to be used like script and executed and we load it to htop instance like now and not touch it again until next re-spawn. Maybe I am missing some point..

I think this format will encourage users to have more complex multi line commands which will cause issues.

I would like to keep current format:

name=Meter name
type=text
caption=Caption
command=<one line command or call script>

FlexMeter provides functionality which will allow users to make custom meters without need of rebuilding every time htop binary and adding source to the project. It can be used to print some device status, free disk space CPU or other specific temeraturer, fan RPM and many more. Everything that can be fetched from linux shell with one line result can be printer. For fething information can be used anything from shell, python, precompiled binary or simply reading file located somewhere in file system. New meter will appear uppon restart of htop in list with available meters. Configuration folder location where metes should be placed: - /home/$USER/.config/htop/FlexMeter/ On start folder will be created if does not exist, together with template file .Template in same folder. Note: Files starting with '.' (.Template for examlpe) are ignored Meter Example: File name : Template name=<NAME SHOWN IN AvailableMeters> command=<COMMAND WHICH WILL BE EXECUTED> type=<METER TYPE FOR NO ONLY "TEXT_METER"> caption="CAPTION TEXT SHOWN IN THE BEGGINING OF THE METER" According to this implementation 30 Flex meter can be added Currently they have hardcoded limit of 30 meter in addition to all that already exist. Signed-off-by: Stoyan Bogdanov <[email protected]>

Signed-off-by: Stoyan Bogdanov <[email protected]>

bogdanovs · 2025-01-16T20:47:46Z

In last update

add XDG_CONFIG_HOME with fallback
Remove checks for NULL
use stat instead of access for folder

Explorer09 · 2025-01-16T21:37:14Z

I think this format will encourage users to have more complex multi line commands which will cause issues.

What issues? It was intentional for me to support multiline commands, because there's no reason not to. Also, how was it any different if I can squeeze multiple commands already into one line?

BenBE · 2025-01-16T23:02:29Z

The only way I think supporting multi-line commands makes sense is if we were to run the config files as a shell script (in which case the shabang+commented-out settings make sense). But this opens up a whole other can of worms. In the end, either way will handle arbitrary scripts run with some set of privileges (TBD), where you will be able to spawn shells and sub-processes. TBH; there's no sane way to make this anywhere "safe".

But, going the "shell script route" encourages to have more complex behaviours packaged in such meters, which is IMO nothing we should encourage, even though it's possible (and there's no real way in stopping the user from doing). Thus I somewhat prefer the single-line approach as it tends to make the config files simpler and cause the nitty-gritty details out into external scripts that can be written in whatever language fits better than a basic shell script used to bootstrap things.

Explorer09 · 2025-01-17T00:37:32Z

But, going the "shell script route" encourages to have more complex behaviours packaged in such meters, which is IMO nothing we should encourage, even though it's possible (and there's no real way in stopping the user from doing). Thus I somewhat prefer the single-line approach as it tends to make the config files simpler and cause the nitty-gritty details out into external scripts that can be written in whatever language fits better than a basic shell script used to bootstrap things.

There is nothing going simpler when you limit the command to one line. The real thing that could make the script simpler is limiting the total length of the commands htop could load per file. For example 4 KiB.

Also we can limit the interpreter shebang to /bin/sh only. Any other language will require launching the interpreter indirectly.

The point of putting multiple commands in a meter file is that htop can keep the commands in memory so that any modification to the meter file after htop started would not affect that htop instance of starting commands. If the commands are launched through another script (be it shell script or Perl or Python), then they lose this advantage.

Explorer09 · 2025-01-17T11:42:17Z

Additional comments...

The only way I think supporting multi-line commands makes sense is if we were to run the config files as a shell script (in which case the shabang+commented-out settings make sense).

The reality is that we are running it as a shell script due to how popen(3) command works.

If we have been forking and then using exec(3) family of functions, without p in function name, then we can model the meter file format as a simpler, one-command directive file.

As I mentioned in a comment before, the last thing I wished to see is an executable script file modeled like an INI that looked harmless in a glance. (Many INI-like files in Microsoft Windows can contain executable code, mind you, the most notable of which are driver installation .inf files. I've worked with such files before and thus I know them.)

In the end, either way will handle arbitrary scripts run with some set of privileges (TBD), where you will be able to spawn shells and sub-processes. TBH; there's no sane way to make this anywhere "safe".

Acknowledged. Although my vision is to makes things simple and utilize Unix mechanism of user privileges whenever possible.

That is to say, (1) don't be afraid of running arbitrary scripts with elevated privileges if there are enough permission bits indicating the users have intent to do so, and (2) we can restrict the execution of scripts that are owned by someone else, except when the owner is root, who we can assume trust with.

By the way, I've changed some mind about the SUID bit on meter files. I think we can allow them. Even though that could result in an elevation of privilege, ultimately it's not the fault of us or the user who execute it, it is the file owner who is responsible.

BenBE · 2025-01-17T12:23:17Z

Additional comments...

The only way I think supporting multi-line commands makes sense is if we were to run the config files as a shell script (in which case the shabang+commented-out settings make sense).

The reality is that we are running it as a shell script due to how popen(3) command works.

If we have been forking and then using exec(3) family of functions, without p in function name, then we can model the meter file format as a simpler, one-command directive file.

As I mentioned in a comment before, the last thing I wished to see is an executable script file modeled like an INI that looked harmless in a glance. (Many INI-like files in Microsoft Windows can contain executable code, mind you, the most notable of which are driver installation .inf files. I've worked with such files before and thus I know them.)

I'm afraid of neither and the point for discussion basically boils down to whether we want soem INI like format which emphasizes/prioritizes single-line commands or if we would prefer to go the route of shell scripts, with the meta-information placed in a header at the top of the file.

I'm fine with either way, but somewhat prefer the INI format as it makes parsing out the information we need to know for the meter much simpler. In the end, you can't run code without somewhere placing the code you want to run.

In the end, either way will handle arbitrary scripts run with some set of privileges (TBD), where you will be able to spawn shells and sub-processes. TBH; there's no sane way to make this anywhere "safe".

Acknowledged. Although my vision is to makes things simple and utilize Unix mechanism of user privileges whenever possible.

No objection here.

That is to say, (1) don't be afraid of running arbitrary scripts with elevated privileges if there are enough permission bits indicating the users have intent to do so,

I just tend to be very careful whenever there is some "arbitrary code execution" on a code path. Occupational hazard …

and (2) we can restrict the execution of scripts that are owned by someone else, except when the owner is root, who we can assume trust with.

ACK. But coming from an IT sec perspective, I'd prefer to look at it that way: How much do we have to allow for things to work properly. Ideally, scripts would run with no more than the privileges of the owner, but also no higher privileges than the user who executes them. Which brings us to the next point:

By the way, I've changed some mind about the SUID bit on meter files. I think we can allow them. Even though that could result in an elevation of privilege, ultimately it's not the fault of us or the user who execute it, it is the file owner who is responsible.

We could use the SUID bit in the sense it is used on regular executables: Run as the owner of that file. That way, by default meters would be executed as the user running htop, except when a meter is SUID, in which case it would inherit that user's privileges when executing the meter. This would leave us with 10 cases basically:

htop as root, meter by root, no SUID: runs as root (htop user)
htop as root, meter by non-root, no SUID: does not run (prevent privesc)
htop as root, meter by root, SUID: runs as root (SUID)
htop as root, meter by non-root, SUID: run as non-root (SUID) -> enforce priv downgrade(!!!)
htop as non-root, meter by root, no SUID: runs as non-root (htop user)
htop as non-root, meter by non-root (same), no SUID: run as non-root (htop user)
htop as non-root, meter by non-root (different), no SUID: does not run (prevent privesc)
htop as non-root, meter by root, SUID: run as root (SUID)
htop as non-root, meter by non-root (same), SUID: run as non-root (SUID)
htop as non-root, meter by non-root (different), SUID: does not run (prevent privesc)

Alternatively, you could ignore SUID if the meter was not owned by root, which gives the following cases:

htop as root, meter by root, no SUID: runs as root (htop user)
htop as root, meter by non-root, no SUID: does not run (prevent privesc)
htop as root, meter by root, SUID: runs as root (SUID)
htop as root, meter by non-root, SUID: does not run (prevent privesc)
htop as non-root, meter by root, no SUID: runs as non-root (htop user)
htop as non-root, meter by non-root (same), no SUID: run as non-root (htop user)
htop as non-root, meter by non-root (different), no SUID: does not run (prevent privesc)
htop as non-root, meter by root, SUID: run as root (SUID)
htop as non-root, meter by non-root (same), SUID: does not run (prevent privesc)
htop as non-root, meter by non-root (different), SUID: does not run (prevent privesc)

BenBE · 2025-01-17T13:59:29Z

FlexMeter.c

+   {
+      case 'n':
+         if (String_startsWith(line, "name=")) {
+            xAsprintf(&meter->uiName, "Flex: %s", line + 5);


Memory leak, if multiple lines with name= are provided.

Similar below.

Have a look at free_and_xStrdup for the cases below.
For the xAsprintf call site calling free on meter->uiName should suffice.

BenBE · 2025-01-17T14:05:59Z

FlexMeter.c

+   bool ret = false;
+   FILE* fp = fopen(file, "r");
+
+   if (fp != NULL) {


Note, that fclose(NULL); is undefined behaviour, usually characterized to crash your program. ;-)

Suggested change

if (fp != NULL) {

if (!fp) {

return false;

}

Doing it that way around also allows to lower the indentation level.

BenBE · 2025-01-17T14:07:33Z

FlexMeter.c

+         }
+      }
+      free(buff);
+      buff = NULL;


Not strictly necessary. should best be removed.

For stack variables, calling free suffices. Only when the variable or its value is exposed to other scopes it's necessary to explicitly set the value to NULL.

BenBE · 2025-01-17T14:12:12Z

FlexMeter.c

+   struct passwd* pw = getpwuid(getuid());
+   const char* homedir = pw->pw_dir;


getpwuid has some bad side effects (calls to libpam and potentially causes network traffic; thus can potentially hang). Furthermore, when building htop with --enable-static this may cause issues, thus call sites should be minimized.

Not to mention the call can fail and is lacking error checks.

Please compare with Settings_new in Settings.c. Possibly, we should refactor this code block to be a separate function. @Explorer09?

BenBE · 2025-01-17T14:29:18Z

FlexMeter.c

+         FlexMeter_class[i].name = (const char*) xStrdup(meter_list[i].name);
+         FlexMeter_class[i].uiName = (const char*) xStrdup(meter_list[i].uiName);
+         FlexMeter_class[i].caption = (const char*) xStrdup(meter_list[i].caption);


For caption= all* >=32 is fine. With just 26<=c && c<=126 you cause two issues:

You allow for inserting ECMA48 escape sequences (which can rebind keys -> security issue)

You break Unicode support for things like caption=運勢 (when executing command=LANG=ja_JA fortune)

*Unicode filtering has its whole other can of worms … But that would apply to the output of command too …

BenBE · 2025-01-17T14:37:33Z

dragonflybsd/Platform.c

@@ -43,7 +44,6 @@ in the source distribution for its full text.
 #include "dragonflybsd/DragonFlyBSDProcessTable.h"
 #include "generic/fdstat_sysctl.h"

-


Second blank line after includes.

BenBE · 2025-01-17T14:38:02Z

openbsd/Platform.c

@@ -47,7 +48,6 @@ in the source distribution for its full text.
 #include "openbsd/OpenBSDMachine.h"
 #include "openbsd/OpenBSDProcess.h"

-


Second blank line after includes.

BenBE · 2025-01-17T14:38:10Z

pcp/Platform.c

@@ -55,7 +56,6 @@ in the source distribution for its full text.
 #include "zfs/ZfsArcStats.h"
 #include "zfs/ZfsCompressedArcMeter.h"

-


Second blank line after includes.

BenBE · 2025-01-17T14:38:19Z

solaris/Platform.c

@@ -43,7 +44,6 @@ in the source distribution for its full text.
 #include "zfs/ZfsArcMeter.h"
 #include "zfs/ZfsCompressedArcMeter.h"

-


Second blank line after includes.

BenBE · 2025-01-17T14:38:27Z

unsupported/Platform.c

@@ -27,7 +28,6 @@ in the source distribution for its full text.
 #include "TasksMeter.h"
 #include "UptimeMeter.h"

-


Second blank line after includes.

Explorer09 · 2025-01-17T19:54:38Z

I'm afraid of neither and the point for discussion basically boils down to whether we want soem INI like format which emphasizes/prioritizes single-line commands or if we would prefer to go the route of shell scripts, with the meta-information placed in a header at the top of the file.

I'm fine with either way, but somewhat prefer the INI format as it makes parsing out the information we need to know for the meter much simpler. In the end, you can't run code without somewhere placing the code you want to run.

The INI-like format is foreign to a Unix like environment when it comes to shell commands. That's the primary reason I suggest against that.

Really. You need to handle the quoting and character escape mechanisms, and perhaps also distinguishing ; (a semicolon) as a command delimiter and ; as a literal argument (to a command such as find(1)). Even though we are unlikely to
get a security vulnerability when misinterpreting a user command, such issue can block any future extensibility to the meter format, and that's what I wished to avoid.

rm -f foo ; rm -f bar
# (Are we supposed to delete the files named ";", "rm" and "-f" ?)

Just for another information, I know that an INI-like format could work if we are executing a program directly without a shell. The closest example we can take as reference is the desktop entry format (.desktop extension, this is the Unix/Linux counterpart to the .lnk files in Windows).

We could use the SUID bit in the sense it is used on regular executables: Run as the owner of that file. That way, by default meters would be executed as the user running htop, except when a meter is SUID, in which case it would inherit that user's privileges when executing the meter. This would leave us with 10 cases basically:

htop as root, meter by root, no SUID: runs as root (htop user)

htop as root, meter by non-root, no SUID: does not run (prevent privesc)

htop as root, meter by root, SUID: runs as root (SUID)

htop as root, meter by non-root, SUID: run as non-root (SUID) -> enforce priv downgrade(!!!)

htop as non-root, meter by root, no SUID: runs as non-root (htop user)

htop as non-root, meter by non-root (same), no SUID: run as non-root (htop user)

htop as non-root, meter by non-root (different), no SUID: does not run (prevent privesc)

htop as non-root, meter by root, SUID: run as root (SUID)

htop as non-root, meter by non-root (same), SUID: run as non-root (SUID)

htop as non-root, meter by non-root (different), SUID: does not run (prevent privesc)

I was about to think of all possible cases before you presented this list to me. Thanks.

For case 2., this is a case I also wish to avoid, and I should mention that htop should also ignore the "others execute" bit in this situation.
Case 5 would require the non-root user to have execute permission on that meter (either "group execute" or "other execute"), but I think everyone got the idea already.
For case 7, I think we can loosen the requirement by allowing the meter to be run if and only if the user and meter file belong to the same group, and it has "group execute" permission. The "others execute" would be ignored, in particular.
Case 10 is like case 7 that we can loosen the requirement to allow the meter to run with "group execute" permission. Here I think even the "others execute" would be safe here, since the privilege is limited to whoever owns the meter as SUID is in effect.

How to combine these points into a privilege evaluation logic/code that we can make it into htop, by the way?

BenBE · 2025-01-17T21:10:14Z

How to combine these points into a privilege evaluation logic/code that we can make it into htop, by the way?

file_uid = stat.uid;
file_mask = stat.mode;
file_suid = file_mask & 04000 != 0;
file_rx = file_mask & 0500 == 0500;
htop_uid = getuid();

if not file_rx:
   // do not exec
   return

if file_uid == htop_uid:
   // Execute as normal
elif file_uid == 0:
   if file_suid:
      // run SUID
   else:
      // run normally
elif htop_uid == 0:
   // Do not execute
else:
   assert(false && "Should not reach here");

If I didn't mess up …

Also, I'm deliberately ignoring any bits apart from the owner bits and SUID.

BenBE added needs-discussion 🤔 Changes need to be discussed and require consent new feature Completely new feature security 👮 Issues with security implications labels Dec 17, 2024

BenBE added this to the 3.4.0 milestone Dec 17, 2024

bogdanovs force-pushed the bogdanov/flexmeter branch from 275830e to 595b1ee Compare December 29, 2024 21:07