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

Improve error reporting from x11_verify().

Browse Source 
Now the return value is a dynamically allocated string instead of a
static one, which means that the error message can include details
taken from the specific failing connection. In particular, if someone
requests an X11 authorisation protocol we don't support, we can print
its name as part of the message, which may help users debug the
problem.

One particularly important special case of this is that if the client
connection presents _no_ authorisation - which is surely by far the
most likely thing to happen by accident, e.g. if the auth file has
gone missing, or the hostname doesn't match for some reason - then we
now give a specific message "No authorisation provided", which I think
is considerably more helpful than just lumping that very common case
in with "Unsupported authorisation protocol". Even changing the latter
to "Unsupported authorisation protocol ''" is still not very sensible.
The problem in that case is not that the user has tried an exotic auth
protocol we've never heard of - it's that they've forgotten, or
failed, to provide one at all.

The error message for "XDM-AUTHORIZATION-1 data was wrong length" is
the other modified one: it now says what the wrong length _was_.
However, all other failures of X-A-1 are still kept deliberately
vague, because saying which part of the decrypted string didn't match
is an obvious information leak.

(cherry picked from commit dff4bd4d14)
This commit is contained in:
Simon Tatham 2023-04-01 15:53:29 +01:00
parent 4d92ca80de
commit 3f5873f1fe
1 changed files with 42 additions and 23 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

65
ssh/x11fwd.c
View File

@ -168,10 +168,10 @@ int x11_authcmp(void *av, void *bv)
#define XDM_MAXSKEW 20*60 /* 20 minute clock skew should be OK */
static const char *x11_verify(unsigned long peer_ip, int peer_port,
tree234 *authtree, char *proto,
unsigned char *data, int dlen,
struct X11FakeAuth **auth_ret)
static char *x11_verify(unsigned long peer_ip, int peer_port,
tree234 *authtree, char *proto,
unsigned char *data, int dlen,
struct X11FakeAuth **auth_ret)
{
struct X11FakeAuth match_dummy; /* for passing to find234 */
struct X11FakeAuth *auth;
@ -197,12 +197,21 @@ static const char *x11_verify(unsigned long peer_ip, int peer_port,
*/
match_dummy.proto = X11_XDM;
match_dummy.xa1_firstblock = data;
} else if (!proto[0]) {
/*
* If the user has attempted to connect to the forwarded X
* display with no authority at all, we can give a better
* error message than the generic "unsupported protocol". We
* at least _recognise_ the null auth protocol, even if we
* don't _accept_ it.
*/
return dupstr("No authorisation provided");
} else {
return "Unsupported authorisation protocol";
return dupprintf("Unsupported authorisation protocol '%s'", proto);
}
if ((auth = find234(authtree, &match_dummy, 0)) == NULL)
return "Authorisation not recognised";
return dupstr("Authorisation not recognised");
/*
* If we're using MIT-MAGIC-COOKIE-1, that was all we needed. If
@ -216,24 +225,32 @@ static const char *x11_verify(unsigned long peer_ip, int peer_port,
struct XDMSeen *seen, *ret;
if (dlen != 24)
return "XDM-AUTHORIZATION-1 data was wrong length";
return dupprintf("XDM-AUTHORIZATION-1 data was wrong length "
"(%d, expected 24)", dlen);
if (peer_port == -1)
return "cannot do XDM-AUTHORIZATION-1 without remote address data";
return dupstr("cannot do XDM-AUTHORIZATION-1 without remote "
"address data");
des_decrypt_xdmauth(auth->data+9, data, 24);
if (memcmp(auth->data, data, 8) != 0)
return "XDM-AUTHORIZATION-1 data failed check"; /* cookie wrong */
if (GET_32BIT_MSB_FIRST(data+8) != peer_ip)
return "XDM-AUTHORIZATION-1 data failed check"; /* IP wrong */
if ((int)GET_16BIT_MSB_FIRST(data+12) != peer_port)
return "XDM-AUTHORIZATION-1 data failed check"; /* port wrong */
if (memcmp(auth->data, data, 8) != 0) {
/* cookie wrong */
return dupstr("XDM-AUTHORIZATION-1 data failed check");
}
if (GET_32BIT_MSB_FIRST(data+8) != peer_ip) {
/* IP wrong */
return dupstr("XDM-AUTHORIZATION-1 data failed check");
}
if ((int)GET_16BIT_MSB_FIRST(data+12) != peer_port) {
/* port wrong */
return dupstr("XDM-AUTHORIZATION-1 data failed check");
}
t = GET_32BIT_MSB_FIRST(data+14);
for (i = 18; i < 24; i++)
if (data[i] != 0) /* zero padding wrong */
return "XDM-AUTHORIZATION-1 data failed check";
if (data[i] != 0) /* zero padding wrong */
return dupstr("XDM-AUTHORIZATION-1 data failed check");
tim = time(NULL);
if (((unsigned long)t - (unsigned long)tim
+ XDM_MAXSKEW) > 2*XDM_MAXSKEW)
return "XDM-AUTHORIZATION-1 time stamp was too far out";
return dupstr("XDM-AUTHORIZATION-1 time stamp was too far out");
seen = snew(struct XDMSeen);
seen->time = t;
memcpy(seen->clientid, data+8, 6);
@ -241,7 +258,7 @@ static const char *x11_verify(unsigned long peer_ip, int peer_port,
ret = add234(auth->xdmseen, seen);
if (ret != seen) {
sfree(seen);
return "XDM-AUTHORIZATION-1 data replayed";
return dupstr("XDM-AUTHORIZATION-1 data replayed");
}
/* While we're here, purge entries too old to be replayed. */
for (;;) {
@ -504,6 +521,7 @@ static size_t x11_send(
*/
if (!xconn->verified) {
const char *err;
char *errmut;
struct X11FakeAuth *auth_matched = NULL;
unsigned long peer_ip;
int peer_port;
@ -529,11 +547,12 @@ static size_t x11_send(
else
peer_port = -1; /* signal no peer address data available */
err = x11_verify(peer_ip, peer_port,
xconn->authtree, xconn->auth_protocol,
xconn->auth_data, xconn->auth_dlen, &auth_matched);
if (err) {
x11_send_init_error(xconn, err);
errmut = x11_verify(peer_ip, peer_port,
xconn->authtree, xconn->auth_protocol,
xconn->auth_data, xconn->auth_dlen, &auth_matched);
if (errmut) {
x11_send_init_error(xconn, errmut);
sfree(errmut);
return 0;
}
assert(auth_matched);