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

Peter Stuge wrote in
 <20210909181705.28572.qmail at stuge.se>:
 |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.

Bah.  What hair-splitting is that?  Sorry.

 |> 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.

Wouldn't you agree that the approach that was chosen covers
exactly that?
Didn't you work on cURL quite some years in the past?  That is
a pretty portable program.

 |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.

The two patches combined do not rely on Linux behaviour.
They do however rely on non-blocking connect(2) to AF_UNIX socket
being delivered and thus causing poll(2) (i hate this interface
which does not give me back remaining time to sleep, but who am i)
to wake up, even if the socket(2) is closed.  My gut feeling is
that this should work everywhere, .. but i have _not_ tested this
on *BSD, Solaris and Linux, and that is all i ever had (but
UnixWare).

 |> 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

To put that straight you mean the opposite, that _if_ socket(2)
fails we will not be able to connect(2), and therefore the loop
will not tick.  That is true.
If it wakes up the next time it _will_ remove the keys.

 |a security feature there really needs to be some feedback, making

Well i mean for the "the FBI is coming to get my private data"
cases you likely will do "pkill -TERM" and "cryptsetup
luksErase"
and then "poweroff", readily at hand as a script available with
a key combination?  Svedish agents, careful with that axe!

Not to mention that you as a Linux user do not have this problem
at all, like you said above.  And BSD users will not either, as
the limits are more friendly.

However if there really would be E[MN]FILE, or such, well, yes, we
then could join the hard OpenBSD way and call the
cleanup_handler() and then fatal_r()ly abort the program instead?
Sure thing.

  diff --git a/ssh-agent.c b/ssh-agent.c
  index d0af500e42..a7e49a2b81 100644
  --- a/ssh-agent.c
  +++ b/ssh-agent.c
  @@ -1376,6 +1376,9 @@ remove_all_handler(int sig)
   		(void) connect(sock, (const struct sockaddr *)&sunaddr,
   		    sizeof(sunaddr));
   		(void) close(sock);
  +	} else {
  +		error("socket: %s", strerror(errno));
  +		cleanup_handler(sig);
   	}
   }

 |direct interaction with the agents look a lot better, with the

How do you do this, Mister, from an ACPI script running as root?

 |significant additional advantage of needing zero new agent code.

Oh nooo!  Spoiler!!!

I personally would use '#ifndef __linux__' or something.
So here the entire thing in all its beauty.

Good night.

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..a7e49a2b81 100644
--- a/ssh-agent.c
+++ b/ssh-agent.c
@@ -171,6 +171,10 @@ 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 int remove_all_needs_sock = 0;
+
 static void
 close_socket(SocketEntry *e)
 {
@@ -529,11 +533,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 +546,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 +1352,36 @@ cleanup_handler(int sig)
 	_exit(2);
 }
 
+/*ARGSUSED*/
+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;
+
+	/* 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);
+	} else {
+		error("socket: %s", strerror(errno));
+		cleanup_handler(sig);
+	}
+}
+
 static void
 check_parent_exists(void)
 {
@@ -1619,6 +1659,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));
@@ -1626,11 +1667,16 @@ 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)
 			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)

Hi Steffen,

Steffen Nurpmeso wrote:
>  |I think the lack of feedback about removal success/failure inherent
>  |with signals is problematic.
> 
> Bah.  What hair-splitting is that?  Sorry.
To clarify, my comment is about using a signal to trigger the deletion;
with a signal the sender can not know whether removal completed successfully,
failed or is ongoing. They can merel hope for the best. That's a very weak
security promise.

I for one would expect suspend to wait for successful removal and
that the suspend is cancelled with an error message should removal
fail, to know the state of agents.

>  |> Since _i think_ the poll(2) will not immediately tick due to
>  |> SA_RESTART if the "timeout" parameter is not -1,
..
>  |"The details vary across UNIX systems" suggests that you may
need to
>  |do research on this.
> 
> Wouldn't you agree that the approach that was chosen covers exactly
that?
Yes! Another fd can make poll() return reliably. I'd probably choose a pipe.

>  |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
> 
> To put that straight you mean the opposite, that _if_ socket(2)
> fails we will not be able to connect(2), and therefore the loop
> will not tick.
Sorry, I wasn't so clear - the patch did test socket() success, but
neither connect() nor close() success, at a minimum connect() must
succeed for poll() to return.

> If it wakes up the next time it _will_ remove the keys.
Without feedback (blocking removal) that could be post resume.

Such an addition would merely provide a false sense of security.

>  |a security feature there really needs to be some feedback, making
..
> call the cleanup_handler() and then fatal_r()ly abort the program instead?
I find that a good addition in error paths but it doesn't help with
the race condition inherent to using a signal.

>  |direct interaction with the agents look a lot better, with the
> 
> How do you do this, Mister, from an ACPI script running as root?
for SSH_AUTH_SOCK in /tmp/ssh-*/agent.*; do ssh-add -D; done # maybe?

>  |significant additional advantage of needing zero new agent code.
> 
> Oh nooo!  Spoiler!!!
When in doubt, keep the trusted codebase small.


//Peter