Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FlexMeter: Add FlexMeter functionality #1571

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bogdanovs
Copy link

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.

htop-flexmeter

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]>
@BenBE
Copy link
Member

BenBE commented Dec 17, 2024

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.

@BenBE 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 BenBE added this to the 3.4.0 milestone Dec 17, 2024
@Explorer09
Copy link
Contributor

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
Copy link
Author

@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
Copy link
Member

BenBE commented Dec 18, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-discussion 🤔 Changes need to be discussed and require consent new feature Completely new feature security 👮 Issues with security implications
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants