nhyatt/putty-source
1
0
Fork 0
mirror of https://git.tartarus.org/simon/putty.git synced 2025-01-09 09:27:59 +00:00
Code Issues Projects Releases Wiki Activity

Make asynchronous agent_query() requests cancellable.

Browse Source 
Now, instead of returning a boolean indicating whether the query has
completed or is still pending, agent_query() returns NULL to indicate
that the query _has_ completed, and if it hasn't, it returns a pointer
to a context structure representing the pending query, so that the
latter can be used to cancel the query if (for example) you later
decide you need to free the thing its callback was using as a context.

This should fix a potential race-condition segfault if you overload an
agent forwarding channel and then close it abruptly. (Which nobody
will be doing for sensible purposes, of course! But I ran across this
while stress-testing other aspects of agent forwarding.)
This commit is contained in:
Simon Tatham 2017-01-29 20:24:15 +00:00
parent f864265e39
commit eb2fe29fc9
7 changed files with 140 additions and 71 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

12
Recipe
View File

@ -244,7 +244,7 @@ NONSSH = telnet raw rlogin ldisc pinger
SSH = ssh sshcrc sshdes sshmd5 sshrsa sshrand sshsha sshblowf
+ sshdh sshcrcda sshpubk sshzlib sshdss x11fwd portfwd
+ sshaes sshccp sshsh256 sshsh512 sshbn wildcard pinger ssharcf
+ sshgssc pgssapi sshshare sshecc
+ sshgssc pgssapi sshshare sshecc aqsync
WINSSH = SSH winnoise wincapi winpgntc wingss winshare winnps winnpc
+ winhsock errsock
UXSSH = SSH uxnoise uxagentc uxgss uxshare
@ -299,7 +299,7 @@ psftp : [C] psftp winsftp wincons WINSSH BE_SSH SFTP wildcard WINMISC
+ psftp.res winnojmp LIBS
pageant : [G] winpgnt pageant sshrsa sshpubk sshdes sshbn sshmd5 version
+ tree234 misc sshaes sshsha winsecur winpgntc sshdss sshsh256
+ tree234 misc sshaes sshsha winsecur winpgntc aqsync sshdss sshsh256
+ sshsh512 winutils sshecc winmisc winhelp conf pageant.res LIBS
puttygen : [G] winpgen sshrsag sshdssg sshprime sshdes sshbn sshmd5 version
@ -331,10 +331,10 @@ cgtest : [UT] cgtest PUTTYGEN_UNIX
pscp : [U] pscp uxsftp uxcons UXSSH BE_SSH SFTP wildcard UXMISC
psftp : [U] psftp uxsftp uxcons UXSSH BE_SSH SFTP wildcard UXMISC
pageant : [X] uxpgnt uxagentc pageant sshrsa sshpubk sshdes sshbn sshmd5
+ version tree234 misc sshaes sshsha sshdss sshsh256 sshsh512 sshecc
+ conf uxsignal nocproxy nogss be_none x11fwd ux_x11 uxcons gtkask
+ gtkmisc UXMISC
pageant : [X] uxpgnt uxagentc aqsync pageant sshrsa sshpubk sshdes sshbn
+ sshmd5 version tree234 misc sshaes sshsha sshdss sshsh256 sshsh512
+ sshecc conf uxsignal nocproxy nogss be_none x11fwd ux_x11 uxcons
+ gtkask gtkmisc UXMISC
ptermapp : [XT] GTKTERM uxmisc misc ldisc settings uxpty uxsel BE_NONE uxstore
+ uxsignal CHARSET cmdline uxpterm version time xpmpterm xpmptcfg

21
aqsync.c Normal file
View File

@ -0,0 +1,21 @@
/*
* aqsync.c: the agent_query_synchronous() wrapper function.
*
* This is a very small thing to have to put in its own module, but it
* wants to be shared between back ends, and exist in any SSH client
* program and also Pageant, and _nowhere else_ (because it pulls in
* the main agent_query).
*/
#include <assert.h>
#include "putty.h"
void agent_query_synchronous(void *in, int inlen, void **out, int *outlen)
{
agent_pending_query *pending;
pending = agent_query(in, inlen, out, outlen, NULL, 0);
assert(!pending);
}

34
pageant.c
View File

@ -1196,12 +1196,12 @@ void *pageant_get_keylist1(int *length)
if (!pageant_local) {
unsigned char request[5], *response;
void *vresponse;
int resplen, retval;
int resplen;
request[4] = SSH1_AGENTC_REQUEST_RSA_IDENTITIES;
PUT_32BIT(request, 1);
retval = agent_query(request, 5, &vresponse, &resplen, NULL, NULL);
assert(retval == 1);
agent_query_synchronous(request, 5, &vresponse, &resplen);
response = vresponse;
if (resplen < 5 || response[4] != SSH1_AGENT_RSA_IDENTITIES_ANSWER) {
sfree(response);
@ -1227,13 +1227,12 @@ void *pageant_get_keylist2(int *length)
if (!pageant_local) {
unsigned char request[5], *response;
void *vresponse;
int resplen, retval;
int resplen;
request[4] = SSH2_AGENTC_REQUEST_IDENTITIES;
PUT_32BIT(request, 1);
retval = agent_query(request, 5, &vresponse, &resplen, NULL, NULL);
assert(retval == 1);
agent_query_synchronous(request, 5, &vresponse, &resplen);
response = vresponse;
if (resplen < 5 || response[4] != SSH2_AGENT_IDENTITIES_ANSWER) {
sfree(response);
@ -1471,7 +1470,7 @@ int pageant_add_keyfile(Filename *filename, const char *passphrase,
if (!pageant_local) {
unsigned char *request, *response;
void *vresponse;
int reqlen, clen, resplen, ret;
int reqlen, clen, resplen;
clen = strlen(rkey->comment);
@ -1504,9 +1503,7 @@ int pageant_add_keyfile(Filename *filename, const char *passphrase,
reqlen += 4 + clen;
PUT_32BIT(request, reqlen - 4);
ret = agent_query(request, reqlen, &vresponse, &resplen,
NULL, NULL);
assert(ret == 1);
agent_query_synchronous(request, reqlen, &vresponse, &resplen);
response = vresponse;
if (resplen < 5 || response[4] != SSH_AGENT_SUCCESS) {
*retstr = dupstr("The already running Pageant "
@ -1524,7 +1521,7 @@ int pageant_add_keyfile(Filename *filename, const char *passphrase,
if (!pageant_local) {
unsigned char *request, *response;
void *vresponse;
int reqlen, alglen, clen, keybloblen, resplen, ret;
int reqlen, alglen, clen, keybloblen, resplen;
alglen = strlen(skey->alg->name);
clen = strlen(skey->comment);
@ -1552,9 +1549,7 @@ int pageant_add_keyfile(Filename *filename, const char *passphrase,
reqlen += clen + 4;
PUT_32BIT(request, reqlen - 4);
ret = agent_query(request, reqlen, &vresponse, &resplen,
NULL, NULL);
assert(ret == 1);
agent_query_synchronous(request, reqlen, &vresponse, &resplen);
response = vresponse;
if (resplen < 5 || response[4] != SSH_AGENT_SUCCESS) {
*retstr = dupstr("The already running Pageant "
@ -1741,8 +1736,7 @@ int pageant_delete_key(struct pageant_pubkey *key, char **retstr)
memcpy(request + 9, key->blob, key->bloblen);
}
ret = agent_query(request, reqlen, &vresponse, &resplen, NULL, NULL);
assert(ret == 1);
agent_query_synchronous(request, reqlen, &vresponse, &resplen);
response = vresponse;
if (resplen < 5 || response[4] != SSH_AGENT_SUCCESS) {
*retstr = dupstr("Agent failed to delete key");
@ -1759,14 +1753,13 @@ int pageant_delete_key(struct pageant_pubkey *key, char **retstr)
int pageant_delete_all_keys(char **retstr)
{
unsigned char request[5], *response;
int reqlen, resplen, success, ret;
int reqlen, resplen, success;
void *vresponse;
PUT_32BIT(request, 1);
request[4] = SSH2_AGENTC_REMOVE_ALL_IDENTITIES;
reqlen = 5;
ret = agent_query(request, reqlen, &vresponse, &resplen, NULL, NULL);
assert(ret == 1);
agent_query_synchronous(request, reqlen, &vresponse, &resplen);
response = vresponse;
success = (resplen >= 4 && response[4] == SSH_AGENT_SUCCESS);
sfree(response);
@ -1778,8 +1771,7 @@ int pageant_delete_all_keys(char **retstr)
PUT_32BIT(request, 1);
request[4] = SSH1_AGENTC_REMOVE_ALL_RSA_IDENTITIES;
reqlen = 5;
ret = agent_query(request, reqlen, &vresponse, &resplen, NULL, NULL);
assert(ret == 1);
agent_query_synchronous(request, reqlen, &vresponse, &resplen);
response = vresponse;
success = (resplen >= 4 && response[4] == SSH_AGENT_SUCCESS);
sfree(response);

25
putty.h
View File

@ -1200,17 +1200,32 @@ void crypto_wrapup();
/*
* Exports from pageantc.c.
*
* agent_query returns 1 for here's-a-response, and 0 for query-in-
* progress. In the latter case there will be a call to `callback'
* at some future point, passing callback_ctx as the first
* agent_query returns NULL for here's-a-response, and non-NULL for
* query-in- progress. In the latter case there will be a call to
* `callback' at some future point, passing callback_ctx as the first
* parameter and the actual reply data as the second and third.
*
* The response may be a NULL pointer (in either of the synchronous
* or asynchronous cases), which indicates failure to receive a
* response.
*
* When the return from agent_query is not NULL, it identifies the
* in-progress query in case it needs to be cancelled. If
* agent_cancel_query is called, then the pending query is destroyed
* and the callback will not be called. (E.g. if you're going to throw
* away the thing you were using as callback_ctx.)
*
* Passing a null pointer as callback forces agent_query to behave
* synchronously, i.e. it will block if necessary, and guarantee to
* return NULL. The wrapper function agent_query_synchronous() makes
* this easier.
*/
int agent_query(void *in, int inlen, void **out, int *outlen,
void (*callback)(void *, void *, int), void *callback_ctx);
typedef struct agent_pending_query agent_pending_query;
agent_pending_query *agent_query(
void *in, int inlen, void **out, int *outlen,
void (*callback)(void *, void *, int), void *callback_ctx);
void agent_cancel_query(agent_pending_query *);
void agent_query_synchronous(void *in, int inlen, void **out, int *outlen);
int agent_exists(void);
/*

53
ssh.c
View File

@ -577,6 +577,7 @@ struct ssh_channel {
unsigned char msglen[4];
unsigned lensofar, totallen;
int outstanding_requests;
agent_pending_query *pending;
} a;
struct ssh_x11_channel {
struct X11Connection *xconn;
@ -985,6 +986,13 @@ struct ssh_tag {
* with a newly cross-certified host key.
*/
int cross_certifying;
/*
* Any asynchronous query to our SSH agent that we might have in
* flight from the main authentication loop. (Queries from
* agent-forwarding channels live in their channel structure.)
*/
agent_pending_query *auth_agent_query;
};
static const char *ssh_pkt_type(Ssh ssh, int type)
@ -3811,6 +3819,8 @@ static void ssh_agent_callback(void *sshv, void *reply, int replylen)
{
Ssh ssh = (Ssh) sshv;
ssh->auth_agent_query = NULL;
ssh->agent_response = reply;
ssh->agent_response_len = replylen;
@ -3843,6 +3853,7 @@ static void ssh_agentf_callback(void *cv, void *reply, int replylen)
struct ssh_channel *c = (struct ssh_channel *)cv;
const void *sentreply = reply;
c->u.a.pending = NULL;
c->u.a.outstanding_requests--;
if (!sentreply) {
/* Fake SSH_AGENT_FAILURE. */
@ -4362,8 +4373,9 @@ static int do_ssh1_login(Ssh ssh, const unsigned char *in, int inlen,
/* Request the keys held by the agent. */
PUT_32BIT(s->request, 1);
s->request[4] = SSH1_AGENTC_REQUEST_RSA_IDENTITIES;
if (!agent_query(s->request, 5, &r, &s->responselen,
ssh_agent_callback, ssh)) {
ssh->auth_agent_query = agent_query(
s->request, 5, &r, &s->responselen, ssh_agent_callback, ssh);
if (ssh->auth_agent_query) {
do {
crReturn(0);
if (pktin) {
@ -4468,8 +4480,10 @@ static int do_ssh1_login(Ssh ssh, const unsigned char *in, int inlen,
memcpy(q, s->session_id, 16);
q += 16;
PUT_32BIT(q, 1); /* response format */
if (!agent_query(agentreq, len + 4, &vret, &retlen,
ssh_agent_callback, ssh)) {
ssh->auth_agent_query = agent_query(
agentreq, len + 4, &vret, &retlen,
ssh_agent_callback, ssh);
if (ssh->auth_agent_query) {
sfree(agentreq);
do {
crReturn(0);
@ -5541,6 +5555,7 @@ static void ssh1_smsg_agent_open(Ssh ssh, struct Packet *pktin)
c->type = CHAN_AGENT; /* identify channel type */
c->u.a.lensofar = 0;
c->u.a.message = NULL;
c->u.a.pending = NULL;
c->u.a.outstanding_requests = 0;
send_packet(ssh, SSH1_MSG_CHANNEL_OPEN_CONFIRMATION,
PKT_INT, c->remoteid, PKT_INT, c->localid,
@ -5707,9 +5722,11 @@ static int ssh_agent_channel_data(struct ssh_channel *c, char *data,
void *reply;
int replylen;
c->u.a.outstanding_requests++;
if (agent_query(c->u.a.message, c->u.a.totallen, &reply, &replylen,
ssh_agentf_callback, c))
ssh_agentf_callback(c, reply, replylen);
c->u.a.pending = agent_query(
c->u.a.message, c->u.a.totallen, &reply, &replylen,
ssh_agentf_callback, c);
if (!c->u.a.pending)
ssh_agentf_callback(c, reply, replylen);
sfree(c->u.a.message);
c->u.a.message = NULL;
c->u.a.lensofar = 0;
@ -8141,6 +8158,8 @@ static void ssh_channel_close_local(struct ssh_channel *c, char const *reason)
msg = "Forwarded X11 connection terminated";
break;
case CHAN_AGENT:
if (c->u.a.pending)
agent_cancel_query(c->u.a.pending);
sfree(c->u.a.message);
break;
case CHAN_SOCKDATA:
@ -8788,6 +8807,7 @@ static void ssh2_msg_channel_open(Ssh ssh, struct Packet *pktin)
c->type = CHAN_AGENT; /* identify channel type */
c->u.a.lensofar = 0;
c->u.a.message = NULL;
c->u.a.pending = NULL;
c->u.a.outstanding_requests = 0;
}
} else {
@ -9291,8 +9311,10 @@ static void do_ssh2_authconn(Ssh ssh, const unsigned char *in, int inlen,
/* Request the keys held by the agent. */
PUT_32BIT(s->agent_request, 1);
s->agent_request[4] = SSH2_AGENTC_REQUEST_IDENTITIES;
if (!agent_query(s->agent_request, 5, &r, &s->agent_responselen,
ssh_agent_callback, ssh)) {
ssh->auth_agent_query = agent_query(
s->agent_request, 5, &r, &s->agent_responselen,
ssh_agent_callback, ssh);
if (ssh->auth_agent_query) {
do {
crReturnV;
if (pktin) {
@ -9730,9 +9752,10 @@ static void do_ssh2_authconn(Ssh ssh, const unsigned char *in, int inlen,
s->q += s->pktout->length - 5;
/* And finally the (zero) flags word. */
PUT_32BIT(s->q, 0);
if (!agent_query(s->agentreq, s->len + 4,
&vret, &s->retlen,
ssh_agent_callback, ssh)) {
ssh->auth_agent_query = agent_query(
s->agentreq, s->len + 4, &vret, &s->retlen,
ssh_agent_callback, ssh);
if (ssh->auth_agent_query) {
do {
crReturnV;
if (pktin) {
@ -11168,6 +11191,8 @@ static const char *ssh_init(void *frontend_handle, void **backend_handle,
CONF_ssh_rekey_data));
ssh->kex_in_progress = FALSE;
ssh->auth_agent_query = NULL;
#ifndef NO_GSSAPI
ssh->gsslibs = NULL;
#endif
@ -11283,6 +11308,10 @@ static void ssh_free(void *handle)
bufchain_clear(&ssh->queued_incoming_data);
sfree(ssh->username);
conf_free(ssh->conf);
if (ssh->auth_agent_query)
agent_cancel_query(ssh->auth_agent_query);
#ifndef NO_GSSAPI
if (ssh->gsslibs)
ssh_gss_cleanup(ssh->gsslibs);

50
unix/uxagentc.c
View File

@ -23,8 +23,8 @@ int agent_exists(void)
return FALSE;
}
static tree234 *agent_connections;
struct agent_connection {
static tree234 *agent_pending_queries;
struct agent_pending_query {
int fd;
char *retbuf;
char sizebuf[4];
@ -34,8 +34,8 @@ struct agent_connection {
};
static int agent_conncmp(void *av, void *bv)
{
struct agent_connection *a = (struct agent_connection *) av;
struct agent_connection *b = (struct agent_connection *) bv;
agent_pending_query *a = (agent_pending_query *) av;
agent_pending_query *b = (agent_pending_query *) bv;
if (a->fd < b->fd)
return -1;
if (a->fd > b->fd)
@ -45,7 +45,7 @@ static int agent_conncmp(void *av, void *bv)
static int agent_connfind(void *av, void *bv)
{
int afd = *(int *) av;
struct agent_connection *b = (struct agent_connection *) bv;
agent_pending_query *b = (agent_pending_query *) bv;
if (afd < b->fd)
return -1;
if (afd > b->fd)
@ -59,7 +59,7 @@ static int agent_connfind(void *av, void *bv)
* (conn->retbuf non-NULL and filled with something useful) or has
* failed totally (conn->retbuf is NULL).
*/
static int agent_try_read(struct agent_connection *conn)
static int agent_try_read(agent_pending_query *conn)
{
int ret;
@ -90,13 +90,21 @@ static int agent_try_read(struct agent_connection *conn)
return 1;
}
void agent_cancel_query(agent_pending_query *conn)
{
uxsel_del(conn->fd);
close(conn->fd);
del234(agent_pending_queries, conn);
sfree(conn);
}
static int agent_select_result(int fd, int event)
{
struct agent_connection *conn;
agent_pending_query *conn;
assert(event == 1); /* not selecting for anything but R */
conn = find234(agent_connections, &fd, agent_connfind);
conn = find234(agent_pending_queries, &fd, agent_connfind);
if (!conn) {
uxsel_del(fd);
return 1;
@ -111,21 +119,19 @@ static int agent_select_result(int fd, int event)
* of that passes to the callback.)
*/
conn->callback(conn->callback_ctx, conn->retbuf, conn->retlen);
uxsel_del(fd);
close(fd);
del234(agent_connections, conn);
sfree(conn);
agent_cancel_query(conn);
return 0;
}
int agent_query(void *in, int inlen, void **out, int *outlen,
void (*callback)(void *, void *, int), void *callback_ctx)
agent_pending_query *agent_query(
void *in, int inlen, void **out, int *outlen,
void (*callback)(void *, void *, int), void *callback_ctx)
{
char *name;
int sock;
struct sockaddr_un addr;
int done;
struct agent_connection *conn;
agent_pending_query *conn;
name = getenv("SSH_AUTH_SOCK");
if (!name)
@ -155,7 +161,7 @@ int agent_query(void *in, int inlen, void **out, int *outlen,
done += ret;
}
conn = snew(struct agent_connection);
conn = snew(agent_pending_query);
conn->fd = sock;
conn->retbuf = conn->sizebuf;
conn->retsize = 4;
@ -179,7 +185,7 @@ int agent_query(void *in, int inlen, void **out, int *outlen,
*out = conn->retbuf;
*outlen = conn->retlen;
sfree(conn);
return 1;
return NULL;
}
/*
@ -188,15 +194,15 @@ int agent_query(void *in, int inlen, void **out, int *outlen,
* response hasn't been received yet, and call the callback when
* select_result comes back to us.
*/
if (!agent_connections)
agent_connections = newtree234(agent_conncmp);
add234(agent_connections, conn);
if (!agent_pending_queries)
agent_pending_queries = newtree234(agent_conncmp);
add234(agent_pending_queries, conn);
uxsel_set(sock, 1, agent_select_result);
return 0;
return conn;
failure:
*out = NULL;
*outlen = 0;
return 1;
return NULL;
}

16
windows/winpgntc.c
View File

@ -24,8 +24,14 @@ int agent_exists(void)
return TRUE;
}
int agent_query(void *in, int inlen, void **out, int *outlen,
void (*callback)(void *, void *, int), void *callback_ctx)
void agent_cancel_query(agent_pending_query *q)
{
assert(0 && "Windows agent queries are never asynchronous!");
}
agent_pending_query *agent_query(
void *in, int inlen, void **out, int *outlen,
void (*callback)(void *, void *, int), void *callback_ctx)
{
HWND hwnd;
char *mapname;
@ -42,7 +48,7 @@ int agent_query(void *in, int inlen, void **out, int *outlen,
hwnd = FindWindow("Pageant", "Pageant");
if (!hwnd)
return 1; /* *out == NULL, so failure */
return NULL; /* *out == NULL, so failure */
mapname = dupprintf("PageantRequest%08x", (unsigned)GetCurrentThreadId());
psa = NULL;
@ -83,7 +89,7 @@ int agent_query(void *in, int inlen, void **out, int *outlen,
0, AGENT_MAX_MSGLEN, mapname);
if (filemap == NULL || filemap == INVALID_HANDLE_VALUE) {
sfree(mapname);
return 1; /* *out == NULL, so failure */
return NULL; /* *out == NULL, so failure */
}
p = MapViewOfFile(filemap, FILE_MAP_WRITE, 0, 0, 0);
memcpy(p, in, inlen);
@ -111,5 +117,5 @@ int agent_query(void *in, int inlen, void **out, int *outlen,
sfree(mapname);
if (psd)
LocalFree(psd);
return 1;
return NULL;
}