Spot the Bug!

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

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.

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.

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.  

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.

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.

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';

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.

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

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

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.

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


it ll never bezero

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.

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.

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.

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.

'

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

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

Web Resources





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

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;
}

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.

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;
}