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

Feature request: allow non-root execution #692

Closed
peesock opened this issue Mar 12, 2024 · 8 comments
Closed

Feature request: allow non-root execution #692

peesock opened this issue Mar 12, 2024 · 8 comments
Labels
3.0 feature request wontfix This will not be worked on

Comments

@peesock
Copy link

peesock commented Mar 12, 2024

Thanks to linux udev rules, it is possible for a normal user in a specific group (usually "input" or "uinput") to access /dev/input files.

In my opinion, hard-coding root-only actions, namely setgid and nice (with negative values) into the daemon, especially when these actions can be taken care of by the init/service manager, isn't a good idea. It reduces flexibility and adds potentially unwanted behavior, like running commands as root.

I was able to get the basic daemon working by removing root functions and adding command line options to customize the paths for SOCKET_PATH and CONFIG_DIR.

Assuming most people run keyd using the systemd service, the setgid and nice behavior can be replaced with these service options

[Service]
...
User=keyd
Group=keyd
Nice=-20
...

to avoid breaking current behavior. If keyd's built-in version of this isn't a big deal for most people, then perhaps it could be locked behind new CLI options, or removed entirely.

@peesock
Copy link
Author

peesock commented Mar 13, 2024

after running tests, it seems that non-root keyd cannot detect new input devices, such as those created by itself and t/runner.py.

t/run.sh test.log with sudo:

CONFIG: parsing /tmp/tmp.oCB10Miv4U/test.conf
	WARNING: line 9: chord_interkey_timeout is not a valid global option
Starting keyd v2.4.3 ()
DEVICE: ignoring 413c:2010  (Dell Dell USB Keyboard)
DEVICE: ignoring 3554:f508  (pulsar X2 V2 Mini)
DEVICE: ignoring 3554:f508  (pulsar X2 V2 Mini Consumer Control)
DEVICE: ignoring 3554:f508  (pulsar X2 V2 Mini)
DEVICE: ignoring 0fac:1ade  (keyd virtual pointer)
DEVICE: match    2fac:2ade  /tmp/tmp.oCB10Miv4U/test.conf	(test keyboard)
DEVICE: removed	2fac:2ade test keyboard

without sudo:

CONFIG: parsing /tmp/tmp.47s7kgris1/test.conf
	WARNING: line 9: chord_interkey_timeout is not a valid global option
Starting keyd v2.4.3 (b7cb828)
failed to open /dev/input/event25
failed to open /dev/input/event26
DEVICE: ignoring 413c:2010  (Dell Dell USB Keyboard)
DEVICE: ignoring 3554:f508  (pulsar X2 V2 Mini)
DEVICE: ignoring 3554:f508  (pulsar X2 V2 Mini Consumer Control)
DEVICE: ignoring 3554:f508  (pulsar X2 V2 Mini)
failed to open /dev/input/event27

which means runner.py detects no keys and fails the test.

this is also seen when unplugging and replugging my keyboard/mouse; same error, different event files.

not sure what's causing this issue. running keyd in gdb and slowly going through functions before getting to evloop() seemed to remove the error, so perhaps time is a factor.

changes: peesock@8639aa7

@peesock
Copy link
Author

peesock commented Mar 13, 2024

by unplugging and replugging my mouse while running a test C program with strace that opens and closes a /dev/input/event* file, i was able to confirm that it's a time issue.

1: openat(AT_FDCWD, "/dev/input/event5", O_RDWR|O_NONBLOCK|O_CLOEXEC) = -1 ENOENT (No such file or directory)
2: clock_nanosleep(CLOCK_REALTIME, 0, {tv_sec=0, tv_nsec=100000000}, NULL) = 0
3: openat(AT_FDCWD, "/dev/input/event5", O_RDWR|O_NONBLOCK|O_CLOEXEC) = -1 EACCES (Permission denied)
4: clock_nanosleep(CLOCK_REALTIME, 0, {tv_sec=0, tv_nsec=100000000}, NULL) = 0
5: openat(AT_FDCWD, "/dev/input/event5", O_RDWR|O_NONBLOCK|O_CLOEXEC) = 3
6: close(3)

line 1: mouse device file does not exist yet
line 3: device file appears, but udev rules have not fully applied
line 5: udev rules apply

so it looks like evloop needs to retry on failed event* file opens.

edit: fixed with peesock@a15c0e1.
non-root users will see temporary errors saying "permission denied" before re-connecting, but i figure that should stay in for the sake of simplicity, unless anyone has a better idea.

tests are passing, so i'm going to start actually using this cool project now that i can run it as a user.

@rvaiya
Copy link
Owner

rvaiya commented Mar 15, 2024

In my opinion, hard-coding root-only actions, namely setgid and nice (with negative values) into the daemon,
especially when these actions can be taken care of by the init/service manager, isn't a good idea.

keyd is deliberately written to run as root. My view is that if a user has compromised an account that can read or write input events, your system is effectively compromised anyway (apart from injection attacks, the attacker can just keylog everything).

It reduces flexibility and adds potentially unwanted behavior, like running commands as root.

This is a valid point, and there was some debate about adding command(), but its intended purpose is for running system level commands. It is not intended to be a substitute for window manager/user level bindings (sxhkd exists for this purpose). If the user can edit /etc/keyd/ they already have root access.

@peesock
Copy link
Author

peesock commented Mar 15, 2024

My view is that if a user has compromised an account that can read or write input events, your system is effectively compromised anyway

yeah, that makes sense. projects like swhkd use a privileged daemon that communicates directly to a specific user-owned process, and that process only spawns shell commands, creating a model where all /dev/input access has to be explicitly granted per-process by root.

this is quite secure, but not how i personally run my desktop. i basically give my user infinite permissions (sudo doesn't even have a password on it) and assume all programs i run are trusted, except in the case of sandboxes or running things as specific users.
i'm aware this is vulnerable to unknown exploits, like if firefox suddenly had a terrible RCE vulnerability, but it really simplifies things to have one user manage almost everything and not have to worry about complicated systems like polkit.

this is just me. i'm not advocating for per-user usage to be a common thing, only the flexibility to permit it. if it doesn't belong, i'm fine with maintaining my 3-commit fork.

@rvaiya
Copy link
Owner

rvaiya commented Apr 10, 2024

Sorry for the prolonged response.

yeah, that makes sense. projects like swhkd https://github.com/waycrate/swhkd/ use a privileged daemon that communicates directly to a specific user-owned process, and that process only spawns shell commands, creating a model where all /dev/input access has to be explicitly granted per-process by root.

I'm not sure how this applies to keyd. As I understand it, swhkd is a root level process that still reads all the input (like keyd), it just delegates command dispatch to a user level daemon ('the server'). The daemon which reads the config file still requires access to /dev/input/*, so it is unclear to me where the additional security comes from.

this is quite secure, but not how i personally run my desktop. i basically give my user infinite permissions
[...]
sudo doesn't even have a password on it

Don't we all ;).

@peesock
Copy link
Author

peesock commented Apr 11, 2024

I'm not sure how this applies to keyd.

It doesn't, just citing an example of something more secure than running things directly, as I initially wanted from this project :)
I'm not positive on how swhkd actually does things under the hood (since i don't want to use it), but at least the creator says it's safe.

Anyway, i dug more into the matter of running as a user and am learning about seat management to maybe add this capability in the future. No promises. I don't even know how lock screens would be handled, in case you want to use keyd as a sxhkd replacement...

After posting this issue, i started thinking a lot more about security and how a universal hotkey daemon could work.
Generally, i have to learn more about wayland.

@rvaiya rvaiya added wontfix This will not be worked on 3.0 feature request labels May 3, 2024
@rvaiya
Copy link
Owner

rvaiya commented May 3, 2024

Adding proper support for this is non trivial and involves significantly increasing the attack surface. It is closely tied to implementing per-user configs, which is a feature I actually have a working prototype for that relies on the same kind of VT monitoring tricks that seatd does.

Adding command() on a per user basis would probably also require a per user daemon to properly account for state, which also adds another moving part. I am not inclined to add such a thing at the moment, but I am toying with the idea for the next major release (which will be backward incompatible).

I am going to close this for now, since I am trying to prune the issue tree, but feel free to reopen it if you want to add something.

@rvaiya rvaiya closed this as completed May 3, 2024
@Konfekt
Copy link

Konfekt commented May 5, 2024

Thank you for being clear on this and still open for future support.

keyd is deliberately written to run as root. My view is that if a user has compromised an account that can read or write input events, your system is effectively compromised anyway

Indeed, any administered company laptop is compromised, but it would still be useful to, say, map Capslock to Escape and Control as a user.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.0 feature request wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

3 participants