[shane.x.x]

I tried e-mailing this to some developers, but probably sent it to the wrong addresses - so am posting it here instead.

I was browsing through the code to eMule (0.26d), and noticed a couple potential problems with the use of UDP sockets with regards to security. It appears the IP of a UDP packet is taken to be trusted, however UDP packets can be easily forged to come from any source.

For example, based on the code from ClientUDPSocket.cpp, it would be possible to send "file not found" UDP packets to people trying to get a particular file, spoofed to be from various sources which have the file. (hope you understand what I mean by this).

The effect being, clients will drop the source and stop asking it for parts. (Through tests I know the client will drop the source, but correct me if Im wrong if this would eventually result in the source dropping the client from its queue).

This problem would seem to affect the following opcodes;

#define OP_REASKFILEPING  0x90	// <HASH 16>
#define OP_REASKACK  0x91	// <HASH 16>
#define OP_FILENOTFOUND  0x92
#define OP_QUEUEFULL  0x93

I haven't checked into this fully, but after a few tests, it would seem to be the case- let me know if you can think of a flaw in this theory. I have written a rough mod that sends a 'FILENOTFOUND' op to every source IP, spoffed to be from each IP (ie. 100 sources = 1000 spoofed packets), just to check if my theory was correct - but it didn't seem to have any effect - but this could be because of routers and filters etc. But when sending spoofed packets to myself, my client does drop the source.

If this is indeed a problem, a quick solution is to just turn off the UDP port. For the protocol, probably some kind of challenge response would be needed, - perhaps send a 'file ask' packet whenever you get a FILENOTFOUND packet just to be sure.

Cheers,
Shane.

ClientUDPSocket.cpp:---------------------

case OP_FILENOTFOUND:
{
	theApp.downloadqueue->AddDownDataOverheadFileRequest(size);
	CUpDownClient* sender = theApp.downloadqueue->GetDownloadClientByIP(inet_addr(host));  //assumes the packet is not spoofed
	if (sender){
  sender->UDPReaskFNF();
	}
	else
  //ASSERT (false); //Need to double check these asserts to make sure it's running well.
	break;
}

DownloadClient.cpp:----------------------

void CUpDownClient::UDPReaskFNF(){
	m_bUDPPending = false;
	theApp.emuledlg->AddDebugLogLine(false,CString("UDP ANSWER FNF : %s"),GetUserName());
	theApp.downloadqueue->RemoveSource(this);
	if (!socket)
  Disconnected();
}

[bluecow]

Indeed, it seems that this type of IP spoofing (if it is really as easy as you wrote -- personally I don't know) could be used to create a lot of traffic.

But I also think that it will not do any real harm out of creating a lot of traffic. If a client receives a lot of OP_FILENOTFOUND messages via UDP, it will remove it's sources with matching (spoofed) IP. When the available sources of that (poor) client gets below the "Max sources" setting, the client will start to ask for sources again and will eventually receive the previously removed sources again. -- That may be the reason why you didn't see any results of your "spoofing" test.

If I'm right, that type of IP spoofing will just create a lot of traffic -- which of course should be avoided. Using a challenge/response may lead up to nearly the same amount of traffic. I'm actually thinking of MLDonkey's "challenge/response" system and I can tell, that they are creating a lot of traffic with this.

Maybe there is a solution for this problem which does not cost that much traffic?

[shane.x.x]

Yeah, its true the clients can refind the sources. What I am concerned about however is people using it to get sources to drop clients from their queue, hence making the person who sent the spoofed packets move up the sources queue. ie It can be used to get ahead in the queue. Not a real serious problem in itself, but the amount of traffic generated to get that gain would be bad - so preferably it shouldnt be possible.

If anyone does decide to fix this problem, please be careful with what gets sent back as a challenge if thats what you decide to do. Sending more data in response to a smaller (potentially spoofed) UDP packet is a very bad idea. (can you say DDOS?). This goes for all the UDP op codes. I haven't looked at all the code too thouroughly, there could be more serious problems lurking there.

I'm not sure, but could any of those opcodes being sent repeatedly result in a client being banned? That is another potential problem if so.

And yes, UDP spoofing is quite simple. To demonstrate, the below code sends a spoofed OP_FILENOTFOUND UDP packet "from" a given IP to a given IP. Try it against yourself with an IP of one of your sources.


-------flinger.cpp--------------
Removed -- Not trying to censor ..But.. Reporting the problem is enough.. I did leave the code up for a little while to get some debat going.. But, I feel it doesn't need to stay up.. --Unknown1

[shane.x.x]

OK, Ive taken a better look at the source, and done a couple more tests.

Although a client will remove a source if it recieves a spoofed UDP packet, the source itself will still attempt to accept the upload request when the client reaches the start of the sources queue - and the client will still accept the connection.

However, if the client is actively downloading from the source - the downloading will stop. So it would still be possible to increase your position in the queue by killing everyone elses active downloads every x minutes - you would quickly make your way to the start of everyones queue especially for popular files where there are a lot of people actively downloading the file and you can predict sources and clients more easily.

There is a solution however. Just check that your not actively downloading before removing the source (patch below). If someone does decide to exploit this problem, just turn off the UDP port until the code gets fixed.

void CUpDownClient::UDPReaskFNF(){
	m_bUDPPending = false;
	theApp.emuledlg->AddDebugLogLine(false,CString("UDP ANSWER FNF : %s"),GetUserName());
	if (GetDownloadState() != DS_DOWNLOADING) //Added to prevent spoofed download kills
  theApp.downloadqueue->RemoveSource(this);
	if (!socket)
  Disconnected();
}

I will take a closer look at the rest of the op codes, but they don't seem to have a problem upon initial inspection.

[Harvey56]

great work.

Why did you show everyone how to manipulate source-IP and port?

[bluecow]

Quote

What I am concerned about however is people using it to get sources to drop clients from their queue, hence making the person who sent the spoofed packets move up the sources queue. ie It can be used to get ahead in the queue.

I forgot about that and checked the code... If a client receives an OP_FILENOTFOUND it removes that source from it's file, but the (removed) source is not deleted nor is it removed from that client's upload queue. So, I think there is no possibility to jump the queue -- at least with spoofed IPs and OP_FILENOTFOUND messages.

Quote

I'm not sure, but could any of those opcodes being sent repeatedly result in a client being banned? That is another potential problem if so.

Repeatedly sent OP_REASKFILEPING, OP_QUEUEFULL, OP_REASKACK and OP_FILENOTFOUND messages will unfortunately not ban the client. We are open for various DDOS attacks

However, it really looks like a potential security problem. Currently it seems to be no problem, but personally I would prefere to fix/improve it now and not when the first "copy&paste" exploit is posted...

Because I do not like any "challenge/response" stuff and because I do not like more overhead in the protocol, I think I'm not the right person for suggesting an improvment for this - maybe someone else can help.

[bluecow]

Quote

Although a client will remove a source if it recieves a spoofed UDP packet, the source itself will still attempt to accept the upload request when the client reaches the start of the sources queue  - and the client will still accept the connection.

Opps, seems that you've answered while I was answering your 2nd post..

Quote

However, if the client is actively downloading from the source - the downloading will stop. So it would still be possible to increase your position in the queue by killing everyone elses active downloads every x minutes ...

Why should this let someone jump the upload queue? That poor client would set it's download state from DS_DOWNLOAD to DS_NONE and would drop all further received data packets. But the uploader does not know that! -> more traffic But I can't see a reason why this should change the position in the sender's upload queue!?

Quote

...would quickly make your way to the start of everyones queue especially for popular files where there are a lot of people actively downloading the file and you can predict sources and clients more easily.

I agree that spoofed OP_FILENOTFOUND messages could be used to do some (much?) harm, at least when dealing with popular files -- but on the other side, why should one use that method for a popular file? A popular is always easy and fast do download.

However, your bug fix seems to be a good improvment!

[shane.x.x]

Quote

Why should this let someone jump the upload queue? That poor client would set it's download state from DS_DOWNLOAD to DS_NONE and would drop all further received data packets. But the uploader does not know that! -> more traffic  But I can't see a reason why this should change the position in the sender's upload queue!?

The source does keep sending for a little while, but the socket seems to get closed at some point because the source stops sending quite soon afterwards. As a consequence, the sender sees the closed connection and assumes the client has gone and moves onto the next in queue. I could be wrong here - I will do some more looking into it.

EDIT: Have taken a look at it again. Youre right, the source keeps sending, eMule keeps receiving, but the data goes to the ether So I guess it is more a way to disrupt other peoples downloading other than advancing your position in the queue.

Quote

Why did you show everyone how to manipulate source-IP and port?

Im an advocate of full disclosure - there isn't much code there over any other UDP spoofing code. It is easy to protect against by simply disabling the UDP port. It would be quite difficult to use that code effectively without some major changes. If you manage that, you can manage looking up "UDP spoofing code" in google.

Quote

Repeatedly sent OP_REASKFILEPING, OP_QUEUEFULL, OP_REASKACK and OP_FILENOTFOUND messages will unfortunately not ban the client. We are open for various DDOS attacks

I think you misunderstand. It is good that you dont ban based on this - because it can be spoofed and you would be able to ban other people.

When I talk about DDoS attacks, Im talking about talking about sending UDP packets with a src of some major site to all users of the eDonkey network - if these users then reply (to the spoofed/victim host) with a larger packet, you have a means of amplifying your bandwidth and can take down a site quite easily.

ie.. I send a 10 byte packet to 1000 users, with a source address of Microsoft.com. They then all reply with a 1000 byte packet (to Microsoft). I have then managed to amplify my attack x100, in a distributed manner, from many different IPs so it cant be blocked. This is how DDoS attacks work.

Quote

I agree that spoofed OP_FILENOTFOUND messages could be used to do some (much?) harm, at least when dealing with popular files -- but on the other side, why should one use that method for a popular file? A popular is always easy and fast do download.

Yep, fortuantly this has limited affect, and is reasonably easy to protect against.

This post has been edited by shane.x.x: 20 February 2003 - 02:07 PM

[bluecow]

shane.x.x, on Feb 20 2003, 03:02 PM, said:

I think you misunderstand. It is good that you dont ban based on this - because it can be spoofed and you would be able to ban other people. When I talk about DDoS attacks, Im talking about talking about sending UDP packets ...

Indeed, I misunderstood you -- I'm not very experienced with that low-level network stuff.

Furthermore, I've modified your patch a little so that we could handle the UDP-FILENOTFOUND in a similar way as the TCP-FILENOTFOUND (OP_FILEREQANSNOFIL).

Quote

void CUpDownClient::UDPReaskFNF(){
    m_bUDPPending = false;
    theApp.emuledlg->AddDebugLogLine(false,CString("UDP ANSWER FNF : %s"),GetUserName());
    if (GetDownloadState() != DS_DOWNLOADING){
        switch (GetDownloadState()) {
            case DS_ONQUEUE:
            case DS_NONEEDEDPARTS:
                if (SwapToAnotherFile(false))
                    break;
                /*fall through*/
            default:
                theApp.downloadqueue->RemoveSource(this);
        }
    }
    if (!socket)
        Disconnected();
}

[Unknown1]

I've been looking at the UDP packets for a while.. And was thinking perviously of changing the contents of them a little to help some.. I guess this now gives me a better reason to change it now..

I was thinking of putting a little challenge into the packet to make sure when I recieve an answer packet that I sent the request packet.. This would prettry much solve this with very little overhead and would require less checking.. If the answer packet has the wrong challenge, it will just be dropped..

[stobo]

one way to solve much of this (not the DDOS) is to use a cookie:

client A = downloader
client B = source
client c = spoofer

TCP a->b ask for file (cookie=1, support cookie)
TCP b->a ack, on queue (cookie = 2, support udp)
...
a->b reaskUDP (cookie = 1) (valid)
b->a ackUDP (cookie = 2) (valid)
...
c->a reaskNOTFOUND (cookie = 3) (invalid, discard)


in other words:
client-personal cookie is negotiated during TCP hello phase. this can be used to authenticate all further UDP from this client.
UDP support should NOT be advertised unless a cookie is supported and received. (old clients should upgrade anyway.)

IMO, UDP should never be accepted or used without this cookie, which cannot be known by the spoofer unless he is eavesdropping on your network (and then he can do a lot more than spoof UDP)

can you see any flaws in this approach?

[shane.x.x]

Quote

one way to solve much of this (not the DDOS) is to use a cookie:

Let me make it clear - as far as I can see, there is no DDoS attack possible using the eDonkey network. (thats not to say there isn't one, I just havent seen it yet). I just mentioned it so that anyone playing with the UDP code should be very careful with what they do. Modifying the code to send a large reply to a small unauthenticated UDP packet is how some DDoS are made possible. UDP spoofing today though, isn't as big a problem, (Modern Day UDP Spoofing), but you should still be careful. Otherwise the network could be abused to DDoS victims (not necessarily eDonkey/mule users).

Quote

Furthermore, I've modified your patch a little so that we could handle the UDP-FILENOTFOUND in a similar way as the TCP-FILENOTFOUND (OP_FILEREQANSNOFIL).

Yeah, that was just a quick and dirty hack. Even the code you suggested has some slight flaws. ie the state could be 'DS_CONNECTING' , 'DS_CONNECTED' or 'DS_NONEEDEDPARTS' (to be honest, I dont have a full understanding of the code yet) and the source would still be removed and left in an unstable state. (DS_NONEEDEDPARTS seems to still accept packets - see below code) I still think it needs to be made a bit cleaner.

Might I also suggest, that when removing a source - it seems strange that eMule would keep accepting packets. The connection to the source should be closed. In fact, the source should be removed/destroyed completely - not just put in a DS_NONE state. Im guessing they are kept around for the purposes of searching etc. (?)

Also, the below code should detect that the source has sent a packet even though its not in a download state and close the socket, or send a cancel download packet, or go into a download state.

void CUpDownClient::ProcessBlockPacket(char *packet, uint32 size, bool packed)
{
  try {
	// Ignore if no data required
	if (!(GetDownloadState() == DS_DOWNLOADING || GetDownloadState() == DS_NONEEDEDPARTS))
  return;

Yes stobo, the use of cookies/sessionids is the typical way to authenticate UDP. If you can think of a way to make this backwards compatible, it looks like a good approach.

Alternatively, you could sign the packets with your private key (look at the leeching userhash thread) - but I think that would be too much overhead, and has a couple flaws - a cookie solution would be nicer.

Quote

I was thinking of putting a little challenge into the packet to make sure when I recieve an answer packet that I sent the request packet.. This would prettry much solve this with very little overhead and would require less checking.. If the answer packet has the wrong challenge, it will just be dropped..

That seems like the most reasonable solution - but are the answer packets only ever sent in response to a request? Will they in future?
If they are, then you could just use some kind of pending flag saying - "I've sent a request to this user, and am expecting a reply". If you get any UDP packets other than the ones in this pending list - you just ignore them. It would be backwards compatible too.

This post has been edited by shane.x.x: 21 February 2003 - 01:46 AM

[stobo]

Quote

Yes stobo, the use of cookies/sessionids is the typical way to authenticate UDP. If you can think of a way to make this backwards compatible, it looks like a good approach.

i'm not sure i could implement it even non-backwards-compatible. don't know the networking code (opcodes?) that well, don't have any testing setups, etc. to be clear i haven't bothered to configure the udp for my firewall (the setup is .. more clumsy than most).

starting point would be to add "mycookie" and "theircookie" into UpDownClient, and negotiation of those (in tcp hello packets, the same as all the extensions). the udp code only needs to verify the cookie before processing (if udp protocol version >= x).

on rethink i suppose the cookie also blocks any possible/future udp DDoS attacks.

Quote

The connection to the source should be closed. In fact, the source should be removed/destroyed completely - not just put in a DS_NONE state. Im guessing they are kept around for the purposes of searching etc. (?)

if the code doesn't go to Ask for another file code (what was it, SwapFiles or something), it's broken. Before disconnection should always be checked if there is another file from the client. the _source_ should be dropped, not the client.

Quote

That seems like the most reasonable solution - but are the answer packets only ever sent in response to a request? Will they in future?
If they are, then you could just use some kind of pending flag saying - "I've sent a request to this user, and am expecting a reply". If you get any UDP packets other than the ones in this pending list - you just ignore them. It would be backwards compatible too.

IIRC, the UDP system has a feature to fall back to TCP if connection is unsuccessful (reply is never received, or icmp error is received). this would already require tracking who has been asked, when and with how many retries.

excess tired, unable to activate dig-stuff-up mode :(

[shane.x.x]

Quote

IIRC, the UDP system has a feature to fall back to TCP if connection is unsuccessful (reply is never received, or icmp error is received). this would already require tracking who has been asked, when and with how many retries.

Yeah, it does. So probably the easiest, cleanest and backwards compatible solution is to just do a check for m_bUDPPending in the ProcessPacket code. No need for cookies, challenge response etc. Of course, you could still be lucky and send a forged UDP packet while the client is waiting for a response, but that is fairly unlikely, and wouldnt pose too much of a problem even if they did.

bool CClientUDPSocket::ProcessPacket(char* packet, int16 size, int8 opcode, char* host, uint16 port){
	
if (a response opcode)
check m_bUDPPending == true

[bluecow]

shane.x.x, on Feb 21 2003, 01:50 AM, said:

Yeah, that was just a quick and dirty hack. Even the code you suggested has some slight flaws. ie the state could be 'DS_CONNECTING' , 'DS_CONNECTED' or 'DS_NONEEDEDPARTS' (to be honest, I dont have a full understanding of the code yet) and the source would still be removed and left in an unstable state. (DS_NONEEDEDPARTS seems to still accept packets - see below code) I still think it needs to be made a bit cleaner.

If the state is DS_CONNECTED ... hmm, I must admit that I never really understood that state. IMO that state should not exist at all, it should be implemented as an additional flag and should be used in conjunction with the download state. Maybe we have a minor bug in the handling of (TCP) OP_FILEREQANSNOFIL too!? Because, if the download state is DS_CONNECTED we probable have just sent an OP_FILEREQUEST which is not to be answered with an UDP(!) OP_FILENOTFOUND and the received UDP OP_FILENOTFOUND could be the answer for some other file we've asked... It's getting rather complicated.
Each other download state should be handled with removing the source (note: the source is just removed, not deleted). The remaining code (especially 'CUpDownClient::ConnectionEstablished' should be capable of recovering properly -- but I'm sure there is still room for improvements/fixes in proper dealing with download/upload state.

shane.x.x, on Feb 21 2003, 01:50 AM, said:

Might I also suggest, that when removing a source - it seems strange that eMule would keep accepting packets.

Yes, I fully(!!) agree. Once I wrote a detailed bug report about that with plenty of statistics which show that there a lot of (which I called "out-dated") blocks received – unfortunately that bug report was pruned.

shane.x.x, on Feb 21 2003, 01:50 AM, said:

The connection to the source should be closed. In fact, the source should be removed/destroyed completely - not just put in a DS_NONE state. Im guessing they are kept around for the purposes of searching etc. (?)

No! The client (note: I use the word 'client' and not 'source') should not be destroyed, it just should be removed as a source from the download file. The most important reasons why this is needed are:

*) A clients instance 'CUpDownClient' is used for downloading (client is used as 'source') and uploading. If we just would delete a 'source' the client would loose it's place in the upload queue.

*) A clients instance also holds some timestamps (last file reask time, last source ask time, ...) which are to be used to not get banned from that client. If we would remove+delete a source, we may receive it again (shortly after removing) via source-exchange or from the server or via passive source collecting and may reask it too early and may get banned. Although this should not happen, it happens from time to time. eMule is still in alpha/beta and there are still bugs in the protocol's "state machine" (if it only would be a real one ). So we have to be careful when ignoring the above noted timestamps and we have to be careful when deleting a client.

shane.x.x, on Feb 21 2003, 01:50 AM, said:

Also, the below code should detect that the source has sent a packet even though its not in a download state and close the socket, or send a cancel download packet, or go into a download state.

Some weeks ago I had the same idea and set the download state to DS_DOWNLOADING and discovered a lot of troubles with compressed packets. You may take a look at Crash After Lot Exceptions In Cupdownclient::unzip
From my own experiences of the last two months I can say, that setting the download state to DS_DOWNLOADING

void CUpDownClient::ProcessBlockPacket(char *packet, uint32 size, bool packed)
...
    if (!(GetDownloadState() == DS_DOWNLOADING || GetDownloadState() == DS_NONEEDEDPARTS))
    {
        if (packed)
            return;
        SetDownloadState(DS_DOWNLOADING);
    }
...

generally is an improvement. I would say the approx. 30% of the otherwise dropped data can be properly "consumed" with this. But this is rather OT.

[SlugFiller]

Even though TCP has more authenticating in the preconnection, and ACKing and the such, is it really any harder to spoof as being sent from somewhere else than UDP? I mean, in the end result, doesn't any "it came from that IP" mark get set by the ISP router? And if such a router that doesn't do so exists, wouldn't it be just as easy to fake a from for the TCP packet as it would for the UDP packet? I mean, aren't UDP packets marked with a "from" just like TCP packets? I mean, that's what's eventually used to determine which client sent that packet? Feel free to correct me.

[Maella]

It's not easy to spoof a TCP packet because before sending any data you must first open a session with the remote side. The session is open with both a request and an ack that would be send back to you 'spoofed' IP.

Maella

[SlugFiller]

It's not easy to open the TCP connection, but once it's open it's pretty easy to send false packets to one side pretending to be the other, unless there's some inherent part in the packet saying who's it's from that can't be modified by the average joe(since it's set in the ISP router), and there should be, if I'm not mistaken.

The same applies to UDP. Technically, isn't there like a mark on the UDP packet put there by the ISP router saying "it came from this IP"? I mean, isn't that how you eventually figure out which of the clients sent you said packet?

The only real difference between UDP and TCP is that in TCP there's a pre-connection process in which both sides verify a connection has been made, an ACK for every packet, and priority when packets need to be dropped due to saturation. The difference in the end-result is in reliability of success of transfer, not in anti-spoofing, in that area they're mostly on even grounds. The TCP simply makes sure that the destination is really there, unlike the UDP which sends blindly, verification of the source is something completely different.

[Maella]

Establishement of a TCP connexion (copy-n-past from a book)

Caller                                                                                                      Called

                                     SYN 1415531521:1415531521(0)
segment 1                    ------------------------------------------>
                                                       <mss 1024>



                                     SYN 1823083521:1823083521(0)
                                     <------------------------------------------                  segment 2
                                        ack 1415531522, <mss 1024>



                                                  ack 1823083522
segement 3                  ------------------------------------------>
                                                       

A TCP stack identify a packet with IP+ port of both client + server. The 1415531521 is a kind of sequence counter.

Spoofing of a TCP packet is only possible with a sniffer, what is far too complicate for a eMule, because you need to know the sequence number to stole a connection.

Maella

This post has been edited by Maella: 21 February 2003 - 09:25 PM

[SlugFiller]

So, what you're saying is that there is some sort of an unreplicatable key linked to a tcp packet linking it to it's appropriate connection? If so then I guess it would make TCP more secure, since said key would then have to be stolen to spoof for that connection. That only applies if said key has to be sent with every TCP packet, not just SYNs and ACKs.