[tHeWiZaRdOfDoS]

Hi there...

whilst browsing the code I found 2 locations where a check for ipfiltering is done but the check for banning seems to be missing...
Parts are:
PartFile.cpp @ AddSource

Quote

if (theApp.ipfilter->IsFiltered(nIP))
{
  if (thePrefs.GetLogFilteredIPs())
  AddDebugLogLine(false, _T("Ignored URL source (IP=%s) \"%s\" - IP filter (%s)"), ipstr(nIP), pszURL, theApp.ipfilter->GetLastHit());
  return;
}
//>>> WiZaRd - forgotten code?
if (theApp.clientlist->IsBannedClient(nIP))
{
  if (thePrefs.GetLogFilteredIPs() && thePrefs.GetLogBanned())
  AddDebugLogLine(false, _T("Ignored URL source (IP=%s) \"%s\" - Banned"), ipstr(nIP), pszURL);
  return;
}
//<<< WiZaRd - forgotten code?
CUrlClient* client = new CUrlClient;

And DownloadQueue @ KademliaSearchFile

Quote

uint32 ED2Kip = ntohl(ip);
if (theApp.ipfilter->IsFiltered(ED2Kip))
{
  if (thePrefs.GetLogFilteredIPs())
  AddDebugLogLine(false, _T("IPfiltered source IP=%s (%s) received from Kademlia"), ipstr(ED2Kip), theApp.ipfilter->GetLastHit());
  return;
}
//>>> WiZaRd - forgotten code?
if (theApp.clientlist->IsBannedClient(ED2Kip))
{
  if (thePrefs.GetLogFilteredIPs() && thePrefs.GetLogBanned())
  AddDebugLogLine(false, _T("Banned source IP=%s (%s) received from Kademlia"), ipstr(ED2Kip), theApp.ipfilter->GetLastHit());
  return;
}
//<<< WiZaRd - forgotten code?
if( (ip == Kademlia::CKademlia::getIPAddress() || ED2Kip == theApp.serverconnect->GetClientID()) && tcp == thePrefs.GetPort())
  return;

Intention? Really missing?

[SlugFiller]

I think ban and ipfilter work differently, as in, the ban acknowledges the source as being there, and allows adding it to the source list, but simply doesn't make requests from it in CPartFile::Process.
After all, unlike ipfilter, ban is temporary.

[tHeWiZaRdOfDoS]

Ban is temporary, right, but bans are cleaned up regularly, so I see no need to add a banned src to the srclist at all (aside that we have a stat value which btw isn't reliable at all)

[SlugFiller]

Quote

but bans are cleaned up regularly

Aren't they only cleared when the soft limit is reached? I haven't looked specifically, so don't hold me up to it.

[tHeWiZaRdOfDoS]

Eurrr no I thought about that part:

Quote

bool CClientList::IsBannedClient(uint32 dwIP) const
{
uint32 dwBantime;
if (m_bannedList.Lookup(dwIP, dwBantime))
{
  if (dwBantime + CLIENTBANTIME > ::GetTickCount())
  return true;
}
return false;
}

And finally:

Quote

///////////////////////////////////////////////////////////////////////////
// Cleanup banned client list
//
const uint32 cur_tick = ::GetTickCount();

if (m_dwLastBannCleanUp + BAN_CLEANUP_TIME < cur_tick)
{
  m_dwLastBannCleanUp = cur_tick;
  POSITION pos = m_bannedList.GetStartPosition();
  uint32 nKey = 0;
  uint32 dwBantime = 0;
  uint32 counter = 0; 
  while(pos)
  {
  m_bannedList.GetNextAssoc(pos, nKey, dwBantime);
  if(dwBantime + CLIENTBANTIME < cur_tick)
  {
    RemoveBannedClient(nKey);
    ++counter;
  }
  }
  if (thePrefs.GetLogBanned() && counter)
  AddDebugLogLine(false, _T("Cleaned up BannedClientsList, %u client(s) removed, %u left on list..."), counter, m_bannedList.GetCount());
}

So there is no need to add and/or keep banned srcs at all

[SlugFiller]

Quote

  if(dwBantime + CLIENTBANTIME < cur_tick)
  {
    RemoveBannedClient(nKey);
    ++counter;
  }

You've made a mistake there:

void CClientList::RemoveBannedClient(uint32 dwIP){
	m_bannedList.RemoveKey(dwIP);
}

This simply cleans up old bans, it doesn't disconnect the client, or remove it from source lists.

A more relevant code from CUpDownClient::TryToConnect:

  // for safety: check again whether that IP is banned
  if (theApp.clientlist->IsBannedClient(uClientIP))
  {
  	if (thePrefs.GetLogBannedClients())
    AddDebugLogLine(false, _T("Refused to connect to banned client %s"), DbgGetClientInfo());
  	if (Disconnected(_T("Banned IP")))
  	{
    delete this;
    return false;
  	}
  	return true;
  }

And in CListenSocket::OnAccept:

       if (theApp.clientlist->IsBannedClient(SockAddr.sin_addr.S_un.S_addr)){
        if (thePrefs.GetLogBannedClients()){
         CUpDownClient* pClient = theApp.clientlist->FindClientByIP(SockAddr.sin_addr.S_un.S_addr);
         AddDebugLogLine(false, _T("Rejecting connection attempt of banned client %s %s"), ipstr(SockAddr.sin_addr.S_un.S_addr), pClient->DbgGetClientInfo());
        }
        newclient->Safe_Delete();
        continue;
       }

This should already take care of all cases, but I did find this in CPartFile::AddSources:

  	if (theApp.clientlist->IsBannedClient(userid)){
#ifdef _DEBUG
    if (thePrefs.GetLogBannedClients()){
    	CUpDownClient* pClient = theApp.clientlist->FindClientByIP(userid);
    	AddDebugLogLine(false, _T("Ignored source (IP=%s) received from server - banned client %s"), ipstr(userid), pClient->DbgGetClientInfo());
    }
#endif
    continue;
  	}

So your AddSource patch is probably correct as far as uniformity goes, though it's only useful for keeping the source-count low since an actual connection is never made.

[tHeWiZaRdOfDoS]

Quote

This simply cleans up old bans, it doesn't disconnect the client, or remove it from source lists.

Exactly, that's the (my) point: why should we add/keep banned srcs anyway? They should be dropped from DL on ban immediately IMHO as they are useless... discard them, after their ban time has elapsed they can come back (and might get banned again...)

[CiccioBastardo]

Ok. I have added the patch to my mod, though I had to change

Quote

thePrefs.GetLogFilteredIPs() && thePrefs.GetLogBanned()

into

Quote

thePrefs.GetLogFilteredIPs() && thePrefs.GetLogBannedClients()

I hope this is how it was intended.

Thanks

[tHeWiZaRdOfDoS]

Ups yeah maybe *g*
I just C&P'ed out of my own mod

[Xman1]

currently I haven't the code here... but one thing you shoudl consider:

if a banned clients is at downloadqueue, it will be disconnected and deleted, because connection to it fails. If you disable this mechanismn, you have to look if you have to delete the clients somewhere else. Maybe it will be deleted in the cleanup of the clientlist... but I'm not sure (as I remeber it only deletes clients with US_NONE, but not US_BANED)