htop-dev
FlexMeter: Add FlexMeter functionality
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.
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.
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.
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.
@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 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 …
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 https://github.com/htop-dev/htop/pull/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
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.
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.
@Explorer09 What do you think about handling group and user sticky bits on the files? How to handle symlinks?
@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
htoprcalready.)
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.
@Explorer09 Thanks for linking that old issue.
@bogdanovs What do you think of the ideas/protocol mentioned in that issue #526?
@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.
@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.
- 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?
- 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
- 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
- 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..
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...
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/SampleMeterImagine 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?
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.
@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.
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
lsorcdcould be linked tormor 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.
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 700and notchmod 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
rmin htop, but if they want to do so, they have tochmod u+xto 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.
Now is clean what you mean with 700.
chmod u+xshould 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.
I could make it work like a standalone script and use watch(1) command to monitor the same thing.
watch -n 1 MyFlexMeterDoes 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.
But I still think
#!/bin/shis 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)
Your popen(3) call will launch the command under the
/bin/shshell 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 aspython -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.
Summary on latest update :
- check
~/.config/htop/FlexMeterfolder 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'
- 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 Is it mandatory to user bool instead of int. If not I would prefer to keep function with int
@BenBE Is it mandatory to user
boolinstead ofint. If not I would prefer to keep function withint
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.
FYI: Just pushed latest version which is without some comments which are already under some kind of discussion.
This is the format of my vision:
#!/bin/sh #htop_meter_name=Meter name #htop_meter_modes=text #htop_meter_caption=Caption # Commands follow... uptimeThat is ok like format, but still cant get the point of having
#!/bin/bashand 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>