maxmustermann/terminal
1
0
Fork 0
Code Issues Pull requests Projects Releases Wiki Activity

Prevent cleanup of object given to handle that failed access check (#3414)

Browse source 
* Ensure rights check and increments pass before assigning an object to a handle (since deallocation of handles will automatically attempt to cleanup).
This commit is contained in:
Michael Niksa 2019-11-05 14:22:55 -08:00 committed by GitHub
parent 53b6f143b3
commit 52d7de38b1
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
12 changed files with 217 additions and 23 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

50
src/host/ft_host/API_BufferTests.cpp
View file

@ -18,6 +18,8 @@ class BufferTests
TEST_METHOD(TestSetConsoleActiveScreenBufferInvalid);
TEST_METHOD(TestCookedReadOnNonShareableScreenBuffer);
BEGIN_TEST_METHOD(TestWritingInactiveScreenBuffer)
TEST_METHOD_PROPERTY(L"Data:UseVtOutput", L"{true, false}")
END_TEST_METHOD()
@ -33,6 +35,54 @@ void BufferTests::TestSetConsoleActiveScreenBufferInvalid()
VERIFY_WIN32_BOOL_FAILED(SetConsoleActiveScreenBuffer(nullptr));
}
void BufferTests::TestCookedReadOnNonShareableScreenBuffer()
{
Log::Comment(L"Get original handles");
const auto in = GetStdInputHandle();
const auto out = GetStdOutputHandle();
Log::Comment(L"Ensure cooked input is on (line input mode) and echoing to the screen.");
DWORD inMode = 0;
VERIFY_WIN32_BOOL_SUCCEEDED(GetConsoleMode(in, &inMode));
inMode |= ENABLE_LINE_INPUT;
inMode |= ENABLE_ECHO_INPUT;
VERIFY_WIN32_BOOL_SUCCEEDED(SetConsoleMode(in, inMode));
Log::Comment(L"Create alternate buffer that is read/writeable but not shareable.");
const auto otherBuffer = CreateConsoleScreenBuffer(GENERIC_READ | GENERIC_WRITE,
0, // This says non-sharable
nullptr,
CONSOLE_TEXTMODE_BUFFER,
nullptr);
VERIFY_WIN32_BOOL_SUCCEEDED(INVALID_HANDLE_VALUE != otherBuffer);
Log::Comment(L"Set the alternate buffer as active.");
VERIFY_WIN32_BOOL_SUCCEEDED(SetConsoleActiveScreenBuffer(otherBuffer));
// On a cooked read with echoing, the act of reading from the buffer will cause a handle to be
// taken to the active output buffer such that the cooked/line reading handler can display
// what is being typed on the screen as it is being typed before the enter key is hit.
// This should fail because we've denied anyone sharing access with us and we hold the primary
// active handle above.
Log::Comment(L"Perform a read operation to attempt to take handle to output buffer and hopefully fail.");
char buffer[1];
DWORD read = 0;
SetLastError(S_OK);
VERIFY_WIN32_BOOL_FAILED(ReadFile(in, buffer, sizeof(buffer), &read, nullptr));
VERIFY_ARE_EQUAL(static_cast<DWORD>(ERROR_SHARING_VIOLATION), GetLastError());
Log::Comment(L"Put the buffer back.");
VERIFY_WIN32_BOOL_SUCCEEDED(SetConsoleActiveScreenBuffer(out));
Log::Comment(L"Close the alternate buffer.");
VERIFY_WIN32_BOOL_SUCCEEDED(CloseHandle(otherBuffer));
Sleep(2000);
Log::Comment(L"Ensure that the console didn't die/crash");
VERIFY_IS_TRUE(IsConsoleStillRunning());
}
void BufferTests::TestWritingInactiveScreenBuffer()
{
bool useVtOutput;

8
src/host/ft_host/Common.cpp
View file

@ -7,6 +7,14 @@ using WEX::Logging::Log;
using namespace WEX::Common;
HANDLE Common::_hConsole = INVALID_HANDLE_VALUE;
extern wil::unique_process_information pi;
bool IsConsoleStillRunning()
{
DWORD exitCode = S_OK;
VERIFY_WIN32_BOOL_SUCCEEDED(GetExitCodeProcess(pi.hProcess, &exitCode));
return exitCode == STILL_ACTIVE;
}
void VerifySucceededGLE(BOOL bResult)
{

2
src/host/ft_host/Common.hpp
View file

@ -68,3 +68,5 @@ BOOL UnadjustWindowRectEx(
HANDLE GetStdInputHandle();
HANDLE GetStdOutputHandle();
bool IsConsoleStillRunning();

2
src/host/ft_host/InitTests.cpp
View file

@ -16,6 +16,7 @@ static PCWSTR pwszForceV2ValueName = L"ForceV2";
// instead of using the Windows-default copy of console host.
wil::unique_handle hJob;
wil::unique_process_information pi;
static FILE* std_out = nullptr;
static FILE* std_in = nullptr;
@ -135,7 +136,6 @@ MODULE_SETUP(ModuleSetup)
// Setup and call create process.
STARTUPINFOW si = { 0 };
si.cb = sizeof(STARTUPINFOW);
wil::unique_process_information pi;
// We start suspended so we can put it in the job before it does anything
// We say new console so it doesn't run in the same window as our test.

1
src/host/ut_host/Host.UnitTests.vcxproj
View file

@ -16,6 +16,7 @@
<ClCompile Include="DbcsTests.cpp" />
<ClCompile Include="HistoryTests.cpp" />
<ClCompile Include="InitTests.cpp" />
<ClCompile Include="ObjectTests.cpp" />
<ClCompile Include="OutputCellIteratorTests.cpp" />
<ClCompile Include="ScreenBufferTests.cpp" />
<ClCompile Include="SearchTests.cpp" />

3
src/host/ut_host/Host.UnitTests.vcxproj.filters
View file

@ -111,6 +111,9 @@
<ClCompile Include="OutputCellIteratorTests.cpp">
<Filter>Source Files</Filter>
</ClCompile>
<ClCompile Include="ObjectTests.cpp">
<Filter>Source Files</Filter>
</ClCompile>
</ItemGroup>
<ItemGroup>
<ClInclude Include="UnicodeLiteral.hpp">

100
src/host/ut_host/ObjectTests.cpp Normal file
View file

@ -0,0 +1,100 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.
#include "precomp.h"
#include "WexTestClass.h"
#include "..\..\inc\consoletaeftemplates.hpp"
#include "CommonState.hpp"
using namespace WEX::Common;
using namespace WEX::Logging;
using namespace WEX::TestExecution;
using Microsoft::Console::Interactivity::ServiceLocator;
class ObjectTests
{
CommonState* m_state;
TEST_CLASS(ObjectTests);
TEST_CLASS_SETUP(ClassSetup)
{
m_state = new CommonState();
m_state->InitEvents();
m_state->PrepareGlobalFont();
m_state->PrepareGlobalScreenBuffer();
m_state->PrepareGlobalInputBuffer();
return true;
}
TEST_CLASS_CLEANUP(ClassCleanup)
{
m_state->CleanupGlobalScreenBuffer();
m_state->CleanupGlobalFont();
m_state->CleanupGlobalInputBuffer();
delete m_state;
return true;
}
TEST_METHOD_SETUP(MethodSetup)
{
return true;
}
TEST_METHOD_CLEANUP(MethodCleanup)
{
return true;
}
TEST_METHOD(TestFailedHandleAllocationWhenNotShared)
{
Log::Comment(L"Create a new output buffer modeled from the default/active one.");
const auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
const auto& existingOutput = gci.GetActiveOutputBuffer();
SCREEN_INFORMATION* newOutput;
VERIFY_NT_SUCCESS(SCREEN_INFORMATION::CreateInstance(existingOutput.GetViewport().Dimensions(),
existingOutput.GetCurrentFont(),
existingOutput.GetBufferSize().Dimensions(),
existingOutput.GetAttributes(),
*existingOutput.GetPopupAttributes(),
existingOutput.GetTextBuffer().GetCursor().GetSize(),
&newOutput));
ConsoleObjectHeader* newOutputAsHeader = static_cast<ConsoleObjectHeader*>(newOutput);
VERIFY_ARE_EQUAL(0ul, newOutputAsHeader->_ulOpenCount);
VERIFY_ARE_EQUAL(0ul, newOutputAsHeader->_ulReaderCount);
VERIFY_ARE_EQUAL(0ul, newOutputAsHeader->_ulReadShareCount);
VERIFY_ARE_EQUAL(0ul, newOutputAsHeader->_ulWriterCount);
VERIFY_ARE_EQUAL(0ul, newOutputAsHeader->_ulWriteShareCount);
std::unique_ptr<ConsoleHandleData> unsharedHandle;
VERIFY_SUCCEEDED(newOutput->AllocateIoHandle(ConsoleHandleData::HandleType::Output,
GENERIC_READ | GENERIC_WRITE,
0,
unsharedHandle));
unsharedHandle.release(); // leak the pointer because destruction will attempt count decrement
VERIFY_ARE_EQUAL(1ul, newOutputAsHeader->_ulOpenCount);
VERIFY_ARE_EQUAL(1ul, newOutputAsHeader->_ulReaderCount);
VERIFY_ARE_EQUAL(0ul, newOutputAsHeader->_ulReadShareCount);
VERIFY_ARE_EQUAL(1ul, newOutputAsHeader->_ulWriterCount);
VERIFY_ARE_EQUAL(0ul, newOutputAsHeader->_ulWriteShareCount);
std::unique_ptr<ConsoleHandleData> secondHandleAttempt;
VERIFY_ARE_EQUAL(HRESULT_FROM_WIN32(ERROR_SHARING_VIOLATION), newOutput->AllocateIoHandle(ConsoleHandleData::HandleType::Output, GENERIC_READ | GENERIC_WRITE, 0, secondHandleAttempt));
VERIFY_ARE_EQUAL(1ul, newOutputAsHeader->_ulOpenCount);
VERIFY_ARE_EQUAL(1ul, newOutputAsHeader->_ulReaderCount);
VERIFY_ARE_EQUAL(0ul, newOutputAsHeader->_ulReadShareCount);
VERIFY_ARE_EQUAL(1ul, newOutputAsHeader->_ulWriterCount);
VERIFY_ARE_EQUAL(0ul, newOutputAsHeader->_ulWriteShareCount);
}
};

1
src/host/ut_host/sources
View file

@ -43,6 +43,7 @@ SOURCES = \
CommandNumberPopupTests.cpp \
CopyFromCharPopupTests.cpp \
CopyToCharPopupTests.cpp \
ObjectTests.cpp \
DefaultResource.rc \

45
src/server/ObjectHandle.cpp
View file

@ -12,20 +12,14 @@
#include "..\interactivity\inc\ServiceLocator.hpp"
ConsoleHandleData::ConsoleHandleData(const ULONG ulHandleType,
const ACCESS_MASK amAccess,
const ULONG ulShareAccess,
_In_ PVOID const pvClientPointer) :
_ulHandleType(ulHandleType),
ConsoleHandleData::ConsoleHandleData(const ACCESS_MASK amAccess,
const ULONG ulShareAccess) :
_ulHandleType(HandleType::NotReady),
_amAccess(amAccess),
_ulShareAccess(ulShareAccess),
_pvClientPointer(pvClientPointer),
_pvClientPointer(nullptr),
_pClientInput(nullptr)
{
if (_IsInput())
{
_pClientInput = std::make_unique<INPUT_READ_HANDLE_DATA>();
}
}
// Routine Description:
@ -41,9 +35,36 @@ ConsoleHandleData::~ConsoleHandleData()
{
THROW_IF_FAILED(_CloseOutputHandle());
}
else
}
// Routine Description:
// - Holds the client pointer handle for future use after we've determined that
// we have the privileges to grant it to a particular client.
// - This is separate from construction so this object can help with
// calculating the access type from the flags, but won't try to
// clean anything up until the ObjectHeader determines we have rights
// to use the object (and get it assigned here.)
// Arguments:
// - ulHandleType - specifies that the pointer given is input or output
// - pvClientPointer - pointer to the object that has an ObjectHeader
// Return Value:
// - <none> - But will throw E_NOT_VALID_STATE (for trying to set twice) or
// E_INVALIDARG (for trying to set the NotReady handle type).
void ConsoleHandleData::Initialize(const ULONG ulHandleType,
PVOID const pvClientPointer)
{
// This can only be used once and it's an erorr if we try to initialize after it's been done.
THROW_HR_IF(E_NOT_VALID_STATE, _ulHandleType != HandleType::NotReady);
// We can't be initialized into the "not ready" state. Only constructed that way.
THROW_HR_IF(E_INVALIDARG, ulHandleType == HandleType::NotReady);
_ulHandleType = ulHandleType;
_pvClientPointer = pvClientPointer;
if (_IsInput())
{
FAIL_FAST_HR(E_UNEXPECTED);
_pClientInput = std::make_unique<INPUT_READ_HANDLE_DATA>();
}
}

12
src/server/ObjectHandle.h
View file

@ -29,10 +29,11 @@ class SCREEN_INFORMATION;
class ConsoleHandleData final
{
public:
ConsoleHandleData(const ULONG ulHandleType,
const ACCESS_MASK amAccess,
const ULONG ulShareAccess,
_In_ PVOID const pvClientPointer);
ConsoleHandleData(const ACCESS_MASK amAccess,
const ULONG ulShareAccess);
void Initialize(const ULONG ulHandleType,
PVOID const pvClientPointer);
~ConsoleHandleData();
ConsoleHandleData(const ConsoleHandleData&) = delete;
@ -62,6 +63,7 @@ public:
enum HandleType
{
NotReady = 0x0,
Input = 0x1,
Output = 0x2
};
@ -73,9 +75,9 @@ private:
[[nodiscard]] HRESULT _CloseInputHandle();
[[nodiscard]] HRESULT _CloseOutputHandle();
ULONG const _ulHandleType;
ACCESS_MASK const _amAccess;
ULONG const _ulShareAccess;
ULONG _ulHandleType;
PVOID _pvClientPointer; // This will be a pointer to a SCREEN_INFORMATION or INPUT_INFORMATION object.
std::unique_ptr<INPUT_READ_HANDLE_DATA> _pClientInput;
};

12
src/server/ObjectHeader.cpp
View file

@ -39,10 +39,8 @@ ConsoleObjectHeader::ConsoleObjectHeader() :
try
{
// Allocate all necessary state.
std::unique_ptr<ConsoleHandleData> pHandleData = std::make_unique<ConsoleHandleData>(ulHandleType,
amDesired,
ulShareMode,
this);
std::unique_ptr<ConsoleHandleData> pHandleData = std::make_unique<ConsoleHandleData>(amDesired,
ulShareMode);
// Check the share mode.
if (((pHandleData->IsReadAllowed()) && (_ulOpenCount > _ulReadShareCount)) ||
@ -50,7 +48,7 @@ ConsoleObjectHeader::ConsoleObjectHeader() :
((pHandleData->IsWriteAllowed()) && (_ulOpenCount > _ulWriteShareCount)) ||
((!pHandleData->IsWriteShared()) && (_ulWriterCount > 0)))
{
RETURN_WIN32(ERROR_SHARING_VIOLATION);
return HRESULT_FROM_WIN32(ERROR_SHARING_VIOLATION);
}
// Update share/open counts and store handle information.
@ -76,6 +74,10 @@ ConsoleObjectHeader::ConsoleObjectHeader() :
_ulWriteShareCount++;
}
// Commit the object into the handle now that we've determined we have the rights to use it and have counted up appropriately.
// This way, the handle will only try to cleanup and decrement its counts after we've validated rights and incremented.
pHandleData->Initialize(ulHandleType, this);
out.swap(pHandleData);
}
CATCH_RETURN();

4
src/server/ObjectHeader.h
View file

@ -47,4 +47,8 @@ private:
ULONG _ulWriterCount;
ULONG _ulReadShareCount;
ULONG _ulWriteShareCount;
#ifdef UNIT_TESTING
friend class ObjectTests;
#endif
};