Skip to content

Hardening: verify that we send signals by PID to programs with expected name #2464

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

Merged
merged 10 commits into from
Jun 10, 2024

Conversation

jimklimov
Copy link
Member

@jimklimov jimklimov commented Jun 8, 2024

Closes: #2463

Per discussion in the issue above, blind PID signalling is a problem on embedded platforms which do not differentiate user accounts, so a NUT driver can kill some unrelated program assuming it is a stuck driver (just by PID file matching) and not be stopped by permissions to send a signal to a program not owned by a nut account.

This is a platform-dependent solution, so by default if we can not determine a program name - we do not forbid signal-sending. Likewise, if a name is detected and seems to match expectations (modulo directory prefix, or on some platforms - case-sensitivity or .exe suffix) we also consider it a match. Finally, only if we detect a name of the running process with the specified PID and it is not anything like our expectation, the check is failed.

With current NUT master (or with the PR in place and NULL instead of progname passed to the method), the test command below signals (and kills) the current shell process as $$ - owned by the same user as upsmon in the test. With the change in the PR, such a name is detected and rejected:

:; make -ksj && ./clients/upsmon -DDDDDD -c reload -P $$
...
Network UPS Tools upsmon 2.8.2-315-g41ce7c7c6
   0.000000     [D3] getprocname: /proc is an accessible directory, investigating
   0.000034     [D3] getprocname: located symlink for PID 40107 at: /proc/40107/exe
   0.000064     [D1] getprocname: determined process name for PID 40107: /usr/bin/bash
   0.000086     [D1] checkprocname: did not find any match of program names for PID 40107 of '/usr/bin/bash'=>'bash' and checked 'upsmon'=>'upsmon'
   0.000106     Tried to signal PID 40107 which exists but is not of program 'upsmon'
   0.000125     [D1] Just failed to send signal, no daemon was running
   0.000142     Failed to signal the currently running daemon (if any)
   0.000149     Try 'systemctl reload nut-monitor.service'

There are a few more platform-specific code paths to handle, expected with later commits.

CC @arnaudquette-eaton @ericclappier-eaton : this internal API change for sendsignal*() methods may impact or benefit your work too, pinging just in case :)

@jimklimov jimklimov added enhancement service/daemon start/stop General subject for starting and stopping NUT daemons (drivers, server, monitor); also BG/FG/Debug portability We want NUT to build and run everywhere possible Shutdowns and overrides and battery level triggers Issues and PRs about system shutdown, especially if battery charge/runtime remaining is involved impacts-release-2.8.2 Issues reported against NUT release 2.8.2 (maybe vanilla or with minor packaging tweaks) labels Jun 8, 2024
@jimklimov jimklimov added this to the 2.8.3 milestone Jun 8, 2024
@AppVeyorBot
Copy link

jimklimov added 2 commits June 8, 2024 20:03
…ignal*() via old PID only to same progname [networkupstools#2463]

Internal API change for common.c/h

Signed-off-by: Jim Klimov <[email protected]>
@jimklimov
Copy link
Member Author

Added /proc/.../cmdline support:

Network UPS Tools upsmon 2.8.2-323-g829dca21f
   0.000000     [D3] getprocname: /proc is an accessible directory, investigating
   0.000036     [D3] getprocname: try to parse some files under /proc
   0.000085     [D3] getprocname: try to parse some files under /proc: processing /proc/40107/cmdline
   0.000090     [D1] getprocname: determined process name for PID 40107: bash
   0.000093     [D1] checkprocname: did not find any match of program names for PID 40107 of 'bash'=>'bash' and checked 'upsmon'=>'upsmon'
   0.000098     Tried to signal PID 40107 which exists but is not of program 'upsmon'
   0.000100     [D1] Just failed to send signal, no daemon was running
   0.000103     Failed to signal the currently running daemon (if any)
   0.000110     Try 'systemctl reload nut-monitor.service'

@AppVeyorBot
Copy link

@jimklimov
Copy link
Member Author

FreeBSD:

Network UPS Tools upsmon 2.8.2-245-gfcd1ed355
   0.000006     [D3] getprocname: /proc is an accessible directory, investigating
   0.000098     [D3] getprocname: located symlink for PID 930 at: /proc/930/file
   0.000135     [D1] getprocname: determined process name for PID 930: /usr/libexec/getty
   0.000148     [D1] checkprocname: did not find any match of program names for PID 930 of '/usr/libexec/getty'=>'getty' and checked 'upsmon'=>'upsmon'
   0.000157     Tried to signal PID 930 which exists but is not of program 'upsmon'
   0.000166     [D1] Just failed to send signal, no daemon was running
   0.000175     Failed to signal the currently running daemon (if any)

@jimklimov
Copy link
Member Author

jimklimov commented Jun 8, 2024

OpenBSD (no /proc!):

Network UPS Tools upsmon 2.8.1-973-g1d768d417
   0.000001     [D3] getprocname: /proc is not a directory or not accessible: No such file or directory
   0.000153     [D3] getprocname: try to parse BSD KVM process info snapsnot
   0.000267     [D5] getprocname: processing reply #0 from BSD kvm_getprocs: pid=4597 name='sndiod'
   0.000312     [D1] getprocname: determined process name for PID 4597: sndiod
   0.000373     [D1] checkprocname: did not find any match of program names for PID 4597 of 'sndiod'=>'sndiod' and checked 'upsmon'=>'upsmon'
   0.000386     Tried to signal PID 4597 which exists but is not of program 'upsmon'
   0.000391     [D1] Just failed to send signal, no daemon was running
   0.000395     Failed to signal the currently running daemon (if any)

UPDATE: FreeBSD has a different set of arguments for kvm_getproc() so can not use this code path easily. Not too pressing to have it though.

@jimklimov
Copy link
Member Author

OmniOS (via cmdline):

Network UPS Tools upsmon 2.8.0-2250-g80767e396
   0.000000     [D3] getprocname: /proc is an accessible directory, investigating
   0.000068     [D3] getprocname: try to parse some files under /proc
   0.001391     [D3] getprocname: try to parse some files under /proc: processing /proc/10374/cmdline
   0.001439     [D1] getprocname: determined process name for PID 10374: /usr/sbin/sshd
   0.001464     [D1] checkprocname: did not find any match of program names for PID 10374 of '/usr/sbin/sshd'=>'sshd' and checked 'upsmon'=>'upsmon'
   0.001490     Tried to signal PID 10374 which exists but is not of program 'upsmon'
   0.001516     [D1] Just failed to send signal, no daemon was running
   0.001541     Failed to signal the currently running daemon (if any)

@jimklimov
Copy link
Member Author

jimklimov commented Jun 9, 2024

OmniOS, staged to use Solaris/illumos native /proc/nnnnn/psinfo:

Network UPS Tools upsmon 2.8.0-2250-g80767e396
   0.000000     [D3] getprocname: /proc is an accessible directory, investigating
   0.000158     [D3] getprocname: try to parse some files under /proc: processing /proc/10374/psinfo
   0.000200     [D1] getprocname: determined process name for PID 10374: sshd
   0.000230     [D1] checkprocname: did not find any match of program names for PID 10374 of 'sshd'=>'sshd' and checked 'upsmon'=>'upsmon'
   0.000257     Tried to signal PID 10374 which exists but is not of program 'upsmon'
   0.000280     [D1] Just failed to send signal, no daemon was running
   0.000303     Failed to signal the currently running daemon (if any)

@jimklimov
Copy link
Member Author

For kicks, also staged a native-Windows process lookup. It is unlikely to fire in real life, as we send "signals" on WIN32 via named pipes. But conceptually, process name lookup works (custom builds to check a 7-zip PID with jumpy casing and a lack of extension).

  • Jumpy casing:
Network UPS Tools upsmon 2.8.2-328-gc1afdacf6
   0.000000     [D5] getprocname: begin to query WIN32 process info
   0.004707     [D3] getprocname: try to parse the name from WIN32 process info
   0.010100     [D1] getprocname: determined process name for PID 37868: C:\Program Files\7-Zip\7zFM.exe
   0.017009     [D1] checkprocname: case-insensitive base name hit for PID 37868 of 'C:\Program Files\7-Zip\7zFM.exe'=>'7zFM.exe' and checked '7zfm.Exe'=>'7zfm.Exe'
   0.028726     [D1] Just failed to send signal, no daemon was running
   0.033601     Failed to signal the currently running daemon (if any)
  • a lack of extension:
Network UPS Tools upsmon 2.8.2-328-gc1afdacf6
   0.000000     [D5] getprocname: begin to query WIN32 process info
   0.005002     [D3] getprocname: try to parse the name from WIN32 process info
   0.010002     [D1] getprocname: determined process name for PID 37868: C:\Program Files\7-Zip\7zFM.exe
   0.017003     [D1] checkprocname: exact case-insensitive base name hit with an executable program extension involved for PID 37868 of 'C:\Program Files\7-Zip\7zFM.exe'=>'7zFM.exe' and checked '7zfm'=>'7zfm'
   0.031003     [D1] Just failed to send signal, no daemon was running
   0.036002     Failed to signal the currently running daemon (if any)

@jimklimov jimklimov added the C-str Issues and PRs about C/C++ methods, headers and data types dealing with strings and memory blocks label Jun 9, 2024
@jimklimov jimklimov merged commit 2229880 into networkupstools:master Jun 10, 2024
27 of 31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-str Issues and PRs about C/C++ methods, headers and data types dealing with strings and memory blocks enhancement impacts-release-2.8.2 Issues reported against NUT release 2.8.2 (maybe vanilla or with minor packaging tweaks) portability We want NUT to build and run everywhere possible service/daemon start/stop General subject for starting and stopping NUT daemons (drivers, server, monitor); also BG/FG/Debug Shutdowns and overrides and battery level triggers Issues and PRs about system shutdown, especially if battery charge/runtime remaining is involved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

upsmon fails to start if the old pid happens to exist for an unrelated process
2 participants