What's wrong with this code, part 2 (the answers)

So here are the intentional bugs in the “what’s wrong with this code, take two” post:

The first two bugs are quite straightforward, but insidious. I can’t think of the number of times I’ve seen code that gets this wrong. 

Reading (or writing) data from (to) a socket is like putting your mouth on the end of hose and turning on the faucet. Water flows out until it’s turned off. But there’s a valve in between you and the faucet, and someone else controls that valve. It’s possible that conditions can cause the data received to be shorter than the amount that was sent (or is shorter than the receive buffer).

Given that, the first two bugs are straightforward: This code:

       //
// Receive the byte count from the socket.
//
cbReceived = recv(socket, (char *)&cbMessage, sizeof(short), 0);
if (cbReceived == SOCKET_ERROR)
{
return WSAGetLastError();
}
 

Should really be written like:

       char lengthBuffer[sizeof(short)];
cbReceived = 0;
do
{
int cbThisReceive = recv(socket, &lengthBuffer[cbReceived], sizeof(short) - cbReceived, 0);
if (cbThisReceive == SOCKET_ERROR)
{
return WSAGetLastError();
}
else
{
cbReceived += cbThisReceive;
}
} while (cbReceived != sizeof(short));
cbMessage = *((short *)lengthBuffer);

Similarly, the code that receives the actual buffer needs to be coded to deal with data received being too short for the input buffer (I’m not going to show the corrected code, it’ll make this post too long).

The third bug’s a spec bug. Nowhere in the specification does it say what byte order is to be used for the length of the message. So if you compile the server for this code on a big endian machine, and then run the client on a little-endian machine, you’ll have a problem.

And the fourth problem’s the big one. This would result in a security hotfix.

The problem is:

       //
// Allocate a buffer to hold the network message.
//
pnetmsg = (PNETMSG)new BYTE[cbMessage + FIELD_OFFSET(NETMSG, Message)];
if (pnetmsg == NULL)
 The cbMessage value isn’t checked, and when we add FIELD_OFFSET(NETMSG, Message) to cbReceived, you have an integer overflow security vulnerability. Jerry Pisk says that it shouldn’t overflow, but if the value sent by the server causes cbMessage to be negative, then it will sign extend the value to a negative 32 bit integer and…

Credits: B.Y. and Jerry Pisk, and Mike for being the first to pick up on the two receive bugs, B.Y. and Jerry for the cbReceived security hole. BJ Herrington (‘d0x) for finding the spec problem

Now for the unintentional bugs. 

B.Y. pointed out that recv() can return 0 if the client gracefully tore down the connection. The code correctly handles that, but unnecessarily calls recv() a second time. He also pointed out that there are two code paths that don’t free the buffer.

Jerry Pisk pointed out that the allocation above should be:

       pnetmsg = (PNETMSG)new BYTE[cbMessage + FIELD_OFFSET(NETMSG, Message)];
if (pnetmsg == NULL)

The result is the same (security hole) but the effect is different (and easier to exploit). The good news is that Jerry’s bug is likely to be caught by tools like PageHeap or appverifier.

 AT also pointed another documentation error – the routine isn’t specified as working only on TCP streams, if you tried to use it on a UDP socket, it would fail.

There were also several other very valid points made during the discussion, thanks all J

I’ll probably have another of these in the future, they’re fascinating.