Add support for new devices attached after kloak starts

[skyzzuu]

Added support for new devices attached after the system starts up:

1. service template that accepts the event file as a parameter then runs /usr/bin/kloak -r /dev/input/%i (%i is replaced with event [0-9]) when the service template is started
2. udev rules file that runs /usr/bin/systemctl start/stop kloak@%k.service when a new keyboard or mouse is attached/removed. this launches the service template and passes the event file name of the device that was attached/removed
3. to avoid the udev rules file triggering again on the devices created by kloak, kloak changes the name of the device created to "kloak output device", the udev rules file skips any devices that have a name starting with kloak
4. when one of the kloak processes receives the rescue key sequence, it executes /usr/bin/pkill kloak, sending a SIGTERM to all other running kloak processes to ensure they are all stopped
5. several permissions are added to the service template and apparmor profile to allow kloak to signal the other kloak processes

Additionally, the old systemd service file is removed, so that only the service template will be used.

The following happens when a new keyboard or mouse is attached:

1. udev makes sure the device is either a keyboard or mouse and that it isn't a device created by kloak (lines in main.c rename the devices created by kloak to "kloak output device" so they can be identified)
2. If true, launches /usr/bin/systemctl start kloak@<event file>.service
3. This launches the service template which runs /usr/sbin/kloak -r /dev/input/<event file>
4. If any of the kloak processes receives the rescue key sequence, they will execute /usr/bin/pkill kloak to signal the other kloak processes to stop

This allows new devices to be picked up by kloak without losing the systemd hardening and still allowing the rescue key sequence to stop all running instances of kloak.

Resolves issue #17

[adrelanos]

    // stop other kloak instances
    char *args[3];
    args[0] = "/usr/bin/pkill";
    args[1] = "kloak";
    args[2] = NULL;
    execve(args[0], args, NULL);

This seems unusual. Process management is usually done at the init (systemd) level. With systemd it should be possible to keep all the process management logic out of the daemon.

This might also look confusing in journal logs because "another application" killed kloak but not restarted.

A better design would be for kloak to test if its safe to run and write to stderr with an error message and exit non-zero if it cannot.

I might be wrong, did you see other system daemons that kill previous instances of themselves when run?

Restart=always
Restart=no

This might cause trouble. Processes can get killed for any reason. Including reasons which aren't their own fault. (Linux kernel out of memory killer.)

In such cases we don't want to end up with broken input devices or non-functional kloak.

Managing the service using sd_notify seems warranted here to ensure the kloak service is always responsive.
(But that could be a different ticket. Just to outline a good future design.)

How is this supposed to work anyhow? It seems now udev is used to to trigger systemd slices which in turn run ExecStart=/usr/sbin/kloak -r /dev/input/%i. That in theory is a great design!

However, on the other hand pkill would kill all other kloak instances. That wound not work with multiple devices unless I am overlooking something?

KERNEL=="event*", ACTION=="add", ENV{ID_INPUT_KEYBOARD}=="1", RUN+="/usr/bin/systemctl start kloak@%k.service"

The k (in k.service) seems to be the variable, /dev/input/... device. This seems to used for:

ExecStart=/usr/sbin/kloak -r /dev/input/%i

Is systemctl start from udev the proper way to do it? It might be. Could you check please if any other packages from Debian are using this method?

grep --color -r "systemctl start" /lib/udev/rules.d

[skyzzuu]

The idea is that each device file (event[0-9]) will have it's own kloak service.

As an example, assume you have a keyboard at /dev/input/event0, and a mouse at /dev/input/event1

Once a keyboard or mouse is attached, udev will replace the %k with the event file name (event0, event1, etc) and run /usr/bin/systemctl start [email protected] and /usr/bin/systemctl start @event1.service.

systemd replaces the %i with the content passed between the @ and the .service (event0, event1, etc)

That will execute /usr/sbin/kloak -r /dev/input/event0 and /usr/sbin/kloak -r /dev/input/event1

The problem I ran into was that now when you send the rescue key sequence, the service listening to event0 will terminate, but the one listening to event1 will continue because it's only receiving events from the mouse. This could create a problem where if a user is having problems with their mouse and they send the rescue key sequence, the kloak instance grabbing the mouse won't stop.

That's why I added the lines to run pkill to signal the other kloak instances that they should stop since one of them received the rescue key sequence.

However, I had never seen the BindsTo= option in systemd unit files and from testing that now, it looks like I could remove the pkill lines and just put that in the unit file.

The restart was originally changed, because when the rescue key sequence was received and kloak signaled the other kloak instances to stop, they would be restarted right after from the Restart=always line. Although if the unit file has the BindsTo= option added, I may be able to switch that back.

I can see if other packages are using systemctl start. I was going off approximately what qubes does for qubes-input-sender as an template for this. In there, they have udev launch qubes-input-trigger, which performs some checks on the device beforehand, then launches systemctl start qubes-input-sender@event[0-9].service.

[skyzzuu]

From testing on Restart= line, I can have it be on-failure which I believe covers cases like the out of memory daemon stopping it. With Restart=always, they still restart after one of the services ends after the rescue key sequence.

[skyzzuu]

Actually, I had just forgotten to comment out the lines calling pkill when I tested. The BindsTo did not work and the other kloak services continued running. From what I can tell, there doesn't seem to be a way to put wildcards into lines like BindsTo or PropagatesStopTo to have it bind to all instances of the template service running.

[adrelanos]

I do understand your design now better. To terminate all kloak instances once the rescue key was pressed is a valid concern.

Does the rescue key logic need to be part of kloak? Or perhaps it's simple to design it with an additional systemd unit / process that does just that? Assuming that's not possible...

What's the supposed action that should happen if rescue key is pressed?

• A) effectively only restart all instances kloak
• B) stop kloak and make sure it stays stopped and will not be restarted by systemd.

I assume B) for now...

This is a bit of a special, non-standard case.

What's the proper signal to send if rescue key is pressed? SIGUSR1 perhaps?

What's the proper exit code if rescue key was pressed or if signal SIGUSR1 was received?

Perhaps this makes sense: rescue key -> send signal SIGUSR1 to all instances of kloak -> all kloak instances terminates with exit code 3 -> systemd SuccessExitStatus=3 results in systemd not restarting kloak. What do you think?

systemd by default sends signal SIGTERM for usual restarts such as on package upgrades or manual user restart. Therefore maybe rescue key shouldn't use SIGTERM (pkill's default), because then kloak will be automatically restarted, which I am not sure is the desired functionality of the rescue key.

---

    // stop other kloak instances
    char *args[3];
    args[0] = "/usr/bin/pkill";
    args[1] = "kloak";
    args[2] = NULL;
    execve(args[0], args, NULL);

    free(pfds);

This code runs only if rescue key is pressed. Could use a comment.

This could result in kloak being killed before free(pfds); is begin run. Maybe a unlikely race condition. Maybe also not a big deal if something isn't freed as after process killing presumably the kernel will force free all previous program memory anyhow. Therefore probably a very minor issue, nitpick.

---

Probably nitpick: exit(0); -> exit(EXIT_SUCCESS);

---

I think we also need a verbose mode that is different from debug mode. Verbose mode isn't important. That could be the default. Some "basic" output

Such as successful startup, device setup notifications and printf("Received SIGTERM, cleaning up\n"); could always be shown. No harm. This is just to monitor if kloak has a normal life cycle and how rescue key interacts with systemd etc. The super spammy output of monitoring all key presses would always only be shown in debug mode only as it's already implemented now.

[ArrayBolt3]

Why run multiple Kloak instances, one for each device? It seems like it would be easy enough to just run kloak without any event device argument at all, so that it detects and captures all devices. When a new device is plugged in, restart kloak and let it pick up everything it needs to. That would come with a few advantages:

• No need to send signals to other processes, one kloak does it all
• The current code looks like (though I could be mistaken) it limits one to 10 input devices, doing everything in one kloak process wouldn't have that issue
• Less exceptions in the syscall filter and AppArmor policy needed

One downside is that typing would be uncloaked for about 500 milliseconds after inserting or removing a device. I would expect most people take slightly longer than that to get back to work after intentionally doing that, and to exploit this fact in an attack, the attacker would have to have physical access to the device while the user was in the middle of actively typing (in which case there are much worse things an attacker could do). About the worst I can imagine happening with this is the user's friend unplugging something from the user's computer, causing a kloak restart while the user is mid-typing (thus de-anonymizing them for that split second).

[adrelanos]

Why run multiple Kloak instances, one for each device? It seems like it would be easy enough to just run kloak without any event device argument at all, so that it detects and captures all devices.

In theory: Yes, why not.

What speaks against it: Maybe it could lead to device over detection? Maybe some devices would need to be excluded? In reference to:

• Wireless and bluetooth card blocked after start #72

Or would we rather exclude devices on the udev level?

One downside is that typing would be uncloaked for about 500 milliseconds after inserting or removing a device.

I would expect most people take slightly longer than that to get back to work after intentionally doing that,

Right. And this would still be a huge improvement over not protecting newly attached devices at all as it is implemented right now.

and to exploit this fact in an attack, the attacker would have to have physical access to the device while the user was in the middle of actively typing (in which case there are much worse things an attacker could do). About the worst I can imagine happening with this is the user's friend unplugging something from the user's computer, causing a kloak restart while the user is mid-typing (thus de-anonymizing them for that split second).

Attacks that require physical presence should be very much out-of-scope for the context of kloak.

[adrelanos]

This was implemented in Whonix/kloak#1