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

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)

Steffen Nurpmeso wrote:
> 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.
I think the lack of feedback about removal success/failure inherent
with signals is problematic.

> Since _i think_ the poll(2) will not immediately tick due to
> SA_RESTART if the "timeout" parameter is not -1,
signal(7) on Linux says: "Which of these two behaviors occurs depends
on the interface and whether or not the signal handler was established
using the SA_RESTART flag (see sigaction(2)). The details vary across
UNIX systems; below, the details for Linux."

"The details vary across UNIX systems" suggests that you may need to
do research on this.

My signal(7) later says: "The following interfaces are never restarted
after
being interrupted by a signal handler, regardless of the use of SA_RESTART;
they always fail with the error EINTR when interrupted by a signal handler:
...
* File descriptor multiplexing interfaces: ... poll(2) ..." - for Linux.

> 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).
There's no error handling after socket(), which is a problem in itself
but also ties into the feedback problem. Since this is intended to be
a security feature there really needs to be some feedback, making
direct interaction with the agents look a lot better, with the
significant additional advantage of needing zero new agent code.


//Peter