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

keyd: add keyd service and test #221321

Merged
merged 1 commit into from
Mar 22, 2023
Merged

keyd: add keyd service and test #221321

merged 1 commit into from
Mar 22, 2023

Conversation

woojiq
Copy link
Contributor

@woojiq woojiq commented Mar 15, 2023

The keyd package already exists but without a systemd service.

Description of changes

Add systemd service for the keyd package, and write a test.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.05 Release Notes (or backporting 22.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Also, I tested manually.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: documentation This PR adds or changes documentation 8.has: module (update) This PR changes an existing module in `nixos/` labels Mar 15, 2023
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Mar 15, 2023
@woojiq
Copy link
Contributor Author

woojiq commented Mar 18, 2023

I added basic hardening to the systemd service. Now systemd-analyze security keyd.service looks like this:

  NAME                                                        DESCRIPTION                                                        EXPOSURE
✗ RootDirectory=/RootImage=                                   Service runs within the host's root directory                           0.1
  SupplementaryGroups=                                        Service runs as root, option does not matter
  RemoveIPC=                                                  Service runs as root, option does not apply
✗ User=/DynamicUser=                                          Service runs as root user                                               0.4
✓ RestrictRealtime=                                           Service realtime scheduling access is restricted
✓ CapabilityBoundingSet=~CAP_SYS_TIME                         Service processes cannot change the system clock
✓ NoNewPrivileges=                                            Service processes cannot acquire new privileges
✓ AmbientCapabilities=                                        Service process does not receive ambient capabilities
✗ PrivateDevices=                                             Service potentially has access to hardware devices                      0.2
✗ SystemCallArchitectures=                                    Service may execute system calls with all ABIs                          0.2
✗ RestrictAddressFamilies=~AF_PACKET                          Service may allocate packet sockets                                     0.2
✗ RestrictAddressFamilies=~AF_NETLINK                         Service may allocate netlink sockets                                    0.1
✗ RestrictAddressFamilies=~AF_UNIX                            Service may allocate local sockets                                      0.1
✗ RestrictAddressFamilies=~…                                  Service may allocate exotic sockets                                     0.3
✗ RestrictAddressFamilies=~AF_(INET|INET6)                    Service may allocate Internet sockets                                   0.3
✓ ProtectSystem=                                              Service has strict read-only access to the OS file hierarchy
✓ CapabilityBoundingSet=~CAP_SYS_RAWIO                        Service has no raw I/O access
✓ CapabilityBoundingSet=~CAP_SYS_PTRACE                       Service has no ptrace() debugging abilities
✓ CapabilityBoundingSet=~CAP_SYS_(NICE|RESOURCE)              Service has no privileges to change resource use parameters
✓ CapabilityBoundingSet=~CAP_NET_ADMIN                        Service has no network configuration privileges
✓ CapabilityBoundingSet=~CAP_NET_(BIND_SERVICE|BROADCAST|RAW) Service has no elevated networking privileges
✗ DeviceAllow=                                                Service has no device ACL                                               0.2
✓ CapabilityBoundingSet=~CAP_AUDIT_*                          Service has no audit subsystem access
✓ CapabilityBoundingSet=~CAP_SYS_ADMIN                        Service has no administrator privileges
✓ PrivateNetwork=                                             Service has no access to the host's network
✓ PrivateTmp=                                                 Service has no access to other software's temporary files
✓ CapabilityBoundingSet=~CAP_SYSLOG                           Service has no access to kernel logging
✓ ProtectHome=                                                Service has no access to home directories
✗ ProcSubset=                                                 Service has full access to non-process /proc files (/proc subset=)      0.1
✓ KeyringMode=                                                Service doesn't share key material with other services
✓ Delegate=                                                   Service does not maintain its own delegated control group subtree
✓ PrivateUsers=                                               Service does not have access to other users
✗ SystemCallFilter=~@clock                                    Service does not filter system calls                                    0.2
✗ SystemCallFilter=~@cpu-emulation                            Service does not filter system calls                                    0.1
✗ SystemCallFilter=~@debug                                    Service does not filter system calls                                    0.2
✗ SystemCallFilter=~@module                                   Service does not filter system calls                                    0.2
✗ SystemCallFilter=~@mount                                    Service does not filter system calls                                    0.2
✗ SystemCallFilter=~@obsolete                                 Service does not filter system calls                                    0.1
✗ SystemCallFilter=~@privileged                               Service does not filter system calls                                    0.2
✗ SystemCallFilter=~@raw-io                                   Service does not filter system calls                                    0.2
✗ SystemCallFilter=~@reboot                                   Service does not filter system calls                                    0.2
✗ SystemCallFilter=~@resources                                Service does not filter system calls                                    0.2
...skipping...
✗ SystemCallFilter=~@raw-io                                   Service does not filter system calls                                    0.2
✗ SystemCallFilter=~@reboot                                   Service does not filter system calls                                    0.2
✗ SystemCallFilter=~@resources                                Service does not filter system calls                                    0.2
✗ SystemCallFilter=~@swap                                     Service does not filter system calls                                    0.2
✗ IPAddressDeny=                                              Service does not define an IP address allow list                        0.2
✓ NotifyAccess=                                               Service child processes cannot alter service state
✓ ProtectClock=                                               Service cannot write to the hardware clock or system clock
✓ CapabilityBoundingSet=~CAP_KILL                             Service cannot send UNIX signals to arbitrary processes
✓ ProtectKernelLogs=                                          Service cannot read from or write to the kernel log ring buffer
✓ CapabilityBoundingSet=~CAP_(DAC_*|FOWNER|IPC_OWNER)         Service cannot override UNIX file/IPC permission checks
✓ ProtectControlGroups=                                       Service cannot modify the control group file system
✓ CapabilityBoundingSet=~CAP_LINUX_IMMUTABLE                  Service cannot mark files immutable
✓ CapabilityBoundingSet=~CAP_IPC_LOCK                         Service cannot lock memory into RAM
✓ ProtectKernelModules=                                       Service cannot load or read kernel modules
✓ CapabilityBoundingSet=~CAP_SYS_MODULE                       Service cannot load kernel modules
✓ CapabilityBoundingSet=~CAP_SYS_BOOT                         Service cannot issue reboot()
✓ CapabilityBoundingSet=~CAP_SYS_CHROOT                       Service cannot issue chroot()
✓ PrivateMounts=                                              Service cannot install system mounts
✓ MemoryDenyWriteExecute=                                     Service cannot create writable executable memory mappings
✓ RestrictNamespaces=~user                                    Service cannot create user namespaces
✓ RestrictNamespaces=~pid                                     Service cannot create process namespaces
✓ RestrictNamespaces=~net                                     Service cannot create network namespaces
✓ RestrictNamespaces=~uts                                     Service cannot create hostname namespaces
✓ RestrictNamespaces=~mnt                                     Service cannot create file system namespaces
✓ CapabilityBoundingSet=~CAP_MKNOD                            Service cannot create device nodes
✓ RestrictNamespaces=~cgroup                                  Service cannot create cgroup namespaces
✓ RestrictNamespaces=~ipc                                     Service cannot create IPC namespaces
✓ ProtectHostname=                                            Service cannot change system host/domainname
✓ CapabilityBoundingSet=~CAP_(CHOWN|FSETID|SETFCAP)           Service cannot change file ownership/access mode/capabilities
✓ CapabilityBoundingSet=~CAP_SET(UID|GID|PCAP)                Service cannot change UID/GID identities/capabilities
✓ LockPersonality=                                            Service cannot change ABI personality
✓ ProtectKernelTunables=                                      Service cannot alter kernel tunables (/proc/sys, …)
✓ CapabilityBoundingSet=~CAP_MAC_*                            Service cannot adjust SMACK MAC
✓ RestrictSUIDSGID=                                           SUID/SGID file creation by service is restricted
✗ ProtectProc=                                                                                                                        0.1
✓ CapabilityBoundingSet=~CAP_SYS_PACCT                        Service cannot use acct()
✓ CapabilityBoundingSet=~CAP_WAKE_ALARM                       Service cannot program timers that wake up the system
✓ CapabilityBoundingSet=~CAP_SYS_TTY_CONFIG                   Service cannot issue vhangup()
✓ CapabilityBoundingSet=~CAP_BLOCK_SUSPEND                    Service cannot establish wake locks
✓ CapabilityBoundingSet=~CAP_LEASE                            Service cannot create file leases
✗ UMask=                                                      Files created by service are world-readable by default                  0.1
→ Overall exposure level for keyd.service: 3.5 OK 🙂

However, I'm not a a systemd expert, so there are options for further improvements.

Some observation:

  • It must be a root user (User=/DynamicUser=)
  • It must have access to hardware devices (PrivateDevices)

@woojiq
Copy link
Contributor Author

woojiq commented Mar 18, 2023

cc @pennae could you review the PR, please? You have already watched half on the matrix server)

nixos/doc/manual/release-notes/rl-2305.section.md Outdated Show resolved Hide resolved
nixos/modules/services/hardware/keyd.nix Outdated Show resolved Hide resolved
nixos/modules/services/hardware/keyd.nix Outdated Show resolved Hide resolved
nixos/modules/services/hardware/keyd.nix Outdated Show resolved Hide resolved
nixos/modules/services/hardware/keyd.nix Outdated Show resolved Hide resolved
nixos/modules/services/hardware/keyd.nix Show resolved Hide resolved
@woojiq
Copy link
Contributor Author

woojiq commented Mar 19, 2023

I replaced extraConfig with next options: settings (ini format) and ids (list of string). Is this solution good? And corrected other recommendations, except hardening.

can this service run without root, but with some capabilities or supplementary groups to grant it access to the necessary devices?

I didn't quite understand what was meant. Keyd is a system-wide service, so it cannot be run as systemd --user....

P.S. Thanks for the detailed review.

@pennae
Copy link
Contributor

pennae commented Mar 19, 2023

the new config format is good. keyConfig should be generated with a runCommand that cats the two fragments together, and the result of that assigned to source of the etc file instead of text. doing as you have is IFD and isn't allowed on all machines.

systemd system services can be run as arbitrary users/groups by setting User/Group/DynamicUser. this is different from systemd user services which run in the context of a user service manager and can only run as that user (to first approximation). keyd should continue to run as a system service, but not with uid/gid 0 if that's at all possible. (that kanata can do it is a sign in favor of this)

@woojiq
Copy link
Contributor Author

woojiq commented Mar 19, 2023

systemd system services can be run as arbitrary users/groups by setting User/Group/DynamicUser. this is different from systemd user services which run in the context of a user service manager and can only run as that user (to first approximation). keyd should continue to run as a system service, but not with uid/gid 0 if that's at all possible. (that kanata can do it is a sign in favor of this)

I am not sure if this is possible due to the following reasons which I have researched. I wrote this as in kanata (only this, without extra hardening):

hardware.uinput.enable = true;
...
{
  DynamicUser = true;
  DeviceAllow = [
      "/dev/uinput rw"
      "char-input r"
  ];
  SupplementaryGroups = with config.users.groups; [
      input.name
      uinput.name
  ];
}

But it didn't work because of the error: failed to open /dev/input/event-*. Permission denied. So I added "/dev/input rw" to the allowed devices. The error remained. I tried writing 20 entries like this /dev/input/event0 /dev/input/event1 .... Almost all errors are gone, but two errors remain:

failed to open /dev/input/event11
failed to open /dev/input/event10

I looked at ls -l /dev/input/, these events are gone, however, they are present without the DynamicUser option. Is this a sign that dynamicuser cannot be applied here? I've read a couple of articles about DynamicUser options, reviewed similar files inside the nixpkgs repository, and it looks like this could be the answer, but it's probably not an "error" on the systemd side.

If necessary, I can try to ask the keyd developer about it.

@pennae
Copy link
Contributor

pennae commented Mar 19, 2023

you don't have to use DynamicUser, or DeviceAllow. leaving out both DeviceAllow and DevicePolicy may work. maybe remove all hardenings except DynamicUser, keep SupplementaryGroups, and reapply all those that don't break it?

@woojiq
Copy link
Contributor Author

woojiq commented Mar 19, 2023

I tested with minimum hardening parameters. First I tried this:

serviceConfig = {
  ExecStart = "${pkgs.keyd}/bin/keyd";
  Restart = "always";

  DynamicUser = true;
  SupplementaryGroups = with config.users.groups; [
    input.name
  ];
};

uinput permission denied -> add uinput to SupplementaryGroups and hardware.uinput.enable = true;
faile to open /dev/input/event*. Permission denied -> add DeviceAllow = ["/dev/input rw"];
The error remains. I looked on the internet (and chatgpt:) how I can fix this, there is a chmod solution, but it looks unsound. I even tried add my user to input/uinput group, but it didn't help.

@woojiq
Copy link
Contributor Author

woojiq commented Mar 21, 2023

I give up. I don't have enough time/patience anymore to complete this task. I asked similar question here rvaiya/keyd#459. From what I read (this doesn't really apply to our problem):

but it should still be run as root since it needs access to your input devices and /dev/uinput. The best way to achieve this is to have your init system start and manage it at boot time.

Although SupplementaryGroups and DeviceAllow should have helped, they didn't 😢

If you accept the current behavior I can squash commits before merging, if not - feel free to close the PR I won't be angry. Anyway, I gained vast knowledge doing this stuff 😌

@pennae
Copy link
Contributor

pennae commented Mar 21, 2023

we've had a look out of curiosity on the hunch that your DeviceAllow settings weren't quite right, and also found out that keyd is a bit strange in how it writes its runtime files. managed to fix both and added another commit if you want to have it.

@woojiq
Copy link
Contributor Author

woojiq commented Mar 22, 2023

A few last questions 🧐.

  1. CONTRIBUTING.md

Pull requests should not be squash merged in order to keep complete commit messages and GPG signatures intact and must not be when the change doesn't make sense as a single commit.

Should I squash all the commits into one and keep the commit messages, or should I just squash my commits and keep yours separately?

  1. Now service is working well, but it feels a little bit of magic for me, especially these two things: KEYD_SOCKET = "/run/keyd/keyd.sock" and "char-input rw". Where did you find information about them? Recursive search inside the keyd repo hasn't found any mentions for KEYD_SOCKET only SOCKET_PATH inside Makefile. Also, char-input rw sounds intuitive but in google I can't find any related information. There is about char-rtc but there is no about char-input.

@pennae
Copy link
Contributor

pennae commented Mar 22, 2023

Should I squash all the commits into one and keep the commit messages, or should I just squash my commits and keep yours separately?

that's more of a guideline for committers than for authors. you can merge your commits all you like (as long as the resulting commit train makes sense). do take the test fix from ours too to make sure the test always passes, but please keep the rest of it intact.

Now service is working well, but it feels a little bit of magic for me

yeah, KEYD_SOCKET only exists in 2.4.2, the development version has replaced it with a compile-time setting. stumbled on that while checking why recompiling with SOCKET_PATH set didn't work as expected. as for char-input, man systemd.resource-control has a short guide on how to get to those. you had already used r for that class but it didn't work, so the next guess was that it needs rw :)

The keyd package already exists, but without a systemd service.

Keyd requires write access to /var/run to create its socket. Currently
the directory it uses can be changed with an environment variable, but
the keyd repo state suggests that this may turn into a compile-time
option. with that set, and some supplementary groups added, we can run
the service under DynamicUser.

Co-authored-by: pennae <[email protected]>
@woojiq
Copy link
Contributor Author

woojiq commented Mar 22, 2023

I think this PR is now ready for landing. If I misunderstood what you said, I can roll back the commit history using a local copy or the git reflog command)

man systemd.resource-control

Ah, cool.

@pennae pennae merged commit 296e7f9 into NixOS:master Mar 22, 2023
@Janik-Haag Janik-Haag added the 12. first-time contribution This PR is the author's first one; please be gentle! label Jun 13, 2023
@woojiq woojiq mentioned this pull request Jul 26, 2023
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: documentation This PR adds or changes documentation 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 12. first-time contribution This PR is the author's first one; please be gentle!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants