[Release]Security Fixes

[Zerux]

Dear Elitepvpers,
I decided to release some exploits before anyone uses them against any servers.
Most of them are not Public as far as I know, most of them need a bit of advanced knowledge to exploit:

SQL-Injection Guild [Function: CDPSrvr::OnQuerySetGuildName]:
In this case there are 2 Bugs which make this attack even possible on most of the servers.
Problem 1:

There is no check if the item that is used is the "Change Guild-Name" item

Problem 2:

The developers validate the String that contains the name ("lpszGuild") but even if this check fails they forgot to return.

Solution[Define for both fixes is __GUILD_RENAME_FAKE]:

 Spoiler

void CDPSrvr::OnQuerySetGuildName( CAr & ar, DPID dpidCache, DPID dpidUser, LPBYTE lpBuf, u_long uBufSize )
{
	BYTE nId;
	char lpszGuild[MAX_G_NAME]	= { 0, };

	ar >> nId;
	ar.ReadString( lpszGuild, MAX_G_NAME );

	CUser* pUser = g_UserMng.GetUser( dpidCache, dpidUser );
	if( IsValidObj( pUser ) )
	{
#ifdef __RULE_0615
		if (prj.IsInvalidName(lpszGuild) || prj.IsAllowedLetter(lpszGuild) == FALSE) {
			pUser->AddDiagText(prj.GetText(TID_DIAG_0020));
#ifdef __GUILD_RENAME_FAKE
			return;
#endif //__GUILD_RENAME_FAKE

		}
#endif	// __RULE_0615

#ifdef __S_SERVER_UNIFY
		if( pUser->m_bAllAction )
#endif // __S_SERVER_UNIFY
		{
			CItemElem* pItemElem = (CItemElem*)pUser->GetItemId( nId );
			if( IsUsableItem( pItemElem ) && pItemElem->m_bQuery == FALSE 
#ifdef __GUILD_RENAME_FAKE
				&& pItemElem->m_dwItemId == II_SYS_SYS_SCR_GCHANAM
#endif // __GUILD_RENAME_FAKE			
				
				)
			{
				pItemElem->m_bQuery	= TRUE;
				g_DPCoreClient.SendQuerySetGuildName( pUser->m_idPlayer, pUser->m_idGuild, lpszGuild, nId );
			}
		}
#ifdef __S_SERVER_UNIFY
		else
		{
			g_DPCoreClient.SendQuerySetGuildName( pUser->m_idPlayer, pUser->m_idGuild, lpszGuild, nId );
		}
#endif // __S_SERVER_UNIFY
	}
}

SQL-Injection Name [Function: CDPSrvr::OnQuerySetPlayerName]:
This is similar to the one above.
Problem :

The developers validate the String that contains the name ("lpszPlayer") but even if this check fails they forgot to return.

Solution[Define for the fix is __CHAR_RENAME_INVALID]:

 Spoiler

void CDPSrvr::OnQuerySetPlayerName( CAr & ar, DPID dpidCache, DPID dpidUser, LPBYTE lpBuf, u_long uBufSize )
{
// II_SYS_SYS_SCR_CHANAM
	DWORD dwData;
	char lpszPlayer[MAX_PLAYER]	= { 0, };

	ar >> dwData;
	ar.ReadString( lpszPlayer, MAX_PLAYER );

	CUser* pUser	= g_UserMng.GetUser( dpidCache, dpidUser );
	if( IsValidObj( pUser ) )
	{
#ifdef __QUIZ
		if( pUser->GetWorld() && pUser->GetWorld()->GetID() == WI_WORLD_QUIZ )
			return;
#endif // __QUIZ
#ifdef __RULE_0615
		if (prj.IsInvalidName(lpszPlayer) || prj.IsAllowedLetter(lpszPlayer) == FALSE) {
			pUser->AddDiagText(prj.GetText(TID_DIAG_0020));
#ifdef __CHAR_RENAME_INVALID
			return;
#endif // __CHAR_RENAME_INVALID
		}
		prj.Formalize( lpszPlayer );
#endif	// __RULE_0615

#ifdef __S_SERVER_UNIFY
		if( pUser->m_bAllAction )
#endif // __S_SERVER_UNIFY
		{
			WORD wId	= LOWORD( dwData );
			WORD wMode	= HIWORD( dwData );
			if( (short)wId >= 0 )
			{
				CItemElem* pItemElem = (CItemElem*)pUser->GetItemId( wId );
				if( IsUsableItem( pItemElem ) && pItemElem->m_dwItemId == II_SYS_SYS_SCR_CHANAM && pItemElem->m_bQuery == FALSE )
				{
					pItemElem->m_bQuery	= TRUE;
					g_dpDBClient.SendQuerySetPlayerName( pUser->m_idPlayer, lpszPlayer, dwData );
				}
			}
		}
#ifdef __S_SERVER_UNIFY
		else
		{
			g_dpDBClient.SendQuerySetPlayerName( pUser->m_idPlayer, lpszPlayer, dwData );
		}
#endif // __S_SERVER_UNIFY
	}
}

Buffer Overflow [Function: CProject::IsInvalidName]:
This is a critical bug, but most of the "flyff hackers" would not be able to abuse this other than just crashing the server.
Problem :

The critical line of this is

"pszName" is a local buffer of size 64 but szName can be up to 100 characters long.
If szName exeeds the limit of 64 ,the stack canary is overwritten and therefore an error will be raised resulting in a servercrash. There are 2 ways to fix this: Either increasing the buffersize up to 100 or preventing the buffer overflowing

Solution[Define for the fix is __BUFFEROVERDLOW_NAMECHECK]:

 Spoiler

BOOL CProject::IsInvalidName( LPCSTR szName )
{
	TCHAR pszName[ 64 ];
#ifdef __BUFFEROVERDLOW_NAMECHECK
	if (strlen(szName) >= 64)
		return TRUE;
#endif
	strcpy( pszName, szName );
	strlwr( pszName );
//	string str	= pszName;
	CString str = pszName;
	for( set<string>::iterator i = m_sInvalidNames.begin(); i != m_sInvalidNames.end(); ++i )
	{
		string strstr = *i;
		
//		if( str.find( *i, 0 ) != -1 )
		if( str.Find( (*i).c_str(), 0 ) != -1 )
			return TRUE;
	}
	return FALSE;
}

Array out of bounds [Function: CItemUpgrade::RemovePetVisItem]:
This can result in a servercrash or even stathacks and more.
Problem :

I will keep this simple. There is no check of the value of "nPosition" so we can easily get out of bounds

Solution[Define for the fix is __PET_PIERCING_BOUNDS]:

 Spoiler

void CItemUpgrade::RemovePetVisItem( CUser* pUser, int nPosition, BOOL bExpired )
{
	if( !IsValidObj( pUser ) )
		return;

	CItemElem* pItemElemPet = pUser->GetVisPetItem();
	if( !IsUsableItem( pItemElemPet ) )
		return;

#ifdef __PET_PIERCING_BOUNDS
	if (nPosition < 0 || nPosition >= pItemElemPet->GetPiercingSize())
		return;
#endif //__PET_PIERCING_BOUNDS

	DWORD dwItemId = pItemElemPet->GetPiercingItem( nPosition );
	if( dwItemId  == 0 )	// ÀÌ¹Ì ºñ¾îÀÖ´Â ½½·Ô
		return;

	pUser->ResetPetVisDST( pItemElemPet );
	if( bExpired )
		PutPetVisItemLog( pUser, "$", "VIS_REMOVE_EXPIRED", pItemElemPet, nPosition );
	else
		PutPetVisItemLog( pUser, "$", "VIS_REMOVE_BYUSER", pItemElemPet, nPosition );
	pUser->UpdateItem( (BYTE)( pItemElemPet->m_dwObjId ), UI_PETVIS_ITEM, MAKELONG( nPosition, 0 ), 0 ); // ÇØ´ç ½½·ÔÀ» ºñ¿î´Ù.
	ItemProp* pItemProp = prj.GetItemProp( dwItemId );
	if( pItemProp )
		pUser->AddDefinedText( TID_GAME_BUFFPET_REMOVEVIS, "\"%s\"", pItemProp->szName );
	pUser->SetPetVisDST( pItemElemPet );
	ChangeVisPetSfx( pUser, pItemElemPet );
}

UPDATE 1 (15.05.2016)
Array out of bounds [Function: CExchange::ResultExchange]:
This can result in a servercrash or even dupes and more.
Problem :

Similar to the one above. There is no check of the value of "nListNum" so we can easily get out of bounds as if its "nListNum < 0" it will pass all checks

 Spoiler

The exact spot where the array gets out of bounds happens in CExchange::CheckCondition when trying to access the condition items (pSetList->vecSet[nListNum].vecCondItem; )

Solution[Define for the fix is __EXCHANGE_NEGATIVE]:

 Spoiler

int CExchange::ResultExchange( CUser* pUser, int nMMIId, int nListNum )
{
	PSETLIST pSetList = FindExchange( nMMIId );
	if( !pSetList )
		return EXCHANGE_FAILED;

	if( nListNum > (int)( pSetList->vecSet.size()-1 ) )
		return EXCHANGE_FAILED;
#ifdef __EXCHANGE_NEGATIVE
	if (nListNum < 0)
		return EXCHANGE_FAILED;
#endif //__EXCHANGE_NEGATIVE
	
	// Á¶°Ç ºñ±³
	if( !CheckCondition( pUser, nMMIId, nListNum ) )
		return EXCHANGE_CONDITION_FAILED;
....
}

UPDATE 2 (02.04.2018)
Nullpointer [Function: CDPSrvr::OnTransformItem]:
This results in a servercrash.
Problem :

ITransformer::Transformer only returns a valid Object if the argument is 0 which is for EggTransformation and otherwise NULL which leads to de-referencing a nullpointer (if you actively use Assertions you might want to add a check in the Transformer function itself)

Solution[Define for the fix is __INVALID_TRANSFORM]:

 Spoiler

void CDPSrvr::OnTransformItem( CAr & ar, DPID dpidCache, DPID dpidUser, LPBYTE, u_long )
{	
    .....
	ITransformer* pTransformer	= ITransformer::Transformer( stuff.GetTransform() );
#ifdef __INVALID_TRANSFORM
    if (!pTransformer)
    {
        return;
    }
#endif //__INVALID_TRANSFORM
	pTransformer->Transform( pUser, stuff );	// º¯È¯
}

UPDATE 3 (09.10.2018)
Out of Memory [Function: CDPSrvr::OnRainbowRaceMiniGamePacket]:
This can result in a servercrash .
Problem :

When serializing __MINIGAME_EXT_PACKET the size of the vector is not checked(nSize) which allows people to create arbitrary size vectors.

Solution[This is NOT a perfect fix but as far as i know the maximum size used in this system is 3. Define for the fix is __MINIGAME_SIZE_VALIDATE]:

 Spoiler

struct __MINIGAME_EXT_PACKET : public __MINIGAME_PACKET
{
....
			int nSize = 0; vecszData.clear();
			ar >> nSize;
			#ifdef __MINIGAME_SIZE_VALIDATE
			if(nSize > 3)
				{				
				nSize = 3;
				}
			#endif
			for( int i=0; i<nSize; i++ )
			{
				char szTemp[256] = {0,};
				ar.ReadString( szTemp, 256 );
				vecszData.push_back( szTemp );
			}
....
}

Array out of bounds[Function: CMiniGame::Bet_FiveSystem]:
This can result in a servercrash or even dupes and more.
Problem :

The developers messed up. They validated the index (nBetNum) but too late.

Solution[Define for the fix is __MINIGAME_EARLY_CHECK]:

 Spoiler

int CMiniGame::Bet_FiveSystem( CUser* pUser, int nBetNum, int nBetPenya )
{
#ifdef __MINIGAME_EARLY_CHECK
	// À߸øµÈ ÀÔÂû ¹øÈ£°¡ µé¾î ¿ÔÀ» ¶§
	if (nBetNum < 0 || 5 < nBetNum)
		return FIVESYSTEM_FAILED;
#endif 
	// ÀÔÂû ¹øÈ£¿¡ ÀÌÀü °ªÀÌ 0ÀÌ°í ÇöÀç ÀÔÂû±ÝÀÌ 0À϶§ ¾Æ¹«Àϵµ ¾ÈÇÔ(·Î±× X)
	if( pUser->m_nBetFiveSystem[nBetNum] == 0 && nBetPenya == 0 )
		return TRUE;
#ifndef __MINIGAME_EARLY_CHECK
	// À߸øµÈ ÀÔÂû ¹øÈ£°¡ µé¾î ¿ÔÀ» ¶§
	if( nBetNum < 0 || 5 < nBetNum )
		return FIVESYSTEM_FAILED;
#endif 
....
}

I will only be releasing bugs that affect the availability of the server. I won't be releasing fixes for anything that is not causing a DoS.

If your server gets crashed by someone and the fix is not public, please don't hesitate to contact me, I'm happy to help

Zerux

[Zerux]

Added another fix which I forgot in the initial thread

UPDATE 1 (15.05.2016)
Array out of bounds [Function: CExchange::ResultExchange]

[Zerux]

Another bug I came across after I was asked to check a specific part of the code

UPDATE 2 (02.04.2018)
Nullpointer [Function: CDPSrvr::OnTransformItem]

[Zerux]

Another one. More coming soon (Fixes for fixes of other Devs )

UPDATE 3 (09.10.2018)
Out of Memory [Function: CDPSrvr::OnRainbowRaceMiniGamePacket]

Array out of bounds[Function: CMiniGame::Bet_FiveSystem]

[Rhyder`]

Return nBetPenya also when get < 0 to avoid crash

if(nBetPenya  < 0)
return FIVESYSTEM_FAILED;

[Zerux]

Originally Posted by Rhyder`

Return nBetPenya also when get < 0 to avoid crash

if(nBetPenya  < 0)
return FIVESYSTEM_FAILED;

This is already present in the official code:

// 입찰금은 0보다 작을 수 없다.
if(nBetPenya < 0)
return FIVESYSTEM_FAILED;