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(










      if(hPipe != NULL)




                  if(ConnectNamedPipe(hPipe, NULL))


                        TCHAR lpszCommand[1024];

                        DWORD dwRead = 0;






                              if(ReadFile(hPipe, lpszCommand, sizeof(lpszCommand), &dwRead, NULL))


                                    if(_tcscmp(lpszCommand, _T(“QUIT”)) == 0)






                                          /* PERFORM MAGICAL ACTION based on user input */


                                          DWORD dwWrote = 0;

                                          WriteFile(hPipe, lpszCommand, dwRead, &dwWrote, NULL);
















Comments (14)

  1. Marco Russo says:

    You passed NULL as lpSecurityAttributes parameter of CreateNamedPipe. Only processes running with the same user as this process can connect to this named pipe.

    Tipically, the programmer tries this code on his own machine with the same account and it works… and it works even when he log on a remote machine with the same account… but when he tries the code in a production environment or when the process become a service running with the LOCALSYSTEM account… any other user can not logon.

    You have to create this security descriptor first (allowing any user to connect):



    printf("Initializing server’s security…");

    if(bRet = InitializeSecurityDescriptor(&sd, SECURITY_DESCRIPTOR_REVISION)) {

    if(bRet = SetSecurityDescriptorDacl(&sd, TRUE, (PACL) NULL, FALSE)) {



    else {




    sa.nLength = (DWORD) sizeof(SECURITY_ATTRIBUTES);

    sa.lpSecurityDescriptor = (LPVOID) & sd;

    sa.bInheritHandle = TRUE;

    // …

    CreateNamedPipe( …. , &sa ); // Pass sa reference as last parameter

  2. Re: NULL as lpSecurityAttributes

    Absolutely a defect. But is using a NULL DACL really the right thing to do? Obviously I didn’t give enough context to know what the "right" thing is, but a NULL DACL is almost always wrong (since it offers no protection).

    But your point is correct – some DACL needs to be applied to the named pipe. The requirements of the project should determine that.


    (By the way – there are several more defects 🙂

  3. Marty Fried says:

    Without even getting to the named pipe code and security concerns, it seems like the first few actions are not very solid. Maybe it was not meant to be complete, but it returns 100 on error, but doesn’t specify a return code on success (return code random?).

    The next block blindly adds a string passes as a parameter without checking the length to make sure it will not overrun the buffer – not that buffer overruns are that important. 🙂

  4. Re: it seems like the first few actions are not very solid.

    Right on, there is a huge window for a buffer over-run in the startup code. Not returning in the success case is a very common consistency bug. People who write command line applications that don’t return proper values on success and error make life very difficult for people and applications that depend on that information (makefiles and scripting languages, for example).

  5. 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.

  6. 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.


    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!

  7. David Larsson says:

    Re: ImpersonateNamedPipeClient()

    Okay, I’ll try some more: The ImpersonateNamedPipeClient()/RevertToSelf() pair should enclose the magical action only, so ReadFile() and WriteFile() on the pipe is called in the server’s context. And ReadFile() _has_ to be called once before ImpersonateNamedPipeClient(), right?

    If RevertToSelf() fails for some bizarre reason, the server shouldn’t try to service more client requests (in the wrong context), but exit immediately.

    Finally, since we seem to be aiming for perfection with this server, perhaps we should also call FlushFileBuffers() before DisconnectNamedPipe(), so the client doesn’t miss any of our ACKs.

    This was fun!

  8. 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!

  9. Feroze Daud says:

    The defects I found are as follows:

    1) Buffer overflow1) Buffer overflow in the line "_tcscat(lpszPipeName, argv[1]);"

    2) There is no adequate protection for security attacks. For eg, there could be some other process squatting on the same pipename that this process is trying to open. The correct coding practice is to try to open the same pipe first (without creating it) and fail if it already exists.

    3) The return value of ImpersonateNamedPipeClient is not checked. If this call fails, then the operation is being done in the context of the running process (and not the client).

    4) Corollary to #3, RevertToSelf() should be checked for error return codes.

    5) The pipe names are easy to guess. They should be made difficult to guess.

  10. Re: Feroze Daud

    #1 – right, exploit waiting to happen

    #2 – that is a ok practice but still open to a DoS (try to open, fail, create … uh oh … between your test open and create someone opened it up). You still have to assume failure might happen.

    #3 – right, this is a surpsingly common mistake (and as noted in an earlier reader comment the placement of the impersonate is wrong too).

    #4 – yes, and failure quickly (and without action) as possible.

    #5 – I’m not sure what you mean here. The pipe name is not known (it’s passed in from the user). The string "\.pipe" is a mandatory prefix. Am I misunderstanding the statement?



  11. Feroze Daud says:

    5) The pipe names are easy to guess. They should be made difficult to guess.

    #5 – I’m not sure what you mean here. The pipe name is not known (it’s passed in from the user). The string "\.pipe" is a mandatory prefix. Am I misunderstanding the statement?

    I guess if the server is going to talk to many clients, it has to listen on a well known port. So, this point doesnt apply. However there are other scenarios where there might be problems due to guessable names. For eg, if you are a cmd.exe, and you use pipes to redirect stdio/err. If the pipename is guessable, and attacker could squat on that name and feed bogus input.

  12. Re: listen on a well known port.

    True. In that case the onus also falls on the client to validate that the server it connected to is the one it expects and not someone squatting on the well-known name.

    Re: you use pipes to redirect stdio/err.

    Since this can be done by the parent process using anonymous pipes is probably the safest way to go here.

    The issue of name squatting is a serious one. Being able to securely validate the that other end of the pipe is who you expect is not necessarily an easy, or fool-proof, task. Of course the complexity of the operation is going to be in relation to the risk associated with the actions being performed.