openssh unix dev - Sep 2021 - ssh-agent: perform AGENTC_REMOVE_ALL_IDENTITIES on SIGUSR1

Steffen Nurpmeso wrote in
 <20210908114659.EuomV%steffen at sdaoden.eu>:
 |A long time ago i asked for the possibility to remove all
 |identities from a running agent by sending a signal.
 ...
 |The necessary code addition to ssh-agent is minimal, and builds
 |upon existing code.  The work is done in a signal handler, because
 |ssh_signal() sets SA_RESTART and thus the loop would not wake up
 |to tick.  On the other hand ssh_signal() fills the signal mask, so
 |signal recursion of any form cannot happen, therefore processing
 |the list in the signal handler seems safe.

Ha!  Not true, i have been kindly enlightened that the signal
can happen at anytime and thus my approach may corrupt state, so
"it cannot be coded this way".
Instead the signal handler shall set a volatile sig_atomic_t that
the main loop later picks up.

diff --git a/ssh-agent.1 b/ssh-agent.1
index ed8c870969..dd2b0e8af1 100644
--- a/ssh-agent.1
+++ b/ssh-agent.1
@@ -178,6 +178,9 @@ will automatically use them if present.
 is also used to remove keys from
 .Nm
 and to query the keys that are held in one.
+.Nm
+will remove all keys when it receives the user signal
+.Dv SIGUSR1 .
 .Pp
 Connections to
 .Nm
diff --git a/ssh-agent.c b/ssh-agent.c
index 48a47d45a9..bc24dadaa8 100644
--- a/ssh-agent.c
+++ b/ssh-agent.c
@@ -171,6 +171,9 @@ static int fingerprint_hash = SSH_FP_HASH_DEFAULT;
 /* Refuse signing of non-SSH messages for web-origin FIDO keys */
 static int restrict_websafe = 1;
 
+/* Request to remove_all_identities() pending */
+static volatile sig_atomic_t remove_all_next_tick = 0;
+
 static void
 close_socket(SocketEntry *e)
 {
@@ -529,11 +532,10 @@ process_remove_identity(SocketEntry *e)
 }
 
 static void
-process_remove_all_identities(SocketEntry *e)
+remove_all_identities(void)
 {
 	Identity *id;
 
-	debug2_f("entering");
 	/* Loop over all identities and clear the keys. */
 	for (id = TAILQ_FIRST(&idtab->idlist); id;
 	    id = TAILQ_FIRST(&idtab->idlist)) {
@@ -543,6 +545,13 @@ process_remove_all_identities(SocketEntry *e)
 
 	/* Mark that there are no identities. */
 	idtab->nentries = 0;
+}
+
+static void
+process_remove_all_identities(SocketEntry *e)
+{
+	debug2_f("entering");
+	remove_all_identities();
 
 	/* Send success. */
 	send_status(e, 1);
@@ -1342,6 +1351,13 @@ cleanup_handler(int sig)
 	_exit(2);
 }
 
+/*ARGSUSED*/
+static void
+remove_all_handler(int sig)
+{
+	remove_all_next_tick = 1;
+}
+
 static void
 check_parent_exists(void)
 {
@@ -1619,6 +1635,7 @@ skip:
 	ssh_signal(SIGINT, (d_flag | D_flag) ? cleanup_handler : SIG_IGN);
 	ssh_signal(SIGHUP, cleanup_handler);
 	ssh_signal(SIGTERM, cleanup_handler);
+	ssh_signal(SIGUSR1, remove_all_handler);
 
 	if (pledge("stdio rpath cpath unix id proc exec", NULL) == -1)
 		fatal("%s: pledge: %s", __progname, strerror(errno));
@@ -1630,7 +1647,11 @@ skip:
 		saved_errno = errno;
 		if (parent_alive_interval != 0)
 			check_parent_exists();
-		(void) reaper();	/* remove expired keys */
+		if (remove_all_next_tick) {
+			remove_all_next_tick = 0;
+			remove_all_identities();
+		} else
+			(void) reaper();	/* remove expired keys */
 		if (result == -1) {
 			if (saved_errno == EINTR)
 				continue;

--steffen
|
|Der Kragenbaer,                The moon bear,
|der holt sich munter           he cheerfully and one by one
|einen nach dem anderen runter  wa.ks himself off
|(By Robert Gernhardt)

Steffen Nurpmeso wrote in
 <20210908220546.suMBN%steffen at sdaoden.eu>:
 |Steffen Nurpmeso wrote in
 | <20210908114659.EuomV%steffen at sdaoden.eu>:
 ||A long time ago i asked for the possibility to remove all
 ||identities from a running agent by sending a signal.
 ...
 |Instead the signal handler shall set a volatile sig_atomic_t that
 |the main loop later picks up.

Well ok, thinking about this for more than just five minutes
(sorry) i think care should be taken to cause graceful loop wakeup
in any case.

Since _i think_ the poll(2) will not immediately tick due to
SA_RESTART if the "timeout" parameter is not -1, which _i think_
will be caused by setting a maximum lifetime for keys, here i add
a few lines of code which create a socket(2), and perform
a non-blocking connect(2) to the agent socket, followed by an
immediate close(2).  Now this tests fine here and causes the
connect(2) to wakeup the poll(2), even if the socket is already
closed once the signal handler returns.  These calls should be ok
from within a signal handler (alternatively a brute setting of a
O_NONBLOCK fcntl(2) on the AF_UNIX socket instead of calling
set_nonblock() would relax problems further).

With that more code than i wanted is necessary to implement key
clearance on SIGUSR1, but at least the patches combined will
(hopefully portably) at least do what they want.

Thank you.

Ciao from Germany,

diff --git a/ssh-agent.c b/ssh-agent.c
index bc24dadaa8..d0af500e42 100644
--- a/ssh-agent.c
+++ b/ssh-agent.c
@@ -173,6 +173,7 @@ static int restrict_websafe = 1;
 
 /* Request to remove_all_identities() pending */
 static volatile sig_atomic_t remove_all_next_tick = 0;
+static int remove_all_needs_sock = 0;
 
 static void
 close_socket(SocketEntry *e)
@@ -1355,7 +1356,27 @@ cleanup_handler(int sig)
 static void
 remove_all_handler(int sig)
 {
+	struct sockaddr_un sunaddr;
+	int sock;
+
+	if (remove_all_next_tick)
+		return;
 	remove_all_next_tick = 1;
+
+	if (!remove_all_needs_sock)
+		return;
+
+	/* (Try to) Cause an immediate tick */
+	if ((sock = socket(PF_UNIX, SOCK_STREAM, 0)) != -1) {
+		memset(&sunaddr, 0, sizeof(sunaddr));
+		sunaddr.sun_family = AF_UNIX;
+		(void) strlcpy(sunaddr.sun_path, socket_name,
+		    sizeof(sunaddr.sun_path));
+		(void) set_nonblock(sock);
+		(void) connect(sock, (const struct sockaddr *)&sunaddr,
+		    sizeof(sunaddr));
+		(void) close(sock);
+	}
 }
 
 static void
@@ -1643,6 +1664,7 @@ skip:
 
 	while (1) {
 		prepare_poll(&pfd, &npfd, &timeout, maxfds);
+		remove_all_needs_sock = (timeout != -1);
 		result = poll(pfd, npfd, timeout);
 		saved_errno = errno;
 		if (parent_alive_interval != 0)

--steffen
|
|Der Kragenbaer,                The moon bear,
|der holt sich munter           he cheerfully and one by one
|einen nach dem anderen runter  wa.ks himself off
|(By Robert Gernhardt)