From 5acd523ae6c5b6a4be83b5131d53826606778335 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Tue, 29 May 2018 22:41:37 +0100 Subject: [PATCH] Rewrite .Xauthority parsing using BinarySource. This rewrite replaces a particularly hairy macro-based system. --- x11fwd.c | 103 ++++++++++++++++++++++++++++++++++--------------------- 1 file changed, 63 insertions(+), 40 deletions(-) diff --git a/x11fwd.c b/x11fwd.c index f59f9e2e..bd4c9289 100644 --- a/x11fwd.c +++ b/x11fwd.c @@ -429,15 +429,28 @@ static const char *x11_verify(unsigned long peer_ip, int peer_port, return NULL; } +ptrlen BinarySource_get_string_xauth(BinarySource *src) +{ + size_t len = get_uint16(src); + return get_data(src, len); +} +#define get_string_xauth(src) \ + BinarySource_get_string_xauth(BinarySource_UPCAST(src)) + void x11_get_auth_from_authfile(struct X11Display *disp, const char *authfilename) { FILE *authfp; - char *buf, *ptr, *str[4]; - int len[4]; + char *buf; + int size; + BinarySource src[1]; int family, protocol; + ptrlen addr, protoname, data; + char *displaynum_string; + int displaynum; int ideal_match = FALSE; char *ourhostname; + const size_t MAX_RECORD_SIZE = 0x80, BUF_SIZE = 2 * MAX_RECORD_SIZE; /* * Normally we should look for precisely the details specified in @@ -468,29 +481,41 @@ void x11_get_auth_from_authfile(struct X11Display *disp, ourhostname = get_hostname(); - /* Records in .Xauthority contain four strings of up to 64K each */ - buf = snewn(65537 * 4, char); + /* + * Allocate enough space to hold two maximally sized records, so + * that a full record can start anywhere in the first half. That + * way we avoid the accidentally-quadratic algorithm that would + * arise if we moved everything to the front of the buffer after + * consuming each record; instead, we only move everything to the + * front after our current position gets past the half-way mark. + * Before then, there's no need to move anyway; so this guarantees + * linear time, in that every byte written into this buffer moves + * at most once (because every move is from the second half of the + * buffer to the first half). + */ + buf = snewn(BUF_SIZE, char); + size = fread(buf, 1, BUF_SIZE, authfp); + BinarySource_BARE_INIT(src, buf, size); while (!ideal_match) { - int c, i, j, match = FALSE; - -#define GET do { c = fgetc(authfp); if (c == EOF) goto done; c = (unsigned char)c; } while (0) - /* Expect a big-endian 2-byte number giving address family */ - GET; family = c; - GET; family = (family << 8) | c; - /* Then expect four strings, each composed of a big-endian 2-byte - * length field followed by that many bytes of data */ - ptr = buf; - for (i = 0; i < 4; i++) { - GET; len[i] = c; - GET; len[i] = (len[i] << 8) | c; - str[i] = ptr; - for (j = 0; j < len[i]; j++) { - GET; *ptr++ = c; - } - *ptr++ = '\0'; - } -#undef GET + int match = FALSE; + + if (src->pos >= MAX_RECORD_SIZE) { + size -= src->pos; + memcpy(buf, buf + src->pos, size); + size += fread(buf + size, 1, BUF_SIZE - size, authfp); + BinarySource_BARE_INIT(src, buf, size); + } + + family = get_uint16(src); + addr = get_string_xauth(src); + displaynum_string = mkstr(get_string_xauth(src)); + displaynum = atoi(displaynum_string); + sfree(displaynum_string); + protoname = get_string_xauth(src); + data = get_string_xauth(src); + if (get_err(src)) + break; /* * Now we have a full X authority record in memory. See @@ -504,7 +529,7 @@ void x11_get_auth_from_authfile(struct X11Display *disp, * connect to the display. 0 means IPv4; 6 means IPv6; * 256 means Unix-domain sockets. * - * - str[0] is the network address itself. For IPv4 and + * - 'addr' is the network address itself. For IPv4 and * IPv6, this is a string of binary data of the * appropriate length (respectively 4 and 16 bytes) * representing the address in big-endian format, e.g. @@ -515,24 +540,23 @@ void x11_get_auth_from_authfile(struct X11Display *disp, * authority entries for Unix-domain displays on * several machines without them clashing). * - * - str[1] is the display number. I've no idea why + * - 'displaynum' is the display number. I've no idea why * .Xauthority stores this as a string when it has a * perfectly good integer format, but there we go. * - * - str[2] is the authorisation method, encoded as its - * canonical string name (i.e. "MIT-MAGIC-COOKIE-1", - * "XDM-AUTHORIZATION-1" or something we don't - * recognise). + * - 'protoname' is the authorisation protocol, encoded as + * its canonical string name (i.e. "MIT-MAGIC-COOKIE-1", + * "XDM-AUTHORIZATION-1" or something we don't recognise). * - * - str[3] is the actual authorisation data, stored in + * - 'data' is the actual authorisation data, stored in * binary form. */ - if (disp->displaynum < 0 || disp->displaynum != atoi(str[1])) + if (disp->displaynum < 0 || disp->displaynum != displaynum) continue; /* not the one */ for (protocol = 1; protocol < lenof(x11_authnames); protocol++) - if (!strcmp(str[2], x11_authnames[protocol])) + if (ptrlen_eq_string(protoname, x11_authnames[protocol])) break; if (protocol == lenof(x11_authnames)) continue; /* don't recognise this protocol, look for another */ @@ -543,7 +567,7 @@ void x11_get_auth_from_authfile(struct X11Display *disp, sk_addrtype(disp->addr) == ADDRTYPE_IPV4) { char buf[4]; sk_addrcopy(disp->addr, buf); - if (len[0] == 4 && !memcmp(str[0], buf, 4)) { + if (addr.len == 4 && !memcmp(addr.ptr, buf, 4)) { match = TRUE; /* If this is a "localhost" entry, note it down * but carry on looking for a Unix-domain entry. */ @@ -556,7 +580,7 @@ void x11_get_auth_from_authfile(struct X11Display *disp, sk_addrtype(disp->addr) == ADDRTYPE_IPV6) { char buf[16]; sk_addrcopy(disp->addr, buf); - if (len[0] == 16 && !memcmp(str[0], buf, 16)) { + if (addr.len == 16 && !memcmp(addr.ptr, buf, 16)) { match = TRUE; ideal_match = !localhost; } @@ -564,7 +588,7 @@ void x11_get_auth_from_authfile(struct X11Display *disp, break; case 256: /* Unix-domain / localhost */ if ((disp->unixdomain || localhost) - && ourhostname && !strcmp(ourhostname, str[0])) + && ourhostname && ptrlen_eq_string(addr, ourhostname)) /* A matching Unix-domain socket is always the best * match. */ match = ideal_match = TRUE; @@ -575,15 +599,14 @@ void x11_get_auth_from_authfile(struct X11Display *disp, /* Current best guess -- may be overridden if !ideal_match */ disp->localauthproto = protocol; sfree(disp->localauthdata); /* free previous guess, if any */ - disp->localauthdata = snewn(len[3], unsigned char); - memcpy(disp->localauthdata, str[3], len[3]); - disp->localauthdatalen = len[3]; + disp->localauthdata = snewn(data.len, unsigned char); + memcpy(disp->localauthdata, data.ptr, data.len); + disp->localauthdatalen = data.len; } } - done: fclose(authfp); - smemclr(buf, 65537 * 4); + smemclr(buf, 2 * MAX_RECORD_SIZE); sfree(buf); sfree(ourhostname); }