FlexMeter: Add FlexMeter functionality

[bogdanovs]

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]

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]

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]

@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]

@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]

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

[Explorer09]

Header order please.

[Explorer09]

Please use String_startsWith when possible. It saves your time manually updating the string length.

By the way, if we are forbidding whitespace between name and equal sign, better state that explicitly in the documentation.

[bogdanovs]

Please use String_startsWith when possible. It saves your time manually updating the string length.

Replacing it with String_startsWith is causing - Segmentation fault.

By the way, if we are forbidding whitespace between name and equal sign, better state that explicitly in the documentation.

Is man page enough ?

[Explorer09]

Is man page enough ?

As I have comments (or maybe "criticisms") about the current format of the template file, I think we can think of updating the man page later. Yes, the format is supposed to be documented, but we can think of documentation later, until the code is stable enough.

[Explorer09]

Use xSnprintf. sprintf is insecure.

[Explorer09]

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.

[bogdanovs]

I will check that, thanks

[Explorer09]

1. Use size_t for iterator when possible.
2. Opening brace on the same line of for keyword, please. (Our style is closer to K&R rather than Allman here.)

[BenBE]

Also note the placement of blanks/spaces according to the styleguide.

[bogdanovs]

I will start applying astyle -xb -s3 -p -xg -c -k1 -W1 -H FlexMeter.c after every change this should be fine, right ?

[Explorer09]

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

1. 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.)

2. 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]

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]

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

[Explorer09]

@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]

@Explorer09 Thanks for linking that old issue.

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

[bogdanovs]

@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]

@BenBE @Explorer09

1. Regarding the permissions of the Meters - I think we can stick with permissions which used for SSH
2. 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]

1. Regarding the permissions of the Meters - I think we can stick with permissions which used for SSH

I didn't get this. Would you explain?

2. 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.

My vision is that this file would work both as a configuration and executable script. The file format would be a polyglot. When htop starts, it loads the whole file into memory (there's a good reason for this - if the file is changed in the middle we don't want the htop instance to reflect the change until it restarts), parsing the first few lines for the meter information. It will not execute the command. When the users adds the meter, htop runs the command and grab its standard output.

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

[bogdanovs]

1. Regarding the permissions of the Meters - I think we can stick with permissions which used for SSH

I didn't get this. Would you explain?

Permissions for .ssh folder and keys

chmod 700 ~/.ssh/ # same for FlexMeter folder like you suggested
chmod 700 ~/.config/htop/FlexMeter

chmod 600 ~/.ssh/authorized_keys # same for FlexMeter meter templates
chmod 600 ~/.config/htop/FlexMeter/SampleMeter

2. 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.

My vision is that this file would work both as a configuration and executable script. The file format would be a polyglot. When htop starts, it loads the whole file into memory (there's a good reason for this - if the file is changed in the middle we don't want the htop instance to reflect the change until it restarts), parsing the first few lines for the meter information. It will not execute the command. When the users adds the meter, htop runs the command and grab its standard output.

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..

[Explorer09]

Permissions for .ssh folder and keys

chmod 700 ~/.ssh/ # same for FlexMeter folder like you suggested
chmod 700 ~/.config/htop/FlexMeter

chmod 600 ~/.ssh/authorized_keys # same for FlexMeter meter templates
chmod 600 ~/.config/htop/FlexMeter/SampleMeter

Imagine you are root and you accidentally put a rm -ri / command in the SampleMeter file. The SampleMeter has mode 600 but htop is happy to run the command and destroy your computer...

[bogdanovs]

Permissions for .ssh folder and keys

chmod 700 ~/.ssh/ # same for FlexMeter folder like you suggested
chmod 700 ~/.config/htop/FlexMeter

chmod 600 ~/.ssh/authorized_keys # same for FlexMeter meter templates
chmod 600 ~/.config/htop/FlexMeter/SampleMeter

Imagine you are root and you accidentally put a rm -ri / command in the SampleMeter file. The SampleMeter has mode 600 but htop is happy to run the command and destroy your computer...

You mentioned that earlier, but this is always possibility if you are running like root. In case we use XDG_CONFIG_HOME, if run like root htop will look for meters in /root/.config/htop/FlexMeter.

You think this is too open, I am confused?

We could try to not allow execution or rm with simple regex while loading meters?

[Explorer09]

You mentioned that earlier, but this is always possibility if you are running like root. In case we use XDG_CONFIG_HOME, if run like root htop will look for meters in /root/.config/htop/FlexMeter.

XDG_CONFIG_HOME can point to any directory. And we cannot assume the directory is always safe.

We could try to not allow execution or rm with simple regex while loading meters?

Any program that can call unlink(2) can come with a risk of ruining your computer. Besides, there can be alias uptime=rm that would get around your regex check.

[bogdanovs]

@Explorer09 all that you stated is true, I agree.

But If we think in that direction even htop binary could be security risk. Every single program running on the system could be malicious and could harm your system. Every systemd service or other daemon or even ls or cd could be linked to rm or other harmful software.

Do you have an idea how this risk could be mitigated?

I understand you concerns so we will not provide automatically generated sample on the system.
Lets allow users to write their own Meters, if user system in compromised or hacked this another topic.

[Explorer09]

But If we think in that direction even htop binary could be security risk. Every single program running on the system could be malicious and could harm your system. Every systemd service or other daemon or even ls or cd could be linked to rm or other harmful software.

Do you have an idea how this risk could be mitigated?

The Unix practice is to avoid running things as root whenever possible. Unix gives so much power to the root user, and it assumes that root is competent and responsible.

How responsible can a root account be for machines that aren't ours? Well, we can't tell. But what we can do is to avoid accidentally running things that a user (root or not) would not intend to run. And there is one feature designed for this purpose: The execute permission bit.

That was why I suggest flex meter files should have chmod 700 and not chmod 600. If a user is not comfortable turning on the execute bit and run that meter file like an ordinary command, then the meter shouldn't be run by htop either. That's the idea.

I won't prevent a root user from running rm in htop, but if they want to do so, they have to chmod u+x to tell they mean it. Basic defense for preventing accidentally running a program.

[bogdanovs]

The Unix practice is to avoid running things as root whenever possible. Unix gives so much power to the root user, and it assumes that root is competent and responsible.

How responsible can a root account be for machines that aren't ours? Well, we can't tell. But what we can do is to avoid accidentally running things that a user (root or not) would not intend to run. And there is one feature designed for this purpose: The execute permission bit.

That was why I suggest flex meter files should have chmod 700 and not chmod 600. If a user is not comfortable turning on the execute bit and run that meter file like an ordinary command, then the meter shouldn't be run by htop either. That's the idea.

I won't prevent a root user from running rm in htop, but if they want to do so, they have to chmod u+x to tell they mean it. Basic defense for preventing accidentally running a program.

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.

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

Now we are on the same page, I will implement it that way.

[Explorer09]

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]

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]

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]

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]

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]

• 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?

[BenBE]

bool is a thing …

[Explorer09]

Some coding styles do use int to return a status of an operation (e.g. Linux kernel). It might be simply that they don't understand htop uses bool for operation statuses and I would personally tolerate that.

But, if there is no need to indicate statuses other than success and failure, then do stick with existing code and use bool.

[bogdanovs]

I personally prefer int tbh

[BenBE]

My preference is to avoid plain int and long as much as possible, especially if there is a proper type that carries the intended semantics (bool, size_t, uintptr_t, …) or provides an indication of what range of numbers (uint32_t) the author expected a given variable to carry.

[BenBE]

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

[bogdanovs]

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

[BenBE]

@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]

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

[BenBE]

Header order; cf. styleguide …

[BenBE]

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

[BenBE]

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.

[BenBE]

My preference is to avoid plain int and long as much as possible, especially if there is a proper type that carries the intended semantics (bool, size_t, uintptr_t, …) or provides an indication of what range of numbers (uint32_t) the author expected a given variable to carry.