2017-05-19 18:27 AEST by Arthur Barrett - We should think carefully about CVSNT and security of passwords.

To some extent - we already have a few enhancements suggested:
Bug 7038 - cvsagent authentication via mobile fingerprint authentication (securid)
Bug 6011 - cvsagent needs replacing with an alternative (SecurID)
Bug 6460 - cvsagent: should auto-start
Bug 5999 - windows client: 'cvs passwd', 'cvs login' warn on first use
Bug 6326 - Win: client: :sserver:should support smartcard FIPS 140 PIV
Bug 6327 - Win: client: :sspi:should support smartcard FIPS 140 PIV


The reason why was recently demonstrated by a failure in Git:
http://iphone.appleinsider.com/articles/17/05/17/source-code-of-several-panic-apps-stolen-via-handbrake-
malware
And
http://www.theregister.co.uk/2017/05/18/but_macs_dont_get_malware/


A developer got Malware on his PC, and his 'git credentials were stolen'.

So how does Git store credentials.  Well in plain text like pserver (which we know is not good, and so does 
Git) or in a password app (like cvsagent) which we think of as 'secure':
https://git-scm.com/docs/git-credential-cache


The problem is, that if the Git or CVS client can query the database, malicious software (or a hacker acting as 
a user) can also query it (maybe requiring a small program - but that's no big deal).

In CVSAGENT we check that the SID of the user is the same as the SID of the owner of the password (which 
is required for some basic level of security in a shared environment like terminal server):
	if(!EqualSid(ownerId, processId))
	{
		LocalFree((HLOCAL)ownerSd);
		LocalFree((HLOCAL)processSd);
		return FALSE;
	}


Now we could do something much more clever - e.g.: SSPI authentication, but again - will that actually help?  
And besides all of this is Windows specific, and this was on Mac.

I could look at what PAEGENT does, but I doubt it's much cleverer.

If the agent stops and asks for a fingerprint, or 'OK' button -that just prevents us running automated jobs.

Something unique we can do is have the password agent check the calling process - and in particular the 
calling process image (exe) digital signature.  It should be ours.  It's a bit difficult to check - we need to not 
only check it's signed, but that it's signed by us, and that the CA is what we expect it to be (not self-signed 
for instance).  But that would plug the hole. 

But only sort-of.  The malicious user could use our client on the host to 'checkout' what they want and then 
copy it where they want it.  It's a lot more data - copying the credentials off the PC is a lot less obvious.

Is it worth all that work, if it still results in data that can be so easily stolen?

But if it was combined with the 'encrypted checkout' then maybe...  Still - if the malware is in the user context 
- won't the encrypted filesystem IFS driver just decrypt it for the malware?  We can't only allow CVS to 
decrypt - because compilers/editors etc. all need to - but we could still require that the code is signed and 
with a valid (not self signed) CA.

2017-05-22 10:52 AEST by Arthur Barrett - How does CVSAGENT communicate with the CVSNT client?

It's done using inter-process communication using WM_COPYDATA (or CWnd::OnCopyData)
see samples:
https://msdn.microsoft.com/en-us/library/aa249870(v=vs.60).aspx
and
https://www.codeproject.com/articles/115/inter-process-communication-using-wm-copydata

So in cvstools library we have GlobalSettings::GetCachedPassword() and 
GlobalSettings::SetCachedPassword() which use FindWindow(L"CvsAgent",NULL) to find the cvsagent 
process and create a memory mapped file (CreateFileMappingA/MapViewOfFile) with the name 
"GetCurrentProcessId():GetTickCount()"  - that's the cvsnt client process id.

The client process then uses SendMessage(hWnd,WM_COPYDATA,NULL,(LPARAM)&cds); to send the 
'filename' to the cvsagent process.  The 'get' function (pMap->state=1;) retrieves the password, and the 'set' 
function (pMap->state=2;) sets the password...  Note: bug 6011 suggests we could significantly expand the 
things cvsagent does - e.g.: tell the client if it is licensed or not.  We could use pMap->state=4; etc. for those 
functions (3 is used for 'delete).  See Bug 6011 "enh: cvsagent needs replacing with an alternative (SecurID)" 
for more info.

In CVSAGENT the CAgentWnd::OnCopyData() function is automatically called when sent the WM_COPYDATA 
message - it uses the data in the message to open the mapped file.

CVSAGENT then gets the SID of its own process and the SID of the owner of the Memory Mapped File - if 
they are not the same - it finishes (that's the limit of security checking there).

The CVSAGENT process stores the passwords exactly as it receives them (not encrypted) in a 
std::map<std::string,std::string>, but it returns them using the 'standard' pserver SCRAMBLE 'A' format.

Compile options used for CVSAGENT include:
- Buffer security check: YES /GS
- Terminal Server Aware /TSAWARE
- Set checksum: YES /RELEASE

We don't follow any of Microsoft's security recommendations - probably because VS2013 and this code 
predates most of this:
https://msdn.microsoft.com/en-us/library/k3a3hzw7.aspx

Security Best Practices for C++
  /guard (Enable Control Flow Guard)
  /NXCOMPAT (Compatible with Data Execution Prevention)
  /DYNAMICBASE (Use address space layout randomization)

2017-05-22 12:02 AEST by Arthur Barrett - We should be storing the passwords using CryptProtectMemory() (well because we need to support WXP we 
need to import Advapi32.dll and use RtlEncryptMemory) - but it has limited use.  At least it stops easy 
harvesting of the passwords form memory in case of a crash dump...

https://msdn.microsoft.com/en-us/library/windows/desktop/aa380262(v=vs.85).aspx

Maybe we also need to consider using VirtualAlloc with an ACE:
http://stackoverflow.com/questions/8451/secure-memory-allocator-in-c

.NET 2.0 has a 'secure string' class :
https://msdn.microsoft.com/en-us/library/system.security.securestring.aspx

The main concern seems to be around zero-ing deallocated memory (because someone else can then 
allocate that memory and get our passwords).  Since CVSAGENT does delete passwords - this is certainly 
something we could do.  Basically we need to replace:
std::map<std::string,std::string> m_Passwords;

with:
std::map<std::string,cvsagent::securestring> m_Passwords;

Then define cvsagent::securestring.

We also need to use that for any temporary variables containing the password, e.g.: dlg.m_szPassword.c_str(

But it's a bit useless, if we can then just send a WM_COPYDATA message and get the password.

2017-05-22 12:02 AEST by Arthur Barrett - Note: the passwords are all 'scrambled' by default in memory - because they are sent from CVS that way.

The 'scramble' function in cvsagent is only used to 'scramble' the password when it's entered into the dialog 
the first time.

2017-05-22 12:05 AEST by Arthur Barrett - Created an attachment (id=3123)
Windows API: find process of message sender (WM_COPYDATA)

So the actual question about validating the sending process image has
apparently come up before
http://stackoverflow.com/questions/20256239/windows-api-find-process-of-message-sender-wm-copydata

2017-05-22 12:06 AEST by Arthur Barrett - Created an attachment (id=3124)
Windows API: find process for a file mapping handle

So the answer is basically - you can't authoritatively do it:
http://stackoverflow.com/questions/20296441/windows-api-find-process-for-a-file-mapping-handle

2017-05-22 12:26 AEST by Arthur Barrett - So it's impossible with the current architecture.

Instead we would need to switch to using RPC (LRPC) and then we could use RpcServerInqCallAttributes() 
function with RPC_CALL_ATTRIBUTES_V2 structure.

But that's a major change.
https://msdn.microsoft.com/en-us/library/windows/desktop/aa378651(v=vs.85).aspx

With a single PC it's OK - but in a virtual desktop environment (terminal server etc.) then do we have a single 
CVSAGENT RPC server per machine, or one per user?
https://msdn.microsoft.com/en-us/library/windows/desktop/aa379013(v=vs.85).aspx

2017-05-22 12:43 AEST by Arthur Barrett - Putty pagent isn't doing anything (yet) either:
https://git.tartarus.org/?p=simon/putty.git;a=tree

The only recent security work has been to set an ACL on the windows process:
+#ifndef UNPROTECT
+    /*
+     * Protect our process.
+     */
+    {
+        char *error = NULL;
+        if (!setprocessacl(error)) {
+            char *message = dupprintf("Could not restrict process ACL: %s",
+                                      error);
+            MessageBox(NULL, message, "PuTTYgen Warning",
+                       MB_ICONWARNING | MB_OK);
+            sfree(message);
+            sfree(error);
+        }
+    }
+#endif
+



int setprocessacl(char *error)
{
    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 != p_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;
} 





And also to stop DLL hijacking:
+void dll_hijacking_protection(void)
+{
+    /*
+     * If the OS provides it, call SetDefaultDllDirectories() to
+     * prevent DLLs from being loaded from the directory containing
+     * our own binary, and instead only load from system32.
+     *
+     * This is a protection against hijacking attacks, if someone runs
+     * PuTTY directly from their web browser's download directory
+     * having previously been enticed into clicking on an unwise link
+     * that downloaded a malicious DLL to the same directory under one
+     * of various magic names that seem to be things that standard
+     * Windows DLLs delegate to.
+     *
+     * It shouldn't break deliberate loading of user-provided DLLs
+     * such as GSSAPI providers, because those are specified by their
+     * full pathname by the user-provided configuration.
+     */
+    static HMODULE kernel32_module;
+    DECL_WINDOWS_FUNCTION(static, BOOL, SetDefaultDllDirectories, (DWORD));
+
+    if (!kernel32_module) {
+        kernel32_module = load_system32_dll("kernel32.dll");
+        GET_WINDOWS_FUNCTION(kernel32_module, SetDefaultDllDirectories);
+    }
+
+    if (p_SetDefaultDllDirectories) {
+        /* LOAD_LIBRARY_SEARCH_SYSTEM32 only */
+        p_SetDefaultDllDirectories(0x800);
+    }
+}
+

2017-05-23 13:35 AEST by Arthur Barrett - Feedback from Simon Tatham from Putty is that finding the source image for the request is not a priority - 
though this may help (using LRPC instead of Named Pipes):
https://www.chiark.greenend.org.uk/~sgtatham/putty/wishlist/pageant-named-pipe.html

However more importantly - due to the reliance on WM_COPYDATA there cannot be any 'confirmation' for 
putty key requests at the moment, and he thinks that would be a good way to address the issue of credential 
theft by malware:
https://www.chiark.greenend.org.uk/~sgtatham/putty/wishlist/pageant-key-confirm.html


However as soon as the service becomes 'system' accessible (rather than just this session - e.g.: multi-user 
switching or terminal services) then lots of other issues arise:
https://www.chiark.greenend.org.uk/~sgtatham//putty/wishlist/pageant-as-service.html

Looking at the Microsoft guide to IPC - it seems that the method most suitable (after WM_COPYDATA) is 
probably file mapping plus semaphores (both of which support the Local\ prefix, and (for better or worse) 
does away with authentication (we can just validate the owner SID as per the current code):
https://msdn.microsoft.com/en-
us/library/windows/desktop/aa365574(v=vs.85).aspx#base.using_pipes_for_ipc

I'd think the above technique could be a faster route for implementing only the 'pagent-key-confirm' wish, 
and I'm struggling a little with understanding the advantages that 'named pipes' would give.  The only 
advantage (as you've pointed out - perhaps a dubious advantage) of 'LRPC' (ncalrpc) is authoratatively 
finding the process/image that started the request - which named pipes doesn't give (semaphores don't
help with that either).

It is theoretically possible to enumerate all the processes and request the open handles to find which one has 
an open handle to the memory mapped file (or maybe more easily an open handle to the semaphore) - but it's 
hardly elegant.

The 'quickest' way to address this issue (credential theft by malware) is therefore to switch from using 
WM_COPYDATA to using semaphores.

i.e.:
cvsagent creates a memory mapped file, and a mutex  to control access to it and a semaphore.  When the 
semaphore is signalled, it opens the 'client' memory mapped file using the name in its own memory mapped 
file (now does the usual SID check, and a new optional 'do you want to allow the client access?'), and writes 
the cvspass, then clears the semaphore.

a cvsnt client creates its own memory mapped file, checks the semaphore is clear, claims the mutex, writes 
the name of the memory mapped file to the cvsagent memory mapped file, and signals the semaphore and 
releases the mutex and waits for the semaphore to clear.  When the semaphore is clear, it now grabs the 
password from the memory mapped file and closes the handles.

The cvsagent will now need a 'server' loop waiting on the semaphore and code to handle the authorisation.
  All the memory mapped files and semaphores should use LOCAL\blah-blah-blah names so that it is 
restricted to the current session (or I suppose we could use global names and the SID like before?).

2017-05-23 13:36 AEST by Arthur Barrett - Note: if we did use RPC (either named pipes or local) this article is interesting about how long the 
connections take:
http://support1499.rssing.com/browser.php?indx=10665300&item=23

2017-05-25 12:40 AEST by Arthur Barrett - This article explains how to restrict named pipes to a single session of terminal server:
https://msdn.microsoft.com/en-us/library/windows/desktop/aa365600(v=vs.85).aspx

To prevent remote users or users on a different terminal services session from accessing a named pipe, use 
the logon SID on the DACL for the pipe. The logon SID is used in run-as logons as well; it is the SID used to 
protect the per-session object namespace. For more information, see Getting the Logon SID in C++.
https://msdn.microsoft.com/en-us/library/windows/desktop/aa446670(v=vs.85).aspx

2017-05-25 12:41 AEST by Arthur Barrett - Names Pipes again.  To force the pipe to be local machine only use PIPE_REJECT_REMOTE_CLIENTS for 
dwPipeMode:
https://msdn.microsoft.com/en-us/library/windows/desktop/aa365150(v=vs.85).aspx

2017-05-26 11:40 AEST by Arthur Barrett - Created an attachment (id=3125)
How RPC works on Windows (PDF)

Link to a nice diagram about how RPC works on Windows:
https://technet.microsoft.com/en-us/library/cc738291(v=ws.10).aspx

2017-06-13 10:08 AEST by Arthur Barrett - any replacement for cvsagent also needs to handle calls from kernel processes, like c4s or the cvs encrypt 
driver, see bug 7095

2022-07-23 14:13 AEST by Arthur Barrett - Note: MS now recommend against using SecureString in .NET (pfft!) as explained here:

https://github.com/dotnet/platform-compat/blob/master/docs/DE0001.md