[Draft] Implement handling SIGUSR2 to record external failed authentication attempts

[SebTM]

DRAFT - #283

[mstoeckl]

schedule_auth_idle calls loop_add_timer which calls calloc, which is not safe to use in a signal handler. (See for example man 7 signal-safety.) Using the self-pipe trick, as was done for do_sigusr, would be more reliable.

[SebTM]

Hey, I'm trying to work this out but I'm a bit confused - it appears to me that do_sigusr sets a "flag" inside sigusr_fds to 1 but I can't find the code that picks it up inside the loop or did I misinterpret something? My confusion is maybe related to sigusr_fds[0] because I can't find any usage of sigusr_fds[1] which I would have suspected is the clue on the self-pipe hack?

My approach would be to use sig in do_sigusr as key for sigusr_fds extending it to 3 (-1,-1,-1) so when SIGUSR2 is received it's status is changed and I can handle the actual code inside the loop/via another function called with loop_add_fd?

Still learning C, sorry for the inconvenience ✌🏻

[kennylevinsen]

The pipe trick involves:

1. Making a pipe with read and write fds
2. Registering the read-side fd on the event loop (as is done with loop_add_fd for sigusr_fds[0]), with an event loop handler to do whatever was intended
3. Register a signal handler that when called writes to the write-side fd (as is done on sigusr_fds[1] in do_sigusr), which is one of the few safe things one can do in a signal handler.

This way, the main event loop will wake up when the signal handler ends, triggering the registered event loop handler which has no limitations.

[lilyinstarlight]

I took a quick stab at it, and couldn't we reuse do_sigusr for this via something like the following diff, or does the writing-the-signal-number approach have drawbacks I'm not thinking of?

diff --git a/main.c b/main.c
index d6eeae6..ebf5c92 100644
--- a/main.c
+++ b/main.c
@@ -341,7 +341,7 @@ static const struct wl_registry_listener registry_listener = {
 static int sigusr_fds[2] = {-1, -1};
 
 void do_sigusr(int sig) {
-	(void)write(sigusr_fds[1], "1", 1);
+	(void)write(sigusr_fds[1], &sig, sizeof(sig));
 }
 
 static cairo_surface_t *select_image(struct swaylock_state *state,
@@ -1085,7 +1085,18 @@ static void comm_in(int fd, short mask, void *data) {
 }
 
 static void term_in(int fd, short mask, void *data) {
-	state.run_display = false;
+	int sig = -1;
+	(void)read(sigusr_fds[0], &sig, sizeof(sig));
+	if (sig == SIGUSR1) {
+		// External unlock succeeded
+		state.run_display = false;
+	}
+	else {
+		state.auth_state = AUTH_STATE_INVALID;
+		schedule_auth_idle(&state);
+		++state.failed_attempts;
+		damage_state(&state);
+	}
 }
 
 // Check for --debug 'early' we also apply the correct loglevel
@@ -1115,6 +1126,8 @@ void log_init(int argc, char **argv) {
 }
 
 int main(int argc, char **argv) {
+	signal(SIGUSR2, SIG_IGN);
+
 	log_init(argc, argv);
 	initialize_pw_backend(argc, argv);
 	srand(time(NULL));
@@ -1273,6 +1286,7 @@ int main(int argc, char **argv) {
 
 	loop_add_fd(state.eventloop, sigusr_fds[0], POLLIN, term_in, NULL);
 	signal(SIGUSR1, do_sigusr);
+	signal(SIGUSR2, do_sigusr);
 
 	state.run_display = true;
 	while (state.run_display) {

[mstoeckl]

There is one drawback. For the signal handler be fully correct, sigusr_fds[1] should be made nonblocking, so that if swaylock receives a few thousand copies of a signal, before running the main loop, the write() call does not block when it tries to write a pipe whose buffer is full. If that is fixed, then the approach of using a single pipe may lose specific signals: a SIGUSR1 may not get through if the pipe is currently full with SIGUSR2 messages.

(If the blocking issue is not fixed, then a single pipe can still lose signals: consider the sequence of a SIGUSR1 followed shortly by a SIGUSR2; the SIGUSR2 could interrupt the write() corresponding to the SIGUSR1 and make it fail with EINTR. I think it's possible to fix this "lose signals" issue using the SA_RESTART flag for sigaction(), but there's probably a subtle issue I can't think of.)

[lilyinstarlight]

@mstoeckl I see you made #318 with both of those fixes. Would the following diff be sufficient then on top of those?

If so I'll PR it unless @SebTM wants to just include it here (I don't really care for attribution, so I'm fine either way)

Thank you again!

commit 841a6f01920219bd8e3dac0050d8ae5f2d2e4b00
Author: Lily Foster <lily@lily.flowers>
Date:   Tue Nov 7 12:01:23 2023 -0500

    Implement handling SIGUSR2 to record external failed authentication attempts
    
    Co-Authored-By: Sebastian Sellmeier <mail@sebastian-sellmeier.de>

diff --git a/comm.c b/comm.c
index 6add38a..1634f10 100644
--- a/comm.c
+++ b/comm.c
@@ -1,3 +1,5 @@
+#define _POSIX_C_SOURCE 200809L
+#include <signal.h>
 #include <stdbool.h>
 #include <stdlib.h>
 #include <sys/types.h>
@@ -62,6 +64,12 @@ bool spawn_comm_child(void) {
 	} else if (child == 0) {
 		close(comm[0][1]);
 		close(comm[1][0]);
+		struct sigaction sa;
+		sa.sa_handler = SIG_IGN;
+		sigemptyset(&sa.sa_mask);
+		sa.sa_flags = 0;
+		sigaction(SIGUSR1, &sa, NULL);
+		sigaction(SIGUSR2, &sa, NULL);
 		run_pw_backend_child();
 	}
 	close(comm[0][0]);
diff --git a/main.c b/main.c
index d27cddf..ef0b4cc 100644
--- a/main.c
+++ b/main.c
@@ -341,7 +341,7 @@ static const struct wl_registry_listener registry_listener = {
 static int sigusr_fds[2] = {-1, -1};
 
 void do_sigusr(int sig) {
-	(void)write(sigusr_fds[1], "1", 1);
+	(void)write(sigusr_fds[1], &sig, sizeof(sig));
 }
 
 static cairo_surface_t *select_image(struct swaylock_state *state,
@@ -1085,7 +1085,21 @@ static void comm_in(int fd, short mask, void *data) {
 }
 
 static void term_in(int fd, short mask, void *data) {
-	state.run_display = false;
+	int sig = -1;
+	if (read(sigusr_fds[0], &sig, sizeof(sig)) != sizeof(sig)) {
+		swaylock_log_errno(LOG_ERROR, "Failed to read signum");
+		return;
+	}
+	if (sig == SIGUSR1) {
+		// External unlock succeeded
+		state.run_display = false;
+	}
+	else {
+		state.auth_state = AUTH_STATE_INVALID;
+		schedule_auth_idle(&state);
+		++state.failed_attempts;
+		damage_state(&state);
+	}
 }
 
 // Check for --debug 'early' we also apply the correct loglevel
@@ -1279,6 +1293,7 @@ int main(int argc, char **argv) {
 	sigemptyset(&sa.sa_mask);
 	sa.sa_flags = SA_RESTART;
 	sigaction(SIGUSR1, &sa, NULL);
+	sigaction(SIGUSR2, &sa, NULL);
 
 	state.run_display = true;
 	while (state.run_display) {
diff --git a/swaylock.1.scd b/swaylock.1.scd
index 61691eb..3d54cee 100644
--- a/swaylock.1.scd
+++ b/swaylock.1.scd
@@ -194,6 +194,9 @@ Locks your Wayland session.
 *SIGUSR1*
 	Unlock the screen and exit.
 
+*SIGUSR2*
+	Increases failed-attempts counter e.g. from external fingerprint-verification.
+
 # AUTHORS
 
 Maintained by Drew DeVault <sir@cmpwn.com>, who is assisted by other open

[lilyinstarlight]

Opened as #324

[SebTM]

Thanks for taking over ✌🏻