From ffbe58d4feb02aa7751c2a3734d52cbdd22b3d09 Mon Sep 17 00:00:00 2001 From: John Safranek Date: Wed, 21 Oct 2026 09:19:53 -0700 Subject: [PATCH] echoserver: fix agent forwarding and its cleanup The forwarded agent socket now lives in a mkdtemp() directory, and the session's end runs the agent cleanup, which the library never calls on a server. The relay addresses the agent channel by its own id and drains each agent reply in full. - Set agentChannelId from ssh->agent once the agent channel is open - Drop agent replies when no local agent client is connected - Close the accepted agent client in WOLFSSH_AGENT_LOCAL_CLEANUP and remove the socket and its directory - Print the socket path; openssh-interop.test forwards ssh-agent through the echoserver, lists a reply larger than the relay buffer, and checks the directory is private and removed Issue: F-10553, F-11936, F-14396 --- examples/echoserver/echoserver.c | 115 ++++++++++++++++++++++++------- scripts/openssh-interop.test | 113 ++++++++++++++++++++++++++++++ 2 files changed, 203 insertions(+), 25 deletions(-) diff --git a/examples/echoserver/echoserver.c b/examples/echoserver/echoserver.c index acd759f1..5e18d0d6 100644 --- a/examples/echoserver/echoserver.c +++ b/examples/echoserver/echoserver.c @@ -199,8 +199,12 @@ typedef struct WS_AppCtx { #ifdef WOLFSSH_AGENT +#define ECHOSERVER_AGENT_DIR "/tmp/wolfserver.XXXXXX" + typedef struct WS_AgentCbActionCtx { struct sockaddr_un name; + /* The private directory holding name, empty when there is none. */ + char dirName[sizeof(ECHOSERVER_AGENT_DIR)]; } WS_AgentCbActionCtx; #endif @@ -425,23 +429,34 @@ static int wolfSSH_AGENT_DefaultActions(WS_AgentCbAction action, void* vCtx) if (action == WOLFSSH_AGENT_LOCAL_SETUP) { struct sockaddr_un* name = &agentCtx->name; - size_t size; + size_t size = 0; int envSet = 0, nameBound = 0; WMEMSET(name, 0, sizeof(struct sockaddr_un)); name->sun_family = AF_LOCAL; - ret = snprintf(name->sun_path, sizeof(name->sun_path), - "/tmp/wolfserver.%d", (int)getpid()); - - if (ret >= 0) { - name->sun_path[sizeof(name->sun_path) - 1] = '\0'; - size = WSTRLEN(name->sun_path); - ret = (size < WSTRLEN("/tmp/wolfserver.")); + /* mkdtemp() makes a new 0700 directory under an unpredictable name, + * so no other user can claim or reach the socket inside it. */ + WMEMCPY(agentCtx->dirName, ECHOSERVER_AGENT_DIR, + sizeof(ECHOSERVER_AGENT_DIR)); + if (mkdtemp(agentCtx->dirName) == NULL) { + agentCtx->dirName[0] = '\0'; + ret = -1; + } + + if (ret == 0) { + ret = snprintf(name->sun_path, sizeof(name->sun_path), + "%s/agent", agentCtx->dirName); + if (ret > 0 && (size_t)ret < sizeof(name->sun_path)) { + size = (size_t)ret + offsetof(struct sockaddr_un, sun_path); + ret = 0; + } + else { + ret = -1; + } } if (ret == 0) { - size += offsetof(struct sockaddr_un, sun_path); ctx->listenFd = socket(AF_UNIX, SOCK_STREAM, 0); ret = (ctx->listenFd == -1) ? -1 : 0; } @@ -462,12 +477,15 @@ static int wolfSSH_AGENT_DefaultActions(WS_AgentCbAction action, void* vCtx) } if (ret == 0) { + printf("Agent socket: %s\n", name->sun_path); + fflush(stdout); ctx->state = APP_STATE_LISTEN; } else { if (nameBound) { unlink(agentCtx->name.sun_path); } + agentCtx->name.sun_path[0] = '\0'; if (envSet) { unsetenv(EnvNameAuthPort); } @@ -475,14 +493,33 @@ static int wolfSSH_AGENT_DefaultActions(WS_AgentCbAction action, void* vCtx) close(ctx->listenFd); ctx->listenFd = -1; } + if (agentCtx->dirName[0] != '\0') { + rmdir(agentCtx->dirName); + agentCtx->dirName[0] = '\0'; + } ret = WS_AGENT_SETUP_E; } } else if (action == WOLFSSH_AGENT_LOCAL_CLEANUP) { - WCLOSESOCKET(ctx->listenFd); - ctx->listenFd = -1; - unlink(agentCtx->name.sun_path); - unsetenv(EnvNameAuthPort); + /* The accepted agent client goes with the listener. */ + if (ctx->appFd >= 0) { + WCLOSESOCKET(ctx->appFd); + ctx->appFd = -1; + } + if (ctx->listenFd >= 0) { + WCLOSESOCKET(ctx->listenFd); + ctx->listenFd = -1; + } + if (agentCtx->name.sun_path[0] != '\0') { + unlink(agentCtx->name.sun_path); + agentCtx->name.sun_path[0] = '\0'; + unsetenv(EnvNameAuthPort); + } + if (agentCtx->dirName[0] != '\0') { + rmdir(agentCtx->dirName); + agentCtx->dirName[0] = '\0'; + } + ctx->state = APP_STATE_INIT; } else ret = WS_AGENT_INVALID_ACTION; @@ -1461,8 +1498,12 @@ static int ssh_worker(thread_ctx_t* threadCtx) if (!agentOpened) { int agentRc = wolfSSH_AGENT_ChannelOpen(ssh); - if (agentRc == WS_SUCCESS) + if (agentRc == WS_SUCCESS) { + /* The relay below addresses the agent channel by its + * own id, which only internal.h exposes. */ + agentChannelId = ssh->agent->channel; agentOpened = 1; + } else if (agentRc == WS_WANT_WRITE) wantWrite = 1; } @@ -1621,23 +1662,41 @@ static int ssh_worker(thread_ctx_t* threadCtx) * pumps below, on every pass. */ #ifdef WOLFSSH_AGENT if (lastChannel == agentChannelId) { - cnt_r = wolfSSH_ChannelIdRead(ssh, agentChannelId, - threadCtx->agentCtx.fdBuffer, - sizeof threadCtx->agentCtx.fdBuffer); - if (cnt_r <= 0) { + int agentRxd = 0; + + /* One message can carry more than fdBuffer + * holds, and no later report asks for the + * rest, so drain it all now. */ + for (;;) { + cnt_r = wolfSSH_ChannelIdRead(ssh, + agentChannelId, + threadCtx->agentCtx.fdBuffer, + sizeof threadCtx->agentCtx.fdBuffer); + if (cnt_r <= 0) + break; + agentRxd = 1; + #ifdef SHELL_DEBUG + buf_dump(threadCtx->agentCtx.fdBuffer, + cnt_r); + #endif + /* A reply for an agent client that has + * hung up has nowhere to go; drop it. */ + if (agentFd >= 0 && app_write_all(agentFd, + threadCtx->agentCtx.fdBuffer, + (word32)cnt_r) < 0) { + agentRxd = -1; + break; + } + } + if (agentRxd < 0) + break; + if (agentRxd == 0) { /* Nothing was buffered. Only an actual data * report makes that a failure. */ if (rc == WS_REKEYING && cnt_r == 0) continue; break; } - #ifdef SHELL_DEBUG - buf_dump(threadCtx->agentCtx.fdBuffer, cnt_r); - #endif - if (app_write_all(agentFd, - threadCtx->agentCtx.fdBuffer, - (word32)cnt_r) < 0) - break; } #endif } @@ -2376,6 +2435,12 @@ static THREAD_RETURN WOLFSSH_THREAD server_worker(void* vArgs) WCLOSESOCKET(threadCtx->fd); threadCtx->fd = -1; } +#ifdef WOLFSSH_AGENT + /* The library never calls the cleanup on a server, so retire the + * listener, its socket and directory, and any agent client here. */ + (void)wolfSSH_AGENT_DefaultActions(WOLFSSH_AGENT_LOCAL_CLEANUP, + &threadCtx->agentCtx); +#endif #ifdef WOLFSSH_FWD if (threadCtx->fwdCbCtx.hostName != NULL) { WFREE(threadCtx->fwdCbCtx.hostName, NULL, 0); diff --git a/scripts/openssh-interop.test b/scripts/openssh-interop.test index 752a6e46..2d46d403 100755 --- a/scripts/openssh-interop.test +++ b/scripts/openssh-interop.test @@ -10,6 +10,9 @@ # peer that disagreed about the counters would fail the first packet that # had to authenticate. # +# Part 1b reuses the OpenSSH client on the echoserver to forward ssh-agent +# through it. +# # Part 3 has the wolfSSH client authenticate to sshd with an RSA key held # by OpenSSH's ssh-agent. The client has only the public half; the agent # makes the signature. @@ -242,15 +245,20 @@ echo "Test the OpenSSH client against the wolfSSH echoserver" # A build with neither has only Ed25519, and the checked-in Ed25519 key is # PKCS#8, which an OpenSSH on LibreSSL won't load, so make a key of # OpenSSH's own and hand the echoserver its public half. +# Which of the attempts below opened the session, so the agent forwarding +# check can repeat it. +echo_keys=ecc run_openssh_client "$keys_dir/hansel-key-ecc.pem" -e session_status=$? if [ $session_status -ne 0 ]; then echo "the ECC attempt failed, retrying with the RSA sample key" + echo_keys=rsa run_openssh_client "$keys_dir/hansel-key-rsa.pem" session_status=$? fi if [ $session_status -ne 0 ] && command -v ssh-keygen > /dev/null 2>&1; then echo "the RSA attempt failed, retrying with an Ed25519 key" + echo_keys=ed25519 ssh-keygen -q -t ed25519 -N '' -C hansel -f "$work_dir/ed25519" \ < /dev/null || fail "couldn't make an Ed25519 client key" run_openssh_client "$work_dir/ed25519" -j "$work_dir/ed25519.pub" @@ -284,6 +292,111 @@ fi echo "the OpenSSH client negotiated strict KEX with the echoserver" +# Part 1b forwards OpenSSH's ssh-agent through the echoserver. A local agent +# client on the echoserver's side lists the forwarded key over the socket +# the echoserver made for it, twice, so the second goes through a fresh +# accept. The socket's directory must be private and gone once the session +# is over. +if ! ./apps/wolfssh-options 2>/dev/null | grep -qx 'AGENT'; then + echo "the echoserver has no agent support, skipping agent forwarding" +elif ! command -v ssh-agent > /dev/null 2>&1 || + ! command -v ssh-add > /dev/null 2>&1 || + ! command -v ssh-keygen > /dev/null 2>&1 || + ! command -v mkfifo > /dev/null 2>&1; then + echo "no ssh-agent, ssh-add, ssh-keygen or mkfifo," \ + "skipping agent forwarding" +else + echo "Test agent forwarding through the wolfSSH echoserver" + + # The agent picks its own socket path; see Part 3. + SSH_AGENT_PID= + eval `ssh-agent -s` > /dev/null + [ -n "$SSH_AGENT_PID" ] || fail "couldn't start ssh-agent" + agent_pid=$SSH_AGENT_PID + + ssh-keygen -q -t ed25519 -N '' -C wolfssh-agent-fwd \ + -f "$work_dir/agent-fwd" < /dev/null \ + || fail "couldn't make a key for the agent" + ssh-add -q "$work_dir/agent-fwd" 2>/dev/null \ + || fail "couldn't add the key to ssh-agent" + # Long comments push the list reply past the echoserver's relay + # buffer, so it has to be drained in more than one read. + agent_pad=`printf '%1500s' '' | tr ' ' x` + for pad in 1 2 3 4; do + ssh-keygen -q -t ed25519 -N '' -C "wolfssh-agent-pad$pad-$agent_pad" \ + -f "$work_dir/agent-pad$pad" < /dev/null \ + || fail "couldn't make a padding key for the agent" + ssh-add -q "$work_dir/agent-pad$pad" 2>/dev/null \ + || fail "couldn't add a padding key to ssh-agent" + done + + # The client key was copied to $work_dir/id by the attempt that worked. + case $echo_keys in + ecc) start_echoserver -e ;; + rsa) start_echoserver ;; + *) start_echoserver -j "$work_dir/ed25519.pub" ;; + esac + + # The session stays open while the agent is used through it. + rm -f "$work_dir/fwd_in" "$client_out" "$client_log" + mkfifo "$work_dir/fwd_in" || fail "couldn't make a fifo" + ssh -A -T -p $port \ + -o StrictHostKeyChecking=no \ + -o UserKnownHostsFile=/dev/null \ + -o IdentitiesOnly=yes \ + -o PreferredAuthentications=publickey \ + -i "$work_dir/id" hansel@127.0.0.1 \ + < "$work_dir/fwd_in" > "$client_out" 2> "$client_log" & + fwd_ssh_pid=$! + exec 3> "$work_dir/fwd_in" + printf '%s\n' "$probe" >&3 + + fwd_sock= + counter=0 + while [ -z "$fwd_sock" ] && [ "$counter" -lt 100 ]; do + fwd_sock=`sed -n 's/^Agent socket: //p' "$work_dir/server.log"` + [ -z "$fwd_sock" ] && sleep 0.1 + counter=$((counter + 1)) + done + if [ -z "$fwd_sock" ]; then + exec 3>&- + cat "$work_dir/server.log" + fail "the echoserver never opened an agent socket" + fi + fwd_dir=`dirname "$fwd_sock"` + + case `ls -ld "$fwd_dir"` in + drwx------*) ;; + *) exec 3>&-; fail "the agent socket's directory is not private" ;; + esac + + for attempt in 1 2; do + SSH_AUTH_SOCK="$fwd_sock" ssh-add -l > "$work_dir/fwd_list" 2>&1 & + wait_with_limit $! + if ! grep -q 'wolfssh-agent-fwd' "$work_dir/fwd_list" || + [ `grep -c 'wolfssh-agent-pad' "$work_dir/fwd_list"` -ne 4 ]; then + exec 3>&- + cat "$work_dir/fwd_list" + fail "the forwarded agent didn't list its keys (try $attempt)" + fi + done + + printf '\003' >&3 + exec 3>&- + wait_with_limit $fwd_ssh_pid + wait_with_limit $server_pid + server_pid=$no_pid + + grep -q "$probe" "$client_out" \ + || fail "the session didn't survive the forwarded agent" + [ -e "$fwd_dir" ] && fail "the echoserver left $fwd_dir behind" + + kill $agent_pid 2>/dev/null + agent_pid=$no_pid + + echo "the forwarded agent answered through the echoserver" +fi + # Part 2 needs an sshd to talk to, and a host key to give it. Neither is # worth failing the test over. sshd_bin=