From 872df143b6284fd83af20744ce6661fd4b291048 Mon Sep 17 00:00:00 2001 From: "Samuel D. Leslie" Date: Fri, 7 Jan 2022 12:51:20 +1100 Subject: [PATCH 1/4] Whitespace fixes for WindowsOpenSshPipe --- SshAgentLib/WindowsOpenSshPipe.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/SshAgentLib/WindowsOpenSshPipe.cs b/SshAgentLib/WindowsOpenSshPipe.cs index e0110cf6..d35de590 100644 --- a/SshAgentLib/WindowsOpenSshPipe.cs +++ b/SshAgentLib/WindowsOpenSshPipe.cs @@ -42,11 +42,11 @@ public class WindowsOpenSshPipe : IDisposable static uint threadId = 0; NamedPipeServerStream listeningServer; - + public delegate void ConnectionHandlerFunc(Stream stream, Process process); public ConnectionHandlerFunc ConnectionHandler { get; set; } - + public WindowsOpenSshPipe() { if (File.Exists(string.Format("//./pipe/{0}", agentPipeId))) { @@ -58,7 +58,7 @@ public WindowsOpenSshPipe() }; thread.Start(); } - + [DllImport("kernel32.dll", SetLastError = true)] static extern bool GetNamedPipeClientProcessId(IntPtr Pipe, out long ClientProcessId); @@ -102,7 +102,7 @@ void connectionThread(object obj) // TODO: add event to notify when there is a problem } } - + public void Dispose() { Dispose(true); From 01faea0c0793e0ef8d2925eefd40a994555bcc35 Mon Sep 17 00:00:00 2001 From: "Samuel D. Leslie" Date: Fri, 7 Jan 2022 12:53:31 +1100 Subject: [PATCH 2/4] Bunch of miscellaneous tweaks to WindowsOpenSshPipe --- SshAgentLib/WindowsOpenSshPipe.cs | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/SshAgentLib/WindowsOpenSshPipe.cs b/SshAgentLib/WindowsOpenSshPipe.cs index d35de590..eb10db68 100644 --- a/SshAgentLib/WindowsOpenSshPipe.cs +++ b/SshAgentLib/WindowsOpenSshPipe.cs @@ -28,8 +28,6 @@ using System.IO; using System.IO.Pipes; using System.Runtime.InteropServices; -using System.Security.AccessControl; -using System.Security.Principal; using System.Threading; namespace dlech.SshAgentLib @@ -39,7 +37,7 @@ public class WindowsOpenSshPipe : IDisposable const string agentPipeId = "openssh-ssh-agent"; const int receiveBufferSize = 5 * 1024; - static uint threadId = 0; + static uint threadId; NamedPipeServerStream listeningServer; @@ -49,7 +47,7 @@ public class WindowsOpenSshPipe : IDisposable public WindowsOpenSshPipe() { - if (File.Exists(string.Format("//./pipe/{0}", agentPipeId))) { + if (File.Exists($"//./pipe/{agentPipeId}")) { throw new PageantRunningException(); } var thread = new Thread(listenerThread) { @@ -60,7 +58,7 @@ public WindowsOpenSshPipe() } [DllImport("kernel32.dll", SetLastError = true)] - static extern bool GetNamedPipeClientProcessId(IntPtr Pipe, out long ClientProcessId); + static extern bool GetNamedPipeClientProcessId(IntPtr Pipe, out uint ClientProcessId); void listenerThread() { @@ -72,7 +70,7 @@ void listenerThread() server.WaitForConnection(); listeningServer = null; var thread = new Thread(connectionThread) { - Name = string.Format("WindowsOpenSshPipe.Connection{0}", threadId++), + Name = $"WindowsOpenSshPipe.Connection{threadId++}", IsBackground = true }; thread.Start(server); @@ -88,8 +86,7 @@ void connectionThread(object obj) try { var server = obj as NamedPipeServerStream; - long clientPid; - if (!GetNamedPipeClientProcessId(server.SafePipeHandle.DangerousGetHandle(), out clientPid)) { + if (!GetNamedPipeClientProcessId(server.SafePipeHandle.DangerousGetHandle(), out var clientPid)) { throw new IOException("Failed to get client PID", Marshal.GetHRForLastWin32Error()); } var proc = Process.GetProcessById((int)clientPid); From 9185094eed7782ae3b5635a579ffed663a3b98d5 Mon Sep 17 00:00:00 2001 From: "Samuel D. Leslie" Date: Sat, 31 Jul 2021 11:50:05 +1000 Subject: [PATCH 3/4] Be more explicit and restrictive with accessibility levels --- SshAgentLib/WindowsOpenSshPipe.cs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/SshAgentLib/WindowsOpenSshPipe.cs b/SshAgentLib/WindowsOpenSshPipe.cs index eb10db68..101de91b 100644 --- a/SshAgentLib/WindowsOpenSshPipe.cs +++ b/SshAgentLib/WindowsOpenSshPipe.cs @@ -34,12 +34,12 @@ namespace dlech.SshAgentLib { public class WindowsOpenSshPipe : IDisposable { - const string agentPipeId = "openssh-ssh-agent"; - const int receiveBufferSize = 5 * 1024; + private const string agentPipeId = "openssh-ssh-agent"; + private const int receiveBufferSize = 5 * 1024; - static uint threadId; + private static uint threadId; - NamedPipeServerStream listeningServer; + private NamedPipeServerStream listeningServer; public delegate void ConnectionHandlerFunc(Stream stream, Process process); @@ -58,9 +58,9 @@ public WindowsOpenSshPipe() } [DllImport("kernel32.dll", SetLastError = true)] - static extern bool GetNamedPipeClientProcessId(IntPtr Pipe, out uint ClientProcessId); + private static extern bool GetNamedPipeClientProcessId(IntPtr Pipe, out uint ClientProcessId); - void listenerThread() + private void listenerThread() { try { while (true) { @@ -81,7 +81,7 @@ void listenerThread() } } - void connectionThread(object obj) + private void connectionThread(object obj) { try { var server = obj as NamedPipeServerStream; @@ -106,7 +106,7 @@ public void Dispose() GC.SuppressFinalize(this); } - void Dispose(bool disposing) + protected virtual void Dispose(bool disposing) { if (disposing) { if (listeningServer != null) { From 0dc59e559fcf8d90f822e625933f1b25d31ddb0b Mon Sep 17 00:00:00 2001 From: "Samuel D. Leslie" Date: Fri, 7 Jan 2022 12:54:56 +1100 Subject: [PATCH 4/4] Overhaul WindowsOpenSshPipe to use asynchronous connections If Windows OpenSSH agent support is enabled and subsequently disabled, the agent named pipe will not be closed. This is a (minor) security vulnerability as KeeAgent will continue to serve SSH agent responses when the functionality is meant to be disabled. There's two reasons why this happens: - While Dispose() is called on the class, a new server is started by listenerThread(). Also, thread objects aren't garbage collected, so this won't eventually fix itself when the garbage collector runs. - The call to WaitForConnection() is a blocking call. That extends to the Dispose() call, which will be blocked until a connection to the pipe is made. It'll then be immediately closed, so the client won't likely get any response, but per above a new named pipe is created. A side-effect of the latter is that re-enabling the feature in the same KeePass process will never work, as the named pipe path will always be still in use. An error message to that effect is shown to the user. Switching to asynchronous connections helps solve both of these issues. Instead of spawning threads we leverage the .NET thread pool, and the Dispose() logic has been improved to ensure the listening server will be reliably closed if the feature is disabled. Some more explicit exception handling and minimal logging for debugging purposes has also been added. --- SshAgentLib/WindowsOpenSshPipe.cs | 98 ++++++++++++++++--------------- 1 file changed, 52 insertions(+), 46 deletions(-) diff --git a/SshAgentLib/WindowsOpenSshPipe.cs b/SshAgentLib/WindowsOpenSshPipe.cs index 101de91b..57f515e4 100644 --- a/SshAgentLib/WindowsOpenSshPipe.cs +++ b/SshAgentLib/WindowsOpenSshPipe.cs @@ -28,17 +28,17 @@ using System.IO; using System.IO.Pipes; using System.Runtime.InteropServices; -using System.Threading; namespace dlech.SshAgentLib { - public class WindowsOpenSshPipe : IDisposable + public sealed class WindowsOpenSshPipe : IDisposable { - private const string agentPipeId = "openssh-ssh-agent"; - private const int receiveBufferSize = 5 * 1024; + private const string agentPipeName = "openssh-ssh-agent"; - private static uint threadId; + private const int BufferSizeIn = 5 * 1024; + private const int BufferSizeOut = 5 * 1024; + private bool disposed; private NamedPipeServerStream listeningServer; @@ -47,73 +47,79 @@ public class WindowsOpenSshPipe : IDisposable public WindowsOpenSshPipe() { - if (File.Exists($"//./pipe/{agentPipeId}")) { + if (File.Exists($"//./pipe/{agentPipeName}")) { throw new PageantRunningException(); } - var thread = new Thread(listenerThread) { - Name = "WindowsOpenSshPipe.Listener", - IsBackground = true - }; - thread.Start(); + + AwaitConnection(); } [DllImport("kernel32.dll", SetLastError = true)] private static extern bool GetNamedPipeClientProcessId(IntPtr Pipe, out uint ClientProcessId); - private void listenerThread() + private void AwaitConnection() { + if (disposed) { + return; + } + + listeningServer = new NamedPipeServerStream(agentPipeName, + PipeDirection.InOut, + NamedPipeServerStream.MaxAllowedServerInstances, + PipeTransmissionMode.Byte, + // TODO: Consider setting PipeOptions.CurrentUserOnly + PipeOptions.Asynchronous | PipeOptions.WriteThrough, + BufferSizeIn, + BufferSizeOut); + try { - while (true) { - var server = new NamedPipeServerStream(agentPipeId, PipeDirection.InOut, NamedPipeServerStream.MaxAllowedServerInstances, - PipeTransmissionMode.Byte, PipeOptions.WriteThrough, receiveBufferSize, receiveBufferSize); - listeningServer = server; - server.WaitForConnection(); - listeningServer = null; - var thread = new Thread(connectionThread) { - Name = $"WindowsOpenSshPipe.Connection{threadId++}", - IsBackground = true - }; - thread.Start(server); - } + listeningServer.BeginWaitForConnection(AcceptConnection, listeningServer); + Debug.WriteLine("Started new server and awaiting connection ..."); + } + catch (ObjectDisposedException) { + // Could happen if we're disposing while starting a server } - catch (Exception) { - // don't crash background thread + catch (Exception ex) { + // Should never happen but we don't want to crash KeePass + Debug.WriteLine($"{ex.GetType()} in AwaitConnection(): {ex.Message}"); + listeningServer.Dispose(); } } - private void connectionThread(object obj) + private void AcceptConnection(IAsyncResult result) { + Debug.WriteLine("Received new connection ..."); + AwaitConnection(); + + var server = result.AsyncState as NamedPipeServerStream; try { - var server = obj as NamedPipeServerStream; + server.EndWaitForConnection(result); if (!GetNamedPipeClientProcessId(server.SafePipeHandle.DangerousGetHandle(), out var clientPid)) { throw new IOException("Failed to get client PID", Marshal.GetHRForLastWin32Error()); } - var proc = Process.GetProcessById((int)clientPid); - ConnectionHandler(server, proc); - server.Disconnect(); - server.Dispose(); + var clientProcess = Process.GetProcessById((int)clientPid); + Debug.WriteLine($"Processing request from process: {clientProcess.MainModule.ModuleName} (PID: {clientPid})"); + ConnectionHandler(server, clientProcess); } - catch (Exception) { - // TODO: add event to notify when there is a problem + catch (ObjectDisposedException) { + // Server has been disposed + } + catch (Exception ex) { + // Should never happen but we don't want to crash KeePass + Debug.WriteLine($"{ex.GetType()} in AcceptConnection(): {ex.Message}"); + } + finally { + server.Dispose(); } } public void Dispose() { - Dispose(true); - GC.SuppressFinalize(this); - } - - protected virtual void Dispose(bool disposing) - { - if (disposing) { - if (listeningServer != null) { - listeningServer.Dispose(); - listeningServer = null; - } - } + disposed = true; + listeningServer?.Dispose(); + listeningServer = null; } } }