Error in processing NMDC search requests allows DC++ to be used for UDP DDOS attacks

Bug #1419454 reported by eMTee
256
This bug affects 1 person
Affects Status Importance Assigned to Milestone
DC++
Fix Released
Critical
Unassigned

Bug Description

I guess it's time to reveal second largest exploit in NMDC history that I found about 9 months ago. Do you remember the good old CTM exploit discovered by Team Elite back in 2004? Well, this one is pretty much alike.

How does it work?

Extremely easy I would say, all you need to do is to send malicious active search request to any vulnerable hub:

How it should be
$Search 2.3.4.5:12345 F?F?0?1?mp3|

How we do it
$Search 1.2.3.4://2.3.4.5:12345 F?F?0?1?mp3|

Where 1.2.3.4 is your own IP address and 2.3.4.5:12345 is the target IP address with port number that we would like to flood. In most cases your own IP address needs to be real, because the hub might ignore your search request or even kick you due to mismatch between your real IP address and the one stated in the request.

If the hub is not filtering request with invalid port number, it will broadcast the message to all its users. Each user who shares any MP3 files on the hub will then respond by sending 10 search results using UDP traffic to address 2.3.4.5:12345 mentioned in our example.

That is one malicious request in one hub, now imagine thousands of bots sending these malicious requests non stop to several public hubs with 5000 users on each, you should be able to hear a bomb exploding on target side. I did alot of tests actually, and came up to receiving traffic at speed of 1 Gbit/s, at that point hardware limits were reached. It should be possible to push the record even higher with better hardware. smile

What is servers mistake?

In above example 1.2.3.4://2.3.4.5:12345, the server takes 1.2.3.4 as the IP address of the request, which is correct, and //2.3.4.5:12345 as the port number of the request. Does the second part look like a port number? Not to me, but to server it does, sadly.

What is clients mistake?

In above example 1.2.3.4://2.3.4.5:12345, the client takes 1.2.3.4:// as invalid protocol of the request, and 2.3.4.5:12345 as the IP address and port number of the request, which are correct. I keep wondering for long time, but can't understand yet why DC++, StrongDC++ and others apply function Util::decodeUrl on active search request address. I was on the other hand very happy when I saw that clients didn't apply that function on active CTM request. biggrin

Which servers are vulnerable?

The only hub software that initially did check for invalid port in CTM and Search request was FlexHub, any other hub servers were forwarding the requests as is. Later I fixed this exploit in Verlihub and Lord_Zero fixed it in HeXHub. Today we can see that most largest hubs on DC run PtokaX, which is still vulnerable.

Which clients are vulnerable?

Every single one of them are. I was only looking at DC++, StrongDC++ and others based on them though.

What's the most interesting part?

The most interesting part of this exploit is that UDP protocol is portless, meaning that target server will receive all UDP traffic regardless of any open ports or firewalls at server level. By sending as much traffic as target server download speed allows, you will overwhelm the target connection and the server will no longer respond.

What can we do about it?

Fix this, sure, but how? If we look back at CTM exploit, yet today we see hubs that don't check for valid IP address in CTM requests, from that we can learn that there will always be hubs running old vulnerable hub servers even after 10 years. By that I'm trying to say that first of all fix should be implemented in DC clients, to protect its users.

Final words

I'm not sure if this is a mistake initially made by server developers or client developers, probably both are involved equally. No matter the answer, this exploit is the second largest exploit after CTM, and it's standing before you.

Refs:
- <http://te-home.net/?id=54&title=Second+largest+exploit+in+NMDC+history>
- <http://pastebin.com/SVpUGnNe>

eMTee (realprogger)
Changed in dcplusplus:
importance: Undecided → Critical
Fredrik Ullner (ullner)
information type: Public → Private Security
poy (poy)
description: updated
Revision history for this message
Francisco Blas Izquierdo Riera (klondike) (klondike) wrote :

poy, since the info is already public and has been made public at http://te-home.net/?id=54&title=Second+largest+exploit+in+NMDC+history ¿shall we keep it private?

Revision history for this message
eMTee (realprogger) wrote :
Revision history for this message
iceman50 (bdcdevel) wrote :

Adding a patch with a listener to shoot to HubFrame

Revision history for this message
poy (poy) wrote :

1) i'd prefer having it in an NMDC-specific part than in "Util".
2) returning the 2 strings instead of filling them as parameters would be more modern. :)
3) i think logging / showing messages goes too far; maybe in a future version, or differently... let's just fix the bug for now.

Fredrik Ullner (ullner)
Changed in dcplusplus:
status: New → Confirmed
Revision history for this message
iceman50 (bdcdevel) wrote :

What about returning a StringPairList (ip,port) ? e.g.

StringPairList parseIpPort(const string& aIpPort) {
 string::size_type i = aIpPort.rfind(':');
 StringPairList ipList;

 if (i == string::npos) {
  ipList.emplace_back(aIpPort, "");
 } else {
  ipList.emplace_back(aIpPort.substr(0, i), aIpPort.substr(i + 1);
 }

 return ipList;
}

and then some check to ensure ipList isn't empty or some weird corner case occurs?

Revision history for this message
iceman50 (bdcdevel) wrote :

poy - per your comments

1) not feasible since it's required in ClientManager::findHub

2) I've went with using std::pair

3) all logging features have been removed

Please advise on #1 as to how you would like to move forward (also note having it in NmdcHub requires static casts which I'm not sure is the best way

these are the functions affected ... ClientManager::findHub (obviously this won't be correct due to my previous comments
<http://pastebin.com/KLGpnUbq>

along with ClientManager::on(NmdcSearch, ... )
<http://pastebin.com/baREUnAy>

Revision history for this message
iceman50 (bdcdevel) wrote :

Updated diff

Revision history for this message
iceman50 (bdcdevel) wrote :

last patch had logging remnants.

poy (poy)
information type: Private Security → Public Security
Revision history for this message
poy (poy) wrote :

nice! adapted a bit (made the parsing func static, in particular) and pushed rev 0bf6563f200c. please test; i have only verified it doesn't crash but we have to make sure the flaw exposed here has actually been fixed.

Changed in dcplusplus:
status: Confirmed → Fix Committed
Revision history for this message
pavel pimenov (pavel-pimenov) wrote :

Small optimisation

const auto isPassive = seeker.size() > 4 && seeker.compare(0, 4, "Hub:") == 0;

- if(ClientManager::getInstance()->isActive() && !isPassive) {
+ if(!isPassive && ClientManager::getInstance()->isActive()) {

Revision history for this message
poy (poy) wrote :

yep, applied.

Revision history for this message
poy (poy) wrote :

bug 1440831 is a follow-up on this one.

Revision history for this message
poy (poy) wrote :
Revision history for this message
poy (poy) wrote :

Fixed in DC++ 0.851.

Changed in dcplusplus:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public Security information  
Everyone can see this security related information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.