diff --git a/windows/wincapi.c b/windows/wincapi.c index 5ff7cdfa..0692f60b 100644 --- a/windows/wincapi.c +++ b/windows/wincapi.c @@ -6,6 +6,9 @@ #if !defined NO_SECURITY +#include "putty.h" +#include "ssh.h" + #define WINCAPI_GLOBAL #include "wincapi.h" @@ -31,4 +34,78 @@ bool got_crypt(void) return successful; } +#ifdef COVERITY +/* + * The hack I use to build for Coverity scanning, using winegcc and + * Makefile.mgw, didn't provide some defines in wincrypt.h last time I + * looked. Therefore, define them myself here, but enclosed in #ifdef + * COVERITY to ensure I don't make up random nonsense values for any + * real build. + */ +#ifndef CRYPTPROTECTMEMORY_BLOCK_SIZE +#define CRYPTPROTECTMEMORY_BLOCK_SIZE 16 +#endif +#ifndef CRYPTPROTECTMEMORY_CROSS_PROCESS +#define CRYPTPROTECTMEMORY_CROSS_PROCESS 1 +#endif +#endif + +char *capi_obfuscate_string(const char *realname) +{ + char *cryptdata; + int cryptlen; + unsigned char digest[32]; + char retbuf[65]; + int i; + + cryptlen = strlen(realname) + 1; + cryptlen += CRYPTPROTECTMEMORY_BLOCK_SIZE - 1; + cryptlen /= CRYPTPROTECTMEMORY_BLOCK_SIZE; + cryptlen *= CRYPTPROTECTMEMORY_BLOCK_SIZE; + + cryptdata = snewn(cryptlen, char); + memset(cryptdata, 0, cryptlen); + strcpy(cryptdata, realname); + + /* + * CRYPTPROTECTMEMORY_CROSS_PROCESS causes CryptProtectMemory to + * use the same key in all processes with this user id, meaning + * that the next PuTTY process calling this function with the same + * input will get the same data. + * + * (Contrast with CryptProtectData, which invents a new session + * key every time since its API permits returning more data than + * was input, so calling _that_ and hashing the output would not + * be stable.) + * + * We don't worry too much if this doesn't work for some reason. + * Omitting this step still has _some_ privacy value (in that + * another user can test-hash things to confirm guesses as to + * where you might be connecting to, but cannot invert SHA-256 in + * the absence of any plausible guess). So we don't abort if we + * can't call CryptProtectMemory at all, or if it fails. + */ + if (got_crypt()) + p_CryptProtectMemory(cryptdata, cryptlen, + CRYPTPROTECTMEMORY_CROSS_PROCESS); + + /* + * We don't want to give away the length of the hostname either, + * so having got it back out of CryptProtectMemory we now hash it. + */ + hash_simple(&ssh_sha256, make_ptrlen(cryptdata, cryptlen), digest); + + sfree(cryptdata); + + /* + * Finally, make printable. + */ + for (i = 0; i < 32; i++) { + sprintf(retbuf + 2*i, "%02x", digest[i]); + /* the last of those will also write the trailing NUL */ + } + + return dupstr(retbuf); +} + #endif /* !defined NO_SECURITY */ diff --git a/windows/wincapi.h b/windows/wincapi.h index a4958d28..3d1d31ca 100644 --- a/windows/wincapi.h +++ b/windows/wincapi.h @@ -1,7 +1,8 @@ /* - * wincapi.h: Windows Crypto API functions defined in wincrypt.c - * that use the crypt32 library. Also centralises the machinery - * for dynamically loading that library. + * wincapi.h: Windows Crypto API functions defined in wincapi.c that + * use the crypt32 library. Also centralises the machinery for + * dynamically loading that library, and our own functions using that + * in turn. */ #if !defined NO_SECURITY @@ -15,4 +16,21 @@ DECL_WINDOWS_FUNCTION(WINCAPI_GLOBAL, BOOL, CryptProtectMemory, bool got_crypt(void); +/* + * Function to obfuscate an input string into something usable as a + * pathname for a Windows named pipe. Uses CryptProtectMemory to make + * the obfuscation depend on a key Windows stores for the owning user, + * and then hashes the string as well to make it have a manageable + * length and be composed of filename-legal characters. + * + * Rationale: Windows's named pipes all live in the same namespace, so + * one user can see what pipes another user has open. This is an + * undesirable privacy leak: in particular, if we used unobfuscated + * names for the connection-sharing pipe names, it would permit one + * user to know what username@host another user is SSHing to. + * + * The returned string is dynamically allocated. + */ +char *capi_obfuscate_string(const char *realname); + #endif diff --git a/windows/winshare.c b/windows/winshare.c index 21edef09..f0e409ac 100644 --- a/windows/winshare.c +++ b/windows/winshare.c @@ -16,91 +16,9 @@ #include "wincapi.h" #include "winsecur.h" -#ifdef COVERITY -/* - * The hack I use to build for Coverity scanning, using winegcc and - * Makefile.mgw, didn't provide some defines in wincrypt.h last time I - * looked. Therefore, define them myself here, but enclosed in #ifdef - * COVERITY to ensure I don't make up random nonsense values for any - * real build. - */ -#ifndef CRYPTPROTECTMEMORY_BLOCK_SIZE -#define CRYPTPROTECTMEMORY_BLOCK_SIZE 16 -#endif -#ifndef CRYPTPROTECTMEMORY_CROSS_PROCESS -#define CRYPTPROTECTMEMORY_CROSS_PROCESS 1 -#endif -#endif - #define CONNSHARE_PIPE_PREFIX "\\\\.\\pipe\\putty-connshare" #define CONNSHARE_MUTEX_PREFIX "Local\\putty-connshare-mutex" -static char *obfuscate_name(const char *realname) -{ - /* - * Windows's named pipes all live in the same namespace, so one - * user can see what pipes another user has open. This is an - * undesirable privacy leak and in particular permits one user to - * know what username@host another user is SSHing to, so we - * protect that information by using CryptProtectMemory (which - * uses a key built in to each user's account). - */ - char *cryptdata; - int cryptlen; - unsigned char digest[32]; - char retbuf[65]; - int i; - - cryptlen = strlen(realname) + 1; - cryptlen += CRYPTPROTECTMEMORY_BLOCK_SIZE - 1; - cryptlen /= CRYPTPROTECTMEMORY_BLOCK_SIZE; - cryptlen *= CRYPTPROTECTMEMORY_BLOCK_SIZE; - - cryptdata = snewn(cryptlen, char); - memset(cryptdata, 0, cryptlen); - strcpy(cryptdata, realname); - - /* - * CRYPTPROTECTMEMORY_CROSS_PROCESS causes CryptProtectMemory to - * use the same key in all processes with this user id, meaning - * that the next PuTTY process calling this function with the same - * input will get the same data. - * - * (Contrast with CryptProtectData, which invents a new session - * key every time since its API permits returning more data than - * was input, so calling _that_ and hashing the output would not - * be stable.) - * - * We don't worry too much if this doesn't work for some reason. - * Omitting this step still has _some_ privacy value (in that - * another user can test-hash things to confirm guesses as to - * where you might be connecting to, but cannot invert SHA-256 in - * the absence of any plausible guess). So we don't abort if we - * can't call CryptProtectMemory at all, or if it fails. - */ - if (got_crypt()) - p_CryptProtectMemory(cryptdata, cryptlen, - CRYPTPROTECTMEMORY_CROSS_PROCESS); - - /* - * We don't want to give away the length of the hostname either, - * so having got it back out of CryptProtectMemory we now hash it. - */ - hash_simple(&ssh_sha256, make_ptrlen(cryptdata, cryptlen), digest); - - sfree(cryptdata); - - /* - * Finally, make printable. - */ - for (i = 0; i < 32; i++) { - sprintf(retbuf + 2*i, "%02x", digest[i]); - /* the last of those will also write the trailing NUL */ - } - - return dupstr(retbuf); -} - static char *make_name(const char *prefix, const char *name) { char *username, *retname; @@ -130,7 +48,7 @@ int platform_ssh_share(const char *pi_name, Conf *conf, * that it also eliminates any characters illegal in Windows pipe * names. */ - name = obfuscate_name(pi_name); + name = capi_obfuscate_string(pi_name); if (!name) { *logtext = dupprintf("Unable to call CryptProtectMemory: %s", win_strerror(GetLastError()));