From a2b376af96072d6741caeb51b5f55fd05fd37157 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sat, 12 Mar 2022 15:02:12 +0000 Subject: [PATCH] Windows Pageant: turn 'has_security' into a global function. Now it can be called from places other than Pageant's WinMain(). In particular, the attempt to make a security descriptor in lock_interprocess_mutex() is gated on it. In return, however, I've tightened up the semantics. In normal PuTTY builds that aren't trying to support pre-NT systems, the function *unconditionally* returns true, on the grounds that we don't expect to target any system that doesn't support the security APIs, and if someone manages to contrive one anyway - or, more likely, if we some day introduce a bug in our loading of the security API functions - then this safety catch should make Pageant less likely to accidentally fall back to 'never mind, just run in insecure mode'. --- windows/pageant.c | 70 ++++++++++++++---------------- windows/platform.h | 1 + windows/utils/interprocess_mutex.c | 4 +- windows/utils/security.c | 14 ++++++ 4 files changed, 49 insertions(+), 40 deletions(-) diff --git a/windows/pageant.c b/windows/pageant.c index 0e3868a0..a08659e5 100644 --- a/windows/pageant.c +++ b/windows/pageant.c @@ -91,8 +91,6 @@ void modalfatalbox(const char *fmt, ...) exit(1); } -static bool has_security; - struct PassphraseProcStruct { bool modal; const char *help_topic; @@ -999,7 +997,7 @@ static char *answer_filemapping_message(const char *mapname) debug("maphandle = %p\n", maphandle); #endif - if (has_security) { + if (should_have_security()) { DWORD retd; if ((expectedsid = get_user_sid()) == NULL) { @@ -1405,14 +1403,7 @@ int WINAPI WinMain(HINSTANCE inst, HINSTANCE prev, LPSTR cmdline, int show) hinst = inst; - /* - * Determine whether we're an NT system (should have security - * APIs) or a non-NT system (don't do security). - */ - init_winver(); - has_security = (osPlatformId == VER_PLATFORM_WIN32_NT); - - if (has_security) { + if (should_have_security()) { /* * Attempt to get the security API we need. */ @@ -1543,39 +1534,42 @@ int WINAPI WinMain(HINSTANCE inst, HINSTANCE prev, LPSTR cmdline, int show) /* * Set up a named-pipe listener. */ - Plug *pl_plug; wpc->plc.vt = &winpgnt_vtable; wpc->plc.suppress_logging = true; - struct pageant_listen_state *pl = - pageant_listener_new(&pl_plug, &wpc->plc); - char *pipename = agent_named_pipe_name(); - Socket *sock = new_named_pipe_listener(pipename, pl_plug); - if (sk_socket_error(sock)) { - char *err = dupprintf("Unable to open named pipe at %s " - "for SSH agent:\n%s", pipename, - sk_socket_error(sock)); - MessageBox(NULL, err, "Pageant Error", MB_ICONERROR | MB_OK); - return 1; - } - pageant_listener_got_socket(pl, sock); - - /* - * If we've been asked to write out an OpenSSH config file - * pointing at the named pipe, do so. - */ - if (openssh_config_file) { - FILE *fp = fopen(openssh_config_file, "w"); - if (!fp) { - char *err = dupprintf("Unable to write OpenSSH config file " - "to %s", openssh_config_file); + if (should_have_security()) { + Plug *pl_plug; + struct pageant_listen_state *pl = + pageant_listener_new(&pl_plug, &wpc->plc); + char *pipename = agent_named_pipe_name(); + Socket *sock = new_named_pipe_listener(pipename, pl_plug); + if (sk_socket_error(sock)) { + char *err = dupprintf("Unable to open named pipe at %s " + "for SSH agent:\n%s", pipename, + sk_socket_error(sock)); MessageBox(NULL, err, "Pageant Error", MB_ICONERROR | MB_OK); return 1; } - fprintf(fp, "IdentityAgent %s\n", pipename); - fclose(fp); - } + pageant_listener_got_socket(pl, sock); - sfree(pipename); + /* + * If we've been asked to write out an OpenSSH config file + * pointing at the named pipe, do so. + */ + if (openssh_config_file) { + FILE *fp = fopen(openssh_config_file, "w"); + if (!fp) { + char *err = dupprintf("Unable to write OpenSSH config " + "file to %s", openssh_config_file); + MessageBox(NULL, err, "Pageant Error", + MB_ICONERROR | MB_OK); + return 1; + } + fprintf(fp, "IdentityAgent %s\n", pipename); + fclose(fp); + } + + sfree(pipename); + } /* * Set up the window class for the hidden window that receives diff --git a/windows/platform.h b/windows/platform.h index d15e1a68..5264e8f0 100644 --- a/windows/platform.h +++ b/windows/platform.h @@ -574,6 +574,7 @@ void dll_hijacking_protection(void); const char *get_system_dir(void); HMODULE load_system32_dll(const char *libname); const char *win_strerror(int error); +bool should_have_security(void); void restrict_process_acl(void); bool restricted_acl(void); void escape_registry_key(const char *in, strbuf *out); diff --git a/windows/utils/interprocess_mutex.c b/windows/utils/interprocess_mutex.c index a22ec820..8612a298 100644 --- a/windows/utils/interprocess_mutex.c +++ b/windows/utils/interprocess_mutex.c @@ -13,8 +13,8 @@ HANDLE lock_interprocess_mutex(const char *mutexname, char **error) PACL acl = NULL; HANDLE mutex = NULL; - if (!make_private_security_descriptor(MUTEX_ALL_ACCESS, - &psd, &acl, error)) + if (should_have_security() && !make_private_security_descriptor( + MUTEX_ALL_ACCESS, &psd, &acl, error)) goto out; SECURITY_ATTRIBUTES sa; diff --git a/windows/utils/security.c b/windows/utils/security.c index 2d899b83..1a4770b6 100644 --- a/windows/utils/security.c +++ b/windows/utils/security.c @@ -20,6 +20,20 @@ DEF_WINDOWS_FUNCTION(GetSecurityInfo); DEF_WINDOWS_FUNCTION(SetSecurityInfo); DEF_WINDOWS_FUNCTION(SetEntriesInAclA); +bool should_have_security(void) +{ +#ifdef LEGACY_WINDOWS + /* Legacy pre-NT platforms are not expected to have any of these APIs */ + init_winver(); + return (osPlatformId == VER_PLATFORM_WIN32_NT); +#else + /* In the up-to-date PuTTY builds which do not support those + * platforms, unconditionally return true, to minimise the risk of + * compiling out security checks. */ + return true; +#endif +} + bool got_advapi(void) { static bool attempted = false;