Spotting Code Defects - #1 (Named Pipe Server)
When you read bad code you learn what not to do. You learn to identify the many classes of errors and the patterns that often lead up to them.
When you read good code you learn how to write good code. You observe “tricks” of good programmers (the best tricks aren’t tricks at all), you see defensive coding, pessimistic coding, efficient coding. Robust code that can run anywhere at any time and handle any situation with grace and security.
The question is – do you know the difference between good and bad code?
This is a basic pipe server. It sits around waiting for a client to connect, reads some input and performs some action (the action is not implemented, it’s not a meaningful detail), and when it’s done it write backs to the client to acknowledge that it performed the action.
To ensure that the client does not do something they don’t have permissions to do, the server impersonates the client before performing that magical action (so assume it could be registry read/write, executing a process, processing some file, etc).
While this is a console application, assume that this could be running under a privileged account on the system (e.g. member of Local Administrators).
I wish that .Text allowed me to hide the comments until I post the defects, but they don’t. So if you don’t want spoilers don’t read the comments. If you do … then do. If someone else has already posted a defect feel free to list it again (if you really identified it on your own). It’s the honor system.
The only prize is a warm fuzzy feeling you get from participating.
#include <tchar.h>
#include <windows.h>
#include <stdio.h>
int _tmain(int argc, TCHAR **argv)
{
if(argc != 2)
{
_tprintf(_T("pipeServer.exe <pipeName>\n"));
return 100;
}
TCHAR lpszPipeName[256] = _T("\\\\.\\pipe\\");
_tcscat(lpszPipeName, argv[1]);
HANDLE hPipe = CreateNamedPipe(
lpszPipeName,
PIPE_ACCESS_DUPLEX,
0,
PIPE_UNLIMITED_INSTANCES,
512,
512,
INFINITE,
NULL);
if(hPipe != NULL)
{
for(;;)
{
if(ConnectNamedPipe(hPipe, NULL))
{
TCHAR lpszCommand[1024];
DWORD dwRead = 0;
ImpersonateNamedPipeClient(hPipe);
for(;;)
{
if(ReadFile(hPipe, lpszCommand, sizeof(lpszCommand), &dwRead, NULL))
{
if(_tcscmp(lpszCommand, _T("QUIT")) == 0)
{
break;
}
else
{
/* PERFORM MAGICAL ACTION based on user input */
DWORD dwWrote = 0;
WriteFile(hPipe, lpszCommand, dwRead, &dwWrote, NULL);
}
}
}
RevertToSelf();
DisconnectNamedPipe(hPipe);
}
else
{
CloseHandle(hPipe);
break;
}
}
}
}
Comments
- Anonymous
January 19, 2004
The comment has been removed - Anonymous
January 19, 2004
The comment has been removed - Anonymous
January 19, 2004
The comment has been removed - Anonymous
January 19, 2004
The comment has been removed - Anonymous
January 19, 2004
Big thing: You're not checking the return value of ImpersonateNamedPipeClient(). If that fails, the "magical action" will be performed in the context of the service (perhaps LOCAL_SYSTEM) and NOT the client, as was the intention.
Reading the docs for ImpersonateNamedPipeClient(), I see in the Remarks section that you should determine if the caller has the SeImpersonatePrivilege priviliege. If not, the call will succeed, but only at Identify level. That could spell trouble too, I believe. (I wonder how much code actually performs that last check...)
Minor thing: You are not checking that lpszCommand is a properly null terminated string before calling _tcscmp( lpszCommand, _T("QUIT") ).
Last thing: If ReadFile() fails because of a client disconnect (without "QUIT"), the server gets stuck in a busy for(;;) loop. Sounds like DoS to me. - Anonymous
January 19, 2004
Re: ImpersonateNamedPipeClient().
That's huge, good catch. But actually it's only part of the problem. Consider this hint: ImpersonateNamedPipeClient, as called, will always fail.
Why?
--
Null termination of the string too, very important.
The ReadFile is also important, this is a DoS'able client.
hint: are there any other ways it could be DoS'd?
Thanks for playing along! - Anonymous
January 19, 2004
The comment has been removed - Anonymous
January 19, 2004
Re: ReadFile() has to be called once before ImpersonateNamedPipeClient(), right?
Bingo - that's a big mistake I've seen made. It can really only be made when not testing the return value of the impersonate call (and checking GetLastError()) because it would fail even in test environments if the test were made.
Re: If RevertToSelf() fails for some bizarre reason,
Another huge one. Even calling TerminateThread is not dangerous because it might fail (and because calling it is generally dangerous).
Re: FlushFileBuffers()
bingo - if the client is waiting on an ack this could really cause problems. This could cause a DoS on the client (blocking for the ack), could cause system instability/inconsistancy/etc.
Still a couple other bugs... :)
Thanks for playing! - Anonymous
January 19, 2004
The comment has been removed - Anonymous
January 20, 2004
The comment has been removed - Anonymous
January 21, 2004
The comment has been removed - Anonymous
January 22, 2004
The comment has been removed - Anonymous
May 29, 2009
The comment has been removed - Anonymous
June 09, 2009
PingBack from http://menopausereliefsite.info/story.php?id=1791