Jump to content
  • Advertisement
Sign in to follow this  
Vortez

The fear of networking...

This topic is 2632 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

Hi, im starting this thread because i have a friend that is afraid to play my games that use windows socket on is real computer, he install them in vmware, but it's not as fast and it's a bit buggy sometime, especially when calling glReadPixel for picking, if i don't start reading at (0, 0) the result is all messed up so i have to make a workaround to read the entire screen instead of one pixel so it's really slow this way. The thing is, i've been programming for a long time and i always use class to manage stuff like my networking code, wich i always make sure to close the socket in the destructor if it haven't been already closed. Also im pretty sure that windows take care of closing the socket when the process terminate, normally or not (a crash for example). Can anyone explain to him that it's not dangerous at all plz.

Ill also post the most relevant code (initialization/cleanup part) of my class, so you'll see how im doing it... The server code is on top and the client one, down below:


#include "VortezNetworkEngine.h"

bool IsWinsockInitialized = false;

//-----------------------------------------------------------------------------
// Name : DllMain() (DllMain Entry Point)
//-----------------------------------------------------------------------------
BOOL WINAPI DllMain(HINSTANCE hInst, DWORD fdwreason, LPVOID lpReserved )
{
switch(fdwreason)
{
case DLL_PROCESS_ATTACH: break;
case DLL_PROCESS_DETACH: break;
case DLL_THREAD_ATTACH: break;
case DLL_THREAD_DETACH: break;
}
return TRUE;
}

/*****************************************************************************************/
/*****************************************************************************************/
/*****************************************************************************************/

bool InitializeWinsock()
{
if(!IsWinsockInitialized){
WSADATA wsaData;
IsWinsockInitialized = WSAStartup(MAKEWORD(1,1), &wsaData) == 0;
}

return IsWinsockInitialized;
}

void ShutdownWinsock()
{
if(IsWinsockInitialized){
WSACleanup();
IsWinsockInitialized = false;
}
}

/*****************************************************************************************/
/*****************************************************************************************/
/*****************************************************************************************/

//-----------------------------------------------------------------------------
// Constructor...
//-----------------------------------------------------------------------------
CVortezNetworkServer::CVortezNetworkServer()
{
m_HostWnd = NULL;
m_Socket = INVALID_SOCKET;
m_MaxPlayers = 2;
ZeroMemory(&m_VerNum[0], MAX_VERNUM_LEN);
ZeroMemory(&m_LoginPass[0], MAX_LOGIN_PASS_LEN);
}

//-----------------------------------------------------------------------------
// Destructor...
//-----------------------------------------------------------------------------
CVortezNetworkServer::~CVortezNetworkServer()
{
Disconnect();
}

//-----------------------------------------------------------------------------
// Listen to an incoming connection on the selected port
//-----------------------------------------------------------------------------
bool CVortezNetworkServer::StartServer(HWND hWnd, short nPort, char *pLoginPass, UINT MaxPlayers, char *pVerNum)
{
if(m_Socket != INVALID_SOCKET)
return false;

// Save the host windows handle
m_HostWnd = hWnd;

// Save the number of maximum clients connections allowed
m_MaxPlayers = MaxPlayers;

// Erase and save the client server version
ZeroMemory(&m_VerNum[0], MAX_VERNUM_LEN);
strcpy(&m_VerNum[0], &pVerNum[0]);

// Erase and save the server password
ZeroMemory(&m_LoginPass[0], MAX_LOGIN_PASS_LEN);
strcpy(&m_LoginPass[0], &pLoginPass[0]);

// Create a TCP/IP stream socket to "listen" with
m_Socket = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);

// Fill in the server address structure
SOCKADDR_IN saServer;
saServer.sin_family = AF_INET;
saServer.sin_addr.s_addr = INADDR_ANY;
saServer.sin_port = htons(nPort);

// Bind socket
if(bind(m_Socket, (LPSOCKADDR)&saServer, sizeof(struct sockaddr)) == SOCKET_ERROR){
closesocket(m_Socket);
return false;
}

// Make the socket asyncronous
WSAAsyncSelect(m_Socket, m_HostWnd, ON_WINSOCK_MESSAGE, (FD_ACCEPT | FD_READ | FD_CLOSE));

// Listen for incoming connection
if(listen(m_Socket, m_MaxPlayers) == SOCKET_ERROR){
closesocket(m_Socket);
return false;
}

return true;
}

//-----------------------------------------------------------------------------
// Disconnect clients
//-----------------------------------------------------------------------------
void CVortezNetworkServer::DisconnectClients()
{
//Disconnect the server's clients
int NumNodes = SocketList.GetNodeCount();
for(int Cpt = NumNodes - 1; Cpt >= 0; Cpt--){
// Get the last node
CSocketNode *pNode = SocketList.GetNode(Cpt);
// Close this socket
closesocket(pNode->Socket);
OnClientDisconnected(pNode);
// Delete the node
SocketList.DeleteNode(Cpt);
}
}

//-----------------------------------------------------------------------------
// Disconnect from the network
//-----------------------------------------------------------------------------
void CVortezNetworkServer::Disconnect()
{
// Disconnect every clients connected
DisconnectClients();

//Disconnect the server
if(m_Socket != INVALID_SOCKET){
closesocket(m_Socket);
OnServerDisconnected(m_Socket);

m_HostWnd = NULL;
m_Socket = INVALID_SOCKET;
m_MaxPlayers = 2;
ZeroMemory(&m_LoginPass[0], MAX_LOGIN_PASS_LEN);
}
}

//***************************************************************************//

//-----------------------------------------------------------------------------
// Constructor...
//-----------------------------------------------------------------------------
CVortezNetworkClient::CVortezNetworkClient()
{
m_HostWnd = NULL;
m_Socket = INVALID_SOCKET;
m_PlayerName.Allocate(MAX_PLAYER_NAME_LEN);
m_LoginPass.Allocate(MAX_LOGIN_PASS_LEN);
m_VerNum.Allocate(MAX_VERNUM_LEN);
}

//-----------------------------------------------------------------------------
// Destructor...
//-----------------------------------------------------------------------------
CVortezNetworkClient::~CVortezNetworkClient()
{
Disconnect();
m_VerNum.Free();
m_LoginPass.Free();
m_PlayerName.Free();
}

//-----------------------------------------------------------------------------
// Start a thread that attemp to connect with the server
//-----------------------------------------------------------------------------
DWORD WINAPI ConnectToServerThread(void *param)
{
CVortezNetworkClient *pClient = (CVortezNetworkClient*)param;
HWND hWnd = pClient->GetHostWnd();
SOCKET *pSocket = pClient->GetSocketPtr();

// Get the number of attemp we shall make
const int NumConnAtmp = 15;
// Not supposed to happen, but better be safe to avoid an infinite loop
if(NumConnAtmp < 0){return 0;}

// Loop the number of requested times
for(int Cpt = 0; Cpt < NumConnAtmp; Cpt++){

// Get the time
DWORD Time = GetTickCount();

// Try to connect
int Res = connect(*pSocket, (SOCKADDR*)pClient->GetSockAddrInPtr(), sizeof(struct sockaddr));
if(Res == SOCKET_ERROR){

// Break the loop when we receive the stop event message
if(pClient->Thread.MustExitThread()){
closesocket(*pSocket);
return 0;
}

// Test if we must abort the connection loop...
bool LastTry = Cpt + 1 == NumConnAtmp;

// Couldn't connect
if(LastTry){
closesocket(*pSocket);
return 0;
}

} else
break; // Break the connect loop if we are connected

// Wait until at least 1 sec have passed...
DWORD ElapsedTime = GetTickCount() - Time;
while(ElapsedTime < 1000){
Sleep(50);
ElapsedTime = GetTickCount() - Time;
}
}

// Set the socket n asyncronous mode (non-blocking)
WSAAsyncSelect(*pSocket, hWnd, ON_WINSOCK_MESSAGE, (FD_READ | FD_CLOSE));

// Simulate a FD_CONNECT message, since we've done it ourselve
SendMessage(hWnd, ON_WINSOCK_MESSAGE, *pSocket, (LPARAM)(DWORD)FD_CONNECT);

return 0;
}


//-----------------------------------------------------------------------------
// Listen to an incoming connection on the selected port
//-----------------------------------------------------------------------------
void CVortezNetworkClient::ConnectToServer(HWND hWnd, char *ip, short nPort, char *pPlayerName, char *pLoginPass, char *pVerNum)
{
if(m_Socket != INVALID_SOCKET)
return;

// Save the host windows handle
m_HostWnd = hWnd;

// Erase and save the player's name and login password
m_PlayerName.Erase();
strcpy(m_PlayerName.GetStrBuffer(), &pPlayerName[0]);
m_LoginPass.Erase();
strcpy(m_LoginPass.GetStrBuffer(), &pLoginPass[0]);
m_VerNum.Erase();
strcpy(m_VerNum.GetStrBuffer(), &pVerNum[0]);

///////////////////////////////////////////////////////////////

// Create a TCP/IP stream socket to "listen" with
m_Socket = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);

// Fill in the address structure
ZeroMemory(&saServer, sizeof(SOCKADDR_IN));
saServer.sin_family = AF_INET;
saServer.sin_addr.s_addr = inet_addr(ip);
saServer.sin_port = htons(nPort);

//////////////////////////////////////////////

//WSAAsyncSelect(m_Socket, m_HostWnd, ON_WINSOCK_MESSAGE, (FD_READ | FD_CONNECT | FD_CLOSE));
//connect(m_Socket, (SOCKADDR*)&saServer, sizeof(struct sockaddr));

Thread.StartThread(ConnectToServerThread, (void*)this, "ConnectToServerThreadEvent");
}

//-----------------------------------------------------------------------------
// Disconnect from the network
//-----------------------------------------------------------------------------
void CVortezNetworkClient::Disconnect()
{
// Wait for the connection thread to finish if he's running...
if(Thread.IsThreadRunning())
Thread.StopThread();

//Disconnect the server
if(m_Socket != INVALID_SOCKET){
closesocket(m_Socket);
OnDisconnect(m_Socket);
}

m_HostWnd = NULL;
m_Socket = INVALID_SOCKET;
m_PlayerName.Erase();
m_LoginPass.Erase();
}


As you can see, both client and server call they're respective Disconnect() method in the destructor, although i don't rely on this in my game and call disconnect anyway before closing the application, so i can't really see what could go wrong... Hell im pretty sure even if i would not call disconnect at all it wouldn't put his computer at risk for one bit. Plz tell me(or rather him) im right?

Share this post


Link to post
Share on other sites
Advertisement
Sadly, your friend is correct to be worried about the security of your code.

For instance, you have several trivial buffer overflow vulnerabilities in the snippet posted. A skilled attacker could potentially gain complete control of a computer running your program.

Share this post


Link to post
Share on other sites
How could it be? I always make sure that nothing could be written past the limit of those buffers in every recv calls. Also, i can limit the number of peer that connect to the server, in my 2 main games, i limit this to 2 players max, and i know he's not playing with anyone else exept me. I don't mean to be rude, but if i follow your logic, every program that use a byte array as a buffer are flawed?? How do you want to exchange data without a buffer?

Share this post


Link to post
Share on other sites
What i mean is, no one except me and him have this game, so i was more looking foward to reassure him that all the socket stuff are closed properly no matter what happen. Now if i show him this thread... lol

Im doing some sort of login procedure when someone connect to the server too, so if someone try to connect using a random socket and don't give the correct password, which is of fixed size, and enter stuff like it should, the server will reject the connection.

Share this post


Link to post
Share on other sites

[color="#660066"]WSAStartup[color="#666600"](MAKEWORD[color="#666600"]([color="#006666"]1[color="#666600"],[color="#006666"]1[color="#666600"]), [color="#666600"]&wsaData[color="#666600"])


The only reason to not pass 2,2 to WSAStartup is if you're worried about Windows 3.11 compatibility (16-bit platforms). Versions of WinSock less than 2,2 have significant flaws that you don't normally want to import into your application for modern platforms.


Yes, under NT-based Windows (NT 4, Windows 2000, Windows XP, and up) the kernel will close any handles that a process has open when that process terminates. This includes socket handles if they haven't been closed already. Sockets are NOT handles on DOS-based Windows (Win95, Win98, WinME etc) and thus you should close them yourself, if you care about those.



[color="#660066"]WSAAsyncSelect[color="#666600"](m_Socket[color="#666600"], m_HostWnd[color="#666600"], ON_WINSOCK_MESSAGE[color="#666600"], [color="#666600"](FD_ACCEPT [color="#666600"]| FD_READ [color="#666600"]| FD_CLOSE[color="#666600"]));
[/quote]


WSAAsyncSelect() is likewise terrible to use for high-performance networking (and, in fact, for any networking at all). It has all kinds of quirks you wouldn't expect (read all the MSDN documentation very carefully!) and after all that, it's pretty low performance. You would be much better off just using select() and recv()/send(). You would be even better off using I/O completion ports and a worker thread that simply posts received data into a FIFO queue of some sort shared with your main thread, and perhaps posts a message if you feel a need to wake that thread up. (Generally, you won't, because you'll pick up the network packet on the next time through your game loop anyway)


I don't see any obvious buffer overruns in the code you posted, assuming the strings for user name and whatnot are well checked for size before you call into your class. However, I also don't see your handlers for receiving and sending data, so it's hard to tell. Also, you are using string objects where you manually call "allocate" functions to allocate space, and "free" functions to release space. That's not very good design, because you may very well forget to call one or the other, or one of the functions in the middle will throw an exception and leave some object forever allocated. Given that members are constructed as part of the containing class, and destroyed after the containing class' destructor, you can just allocate and release your memory in the constructor/destructor of your buffer objects. Although, because you already know the size (it's constant), you're better off just using a fixed buffer, such as through a template size_t argument for example, if for some reason std::vector and std::string aren't good enough for you.


So, that's the advice I can give based on the code you posted and the 5 minutes I took to review it. Good luck!

Share this post


Link to post
Share on other sites
Not closing sockets is a non-issue. If you leave a socket dangling somehow, the worst that can happen is someone attempts to connect and doesn't get any response. No big deal. This has basically zero bearing on how secure your program is.

What's much more important is what happens if someone sends garbage data (or, worse, carefully crafted data) to your program while it's running. They don't need to have a copy of your code or even know what program it is; a trivial port scan can tell an attacker that you have something running, at which point they can begin exploring the protocol and doing nasty things to your program. Judging by the code you've posted, I would bet that you have no security in place to prevent a random attacker from sending you malformed data - or, in the best case, you probably just close the connection without recording or rate-limiting it in any way. This scenario means you have a false sense of security; someone could easily probe your program over the course of hours or even days without you having any clue, until suddenly it crashes on you (or worse).

You didn't post your socket-read calls so I don't know for sure, but it seems like it would be trivial to pass a carefully constructed buffer to, say, the login code; since you do no safety checking of your buffers in the posted code, I'm just extrapolating that you similarly have no safety in the actual socket communication code either, or that (worst case) you think you have safety which is in fact easy to circumvent. For instance, what happens if I just send you 100KB of random bytes with no NULLs in it? Your strcpy() style calls will gleefully tromp all over the buffers because you don't tell them not to.


Anything which involves an unchecked buffer copy is a security risk, even if you think it's protected by some other layer of code. All it takes is a minor oversight to expose that code path to something an outside party can exploit. And if you're lax about buffer overflows in your code as shown here, what's to say you're not making similar mistakes elsewhere?

And buffer overflows, while one of the easier attack vectors to exploit in many cases, are sadly just the beginning of security vulnerabilities.


I don't mean this as a slight against you in any way, but your friend has the right idea in not trusting your code. Security is very hard, and it's perfectly reasonable for him to be cautious.

Share this post


Link to post
Share on other sites
Thanks for anwsering me both of you, the reason i didn't post the recv/send code is because it's a pretty overwelming bit of code. I've written it when i was "in the zone" 2 years ago and now i don't remember everything on the top of my head. The reason im using FD_READ is because my games don't requires very high networking performance, i have a chess game and a card game that use it. In another win32 project, i indeed avoid this and use select to speed things up.

Also, i didn't know there was much difference with winsock 1.1 and 2.2, i guess i could just change it without changing any code?

As for the buffer object in my code, im using it because i can free itself in the destructor in the unlikely even that i forget, and also calling allocate twice on it could have 2 effects, if the requested size is the same as the current allocated size, it simply erase it, otherwise it delete the old one and create a new one, then zeromemory it. So as you can see, it's pretty safe.

As for my read/recv buffer, i'll do my best to explain how i've implemented it. First off, i have this class to handle sending and receving a message, in bytes, of any length:

Packet.h

#ifndef _CPACKET_H_
#define _CPACKET_H_
#ifdef __cplusplus
//----------------------------------------------------------------------//
#ifndef EXP_FUNC
#define EXP_FUNC __declspec(dllexport)
#endif
//----------------------------------------------------------------------//
#include "Windows.h"
//----------------------------------------------------------------------//
#include <SafeKill.h>
//----------------------------------------------------------------------//
#define MAX_PACKET_SIZE 1024
//----------------------------------------------------------------------//
#define PACKET_HEADER_SIZE 8
//----------------------------------------------------------------------//
#define MSG_HEADER_BUFFER 1
#define MSG_DATA_BUFFER 2
//----------------------------------------------------------------------//

struct EXP_FUNC PacketHeaderStruct {
DWORD MsgSize;
DWORD MsgID;
};

struct EXP_FUNC CReadResult {
UINT BytesReaden;
BOOL IsPacketLimitReach;
BOOL IsBufferLimitReach;
BOOL Succes;
};

//----------------------------------------------------------------------//
//----------------------------------------------------------------------//

class EXP_FUNC CPacket {
public:
CPacket();
private:
DWORD HeaderIndx;
DWORD DataIndx;

void EraseDataBuffer();
void FreeDataBuffer();
public:
PacketHeaderStruct Header;
BYTE *pHeader;
BYTE *pData;

void Init();
void FillHeader(DWORD dwMsgSize, DWORD dwMsgID);

void AllocateDataBuffer();

bool IsHeaderDoneProcessing();
bool IsDataDoneProcessing();

DWORD GetHeaderIndx() {return HeaderIndx;}
DWORD *GetHeaderIndxPtr() {return &HeaderIndx;}
DWORD GetDataIndx() {return DataIndx;}
DWORD *GetDataIndxPtr() {return &DataIndx;}
//void IncDataIndx(DWORD Indx){DataIndx += Indx;}
};
//----------------------------------------------------------------------//
//----------------------------------------------------------------------//
#endif
#endif //_CPACKET_H_


Packet.cpp

#include "Packet.h"

//-----------------------------------------------------------------------------
// Constructor
//-----------------------------------------------------------------------------
CPacket::CPacket()
{
pHeader = (BYTE*)&Header;
pData = NULL;
Init();
}

//-----------------------------------------------------------------------------
// Initialise the object
//-----------------------------------------------------------------------------
void CPacket::Init()
{
HeaderIndx = 0;
DataIndx = 0;
Header.MsgSize = 0;
Header.MsgID = 0;
FreeDataBuffer();
}

//-----------------------------------------------------------------------------
// Allocate our Data buffer
//-----------------------------------------------------------------------------
void CPacket::AllocateDataBuffer()
{
if(!pData && IsHeaderDoneProcessing()){
pData = new BYTE[Header.MsgSize];
EraseDataBuffer();
}
}

//-----------------------------------------------------------------------------
// 0's out the buffer
//-----------------------------------------------------------------------------
void CPacket::EraseDataBuffer()
{
if(pData && IsHeaderDoneProcessing())
ZeroMemory(&pData[0], Header.MsgSize);
}

//-----------------------------------------------------------------------------
// Free our Data buffer
//-----------------------------------------------------------------------------
void CPacket::FreeDataBuffer()
{
SAFE_DELETE_ARRAY(pData);
}

//-----------------------------------------------------------------------------
// Used to fill the Header structure
//-----------------------------------------------------------------------------
void CPacket::FillHeader(DWORD dwMsgSize, DWORD dwMsgID)
{
Header.MsgSize = dwMsgSize;
Header.MsgID = dwMsgID;
}

//-----------------------------------------------------------------------------
// Tell if we have recv/send all the Header bytes
//-----------------------------------------------------------------------------
bool CPacket::IsHeaderDoneProcessing()
{
return HeaderIndx == PACKET_HEADER_SIZE;
}

//-----------------------------------------------------------------------------
// Tell if we have recv/send all the Data bytes
//-----------------------------------------------------------------------------
bool CPacket::IsDataDoneProcessing()
{
return (IsHeaderDoneProcessing()) && (DataIndx == Header.MsgSize);
}


Then, i have another class that manage this packet class automatically

MsgReader.h

#ifndef _MSG_READER_H_
#define _MSG_READER_H_
#ifdef __cplusplus

#define WIN32_LEAN_AND_MEAN // Trim libraies.
#define VC_LEANMEAN // Trim even further.

#include "Windows.h"
#include "Winsock.h"

#include "Packet.h"

#include <..\Common.h>
#include <SafeKill.h>

#define MAX_PLAYER_NAME_LEN 32
#define MAX_LOGIN_PASS_LEN 32
#define MAX_VERNUM_LEN 8

struct EXP_FUNC CNetworkMsg {
DWORD MsgSize;
DWORD MsgID;
BYTE* pData;
};

struct EXP_FUNC CLoginRequestMsg {
DWORD MsgSize;
DWORD MsgID;

char VerNum[MAX_VERNUM_LEN];
char LoginPass[MAX_LOGIN_PASS_LEN];
char PlayerName[MAX_PLAYER_NAME_LEN];
};

struct EXP_FUNC CLoginReplyMsg {
DWORD MsgSize;
DWORD MsgID;

DWORD Res;
};

//----------------------------------------------------------------------//
// The class used to buffer data input/output
//----------------------------------------------------------------------//
class EXP_FUNC CMsgReader {
public:
CMsgReader();
private:
bool CanRead(SOCKET *pSock);
CReadResult Read(SOCKET *pSock, UINT Source, DWORD BufSize);
public:
CPacket MsgBuffer;
public:
void Reset();

BOOL ReadMsg(SOCKET *pSock, HWND hHostWnd);
};


#endif
#endif //_MSG_READER_H_



MsgReader.cpp

#include "MsgReader.h"

/////////////////////////////////////////////////////////////////////////////////////////////////
/////////////////////////////////////////////////////////////////////////////////////////////////

CMsgReader::CMsgReader()
{
this->Reset();
}

/////////////////////////////////////////////////////////////////////////////////////////////////

void CMsgReader::Reset()
{
MsgBuffer.Init();
}

//-----------------------------------------------------------------------------
// Tell if we are ready to read data
//-----------------------------------------------------------------------------
bool CMsgReader::CanRead(SOCKET *pSock)
{
// Setup the timeout for the select() call
timeval waitd;
waitd.tv_sec = 0;
waitd.tv_usec = 10;

// Zero the flags ready for using
fd_set flags;
FD_ZERO(&flags);

// Set the read flag to check the write status of the socket
FD_SET(*pSock, &flags);

int Res = select(*pSock, &flags,(fd_set*)0,(fd_set*)0,&waitd);
if(Res < 0) // Socket not ready to read
return false;


if(FD_ISSET(*pSock, &flags)){
// Clear the read flag
FD_CLR(*pSock, &flags);
// Socket is ready to read
return true;
} else {
// Not supposed to happen...
return false;
}
}

//-----------------------------------------------------------------------------
// Read data
//-----------------------------------------------------------------------------
CReadResult CMsgReader::Read(SOCKET *pSock, UINT Source, DWORD BufSize)
{
BYTE *pBuf = NULL;
DWORD *pBufIndx = NULL;

if(Source == MSG_HEADER_BUFFER){
pBuf = MsgBuffer.pHeader;
pBufIndx = MsgBuffer.GetHeaderIndxPtr();
BufSize = PACKET_HEADER_SIZE;
} else {
pBuf = MsgBuffer.pData;
pBufIndx = MsgBuffer.GetDataIndxPtr();

}

// Find how many bytes we have to read, in chunk of MAX_PACKET_SIZE
DWORD MaxBytesToProcess = (BufSize - *pBufIndx);
if(MaxBytesToProcess > MAX_PACKET_SIZE)
MaxBytesToProcess = MAX_PACKET_SIZE;

/////////////////////////////////////////////////

// Read/Write data
DWORD NumBytesProcessed = recv(*pSock, (char*)&pBuf[*pBufIndx], MaxBytesToProcess, 0);

/////////////////////////////////////////////////

CReadResult Res;
ZeroMemory(&Res, sizeof(CReadResult));

/////////////////////////////////////////////////

if(NumBytesProcessed != 0 && NumBytesProcessed != -1){
// Inc. our counters
*pBufIndx += NumBytesProcessed;
// Store how many bytes we've read
Res.BytesReaden = NumBytesProcessed;
// Have we finished reading/writing data?
Res.IsBufferLimitReach = *pBufIndx == BufSize;
// Have we hit M or Size limit?
Res.IsPacketLimitReach = NumBytesProcessed == MAX_PACKET_SIZE;
// Return sucess...
Res.Succes = TRUE;
}

return Res;
}

/////////////////////////////////////////////////////////////////////////////////////////////////

BOOL CMsgReader::ReadMsg(SOCKET *pSock, HWND hHostWnd)
{
// Read the Header of the message
if(!MsgBuffer.IsHeaderDoneProcessing()){

// Begin or resume reading the message header
CReadResult Res = Read(pSock, MSG_HEADER_BUFFER, PACKET_HEADER_SIZE);
if(!Res.Succes)
return FALSE;

// Are we done reading the msg header?
if(MsgBuffer.IsHeaderDoneProcessing()){

// Do we have some msg data left or available to read?
if(MsgBuffer.IsDataDoneProcessing()){

CNetworkMsg Msg;
Msg.MsgID = MsgBuffer.Header.MsgID;
Msg.MsgSize = MsgBuffer.Header.MsgSize;
Msg.pData = MsgBuffer.pData;

//ProcessMsg(&Msg);
SendMessage(hHostWnd, ON_NETWORK_MESSAGE, (WPARAM)*pSock, (LPARAM)&Msg);

MsgBuffer.Init();
return TRUE;
} else {
MsgBuffer.AllocateDataBuffer();
}
}
} else if(!MsgBuffer.IsDataDoneProcessing()){ // The header is completely readen,
// but not the data
// Begin or resume reading the message data
CReadResult Res = Read(pSock, MSG_DATA_BUFFER, MsgBuffer.Header.MsgSize);
if(!Res.Succes)
return FALSE;

// Do we have some msg data left or available to read?
if(MsgBuffer.IsDataDoneProcessing()){

CNetworkMsg Msg;
Msg.MsgID = MsgBuffer.Header.MsgID;
Msg.MsgSize = MsgBuffer.Header.MsgSize;
Msg.pData = MsgBuffer.pData;

//ProcessMsg(&Msg);
SendMessage(hHostWnd, ON_NETWORK_MESSAGE, (WPARAM)*pSock, (LPARAM)&Msg);

MsgBuffer.Init();
return TRUE;
}
}

return 0;
}


To resume what the code is doing, im using the packet structure to store the message size and ID, type, whatever in the first 2 DWORD of the message to send/recv, i call it the packet header. Then, i allocate dynamically an array based on the size in the header to read/send the data.

Which mean, if someone would mess up the message size to 20 instead of 10, for example(which should never happen normally) only half of the buffer would be filled, and if the message size would be 10 instead of 20, it would't overflow, but half the message would be stuck for the next read pass. I reallize that the latter would probably crash the program latter on, but as i said, only me and him would have the application, so it should't happen.

The first thing the server do when accepting a connection is wait for a fixed number of bytes of data that contain a bunch of randomized bytes, if it dosen't match, the client is disconected immediately before he is considered "loged in".

Im sure nobody understood but i did my best to explain lol.

Share this post


Link to post
Share on other sites
And this is the buffer class i was talking about


#include "RawBuffer.h"

CRawBuffer::CRawBuffer()
{
BufSize = 0;
pBuf = NULL;
}

CRawBuffer::CRawBuffer(UINT Size)
{
BufSize = 0;
pBuf = NULL;
Allocate(Size);
}

CRawBuffer::~CRawBuffer()
{
Free();
}

bool CRawBuffer::OverflowCheck(UINT Offset)
{
return Offset >= BufSize;
}

bool CRawBuffer::IsAllocated()
{
return pBuf != NULL;
}

BYTE* CRawBuffer::GetBuffer(UINT Offset){
if(OverflowCheck(Offset)){return NULL;}
return &pBuf[Offset];
}

char* CRawBuffer::GetStrBuffer(UINT Offset)
{
if(OverflowCheck(Offset)){return NULL;}
return (char*)&pBuf[Offset];
}

UINT CRawBuffer::GetBufferSize()
{
return BufSize;
}

UINT CRawBuffer::GetStrBufferSize(UINT Offset)
{
if(OverflowCheck(Offset)){return 0;}
if(IsAllocated()){
return strlen((char*)&pBuf[0]);
} else {
return 0;
}
}

bool CRawBuffer::Allocate(UINT Size)
{
bool Res = false;
if(!IsAllocated()){
pBuf = new BYTE[Size];
if(IsAllocated()){
BufSize = Size;
Erase();
Res = true;
}
}

return Res;
}

void CRawBuffer::Erase()
{
if(IsAllocated())
ZeroMemory(pBuf, BufSize);
}

void CRawBuffer::Set(BYTE Val)
{
if(IsAllocated())
memset(pBuf, Val, BufSize);
}

void CRawBuffer::Free()
{
if(IsAllocated()){
SAFE_DELETE_ARRAY(pBuf);
BufSize = 0;
}
}

Share this post


Link to post
Share on other sites
I just got an idea, if i was going to use some kind of hash value(stored just after or before the message size for example) to verify that the message data is correct, would it work? If so, could anyone point me to a good tutorial to generate and verify such a hash, like utorrent does for example?

Thx for your patience.

Share this post


Link to post
Share on other sites
A hash doesn't add any real security, it just makes it a little bit harder to exploit any problems.

As for your code, one thing I noticed is that your Buffer seems to think that new will return NULL on failure. Unless you use std::nothrow, new will throw a std::bad_alloc instance. If your program exhausts memory, and you inadvertently catch this exception, it could be left in an invalid state.

Your CRawBuffer::GetStrBufferSize() assumes there will be a NUL character in the buffer. I'd advise you actually check that, rather than using strlen() (I would include a similar sanity check in GetStrBuffer()). It also doesn't seem to respect the Offset parameter.

Overall, your program does a lot of explicit buffer management and character array manipulation, which is one of the reasons why we think it would be suspect. It is also "null happy", that is it uses lazy allocation, which means that you now have an additional state to verify in all your classes.

An interesting example would be to produce a minimal program - e.g. a console program featuring your login procedure, and then a simple echo server. If you could upload the source for this project somewhere, some kind soul (could be me later on today if I have time) might take the time to see if there are any obvious flaws in it (the problem with peer review, it can only confirm the presence, not absence, of security bugs).

If I was to write this program, I'd try encapsulate as much as possible of this into a handful of hardened classes/functions which can be "proved" correct (for a given definition of proof, enough to pass peer review here certainly). I'd try to minimise the number of things that can go wrong, by setting up and enforcing invariants, and limiting the kind of operations (e.g. don't put "handy" string functions into a buffer class).

For example:

  • Replace pointers with const references to wrapping objects (e.g. a Socket class)
  • No null states
  • No lazy allocation
  • No explicit memory management, where possible (use RAII)
  • std::string over "hopefully nul terminated character arrays"
  • std::vector<char> as a buffer class.

    The key is to keep everything minimal. Small, hardened classes with few invariants are much easier to check.

Share this post


Link to post
Share on other sites
Sign in to follow this  

  • Advertisement
×

Important Information

By using GameDev.net, you agree to our community Guidelines, Terms of Use, and Privacy Policy.

GameDev.net is your game development community. Create an account for your GameDev Portfolio and participate in the largest developer community in the games industry.

Sign me up!