From aba7234bc167c8c056a9ea4f939a6dcda10e84f3 Mon Sep 17 00:00:00 2001 From: Owen Dunn Date: Sun, 22 Nov 2015 12:04:04 +0000 Subject: [PATCH 1/5] Move SID-getting code into a separate function so it can be shared by make_private_security_descriptor and a new function protectprocess(). protectprocess() opens the running PuTTY process and adjusts the Everyone and user access control entries in its ACL to deny a selection of permissions which malicious processes running as the same user could use to hijack PuTTY. --- windows/winsecur.c | 155 +++++++++++++++++++++++++++++++++++---------- 1 file changed, 122 insertions(+), 33 deletions(-) diff --git a/windows/winsecur.c b/windows/winsecur.c index df93a74c..6e4bd7d4 100644 --- a/windows/winsecur.c +++ b/windows/winsecur.c @@ -12,6 +12,10 @@ #define WINSECUR_GLOBAL #include "winsecur.h" +/* Initialised once, then kept around to reuse forever */ +static PSID worldsid, networksid, usersid; + + int got_advapi(void) { static int attempted = FALSE; @@ -99,6 +103,52 @@ PSID get_user_sid(void) return ret; } +int getsids(char *error) +{ + SID_IDENTIFIER_AUTHORITY world_auth = SECURITY_WORLD_SID_AUTHORITY; + SID_IDENTIFIER_AUTHORITY nt_auth = SECURITY_NT_AUTHORITY; + int ret; + + error=NULL; + + if (!usersid) { + if ((usersid = get_user_sid()) == NULL) { + error = dupprintf("unable to construct SID for current user: %s", + win_strerror(GetLastError())); + goto cleanup; + } + } + + if (!worldsid) { + if (!AllocateAndInitializeSid(&world_auth, 1, SECURITY_WORLD_RID, + 0, 0, 0, 0, 0, 0, 0, &worldsid)) { + error = dupprintf("unable to construct SID for world: %s", + win_strerror(GetLastError())); + goto cleanup; + } + } + + if (!networksid) { + if (!AllocateAndInitializeSid(&nt_auth, 1, SECURITY_NETWORK_RID, + 0, 0, 0, 0, 0, 0, 0, &networksid)) { + error = dupprintf("unable to construct SID for " + "local same-user access only: %s", + win_strerror(GetLastError())); + goto cleanup; + } + } + + ret=TRUE; + + cleanup: + if (ret) { + sfree(error); + error = NULL; + } + return ret; +} + + int make_private_security_descriptor(DWORD permissions, PSECURITY_DESCRIPTOR *psd, PACL *acl, @@ -110,44 +160,13 @@ int make_private_security_descriptor(DWORD permissions, int acl_err; int ret = FALSE; - /* Initialised once, then kept around to reuse forever */ - static PSID worldsid, networksid, usersid; *psd = NULL; *acl = NULL; *error = NULL; - if (!got_advapi()) { - *error = dupprintf("unable to load advapi32.dll"); - goto cleanup; - } - - if (!usersid) { - if ((usersid = get_user_sid()) == NULL) { - *error = dupprintf("unable to construct SID for current user: %s", - win_strerror(GetLastError())); - goto cleanup; - } - } - - if (!worldsid) { - if (!AllocateAndInitializeSid(&world_auth, 1, SECURITY_WORLD_RID, - 0, 0, 0, 0, 0, 0, 0, &worldsid)) { - *error = dupprintf("unable to construct SID for world: %s", - win_strerror(GetLastError())); - goto cleanup; - } - } - - if (!networksid) { - if (!AllocateAndInitializeSid(&nt_auth, 1, SECURITY_NETWORK_RID, - 0, 0, 0, 0, 0, 0, 0, &networksid)) { - *error = dupprintf("unable to construct SID for " - "local same-user access only: %s", - win_strerror(GetLastError())); - goto cleanup; - } - } + if (!getsids(*error)) + goto cleanup; memset(ea, 0, sizeof(ea)); ea[0].grfAccessPermissions = permissions; @@ -218,4 +237,74 @@ int make_private_security_descriptor(DWORD permissions, return ret; } +int protectprocess(char *error) +{ + SID_IDENTIFIER_AUTHORITY world_auth = SECURITY_WORLD_SID_AUTHORITY; + SID_IDENTIFIER_AUTHORITY nt_auth = SECURITY_NT_AUTHORITY; + EXPLICIT_ACCESS ea[2]; + int acl_err; + int ret=FALSE; + PACL acl = NULL; + + static const nastyace=WRITE_DAC | WRITE_OWNER | + PROCESS_CREATE_PROCESS | PROCESS_CREATE_THREAD | + PROCESS_DUP_HANDLE | PROCESS_QUERY_INFORMATION | + PROCESS_SET_QUOTA | PROCESS_SET_INFORMATION | + PROCESS_VM_OPERATION | PROCESS_VM_READ | PROCESS_VM_WRITE | + PROCESS_SUSPEND_RESUME; + + if (!getsids(error)) + goto cleanup; + + memset(ea, 0, sizeof(ea)); + + /* Everyone: deny */ + ea[0].grfAccessPermissions = nastyace; + ea[0].grfAccessMode = DENY_ACCESS; + ea[0].grfInheritance = SUB_CONTAINERS_AND_OBJECTS_INHERIT; + ea[0].Trustee.TrusteeForm = TRUSTEE_IS_SID; + ea[0].Trustee.ptstrName = (LPTSTR)worldsid; + + /* User: user ace */ + ea[1].grfAccessPermissions = ~nastyace & 0x1fff; + ea[1].grfAccessMode = GRANT_ACCESS; + ea[1].grfInheritance = SUB_CONTAINERS_AND_OBJECTS_INHERIT; + ea[1].Trustee.TrusteeForm = TRUSTEE_IS_SID; + ea[1].Trustee.ptstrName = (LPTSTR)usersid; + + acl_err = p_SetEntriesInAclA(2, ea, NULL, &acl); + + if (acl_err != ERROR_SUCCESS || acl == NULL) { + error = dupprintf("unable to construct ACL: %s", + win_strerror(acl_err)); + goto cleanup; + } + + if (ERROR_SUCCESS != + SetSecurityInfo( + GetCurrentProcess(), + SE_KERNEL_OBJECT, + OWNER_SECURITY_INFORMATION | DACL_SECURITY_INFORMATION, + usersid, + NULL, + acl, + NULL + )) { + error=dupprintf("Unable to set process ACL: %s", + win_strerror(GetLastError())); + goto cleanup; + } + + + ret=TRUE; + + cleanup: + if (!ret) { + if (acl) { + LocalFree(acl); + acl = NULL; + } + } + return ret; +} #endif /* !defined NO_SECURITY */ From 48db456801cf90369330248075b7e480252696ff Mon Sep 17 00:00:00 2001 From: Owen Dunn Date: Tue, 24 Nov 2015 22:02:24 +0000 Subject: [PATCH 2/5] Make our process's ACL more restrictive. By default Windows processes have wide open ACLs which allow interference by other processes running as the same user. Adjust our ACL to make this a bit harder. Because it's useful to protect PuTTYtel as well, carve winsecur.c into advapi functions and wincapi.c for crypt32 functions. --- Recipe | 4 ++-- windows/wincapi.c | 27 +++++++++++++++++++++++++++ windows/wincapi.h | 18 ++++++++++++++++++ windows/window.c | 15 +++++++++++++++ windows/winsecur.c | 17 +---------------- windows/winsecur.h | 9 ++------- windows/winshare.c | 2 +- 7 files changed, 66 insertions(+), 26 deletions(-) create mode 100644 windows/wincapi.c create mode 100644 windows/wincapi.h diff --git a/Recipe b/Recipe index ebc9908f..67435c68 100644 --- a/Recipe +++ b/Recipe @@ -224,7 +224,7 @@ 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 -WINSSH = SSH winnoise winsecur winpgntc wingss winshare winnps winnpc +WINSSH = SSH winnoise wincapi winpgntc wingss winshare winnps winnpc + winhsock errsock UXSSH = SSH uxnoise uxagentc uxgss uxshare @@ -235,7 +235,7 @@ SFTP = sftp int64 logging # Pageant or PuTTYgen). MISC = timing callback misc version settings tree234 proxy conf WINMISC = MISC winstore winnet winhandl cmdline windefs winmisc winproxy - + wintime winhsock errsock + + wintime winhsock errsock winsecur UXMISC = MISC uxstore uxsel uxnet uxpeer cmdline uxmisc uxproxy time OSXMISC = MISC uxstore uxsel osxsel uxnet uxpeer uxmisc uxproxy time diff --git a/windows/wincapi.c b/windows/wincapi.c new file mode 100644 index 00000000..2550b6de --- /dev/null +++ b/windows/wincapi.c @@ -0,0 +1,27 @@ +/* + * wincapi.c: implementation of wincapi.h. + */ + +#include "putty.h" + +#if !defined NO_SECURITY + +#define WINCAPI_GLOBAL +#include "wincapi.h" + +int got_crypt(void) +{ + static int attempted = FALSE; + static int successful; + static HMODULE crypt; + + if (!attempted) { + attempted = TRUE; + crypt = load_system32_dll("crypt32.dll"); + successful = crypt && + GET_WINDOWS_FUNCTION(crypt, CryptProtectMemory); + } + return successful; +} + +#endif /* !defined NO_SECURITY */ diff --git a/windows/wincapi.h b/windows/wincapi.h new file mode 100644 index 00000000..06ee2d36 --- /dev/null +++ b/windows/wincapi.h @@ -0,0 +1,18 @@ +/* + * wincapi.h: Windows Crypto API functions defined in wincrypt.c + * that use the crypt32 library. Also centralises the machinery + * for dynamically loading that library. + */ + +#if !defined NO_SECURITY + +#ifndef WINCAPI_GLOBAL +#define WINCAPI_GLOBAL extern +#endif + +DECL_WINDOWS_FUNCTION(WINCAPI_GLOBAL, BOOL, CryptProtectMemory, + (LPVOID,DWORD,DWORD)); + +int got_crypt(void); + +#endif diff --git a/windows/window.c b/windows/window.c index 23f98a47..db426347 100644 --- a/windows/window.c +++ b/windows/window.c @@ -19,6 +19,7 @@ #include "terminal.h" #include "storage.h" #include "win_res.h" +#include "winsecur.h" #ifndef NO_MULTIMON #include @@ -390,6 +391,20 @@ int WINAPI WinMain(HINSTANCE inst, HINSTANCE prev, LPSTR cmdline, int show) return 1; } + /* + * Protect our process + */ + { + char *error = NULL; + + if (! setprocessacl(error)) { + /* FIXME: prepare to stuff this into event log somehow */ + MessageBox(NULL, "Process protection", + error, MB_OK | MB_ICONEXCLAMATION); + } + sfree(error); + + } /* * Process the command line. */ diff --git a/windows/winsecur.c b/windows/winsecur.c index 6e4bd7d4..9cdac26c 100644 --- a/windows/winsecur.c +++ b/windows/winsecur.c @@ -36,21 +36,6 @@ int got_advapi(void) return successful; } -int got_crypt(void) -{ - static int attempted = FALSE; - static int successful; - static HMODULE crypt; - - if (!attempted) { - attempted = TRUE; - crypt = load_system32_dll("crypt32.dll"); - successful = crypt && - GET_WINDOWS_FUNCTION(crypt, CryptProtectMemory); - } - return successful; -} - PSID get_user_sid(void) { HANDLE proc = NULL, tok = NULL; @@ -237,7 +222,7 @@ int make_private_security_descriptor(DWORD permissions, return ret; } -int protectprocess(char *error) +int setprocessacl(char *error) { SID_IDENTIFIER_AUTHORITY world_auth = SECURITY_WORLD_SID_AUTHORITY; SID_IDENTIFIER_AUTHORITY nt_auth = SECURITY_NT_AUTHORITY; diff --git a/windows/winsecur.h b/windows/winsecur.h index bd649827..03e8314d 100644 --- a/windows/winsecur.h +++ b/windows/winsecur.h @@ -32,13 +32,6 @@ DECL_WINDOWS_FUNCTION(WINSECUR_GLOBAL, DWORD, SetEntriesInAclA, (ULONG, PEXPLICIT_ACCESS, PACL, PACL *)); int got_advapi(void); -/* - * Functions loaded from crypt32.dll. - */ -DECL_WINDOWS_FUNCTION(WINSECUR_GLOBAL, BOOL, CryptProtectMemory, - (LPVOID, DWORD, DWORD)); -int got_crypt(void); - /* * Find the SID describing the current user. The return value (if not * NULL for some error-related reason) is smalloced. @@ -60,4 +53,6 @@ int make_private_security_descriptor(DWORD permissions, PACL *acl, char **error); +int setprocessacl(char *error); + #endif diff --git a/windows/winshare.c b/windows/winshare.c index 2f21638e..5f1c7244 100644 --- a/windows/winshare.c +++ b/windows/winshare.c @@ -14,7 +14,7 @@ #include "proxy.h" #include "ssh.h" -#include "winsecur.h" +#include "wincapi.h" #ifdef COVERITY /* From 0014ffb70c9ed234984595d6c9ca3b5c175996aa Mon Sep 17 00:00:00 2001 From: Owen Dunn Date: Tue, 24 Nov 2015 22:57:46 +0000 Subject: [PATCH 3/5] Enable DEP and ASLR flags on VC++ linker command line /dynamicbase and /nxcompat on the VC linker command line should enable DEP and ASLR according to this MSDN article. https://msdn.microsoft.com/en-us/library/bb430720.aspx --- mkfiles.pl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mkfiles.pl b/mkfiles.pl index 59ecfdc4..90e4f9dc 100755 --- a/mkfiles.pl +++ b/mkfiles.pl @@ -636,7 +636,7 @@ if (defined $makefiles{'vc'}) { "CFLAGS = /nologo /W3 /O1 " . (join " ", map {"-I$dirpfx$_"} @srcdirs) . " /D_WINDOWS /D_WIN32_WINDOWS=0x500 /DWINVER=0x500\n". - "LFLAGS = /incremental:no /fixed\n". + "LFLAGS = /incremental:no /dynamicbase /nxcompat\n". "RCFLAGS = ".(join " ", map {"-I$dirpfx$_"} @srcdirs). " -DWIN32 -D_WIN32 -DWINVER=0x0400\n". "\n". From 8b65fef55c688d8a52bd56f426e345671fab0303 Mon Sep 17 00:00:00 2001 From: Owen Dunn Date: Tue, 24 Nov 2015 23:12:33 +0000 Subject: [PATCH 4/5] Surround process protection with an #ifndef UNPROTECT --- windows/window.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/windows/window.c b/windows/window.c index db426347..2677145f 100644 --- a/windows/window.c +++ b/windows/window.c @@ -395,15 +395,14 @@ int WINAPI WinMain(HINSTANCE inst, HINSTANCE prev, LPSTR cmdline, int show) * Protect our process */ { - char *error = NULL; - - if (! setprocessacl(error)) { - /* FIXME: prepare to stuff this into event log somehow */ - MessageBox(NULL, "Process protection", - error, MB_OK | MB_ICONEXCLAMATION); - } - sfree(error); - +#ifndef UNPROTECT + char *error = NULL; + if (! setprocessacl(error)) { + logevent(NULL, "Could not restrict process ACL: %s", + error); + } + sfree(error); +#endif } /* * Process the command line. From 21a37d287cced473c12d23581fc1a200552ad1e0 Mon Sep 17 00:00:00 2001 From: Owen Dunn Date: Tue, 24 Nov 2015 23:13:03 +0000 Subject: [PATCH 5/5] Document UNPROTECT define that disables tightened ACL. --- Recipe | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Recipe b/Recipe index 67435c68..5fa7356d 100644 --- a/Recipe +++ b/Recipe @@ -114,6 +114,10 @@ # - XFLAGS=/DDEBUG # Causes PuTTY to enable internal debugging. # +# - XFLAGS=/DUNPROTECT +# Disable tightened ACL on PuTTY process so that e.g. debuggers +# can attach to it. +# # - XFLAGS=/DMALLOC_LOG # Causes PuTTY to emit a file called putty_mem.log, logging every # memory allocation and free, so you can track memory leaks.