Strengthen unique pipe name generation.

* PIDs are recycled, so include the system time.

 * A program running on the same machine could predict the names of pipes
   winpty uses and block winpty, causing a denial-of-service.  I'm not sure
   this is really a concern, but I suppose in principle it is?  Try to
   guard against it by appending random bytes to the pipe name.  The
   CreatePrivateNamespace API looks relevant, but I'm not sure it applies
   to named pipes, and it's Vista only.

src/agent/Agent.cc

@@ -51,8 +51,6 @@
 const int SC_CONSOLE_MARK = 0xFFF2;
 const int SC_CONSOLE_SELECT_ALL = 0xFFF5;
 
-static volatile LONG g_pipeCounter;
-
 #define COUNT_OF(x) (sizeof(x) / sizeof((x)[0]))
 
 namespace {
@@ -249,9 +247,8 @@ NamedPipe *Agent::makeDataPipe(bool write)
 {
     std::wstringstream nameSS;
     nameSS << L"\\\\.\\pipe\\winpty-data-"
-           << (write ? L"out-" : L"in-")
+           << (write ? L"conout-" : L"conin-")
-           << GetCurrentProcessId() << L"-"
+           << m_genRandom.uniqueName();
-           << InterlockedIncrement(&g_pipeCounter);
     const auto name = nameSS.str();
     const DWORD openMode =
         (write ? PIPE_ACCESS_OUTBOUND : PIPE_ACCESS_INBOUND)

src/agent/Agent.h

@@ -27,6 +27,7 @@
 #include <string>
 #include <vector>
 
+#include "../shared/GenRandom.h"
 #include "EventLoop.h"
 #include "DsrSender.h"
 #include "Coord.h"
@@ -125,6 +126,8 @@ private:
     int m_dirtyLineCount;
 
     std::wstring m_currentTitle;
+
+    winpty_shared::GenRandom m_genRandom;
 };
 
 #endif // AGENT_H

src/agent/subdir.mk

@@ -38,6 +38,7 @@ AGENT_OBJECTS = \
 	build/mingw/agent/main.o \
 	build/mingw/shared/Buffer.o \
 	build/mingw/shared/DebugClient.o \
+	build/mingw/shared/GenRandom.o \
 	build/mingw/shared/WinptyAssert.o \
 	build/mingw/shared/WinptyVersion.o \
 	build/mingw/shared/winpty_wcsnlen.o

src/libwinpty/subdir.mk

@@ -26,7 +26,8 @@ LIBWINPTY_OBJECTS = \
 	build/mingw/libwinpty/WinptyException.o \
 	build/mingw/libwinpty/winpty.o \
 	build/mingw/shared/Buffer.o \
-	build/mingw/shared/DebugClient.o
+	build/mingw/shared/DebugClient.o \
+	build/mingw/shared/GenRandom.o
 
 build/winpty.dll : $(LIBWINPTY_OBJECTS)
 	@echo Linking $@

src/libwinpty/winpty.cc

@@ -38,6 +38,7 @@
 #include "../shared/AgentMsg.h"
 #include "../shared/Buffer.h"
 #include "../shared/DebugClient.h"
+#include "../shared/GenRandom.h"
 #include "BackgroundDesktop.h"
 #include "Util.h"
 #include "WinptyException.h"
@@ -53,8 +54,6 @@ using namespace libwinpty;
 
 #define AGENT_EXE L"winpty-agent.exe"
 
-static volatile LONG g_consoleCounter;
-
 
 
 /*****************************************************************************
@@ -398,10 +397,9 @@ winpty_open(const winpty_config_t *cfg,
         wp->ioEvent = createEvent();
 
         // Create control server pipe.
-        std::wstringstream pipeNameSS;
+        winpty_shared::GenRandom genRandom;
-        pipeNameSS << L"\\\\.\\pipe\\winpty-control-" << GetCurrentProcessId()
+        const auto pipeName =
-                   << L"-" << InterlockedIncrement(&g_consoleCounter);
+            L"\\\\.\\pipe\\winpty-control-" + genRandom.uniqueName();
-        const auto pipeName = pipeNameSS.str();
         wp->controlPipe = createControlPipe(pipeName);
 
         // Create a background desktop.

src/shared/GenRandom.cc

@@ -0,0 +1,138 @@
+// 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 "GenRandom.h"
+
+#include <stdint.h>
+#include <string.h>
+
+#include <sstream>
+
+#include "DebugClient.h"
+
+namespace winpty_shared {
+
+static volatile LONG g_pipeCounter;
+
+GenRandom::GenRandom() : m_advapi32(L"advapi32.dll"), m_rtlGenRandom(nullptr) {
+    // First try to use the pseudo-documented RtlGenRandom function from
+    // advapi32.dll.  Creating a CryptoAPI context is slow, and RtlGenRandom
+    // avoids the overhead.  It's documented in this blog post[1] and on
+    // MSDN[2] with a disclaimer about future breakage.  This technique is
+    // apparently built-in into the MSVC CRT, though, for the rand_s function,
+    // so perhaps it is stable enough.
+    //
+    // [1] http://blogs.msdn.com/b/michael_howard/archive/2005/01/14/353379.aspx
+    // [2] https://msdn.microsoft.com/en-us/library/windows/desktop/aa387694(v=vs.85).aspx
+    //
+    // Both RtlGenRandom and the Crypto API functions exist in XP and up.
+    m_rtlGenRandom = reinterpret_cast<RtlGenRandom_t*>(
+        m_advapi32.proc("SystemFunction036"));
+    // The OsModule class logs an error message if the proc is nullptr.
+    if (m_rtlGenRandom != nullptr) {
+        return;
+    }
+
+    // Fall back to the crypto API.
+    m_cryptProvIsValid =
+        CryptAcquireContext(&m_cryptProv, nullptr, nullptr,
+                            PROV_RSA_FULL, CRYPT_VERIFYCONTEXT);
+    if (!m_cryptProvIsValid) {
+        trace("GenRandom: CryptAcquireContext failed: %u",
+            static_cast<unsigned>(GetLastError()));
+    }
+}
+
+GenRandom::~GenRandom() {
+    if (m_cryptProvIsValid) {
+        CryptReleaseContext(m_cryptProv, 0);
+    }
+}
+
+// Returns false if the context is invalid or the generation fails.
+bool GenRandom::fillBuffer(void *buffer, size_t size) {
+    memset(buffer, 0, size);
+    bool success = false;
+    if (m_rtlGenRandom != nullptr) {
+        success = m_rtlGenRandom(buffer, size);
+        if (!success) {
+            trace("GenRandom: RtlGenRandom/SystemFunction036 failed: %u",
+                static_cast<unsigned>(GetLastError()));
+        }
+    } else if (m_cryptProvIsValid) {
+        success = CryptGenRandom(m_cryptProv, size,
+                                 reinterpret_cast<BYTE*>(buffer));
+        if (!success) {
+            trace("GenRandom: CryptGenRandom failed, size=%d, lasterror=%u",
+                static_cast<int>(size),
+                static_cast<unsigned>(GetLastError()));
+        }
+    }
+    return success;
+}
+
+// Returns an empty string if either of CryptAcquireContext or CryptGenRandom
+// fail.
+std::string GenRandom::randomBytes(size_t numBytes) {
+    std::string ret(numBytes, '\0');
+    if (!fillBuffer(&ret[0], numBytes)) {
+        return std::string();
+    }
+    return ret;
+}
+
+std::wstring GenRandom::randomHexString(size_t numBytes) {
+    const std::string bytes = randomBytes(numBytes);
+    std::wstring ret(bytes.size() * 2, L'\0');
+    for (size_t i = 0; i < bytes.size(); ++i) {
+        static const wchar_t hex[] = L"0123456789abcdef";
+        ret[i * 2]     = hex[static_cast<uint8_t>(bytes[i]) >> 4];
+        ret[i * 2 + 1] = hex[static_cast<uint8_t>(bytes[i]) & 0xF];
+    }
+    return ret;
+}
+
+// Generates a unique and hard-to-guess case-insensitive string suitable for
+// use in a pipe filename or a Windows object name.
+std::wstring GenRandom::uniqueName() {
+    // First include enough information to avoid collisions assuming
+    // cooperative software.  This code assumes that a process won't die and
+    // be replaced with a recycled PID within a single GetSystemTimeAsFileTime
+    // interval.
+    FILETIME monotonicTime = {};
+    GetSystemTimeAsFileTime(&monotonicTime);
+    uint64_t monotonicTime64;
+    memcpy(&monotonicTime64, &monotonicTime, sizeof(uint64_t));
+    std::wstringstream ss;
+    ss << GetCurrentProcessId()
+       << L'-' << InterlockedIncrement(&g_pipeCounter)
+       << std::hex
+       << L'-' << monotonicTime64;
+    // It isn't clear to me how the crypto APIs would fail.  It *probably*
+    // doesn't matter that much anyway?  In principle, a predictable pipe name
+    // is subject to a local denial-of-service attack.
+    auto random = randomHexString(12);
+    if (!random.empty()) {
+        ss << '-' << random;
+    }
+    return ss.str();
+}
+
+} // winpty_shared namespace

src/shared/GenRandom.h

@@ -0,0 +1,64 @@
+// 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_GEN_RANDOM_H
+#define WINPTY_GEN_RANDOM_H
+
+// The original MinGW requires that we include wincrypt.h.  With MinGW-w64 and
+// MSVC, including windows.h is sufficient.
+#include <windows.h>
+#include <wincrypt.h>
+
+#include <string>
+
+#include "OsModule.h"
+
+namespace winpty_shared {
+
+class GenRandom {
+    typedef BOOLEAN WINAPI RtlGenRandom_t(PVOID, ULONG);
+
+    // The m_rtlGenRandom field is not initialized here, because doing so
+    // hits a bug in G++ 4.7, which Cygwin/MinGW is still using.  Namely, that
+    // gcc version confuses the field with a pure virtual method.  It complains
+    // that only "= 0" syntax is valid, and if you use that syntax, then g++
+    // segfaults.
+    OsModule m_advapi32;
+    RtlGenRandom_t *m_rtlGenRandom; // = nullptr;
+    bool m_cryptProvIsValid = false;
+    HCRYPTPROV m_cryptProv = 0;
+
+public:
+    GenRandom();
+    ~GenRandom();
+    bool fillBuffer(void *buffer, size_t size);
+    std::string randomBytes(size_t numBytes);
+    std::wstring randomHexString(size_t numBytes);
+    std::wstring uniqueName();
+
+    // Return true if the crypto context was successfully initialized.
+    bool valid() const {
+        return m_rtlGenRandom != nullptr || m_cryptProvIsValid;
+    }
+};
+
+} // winpty_shared namespace
+
+#endif // WINPTY_GEN_RANDOM_H

src/winpty.gyp

@@ -32,6 +32,7 @@
             'target_name' : 'winpty-agent',
             'type' : 'executable',
             'libraries' : [
+                '-ladvapi32',
                 '-luser32',
             ],
             'sources' : [
@@ -72,6 +73,8 @@
                 'shared/Buffer.cc',
                 'shared/DebugClient.h',
                 'shared/DebugClient.cc',
+                'shared/GenRandom.h',
+                'shared/GenRandom.cc',
                 'shared/OsModule.h',
                 'shared/UnixCtrlChars.h',
                 'shared/WinptyAssert.h',
@@ -87,6 +90,7 @@
             'target_name' : 'winpty',
             'type' : 'shared_library',
             'libraries' : [
+                '-ladvapi32',
                 '-luser32',
             ],
             'sources' : [
@@ -104,6 +108,8 @@
                 'shared/Buffer.cc',
                 'shared/DebugClient.h',
                 'shared/DebugClient.cc',
+                'shared/GenRandom.h',
+                'shared/GenRandom.cc',
                 'shared/c99_snprintf.h',
                 'shared/cxx11_mutex.h',
                 'shared/cxx11_noexcept.h',