SOLUTION: Spotting Code Defects - #1 (Named Pipe Server)
So the results are back – and that was some buggy code!
First I want to thank everyone who played along. Both those that had the courage to post to the public comments and those that replied offline.
Also - before I go further - is this something people would like to see done again?
There are no fewer then 12 defects (several critical), several design issues and still a few questions left to be answered.
I’m glad to see that most of the critical defects were identified however there were a few that were missed. I’ll summarize those:
1) ConnectNamedPipe might lie! If ConnectNamedPipe fails (returns FALSE) it still might have actually connected. You need to check GetLastError() == ERROR_PIPE_CONNECTED to find out if it actually succeeded. This will happen when a client connects to a pipe after CreateNamedPipe but before ConnectNamedPipe is called (I see a lot when debugging).
2) If DisconnectNamedPipe fails we need to abort. If we can’t disconnect then we can’t reconnect and our state is all screwed up.
3) We weren’t closing the pipe handle except in error conditions (this is actually more before we weren’t giving ourselves any way to leave the loop, really a design issue too).
The other defects were:
1) Chance for buffer overflow when appending the pipe name together. Can you say “owned”?
2) We need to provide a reasonable security descriptor when creating the pipe
3) Must test the return value of ImpersonateNamedPipeClient (and handle appropriately)
4) Must only call ImpersonateNamedPipeClient after performing a data transfer on the pipe (a read, in our case)
5) Must only impersonate the magical action
6) Must test the success of RevertToSelf and abort very quickly on failure.
7) Must test the result of our WriteFile on the ACK and close the connection if we fail.
8) Flush the file buffers when (gracefully) closing the connection.
9) Return a reasonable error code at the program’s exit.
There, at least 12 issues.
A 13th issue was brought up by David Larsson. He noted that the SeImpersonatePrivilage must be present on the caller (the service) otherwise the impersonate might succeed but only at the identity level. It’s really not hard to check for privileges – consider something like this:
HRESULT haveImpersonatePrivilege(HANDLE hProcess, LPBOOL fResult)
{
if(!fResult)
{
return E_INVALIDARG;
}
HANDLE hToken = NULL;
HRESULT hr = E_FAIL;
if(OpenProcessToken(hProcess, TOKEN_QUERY, &hToken))
{
PRIVILEGE_SET privImpersonate = {0};
privImpersonate.PrivilegeCount = 1;
privImpersonate.Control = PRIVILEGE_SET_ALL_NECESSARY;
if(LookupPrivilegeValue(NULL, _T("SeImpersonatePrivilege"), &privImpersonate.Privilege[0].Luid))
{
if(!PrivilegeCheck(hToken, &privImpersonate, fResult))
{
hr = HRESULT_FROM_GetLastError();
}
}
else
{
hr = HRESULT_FROM_GetLastError();
}
CloseHandle(hToken);
}
else
{
hr = HRESULT_FROM_GetLastError();
}
return hr;
}
Basically open your token, build your privilege set and check your token against the set.
However in our case I didn’t worry about it. I could assert that we had the privilege but another thread could adjust our token privilege between my check and the action. Short of using a critical section to make the operations atomic there is no way to assert the success – and since it is not a privilege elevation risk I’m content letting it ride (until the magical action is well defined, and then maybe it matters more).
So what does our new code look like?
I made the changes based on reader feedback, some personal design choices and the list of defects.
Is it perfect? Probably not. But it’s better.
Will it have defects? If it does please call me on it. These exercises are for my education as much as anything else! J
#include <tchar.h>
#include <windows.h>
#include <stdio.h>
#include <strsafe.h>
HRESULT createReasonableSD(SECURITY_DESCRIPTOR *sd)
{
/// create reasonable SD for intented action
}
void TRACE_Error(HRESULT hr)
{
// do tracing actions...
}
inline HRESULT HRESULT_FROM_GetLastError()
{
DWORD dw = GetLastError();
return HRESULT_FROM_WIN32(dw);
}
const int PIPE_BUFFER_SIZE = 1024
const int PIPE_MAX_NAME_LENGTH = 256;
int _tmain(int argc, TCHAR **argv)
{
if(argc != 2)
{
// DESIGN ISSUE: not fixing in example, but remove to avoid loading the CRT.
_tprintf(_T("pipeServer.exe <pipeName>\n"));
// DESIGN ISSUE: use standard return codes.
return EXIT_FAILURE;
}
HRESULT hr = E_FAIL;
// DESIGN ISSUE: avoid magic numbers (was 256).
// This value is documented in MSDN.
TCHAR lpszPipeName[PIPE_MAX_NAME_LENGTH] = _T("\\\\.\\pipe\\");
// DEFECT: use strsafe library functions for string actions.
// this enforces bounds checking and promotes error checking
hr = StringCbCat(lpszPipeName, sizeof(lpszPipeName), argv[1]);
if(FAILED(hr))
{
// DESIGN ISSUE: trace errors for debugging assistance
TRACE_Error(hr);
return EXIT_FAILURE;
}
// DEFECT: use a reasonable security descriptor.
// a NULL DACL is not generally reasonable.
SECURITY_DESCRIPTOR sd;
hr = createReasonableSD(&sd);
if(SUCCEEDED(hr))
{
SECURITY_ATTRIBUTES sa = {0};
sa.nLength = sizeof(SECURITY_ATTRIBUTES);
sa.lpSecurityDescriptor = &sd;
sa.bInheritHandle = TRUE;
HANDLE hPipe = CreateNamedPipe(
lpszPipeName,
PIPE_ACCESS_DUPLEX,
0,
PIPE_UNLIMITED_INSTANCES,
512,
512,
INFINITE,
&sa);
if(hPipe != INVALID_HANDLE_VALUE)
{
// DESIGN ISSUE: use conditional loops, provide some means of graceful escape.
bool continueAcceptingConnections = true;
while(continueAcceptingConnections)
{
BOOL bConnected = ConnectNamedPipe(hPipe, NULL);
// DEFECT: there is an edge case with ConnectNamedPipe.
// If it returns false, but GetLastError == ERROR_PIPE_CONNECTED
// then everything actually worked.
if(!bConnected)
{
DWORD dwError = GetLastError();
if(ERROR_PIPE_CONNECTED == dwError)
{
bConnected = TRUE;
}
else
{
hr = HRESULT_FROM_WIN32(dwError);
TRACE_Error(hr);
continueAcceptingConnections = false;
}
}
if(bConnected)
{
// DESIGN ISSUE: avoid magic numbers. (was 1024).
TCHAR lpszCommand[PIPE_BUFFER_SIZE];
DWORD dwRead = 0;
// DESIGN ISSUE: conditional loop rather then infinite with breaks.
bool continueReading = true;
while(continueReading)
{
if(ReadFile(hPipe, lpszCommand, sizeof(lpszCommand), &dwRead, NULL))
{
// DESIGN ISSUE: I prefer the Win32 lstrcmp to _tcscmp.
// especially when this can mean not loading the CRT.
if(lstrcmp(lpszCommand, _T("QUIT")) == 0)
{
continueReading = false;
}
else
{
// DEFECT: Test return value of INPC
// DEFECT: Only call INPC after data transfer on the pipe
// DEFECT: Only impersonate the magical action
if(ImpersonateNamedPipeClient(hPipe))
{
/* PERFORM MAGICAL ACTION based on user input */
// DEFECT: Test return value and exit
// without additional action on failure.
if(!RevertToSelf())
{
exit(EXIT_FAILURE);
}
// DEFECT: test return value of ACK and handle failure
DWORD dwWrote = 0;
if(!WriteFile(hPipe, lpszCommand, dwRead, &dwWrote, NULL))
{
hr = HRESULT_FROM_GetLastError();
TRACE_Error(hr);
continueReading = false;
}
}
else
{
hr = HRESULT_FROM_GetLastError();
TRACE_Error(hr);
continueReading = false;
}
}
}
}
// DEFECT: flush file buffers before disconnecting the pipe
if(!FlushFileBuffers(hPipe))
{
hr = HRESULT_FROM_GetLastError();
TRACE_Error(hr);
}
// DEFECT: if you can't disconnect the pipe then gracefully quit.
if(!DisconnectNamedPipe(hPipe))
{
hr = HRESULT_FROM_GetLastError();
TRACE_Error(hr);
continueAcceptingConnections = false;
}
}
}
// DEFECT: Close the handle in success, as well as error, conditions
if(!CloseHandle(hPipe))
{
TRACE_Error(HRESULT_FROM_GetLastError());
}
}
else
{
TRACE_Error(HRESULT_FROM_GetLastError());
}
}
// DEFECT: return a reasonable error code at program end.
return SUCCEEDED(hr) ? EXIT_SUCCESS : EXIT_FAILURE;
}
Valuable?
Waste of time?
Let me know!
Thanks.