Add a GetNamedPipeClientProcessId security check on supported OSs (Vista+)

Also:

 * Removed some obsoleted comments.

 * Factor out the Handle class.  Rename to OwnedHandle.  Merge some
   improvements from libwinpty-rewrite, including fixing move-assignment
   so that it closes the existing handle, if one exists.

src/libwinpty/subdir.mk

@@ -27,6 +27,7 @@ LIBWINPTY_OBJECTS = \
 	build/libwinpty/shared/Buffer.o \
 	build/libwinpty/shared/DebugClient.o \
 	build/libwinpty/shared/GenRandom.o \
+	build/libwinpty/shared/OwnedHandle.o \
 	build/libwinpty/shared/StringUtil.o \
 	build/libwinpty/shared/WindowsSecurity.o \
 	build/libwinpty/shared/WinptyAssert.o \

src/libwinpty/winpty.cc

@@ -31,6 +31,7 @@
 #include "../shared/AgentMsg.h"
 #include "../shared/Buffer.h"
 #include "../shared/GenRandom.h"
+#include "../shared/OwnedHandle.h"
 #include "../shared/StringBuilder.h"
 #include "../shared/StringUtil.h"
 #include "../shared/WindowsSecurity.h"
@@ -257,7 +258,9 @@ static std::wstring getDesktopFullName()
 static void startAgentProcess(const BackgroundDesktop &desktop,
                               const std::wstring &controlPipeName,
                               const std::wstring &dataPipeName,
-                              int cols, int rows)
+                              int cols, int rows,
+                              HANDLE &agentProcess,
+                              DWORD &agentPid)
 {
     const std::wstring exePath = findAgentProgram();
     const std::wstring cmdline =
@@ -295,8 +298,36 @@ static void startAgentProcess(const BackgroundDesktop &desktop,
         exit(1);
     }
 
-    CloseHandle(pi.hProcess);
     CloseHandle(pi.hThread);
+
+    agentProcess = pi.hProcess;
+    agentPid = pi.dwProcessId;
+}
+
+static bool verifyPipeClientPid(HANDLE serverPipe, DWORD agentPid)
+{
+    const auto client = getNamedPipeClientProcessId(serverPipe);
+    const auto err = GetLastError();
+    const auto success = std::get<0>(client);
+    if (success == GetNamedPipeClientProcessId_Result::Success) {
+        const auto clientPid = std::get<1>(client);
+        if (clientPid != agentPid) {
+            trace("Security check failed: pipe client pid (%u) does not "
+                "match agent pid (%u)",
+                static_cast<unsigned int>(clientPid),
+                static_cast<unsigned int>(agentPid));
+            return false;
+        }
+        return true;
+    } else if (success == GetNamedPipeClientProcessId_Result::UnsupportedOs) {
+        trace("Pipe client PID security check skipped: "
+            "GetNamedPipeClientProcessId unsupported on this OS version");
+        return true;
+    } else {
+        trace("GetNamedPipeClientProcessId failed: %u",
+            static_cast<unsigned int>(err));
+        return false;
+    }
 }
 
 WINPTY_API winpty_t *winpty_open(int cols, int rows)
@@ -323,7 +354,11 @@ WINPTY_API winpty_t *winpty_open(int cols, int rows)
     BackgroundDesktop desktop = setupBackgroundDesktop();
 
     // Start the agent.
-    startAgentProcess(desktop, controlPipeName, dataPipeName, cols, rows);
+    HANDLE agentProcess = NULL;
+    DWORD agentPid = INFINITE;
+    startAgentProcess(desktop, controlPipeName, dataPipeName, cols, rows,
+                      agentProcess, agentPid);
+    OwnedHandle autoClose(agentProcess);
 
     // TODO: Frequently, I see the CreateProcess call return successfully,
     // but the agent immediately dies.  The following pipe connect calls then
@@ -349,6 +384,13 @@ WINPTY_API winpty_t *winpty_open(int cols, int rows)
     // destroyed before the agent can connect with them.
     restoreOriginalDesktop(desktop);
 
+    // Check that the pipe clients are correct.
+    if (!verifyPipeClientPid(pc->controlPipe, agentPid) ||
+            !verifyPipeClientPid(pc->dataPipe, agentPid)) {
+        delete pc;
+        return NULL;
+    }
+
     // TODO: This comment is now out-of-date.  The named pipes now have a DACL
     // that should prevent arbitrary users from connecting, even just to read.
     //
@@ -366,38 +408,6 @@ WINPTY_API winpty_t *winpty_open(int cols, int rows)
         return NULL;
     }
 
-    // TODO: On Windows Vista and forward, we could call
-    // GetNamedPipeClientProcessId and verify that the PID is correct.  We could
-    // also pass the PIPE_REJECT_REMOTE_CLIENTS flag on newer OS's.
-    // TODO: I suppose this code is still subject to a denial-of-service attack
-    // from untrusted accounts making read-only connections to the pipe.  It
-    // should probably provide a SECURITY_DESCRIPTOR for the pipe, but the last
-    // time I tried that (using SDDL), I couldn't get it to work (access denied
-    // errors).
-
-    // Aside: An obvious way to setup these handles is to open both ends of the
-    // pipe in the parent process and let the child inherit its handles.
-    // Unfortunately, the Windows API makes inheriting handles problematic.
-    // MSDN says that handles have to be marked inheritable, and once they are,
-    // they are inherited by any call to CreateProcess with
-    // bInheritHandles==TRUE.  To avoid accidental inheritance, the library's
-    // clients would be obligated not to create new processes while a thread
-    // was calling winpty_open.  Moreover, to inherit handles, MSDN seems
-    // to say that bInheritHandles must be TRUE[*], but I don't want to use a
-    // TRUE bInheritHandles, because I want to avoid leaking handles into the
-    // agent process, especially if the library someday allows creating the
-    // agent process under a different user account.
-    //
-    // [*] The way that bInheritHandles and STARTF_USESTDHANDLES work together
-    // is unclear in the documentation.  On one hand, for STARTF_USESTDHANDLES,
-    // it says that bInheritHandles must be TRUE.  On Vista and up, isn't
-    // PROC_THREAD_ATTRIBUTE_HANDLE_LIST an acceptable alternative to
-    // bInheritHandles?  On the other hand, KB315939 contradicts the
-    // STARTF_USESTDHANDLES documentation by saying, "Your pipe handles will
-    // still be duplicated because Windows will always duplicate the STD
-    // handles, even when bInheritHandles is set to FALSE."  IIRC, my testing
-    // showed that the KB article was correct.
-
     return pc;
 }
 

src/shared/OwnedHandle.cc

@@ -0,0 +1,36 @@
+// Copyright (c) 2016 Ryan Prichard
+//
+// Permission is hereby granted, free of charge, to any person obtaining a copy
+// of this software and associated documentation files (the "Software"), to
+// deal in the Software without restriction, including without limitation the
+// rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
+// sell copies of the Software, and to permit persons to whom the Software is
+// furnished to do so, subject to the following conditions:
+//
+// The above copyright notice and this permission notice shall be included in
+// all copies or substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+// FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+// IN THE SOFTWARE.
+
+#include "OwnedHandle.h"
+
+#include "DebugClient.h"
+#include "WinptyException.h"
+
+void OwnedHandle::dispose(bool nothrow) {
+    if (m_h != nullptr && m_h != INVALID_HANDLE_VALUE) {
+        if (!CloseHandle(m_h)) {
+            trace("CloseHandle(%p) failed", m_h);
+            if (!nothrow) {
+                throwWindowsError(L"CloseHandle failed");
+            }
+        }
+    }
+    m_h = nullptr;
+}

src/shared/OwnedHandle.h

@@ -0,0 +1,45 @@
+// Copyright (c) 2016 Ryan Prichard
+//
+// Permission is hereby granted, free of charge, to any person obtaining a copy
+// of this software and associated documentation files (the "Software"), to
+// deal in the Software without restriction, including without limitation the
+// rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
+// sell copies of the Software, and to permit persons to whom the Software is
+// furnished to do so, subject to the following conditions:
+//
+// The above copyright notice and this permission notice shall be included in
+// all copies or substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+// FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+// IN THE SOFTWARE.
+
+#ifndef WINPTY_SHARED_OWNED_HANDLE_H
+#define WINPTY_SHARED_OWNED_HANDLE_H
+
+#include <windows.h>
+
+class OwnedHandle {
+    HANDLE m_h;
+public:
+    OwnedHandle() : m_h(nullptr) {}
+    explicit OwnedHandle(HANDLE h) : m_h(h) {}
+    ~OwnedHandle() { dispose(true); }
+    void dispose(bool nothrow=false);
+    HANDLE get() const { return m_h; }
+    HANDLE release() { HANDLE ret = m_h; m_h = nullptr; return ret; }
+    OwnedHandle(const OwnedHandle &other) = delete;
+    OwnedHandle(OwnedHandle &&other) : m_h(other.release()) {}
+    OwnedHandle &operator=(const OwnedHandle &other) = delete;
+    OwnedHandle &operator=(OwnedHandle &&other) {
+        dispose();
+        m_h = other.release();
+        return *this;
+    }
+};
+
+#endif // WINPTY_SHARED_OWNED_HANDLE_H

src/shared/PrecompiledHeader.h

@@ -35,6 +35,7 @@
 #include <memory>
 #include <new>
 #include <string>
+#include <tuple>
 #include <type_traits>
 #include <utility>
 #include <vector>

src/shared/WindowsSecurity.cc

@@ -24,6 +24,7 @@
 
 #include "DebugClient.h"
 #include "OsModule.h"
+#include "OwnedHandle.h"
 #include "StringBuilder.h"
 #include "WinptyAssert.h"
 #include "WinptyException.h"
@@ -66,34 +67,12 @@ Sid allocatedSid(PSID v) {
     return Sid(v, std::unique_ptr<Impl>(new Impl { v }));
 }
 
-class Handle {
-    HANDLE m_h;
-public:
-    explicit Handle(HANDLE h) : m_h(h) {}
-    ~Handle() {
-        if (m_h != nullptr) {
-            CloseHandle(m_h);
-        }
-    }
-    HANDLE get() const { return m_h; }
-    Handle(const Handle &other) = delete;
-    Handle &operator=(const Handle &other) = delete;
-    Handle(Handle &&other) : m_h(other.m_h) {
-        other.m_h = nullptr;
-    }
-    Handle &operator=(Handle &&other) {
-        m_h = other.m_h;
-        other.m_h = nullptr;
-        return *this;
-    }
-};
-
 } // anonymous namespace
 
 // Returns a handle to the thread's effective security token.  If the thread
 // is impersonating another user, its token is returned, and otherwise, the
 // process' security token is opened.  The handle is opened with TOKEN_QUERY.
-static Handle openSecurityTokenForQuery() {
+static OwnedHandle openSecurityTokenForQuery() {
     HANDLE token = nullptr;
     // It is unclear to me whether OpenAsSelf matters for winpty, or what the
     // most appropriate value is.
@@ -108,7 +87,7 @@ static Handle openSecurityTokenForQuery() {
     }
     ASSERT(token != nullptr &&
         "OpenThreadToken/OpenProcessToken token is NULL");
-    return Handle(token);
+    return OwnedHandle(token);
 }
 
 // Returns the TokenOwner of the thread's effective security token.
@@ -117,7 +96,7 @@ Sid getOwnerSid() {
         std::unique_ptr<char[]> buffer;
     };
 
-    Handle token = openSecurityTokenForQuery();
+    OwnedHandle token = openSecurityTokenForQuery();
     DWORD actual = 0;
     BOOL success;
     success = GetTokenInformation(token.get(), TokenOwner,
@@ -461,3 +440,27 @@ DWORD rejectRemoteClientsPipeFlag() {
         return 0;
     }
 }
+
+typedef BOOL WINAPI GetNamedPipeClientProcessId_t(
+    HANDLE Pipe,
+    PULONG ClientProcessId);
+
+std::tuple<GetNamedPipeClientProcessId_Result, DWORD>
+getNamedPipeClientProcessId(HANDLE serverPipe) {
+    OsModule kernel32(L"kernel32.dll");
+    const auto pGetNamedPipeClientProcessId =
+        reinterpret_cast<GetNamedPipeClientProcessId_t*>(
+            kernel32.proc("GetNamedPipeClientProcessId"));
+    if (pGetNamedPipeClientProcessId == nullptr) {
+        return std::make_tuple(
+            GetNamedPipeClientProcessId_Result::UnsupportedOs, 0);
+    }
+    ULONG pid = 0;
+    if (!pGetNamedPipeClientProcessId(serverPipe, &pid)) {
+        return std::make_tuple(
+            GetNamedPipeClientProcessId_Result::Failure, 0);
+    }
+    return std::make_tuple(
+        GetNamedPipeClientProcessId_Result::Success,
+        static_cast<DWORD>(pid));
+}

src/shared/WindowsSecurity.h

@@ -26,6 +26,7 @@
 
 #include <memory>
 #include <string>
+#include <tuple>
 #include <utility>
 
 // PSID and PSECURITY_DESCRIPTOR are both pointers to void, but we want
@@ -79,13 +80,25 @@ Sid wellKnownSid(
 Sid builtinAdminsSid();
 Sid localSystemSid();
 Sid everyoneSid();
+
 SecurityDescriptor createPipeSecurityDescriptorOwnerFullControl();
 SecurityDescriptor createPipeSecurityDescriptorOwnerFullControlEveryoneWrite();
 SecurityDescriptor getObjectSecurityDescriptor(HANDLE handle);
+
 std::wstring sidToString(PSID sid);
 Sid stringToSid(const std::wstring &str);
 SecurityDescriptor stringToSd(const std::wstring &str);
 std::wstring sdToString(PSECURITY_DESCRIPTOR sd);
+
 DWORD rejectRemoteClientsPipeFlag();
 
+enum class GetNamedPipeClientProcessId_Result {
+    Success,
+    Failure,
+    UnsupportedOs,
+};
+
+std::tuple<GetNamedPipeClientProcessId_Result, DWORD>
+getNamedPipeClientProcessId(HANDLE serverPipe);
+
 #endif // WINPTY_WINDOWS_SECURITY_H

src/winpty.gyp

@@ -113,6 +113,8 @@
                 'shared/DebugClient.cc',
                 'shared/GenRandom.h',
                 'shared/GenRandom.cc',
+                'shared/OwnedHandle.h',
+                'shared/OwnedHandle.cc',
                 'shared/StringBuilder.h',
                 'shared/StringUtil.cc',
                 'shared/StringUtil.h',