From 1fafbc2ef5746f87c6aec3e2ddc133786acb4020 Mon Sep 17 00:00:00 2001 From: Ryan Prichard Date: Thu, 14 Jan 2016 02:09:29 -0600 Subject: [PATCH] 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 | 7 +- src/agent/Agent.h | 3 + src/agent/subdir.mk | 1 + src/libwinpty/subdir.mk | 3 +- src/libwinpty/winpty.cc | 10 ++- src/shared/GenRandom.cc | 138 ++++++++++++++++++++++++++++++++++++++++ src/shared/GenRandom.h | 64 +++++++++++++++++++ src/winpty.gyp | 6 ++ 8 files changed, 220 insertions(+), 12 deletions(-) create mode 100755 src/shared/GenRandom.cc create mode 100755 src/shared/GenRandom.h diff --git a/src/agent/Agent.cc b/src/agent/Agent.cc index ad395cf..6e47666 100644 --- a/src/agent/Agent.cc +++ b/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-") - << GetCurrentProcessId() << L"-" - << InterlockedIncrement(&g_pipeCounter); + << (write ? L"conout-" : L"conin-") + << m_genRandom.uniqueName(); const auto name = nameSS.str(); const DWORD openMode = (write ? PIPE_ACCESS_OUTBOUND : PIPE_ACCESS_INBOUND) diff --git a/src/agent/Agent.h b/src/agent/Agent.h index d3fe244..63d02c1 100644 --- a/src/agent/Agent.h +++ b/src/agent/Agent.h @@ -27,6 +27,7 @@ #include #include +#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 diff --git a/src/agent/subdir.mk b/src/agent/subdir.mk index bb4d37d..be9bee9 100644 --- a/src/agent/subdir.mk +++ b/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 diff --git a/src/libwinpty/subdir.mk b/src/libwinpty/subdir.mk index 45df18a..b5387a7 100644 --- a/src/libwinpty/subdir.mk +++ b/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 $@ diff --git a/src/libwinpty/winpty.cc b/src/libwinpty/winpty.cc index 711dab4..354cb5c 100644 --- a/src/libwinpty/winpty.cc +++ b/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; - pipeNameSS << L"\\\\.\\pipe\\winpty-control-" << GetCurrentProcessId() - << L"-" << InterlockedIncrement(&g_consoleCounter); - const auto pipeName = pipeNameSS.str(); + winpty_shared::GenRandom genRandom; + const auto pipeName = + L"\\\\.\\pipe\\winpty-control-" + genRandom.uniqueName(); wp->controlPipe = createControlPipe(pipeName); // Create a background desktop. diff --git a/src/shared/GenRandom.cc b/src/shared/GenRandom.cc new file mode 100755 index 0000000..75c3e5c --- /dev/null +++ b/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 +#include + +#include + +#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( + 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(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(GetLastError())); + } + } else if (m_cryptProvIsValid) { + success = CryptGenRandom(m_cryptProv, size, + reinterpret_cast(buffer)); + if (!success) { + trace("GenRandom: CryptGenRandom failed, size=%d, lasterror=%u", + static_cast(size), + static_cast(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(bytes[i]) >> 4]; + ret[i * 2 + 1] = hex[static_cast(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 diff --git a/src/shared/GenRandom.h b/src/shared/GenRandom.h new file mode 100755 index 0000000..8b195b5 --- /dev/null +++ b/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 +#include + +#include + +#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 diff --git a/src/winpty.gyp b/src/winpty.gyp index 53a543a..adf9210 100644 --- a/src/winpty.gyp +++ b/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',