Spot the Bug - March 13, 2006

re: Spot the Bug - March 13, 2006

Dean Harding — Tue, 14 Mar 2006 07:40:46 GMT

The only thing I can spot is that it's assuming the client has null-terminated the string. That is, it doesn't use the value returned by ::recv.

But this is more than just a security bug. The networking layer can split the underlying packets, and if one of those packets is delayed, ::recv may only return part of the data (subsequent calls to ::recv would then return the rest of the data that was delayed).

re: Spot the Bug - March 13, 2006

Dean Harding — Tue, 14 Mar 2006 07:52:31 GMT

Oh yeah, the other point is that the ::listen can be the one to fail with WSAEADDRINUSE when you use ADDR_ANY in the ::bind. And you don't check the return value from ::listen. But if that happens, ::accept would give an error WSAEINVAL as would the subsequent ::recv.

re: Spot the Bug - March 13, 2006

Ed Kaim — Tue, 14 Mar 2006 08:16:15 GMT

To add to Dean's comment, not using the return value can cause interesting results after successive calls, even with valid data and no malicious intent. Let's say Socket_Setup is called and one person connects, sends valid data (NULL-terminated and < 1024 bytes), disconnects, it's printed, and then the method returns. Next, Socket_Setup is immediately called again and set up with the same exact stack pointer. However, on the second request, the network client exits without sending any data (such as by losing the connection due to latency). Theoretically, strData would still contain the same data it held on the last iteration, resulting in the same string being printed to the screen. There is a definite security bug here in that it would indicate the first set of data was repeated when it really wasn't. In this specific case it would just impact logging, although in a credit card system it could result in multiple charges, in a voting system it could mean extra votes, etc.

re: Spot the Bug - March 13, 2006

Dinhduy Tran — Tue, 14 Mar 2006 21:40:44 GMT

One more comment, if the strData contains more than one strings (NULL-terminated) then printf only print out the first string, other strings in the buffer will be lost.

re: Spot the Bug - March 13, 2006

Hariharan Jayaraman — Thu, 16 Mar 2006 09:31:38 GMT

1> No Error check done on accept, incase of a failure invalid socket is returned and the same has been used by recv. same is the case with listen.

2> The code does not check the return value of recv, and blindly prints the strData, by default locals char types have no gaurantee of having null termination, hence this could print the stack and eventually segfault

3> Even if error checks are done, specific care needs to be taken as it is a stream socket, like for example incase of stream sockets , even if data is there in the buffer, during the call if there is connection reset received than recv would return with zero bytes read.

4> if(0 != iFail)
{
dwErr = ::WSAGetLastError();
printf("\n\t Error occured.\n");
return;
}

In this error condition, no call to WSACleanup is done, this will lead to resource usage in the networking subsystem for the application.

5> Minor,Same thing above dwErr is not used anywhere.

re: Spot the Bug - March 13, 2006

Appa Desai — Thu, 16 Mar 2006 15:05:49 GMT

re: Spot the Bug - March 13, 2006

Ranju. V — Thu, 16 Mar 2006 19:19:36 GMT

Adding to all that has been observed so far, "recv" returns the number of bytes read from the socket. The program should probably use that information to ensure that a null terminator has been set appropriately. Something like:

len = recv( sClient, strData, 1023, 0 );
// handle error if "len" equals SOCKET_ERROR
strData[len] = '\0';

re: Spot the Bug - March 13, 2006

Dinesh — Fri, 17 Mar 2006 07:45:47 GMT

Theer are two major bugs in the code.
1) If the Bind fails we return without doing the Socket cleanup which is usually done by WSACleanup.
2) The array 'char strData[1024]' is not initialized. This can be dangerous and can be a security threat.

re: Spot the Bug - March 13, 2006

devi — Fri, 17 Mar 2006 08:41:07 GMT

I think, the same bug has been spotted already and the solution has been identified

http://blogs.msdn.com/rsamona/archive/2005/11/28/497712.aspx

re: Spot the Bug - March 13, 2006

Jayant Dusane — Fri, 17 Mar 2006 16:02:34 GMT

While result of failing the bind operation no ::WSACleanup() is called.
Also the strData for recv is not initialized with NULL.

re: Spot the Bug - March 13, 2006

Murtaza Ghiya — Mon, 20 Mar 2006 08:06:21 GMT

What I have observed with sockets on Windows is that the recv is not a blocking call and need not necessarily return the no. of bytes it has been asked to recieve. So it is very important to actually check if 1024 bytes were actually read and make the buffer null-terminated. What we usually do is write a wrapper over the recv() call to actually receive the expected no. of bytes.

re: Spot the Bug - March 13, 2006

athul h — Wed, 22 Mar 2006 02:51:29 GMT

int iFail =::bind(sTCPServer, (struct sockaddr*)&saTCPServAddr, len);
DWORD dwErr;
if(0 != iFail)

it ll never bezero

re: Spot the Bug - March 13, 2006

gaurav — Wed, 22 Mar 2006 08:01:12 GMT

Return value of WSAStartUp itself is not checked.
moreover to ensure WSACleanup is called before every exit from the function, there could be a AutoRelease class whose destructor should have a WSACleanup call.

and

While result of failing the bind operation no ::WSACleanup() is called.
Also the strData for recv is not initialized with NULL.

re: Spot the Bug - March 13, 2006

Danda Ramesh Kumar — Wed, 22 Mar 2006 08:14:18 GMT

The code does not checking the return value of recv.
incase of a failure invalid socket is returned and the same has been used by recv.

re: Spot the Bug - March 13, 2006

Black Horse — Wed, 22 Mar 2006 14:13:25 GMT

Microsoft and security? You must be joking. If you are looking for security, abandon Microsoft. Down load any distribution of Linux.

When the whole operating system and all Microsoft development / business products are buggy, have hole and have back doors (for NSA, US Gov), you must be really off your rockers trying to fool yourself with this exercise.

No, you wont put it on the site, too afraid.

re: Spot the Bug - March 13, 2006

Nagendra — Thu, 23 Mar 2006 04:26:35 GMT

Considering these lines
1. char strData[1024];
2. ::recv(sClient, strData, 1024, 0);
3. printf("\n\nRealServer--Data from client --- %s ---", strData);

It think a hacker can override the no 1024 in the line 2 above and this can push his own code to execute since the buffer strData just has 1024 bytes to store , and all the data overflown might become executable code.

re: Spot the Bug - March 13, 2006

~!@#$%^&*() — Thu, 23 Mar 2006 14:01:24 GMT

re: Spot the Bug - March 13, 2006

''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''' — Thu, 23 Mar 2006 14:03:22 GMT

'''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''

re: Spot the Bug - March 13, 2006

Karthik Narasimhan — Mon, 27 Mar 2006 19:30:27 GMT

Hi,

I found the following 2 items as probable bugs in this function.
1] INADDR_ANY :-
When receiving, a socket bound to this address receives packets from all interfaces show have used a specific ip address rather.

2] Port # 5678
A known exploit is already available as described below:
Port 5678
(TCP+UDP) A port for remote execution using the crexd/srexd services.
(TCP+UDP) A frequent port some picks at random.

(TCP+UDP) Port 5678 was originally specified for the PPTP protocol, but when the standard was ratified, port 1723 was chosen instead.

(TCP) Port 5678 is the default port for the com.hp.util.rcat Java package (from Hewlett-Packard). This is a simple debugging package.

(UDP) osagent communication

That's all from my side.
Thanks & have a nice day.
From,
Karthik Narasimhan

MSDN Flash Ireland Resources - 31 Mar 06

Robert Burke's Weblog — Fri, 31 Mar 2006 19:17:27 GMT

Web Resources

[.NET Framework] GotDotNet CodeGallery
Share, find, download and discuss evolving...

re: Spot the Bug - March 13, 2006

Manoj Dalei — Tue, 04 Apr 2006 10:11:58 GMT

Hi,
here is the revised program.

void Socket_Setup(void)
{
WORD wVersionRequested;
WSADATA wsaData;
wVersionRequested = MAKEWORD( 2, 2 );

if( ::WSAStartup(wVersionRequested, &wsaData) !=0)
{
dwErr = ::WSAGetLastError();
printf("\n\t Error occured.\n");
return;
}
float socklib_ver; /* socket dll version */
socklib_ver = HIBYTE( wsaData.wVersion ) / 10.0;
socklib_ver += LOBYTE( wsaData.wVersion );

if ( socklib_ver < 2.0 )
{
::WSACleanup(); /* clean up before exit */
return ;
}

SOCKET sTCPServer = ::socket(AF_INET, SOCK_STREAM, 0);
if(sTCPServer == INVALID_SOCKET)
{
::WSACleanup(); /* clean up before exit */
return ;
}
struct sockaddr_in saTCPServAddr;
saTCPServAddr.sin_family = AF_INET;
saTCPServAddr.sin_addr.S_un.S_addr = ::htonl(INADDR_ANY);
saTCPServAddr.sin_port = ::htons(5678);
int len = sizeof(saTCPServAddr);

int iFail =::bind(sTCPServer, (struct sockaddr*)&saTCPServAddr, len);
DWORD dwErr;
if(0 != iFail)
{
dwErr = ::WSAGetLastError();
printf("\n\t Error occured.\n");
::closesocket(sTCPServer);
::WSACleanup(); /* clean up before exit */
return;
}

iFail = ::listen(sTCPServer, 2);

struct sockaddr_in saClient;
int iClsize = sizeof(saClient);
SOCKET sClient = ::accept(sTCPServer, (struct sockaddr*)&saClient ,&iClsize);
if(sClient == INVALID_SOCKET)
{
::closesocket(sTCPServer);
::WSACleanup(); /* clean up before exit */
return ;
}
char strData[1024];
u_long arg = 0;
if (::ioctlsocket(sClient, FIONREAD, &arg) == 0)
{
if (arg > 0)
{
if (arg > 1024)arg = 1024;
int rv = ::recv(sClient, strData, arg, 0);
printf("\n\nRealServer--Data from client --- %s ---", strData);
}
}
::shutdown(sTCPServer, SD_BOTH);
::WSACleanup();
return;
}

re: Spot the Bug - March 13, 2006

Ashish Singhal — Tue, 04 Apr 2006 14:32:31 GMT

The problem I see is with the size of sClient i.e. 1024. The socket will wait for a client to connect and let it send a buffer. Assuming null termination of the string has been taken care of.
If the client sends a buffer larger than 1024 bytes, the code will terminate abruptly and result in a crash, very coomonly referred to as "Buffer overflow". These attacks are primarily used for Denial of service scenario's.
We should add a check i.e. if the received bytes (value returned by recv) is greater than the buffer we allocated the bytes be dropped or the buffer size is increased correspondingly.

re: Spot the Bug - March 13, 2006

TSS — Tue, 11 Apr 2006 12:46:40 GMT

Bugs? Bugs! =)
5 bugs, 1 critical and one potentional dangerous condition
I'm review code and add some IMHO useful comments =)

void Socket_Setup(void)
{
WORD wVersionRequested;
WSADATA wsaData;
wVersionRequested = MAKEWORD( 2, 2 );
// BUG_1: Return value check
if (0 != ::WSAStartup(wVersionRequested, &wsaData))
// handle error here...

SOCKET sTCPServer = ::socket(AF_INET, SOCK_STREAM, 0);
// BUG_2: Return value check
if (INVALID_SOCKET == sTCPServer)
// handle error here...

struct sockaddr_in saTCPServAddr;
saTCPServAddr.sin_family = AF_INET;
saTCPServAddr.sin_addr.S_un.S_addr = ::htonl(INADDR_ANY);
saTCPServAddr.sin_port = ::htons(5678);
int len = sizeof(saTCPServAddr);

int iFail =::bind(sTCPServer, (struct sockaddr*)&saTCPServAddr, len);
DWORD dwErr;
// if(0 != iFail)
// BUG_3: Check for SOCKET_ERROR, not for 0
if (SOCKET_ERROR == iFail)
{
dwErr = ::WSAGetLastError();
printf("\n\t Error occured.\n");
return;
}

// BUG_4: Return value check
iFail = ::listen(sTCPServer, 2);
if (SOCKET_ERROR == iFail)
// handle error here

struct sockaddr_in saClient;
int iClsize = sizeof(saClient);
SOCKET sClient = ::accept(sTCPServer, (struct sockaddr*)&saClient ,&iClsize);
// BUG_5: Return value check
if (INVALID_SOCKET == sClient)
// handle error here

// BUG_5: Critical !!!. Buffer overflow here!
char strData[1024];
// ::recv(sClient, strData, 1024, 0);
int nBytesReceived = ::recv (sClient, strData,
sizeof (strData) - 1, 0); // !!!
if (nBytesReceived >= 0) // !!!
strData[nBytesReceived] = '\0'; // !!!

// Minor bug: trying to dump out binary information on console...
printf("\n\nRealServer--Data from client --- %s ---", strData);

::shutdown (sTCPServer, SD_BOTH);

// BUG_6: Resourses leak:
closesocket (sClient);
closesocket (sTCPServer);

::WSACleanup();

return;
}

Spot the Bug Spot the Bug March 13 2006 | adirondack chairs

Spot the Bug Spot the Bug March 13 2006 | adirondack chairs — Fri, 19 Jun 2009 12:00:22 GMT

PingBack from http://adirondackchairshub.info/story.php?id=3062

Spot the Bug Spot the Bug March 13 2006 | patio cushions

Spot the Bug Spot the Bug March 13 2006 | patio cushions — Fri, 19 Jun 2009 13:10:57 GMT

PingBack from http://patiocushionsource.info/story.php?id=1846