Mailing List Archive

Steffen Nurpmeso wrote in
<20210908114659.EuomV%steffen@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)
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev

Steffen Nurpmeso wrote in
<20210908220546.suMBN%steffen@sdaoden.eu>:
|Steffen Nurpmeso wrote in
| <20210908114659.EuomV%steffen@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)
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev

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
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev

Peter Stuge wrote in
<20210909181705.28572.qmail@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)
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev

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
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev

On Fri, 10 Sept 2021 at 09:04, Peter Stuge <peter@stuge.se> wrote:

> Hi Steffen,
>
> Steffen Nurpmeso wrote:
> [...]
> > |"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.
>

You could change the poll() to ppoll() and give it an appropriate signal
mask, then the signal handler would only have to set a sig_atomic_t flag.

Not every platform has ppoll(), but we already have compat code
implementing both poll() and pselect() on top of select(), so we've already
got most of what would be needed to do ppoll too.

--
Darren Tucker (dtucker at dtucker.net)
GPG key 11EAA6FA / A86E 3E07 5B19 5880 E860 37F4 9357 ECEF 11EA A6FA (new)
Good judgement comes with experience. Unfortunately, the experience
usually comes from bad judgement.
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev

Hello Peter.

Peter Stuge wrote in
<20210909225708.2121.qmail@stuge.se>:
|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;

Well in the end i am only def and blind and need some time for
wrapping my head around a problem. I think in fact that de
Raadt's initial comment pointed into the right direction, and that
requires nothing such, but the patch would be

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..014cf7b38c 100644
--- a/ssh-agent.c
+++ b/ssh-agent.c
@@ -171,6 +171,11 @@ 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_asap = 0;
+
+static enum run_state { RS_RUN, RS_POLL } run_state = RS_RUN;
+
static void
close_socket(SocketEntry *e)
{
@@ -529,11 +534,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 +547,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 +1353,16 @@ cleanup_handler(int sig)
_exit(2);
}

+/*ARGSUSED*/
+static void
+remove_all_handler(int sig)
+{
+ if (run_state == RS_POLL)
+ remove_all_identities();
+ else
+ remove_all_asap = 1;
+}
+
static void
check_parent_exists(void)
{
@@ -1619,6 +1640,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 +1648,17 @@ skip:

while (1) {
prepare_poll(&pfd, &npfd, &timeout, maxfds);
+ run_state = RS_POLL;
result = poll(pfd, npfd, timeout);
+ run_state = RS_RUN;
saved_errno = errno;
if (parent_alive_interval != 0)
check_parent_exists();
- (void) reaper(); /* remove expired keys */
+ if (remove_all_asap) {
+ remove_all_asap = 0;
+ remove_all_identities();
+ } else
+ (void) reaper(); /* remove expired keys */
if (result == -1) {
if (saved_errno == EINTR)
continue;

And if you really want to be super exact, one would add

diff --git a/ssh-agent.c b/ssh-agent.c
index 014cf7b38c..1ca8d3229e 100644
--- a/ssh-agent.c
+++ b/ssh-agent.c
@@ -1647,6 +1647,10 @@ skip:
platform_pledge_agent();

while (1) {
+ if (remove_all_asap) {
+ remove_all_asap = 0;
+ remove_all_identities();
+ }
prepare_poll(&pfd, &npfd, &timeout, maxfds);
run_state = RS_POLL;
result = poll(pfd, npfd, timeout);

So with these two there is the security guarantee you were asking
for, that the removal happens instantly. It is also portable.
Since all signals are blocked, all we need to know is whether we
are hanging on poll(2), in which case it is "safe" to remove from
within the signal handler in that no other code is currently
running in user space. I'd say. (Of course on Cygwin poll(2)
could be implemented like grazy, i have not looked in that for
i would say really twenty years, select(2) by then, but then again
i think they should block signals while working this tremendous
piece of code in something that people think are system calls.)

|with a signal the sender can not know whether removal completed successf\
|ully,
|failed or is ongoing. They can merel hope for the best. That's a very weak
|security promise.

But why so Peter? Signals are used for communication on Unix
since ever? If you send a signal to a process, and it has not
ignored the signal (if it can), the action behind happens.

You can see that easily on your Linux system, it kill aborts any
program you have rights for, just specify a realtime signal.

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

What should fail when zeroing memory? There is no status for this
operation anyhow, is it? It is merely you calling ssh-add, which
then reports interaction with the agent was successful.

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

This is overly funny.

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

Done. I always hate it to look in codebases that i do not know.
There is so much work and time in such a codebase, done by people
knowing the context exactly, and i just hate it. I mean this is
effectively an easy thing here (but for me), but in general
someone who knows a codebase can implement something _right_
(avoiding pitfalls) in a very short time, a state that third
parties can achieve only with real effort.

So the patch above should possibly have been it from the start.
Maybe it will be considered, that would be nice. (Just 's/^ //.)

Ciao from Germany,

--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)
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev

Steffen Nurpmeso wrote in
<20210910120610._EV-u%steffen@sdaoden.eu>:
...
||failed or is ongoing. They can merel hope for the best. That's a very weak
||security promise.

And ... a test addition that somehow also escaped me until now.
(Btw i tried "make tests LTESTS=agent" after having run the
complete test once, and for me it seems to run all tests starting
with the one given in LTESTS, or at least:

make[1]: Entering directory '/tmp/x/openssh.tar_bomb_git/regress'
run test agent.sh ...
ok simple agent test
make[1]: Leaving directory '/tmp/x/openssh.tar_bomb_git/regress'
all t-exec passed
BUILDDIR=`pwd`; \
cd ./regress || exit $?; \
EGREP='/usr/bin/grep -E'
...interop-tests
...test_sshbuf: ......^Cmake[1]:
*** [Makefile:251: unit] Interrupt
make: *** [Makefile:713: unit] Interrupt

So i hope i finally made my homework and can now stop making noise.

Ciao.
And a nice weekend everybody.

diff --git a/regress/agent.sh b/regress/agent.sh
index f187b67572..2544f932eb 100644
--- a/regress/agent.sh
+++ b/regress/agent.sh
@@ -157,30 +157,42 @@ done

## Deletion tests.

+delete_cycle() {
+ # make sure they're gone
+ ${SSHADD} -l > /dev/null 2>&1
+ r=$?
+ if [ $r -ne 1 ]; then
+ fail "ssh-add -l returned unexpected exit code: $r"
+ fi
+ trace "readd keys"
+ # re-add keys/certs to agent
+ for t in ${SSH_KEYTYPES}; do
+ ${SSHADD} $OBJ/$t-agent-private >/dev/null 2>&1 || \
+ fail "ssh-add failed exit code $?"
+ done
+ # make sure they are there
+ ${SSHADD} -l > /dev/null 2>&1
+ r=$?
+ if [ $r -ne 0 ]; then
+ fail "ssh-add -l failed: exit code $r"
+ fi
+}
+
trace "delete all agent keys"
${SSHADD} -D > /dev/null 2>&1
r=$?
if [ $r -ne 0 ]; then
fail "ssh-add -D failed: exit code $r"
fi
-# make sure they're gone
-${SSHADD} -l > /dev/null 2>&1
-r=$?
-if [ $r -ne 1 ]; then
- fail "ssh-add -l returned unexpected exit code: $r"
-fi
-trace "readd keys"
-# re-add keys/certs to agent
-for t in ${SSH_KEYTYPES}; do
- ${SSHADD} $OBJ/$t-agent-private >/dev/null 2>&1 || \
- fail "ssh-add failed exit code $?"
-done
-# make sure they are there
-${SSHADD} -l > /dev/null 2>&1
+delete_cycle
+
+trace "delete all agent keys via SIGUSR1"
+kill -USR1 $SSH_AGENT_PID >/dev/null 2>&1
r=$?
if [ $r -ne 0 ]; then
- fail "ssh-add -l failed: exit code $r"
+ fail "kill -USR1: exit code $r"
fi
+delete_cycle

check_key_absent() {
${SSHADD} -L | grep "^$1 " >/dev/null

--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)
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev

Hi Steffen,

Steffen Nurpmeso wrote:
> So with these two there is the security guarantee you were asking
> for, that the removal happens instantly.

Sorry but no, as long as the trigger is a signal there's no guarantee.
See below.


> It is also portable.

Nice!


> |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.
>
> But why so Peter? Signals are used for communication on Unix since ever?

In your stated use case of suspend on laptop lid close there is a race
condition between the actual suspend and SIGUSR1 being handled in agents.

The ACPI script runs various commands to prepare for suspend, one of
them would be pkill or killall. That command would find all agent
processes and call kill(2) to send SIGUSR1 to each one.

There's a race because signal handling is asynchronous.

kill(2) returns as soon as the signal has been *generated*, not when the
signal handler in the receiving process has completed, in any case quite
possibly before the signal handler has even started.

After generating signals the pkill/killall command exits and from there
execution of the suspend script (which continues towards suspending) and
execution of signal handlers in agents (clearing keys) are decoupled
without any way to synchronize - the suspend script can't get feedback
from the agents.


Consider as an exaggerated example the pkill/killall command being the
very last command in the ACPI script before echo mem > /sys/power/state
and a system with two CPU threads and 100+ running agents.

I wouldn't be surprised if the kernel will have suspended the machine
before all 100+ signal handlers have finished clearing keys. Maybe some
or many kernels will reliably deliver all pending signals before suspending,
but that doesn't mean much beyond scheduling the signal handlers, right?

In particular I don't expect any kernel to wait for all processes to be
idle or otherwise wait for signal handlers to return.

I also wouldn't be surprised if the kernel would suspend the machine
at least sometimes before keys were removed with just one agent running.

That may be less likely, but the point is that with a signal there's no
way to know *for sure* that signal handlers have completed before suspend.


> ignored the signal (if it can), the action behind happens.

Yes. The signal handler will run at some point, but maybe not until
after resume, leaving keys in the agent while the machine is suspended,
ie. silently failing to do what was intended.


> |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.
>
> What should fail when zeroing memory?

Zeroing can't fail - later patch iterations have improved in this regard!

In earlier iterations at least socket() and connect() could have failed.


> There is no status for this operation anyhow, is it?

There is; anything but successful exit of ssh-add -D indicates that the
intended operation was not (or not yet) completed as intended.

The operation is logically simple, but technically complex enough that
there can be many reasons for failure. Rather than trying to explicitly
check them all I like that if ssh-add -D exits with success then, and
only then, I do know that the intended operation has completed.


> |>|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?
>
> This is overly funny.

Why? Did you test it and/or spot a problem? It does work for me.

If this is run in the suspend script then there's no race condition.

With added error checking of ssh-add exit status the machine could
be made to not suspend unless *after* all keys had been removed.


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

I disagree; every addition grows the codebase.


> I always hate it to look in codebases that i do not know.

I can understand that!

I see that a bit differently: I find it interesting to look in code that
I don't know, because I may learn from it.

You're right that it requires effort, but so does working with my own code. :)


Kind regards

//Peter
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev

Dear Peter Stuge.

Peter Stuge wrote in
<20210911164735.15145.qmail@stuge.se>:
|Steffen Nurpmeso wrote:
|> So with these two there is the security guarantee you were asking
|> for, that the removal happens instantly.
|
|Sorry but no, as long as the trigger is a signal there's no guarantee.
|See below.
...
|> But why so Peter? Signals are used for communication on Unix since ever?
|
|In your stated use case of suspend on laptop lid close there is a race
|condition between the actual suspend and SIGUSR1 being handled in agents.
|
|The ACPI script runs various commands to prepare for suspend, one of
|them would be pkill or killall. That command would find all agent
|processes and call kill(2) to send SIGUSR1 to each one.
|
|There's a race because signal handling is asynchronous.

Yes. Well ... yes.
Actually it is worse since we do

act() {
logger -t /root/bin/zzz.sh "$*"
eval "$@"
}

act 'SSH_AUTH_SOCK="$a" ssh-add -D </dev/null >/dev/null 2>&1 &'

to be eventually replaced by

act 'pkill -USR1 ssh-agent </dev/null >/dev/null 2>&1 &'

Especially on those fewest-CPU systems where the parent continues
to run until it no longer can :)

Note i already do

act 'pkill -TERM --full setup-privacy.sh </dev/null >/dev/null 2>&1 &'

for good.

...
|Consider as an exaggerated example the pkill/killall command being the
|very last command in the ACPI script before echo mem > /sys/power/state
|and a system with two CPU threads and 100+ running agents.

But this is not the way it is done here. If anything had to
happen, like unmounting encrypted directories, we are sleeping
a while. That is three seconds at the moment. Which is a very,
very long time on a modern computer that does the work this thing
here does. Also it _then_ really syncs and sleeps in between the
syncs thereafter. It is an interesting thing however, maybe one
should pgrep first and count how many agents will be contacted.
Interesting.

...
|I see that a bit differently: I find it interesting to look in code that
|I don't know, because I may learn from it.

That sometimes happens indeed. But very rarely in user space code
i really have to say. Mostly i get frightened how much code can
be soiled. For example yesterday i looked in shadow's login.c,
and was almost on side-by-side comparison with FreeBSD's one, and
then also busybox's one. (In order to find out whether it would
make sense to provide a pam_session_reaper module in order to be
able to handle sessions gracefully without systemd. Not for the
former). It is an experience! You become a BSD fan.

Or for example today i have seen a zstd code addition request to
busybox, and it was the first time in at least 17 years that
i have seen C code that uses p2align asm() statements to
code-align loop starts (i only ever did so in assembler), and
comments like "better for gcc-9 and gcc-10, worse for clang and
gcc-8, gcc-11". It seems if you work for Facebook you have quite
some systems and combinations for testing. (I did such stuff
myself by then, really!)

|You're right that it requires effort, but so does working with my own \
|code. :)

Yes. Yes.

--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)
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev

Hello.

So please let me ping this once, i thought it is a good time, now
that OpenBSD 7.1 is out.

It is a small and pretty local change, and it incorporates the
comments of Darren Tucker (i finally understood what you were
thinking) in another now closed pull on github (being there again
for such things, but i did not read the docs..), the still open
pull request is [1]. Tests run. (I will not mail again.)

Thanks for your consideration, and
Ciao from Germany already here.

[1] https://github.com/openssh/openssh-portable/pull/297

From: Steffen Nurpmeso <steffen@sdaoden.eu>
Date: Fri, 21 Jan 2022 00:11:18 +0100
Subject: [PATCH] ssh-agent: remove all keys upon SIGUSR1..

With the advent of per-user temporary directories it became
hard for an administrator to remove all keys from all running
ssh-agent instances; what formerly could be done like so

if command -v ssh-add >/dev/null 2>&1; then
for a in /tmp/ssh-*/agent.*; do
[ -e "$a" ] || continue
act "SSH_AUTH_SOCK=\"$a\" ssh-add -D </dev/null >/dev/null 2>&1 &"
inc
done
fi

has become a major undertaking, especially with even more
containerization. Being able to remove all keys from all agents
with a single command seems so desirable that it is available in
other agents in the software world.
---
regress/agent.sh | 45 +++++++++++++++++++++++----------
ssh-agent.1 | 3 +++
ssh-agent.c | 66 ++++++++++++++++++++++++++++++++++++++----------
3 files changed, 87 insertions(+), 27 deletions(-)

diff --git a/regress/agent.sh b/regress/agent.sh
index f187b67572..43d4bd0724 100644
--- a/regress/agent.sh
+++ b/regress/agent.sh
@@ -157,30 +157,49 @@ done

## Deletion tests.

+delete_cycle() {
+ # make sure they're gone
+ ${SSHADD} -l > /dev/null 2>&1
+ r=$?
+ if [ $r -ne 1 ]; then
+ fail "ssh-add -l returned unexpected exit code: $r"
+ fi
+ trace "readd keys"
+ # re-add keys/certs to agent
+ for t in ${SSH_KEYTYPES}; do
+ ${SSHADD} $OBJ/$t-agent-private >/dev/null 2>&1 || \
+ fail "ssh-add failed exit code $?"
+ done
+ # make sure they are there
+ ${SSHADD} -l > /dev/null 2>&1
+ r=$?
+ if [ $r -ne 0 ]; then
+ fail "ssh-add -l failed: exit code $r"
+ fi
+}
+
trace "delete all agent keys"
${SSHADD} -D > /dev/null 2>&1
r=$?
if [ $r -ne 0 ]; then
fail "ssh-add -D failed: exit code $r"
fi
-# make sure they're gone
-${SSHADD} -l > /dev/null 2>&1
+delete_cycle
+
+trace "delete all agent keys via SIGUSR1"
+kill -USR1 $SSH_AGENT_PID >/dev/null 2>&1
r=$?
-if [ $r -ne 1 ]; then
- fail "ssh-add -l returned unexpected exit code: $r"
+if [ $r -ne 0 ]; then
+ fail "kill -USR1: exit code $r"
fi
-trace "readd keys"
-# re-add keys/certs to agent
-for t in ${SSH_KEYTYPES}; do
- ${SSHADD} $OBJ/$t-agent-private >/dev/null 2>&1 || \
- fail "ssh-add failed exit code $?"
-done
-# make sure they are there
-${SSHADD} -l > /dev/null 2>&1
+delete_cycle
+trace ".. and again"
+kill -USR1 $SSH_AGENT_PID >/dev/null 2>&1
r=$?
if [ $r -ne 0 ]; then
- fail "ssh-add -l failed: exit code $r"
+ fail "kill -USR1: exit code $r"
fi
+delete_cycle

check_key_absent() {
${SSHADD} -L | grep "^$1 " >/dev/null
diff --git a/ssh-agent.1 b/ssh-agent.1
index ea43cd156b..7748293791 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 03ae2b022e..ff091c3f8a 100644
--- a/ssh-agent.c
+++ b/ssh-agent.c
@@ -189,6 +189,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 received_sigusr1 = 0;
+
static void
close_socket(SocketEntry *e)
{
@@ -915,11 +918,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)) {
@@ -929,6 +931,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);
@@ -1874,7 +1883,8 @@ after_poll(struct pollfd *pfd, size_t npfd, u_int maxfds)
}

static int
-prepare_poll(struct pollfd **pfdp, size_t *npfdp, int *timeoutp, u_int maxfds)
+prepare_poll(struct pollfd **pfdp, size_t *npfdp, struct timespec **ptimeoutpp,
+ u_int maxfds)
{
struct pollfd *pfd = *pfdp;
size_t i, j, npfd = 0;
@@ -1936,17 +1946,17 @@ prepare_poll(struct pollfd **pfdp, size_t *npfdp, int *timeoutp, u_int maxfds)
break;
}
}
+
+ /* This also removes expired keys */
deadline = reaper();
if (parent_alive_interval != 0)
deadline = (deadline == 0) ? parent_alive_interval :
MINIMUM(deadline, parent_alive_interval);
- if (deadline == 0) {
- *timeoutp = -1; /* INFTIM */
- } else {
- if (deadline > INT_MAX / 1000)
- *timeoutp = INT_MAX / 1000;
- else
- *timeoutp = deadline * 1000;
+ if (deadline == 0)
+ *ptimeoutpp = NULL; /* INFTIM */
+ else {
+ (*ptimeoutpp)->tv_nsec = 0;
+ (*ptimeoutpp)->tv_sec = deadline;
}
return (1);
}
@@ -1981,6 +1991,13 @@ cleanup_handler(int sig)
_exit(2);
}

+/*ARGSUSED*/
+static void
+onsigusr1(int sig)
+{
+ received_sigusr1 = 1;
+}
+
static void
check_parent_exists(void)
{
@@ -2022,7 +2039,8 @@ main(int ac, char **av)
char pidstrbuf[1 + 3 * sizeof pid];
size_t len;
mode_t prev_mask;
- int timeout = -1; /* INFTIM */
+ sigset_t psigset, psigseto;
+ struct timespec ptimeout, *ptimeoutp;
struct pollfd *pfd = NULL;
size_t npfd = 0;
u_int maxfds;
@@ -2258,18 +2276,38 @@ 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, onsigusr1);

if (pledge("stdio rpath cpath unix id proc exec", NULL) == -1)
fatal("%s: pledge: %s", __progname, strerror(errno));
platform_pledge_agent();

+ /*
+ * Prepare signal mask that we use to block signals that might set
+ * received_sigusr1, so that we are guaranteed to immediately wake up
+ * the ppoll if a signal is received after the flag is checked.
+ */
+ sigemptyset(&psigset);
+ if (d_flag | D_flag)
+ sigaddset(&psigset, SIGINT);
+ sigaddset(&psigset, SIGHUP);
+ sigaddset(&psigset, SIGTERM);
+ sigaddset(&psigset, SIGUSR1);
+
while (1) {
- prepare_poll(&pfd, &npfd, &timeout, maxfds);
- result = poll(pfd, npfd, timeout);
+ sigprocmask(SIG_BLOCK, &psigset, &psigseto);
+ if (received_sigusr1) {
+ received_sigusr1 = 0;
+ remove_all_identities();
+ }
+ /* This also removes expired keys */
+ ptimeoutp = &ptimeout;
+ prepare_poll(&pfd, &npfd, &ptimeoutp, maxfds);
+ result = ppoll(pfd, npfd, ptimeoutp, &psigseto);
saved_errno = errno;
+ sigprocmask(SIG_SETMASK, &psigseto, NULL);
if (parent_alive_interval != 0)
check_parent_exists();
- (void) reaper(); /* remove expired keys */
if (result == -1) {
if (saved_errno == EINTR)
continue;
--
2.35.3


--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)
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev