[Enig123]

When looking into the cause of some strange logs, I was able to locate into CSearch::ProcessResultKeyword where uFromKadVersion was needed from pFromContact found earlier. That's when I realized sometimes peer may respond with another udp port due to outbound udp traffic sent to our ip and our listening udp port.

I did changed into the following codes

void CSearch::ProcessResultKeyword(const CUInt128 &uAnswer, TagList *plistInfo, uint32 uFromIP, uint16 uFromPort)
{
	// Find the contact who sent the answer - we need to know its protocol version
	// Special publish answer tags need to be filtered based on its remote protocol version, because if an old node is not aware
	// of those special tags, it doesn't knows it is not supposed accept and store such tags on publish request, so a malicious
	// publisher could fake them and our remote node would relay them on answers
	uint8 uFromKadVersion = 0;
	CContact* pFromContact = NULL;
	for (ContactMap::const_iterator itContactMap = m_mapTried.begin(); itContactMap != m_mapTried.end(); ++itContactMap)
	{
		CContact* pTmpContact = itContactMap->second;
		if (pTmpContact->GetIPAddress() == uFromIP)
		{
			pFromContact = pTmpContact;
			if (pTmpContact->GetUDPPort() == uFromPort)
				break;
		}
	}

	//Enig123::Only process tags with sender
	if (pFromContact == NULL)
	{
		DebugLogWarning(_T("Unable to find answering contact in ProcessResultKeyword - %s"), (LPCTSTR)ipstr(ntohl(uFromIP)));
		deleteTagListEntries(plistInfo);
		delete plistInfo;
		return;
	}
	else {	//if (pFromContact != NULL)
		uFromKadVersion = pFromContact->GetVersion();
		if (pFromContact->GetUDPPort() != uFromPort)
			DebugLogWarning(_T("Unable to find answering contact in ProcessResultKeyword with same port - %s"), (LPCTSTR)ipstr(ntohl(uFromIP)));
	}

to resolve it.

Today when I tried with similar situation in CSearch::ProcessResponse, the result logs show this is a quite common situation. I am suggesting similar changes of code there too:

void CSearch::ProcessResponse(uint32 uFromIP, uint16 uFromPort, ContactList *plistResults, uint32 uNumContacts)
{
	// Remember the contacts to be deleted when finished
	m_listDelete.insert(m_listDelete.end(), plistResults->begin(), plistResults->end());

	m_uLastResponse = time(NULL);

	//Find contact that is responding.
	CUInt128 uFromDistance((ULONG)0);
	CContact* pFromContact = NULL;
	for (ContactMap::const_iterator itContactMap = m_mapTried.begin(); itContactMap != m_mapTried.end(); ++itContactMap)
	{
		CContact* pTmpContact = itContactMap->second;
		if (pTmpContact->GetIPAddress() == uFromIP)
		{
			uFromDistance = itContactMap->first;
			pFromContact = pTmpContact;
			if (pTmpContact->GetUDPPort() == uFromPort)
				break;
		}
	}

	//Enig123::Only process results with sender
	if (pFromContact == NULL)
	{
		DebugLogWarning(_T("Unable to find answering contact in ProcessResponse - %s"), (LPCTSTR)ipstr(ntohl(uFromIP)));
		delete plistResults;
		return;
	}
	else {	// this part can be commented out after confirming the situation is commonly seen
		if (pFromContact->GetUDPPort() != uFromPort)
			DebugLogWarning(_T("Unable to find answering contact in ProcessResponse with same port - %s"), (LPCTSTR)ipstr(ntohl(uFromIP)));
	}

What are your thoughts on this?

[fox88]

Enig123, on 13 March 2025 - 05:39 AM, said:

When looking into the cause of some strange logs

How to know what is "strange logs"?

Enig123, on 13 March 2025 - 05:39 AM, said:

sometimes peer may respond with another udp port due to outbound udp traffic sent to our ip and our listening udp port.

A bug in KAD implementation?

[Enig123]

fox88, on 13 March 2025 - 01:55 AM, said:

Enig123, on 13 March 2025 - 05:39 AM, said:

When looking into the cause of some strange logs

How to know what is "strange logs"?

These logs have been long gone since the code changes. They were related to line 1145 of kademlia\kademlia\Search.cpp in 0.70b code base.

fox88, on 13 March 2025 - 01:55 AM, said:

Enig123, on 13 March 2025 - 05:39 AM, said:

sometimes peer may respond with another udp port due to outbound udp traffic sent to our ip and our listening udp port.

A bug in KAD implementation?

These are indications of the variety of udp NAT I suppose.

I don't think these responses would pose any security thread since we were the one who sent the requests to these ips in the first place.

This post has been edited by Enig123: 13 March 2025 - 05:55 PM

[fox88]

None of my log files contains TAG_KADAICHHASHRESULT string, which is present in line 1145.

[Enig123]

fox88, on 13 March 2025 - 11:17 AM, said:

None of my log files contains TAG_KADAICHHASHRESULT string, which is present in line 1145.

That's not very common, as I remembered.

Would you please try the latter patch, which was more commonly seen. I've got 533 matches across 4 log files.

[fox88]

Suggested contact map iterator uses incorrect conditions; patches do not change this.
Returning to the official code should be beneficial.

[Enig123]

fox88, on 14 March 2025 - 08:29 AM, said:

Suggested contact map iterator uses incorrect conditions; patches do not change this.
Returning to the official code should be beneficial.

The purpose of the codes is just to demonstrate how common the situation could be, not arguing which is correct or which is wrong.

But if it's a normal action for remote peer due to udp NAT, then potentially the kademlia could be adapted by ignoring udp port match.

[Enig123]

fox88, on 13 March 2025 - 11:17 AM, said:

None of my log files contains TAG_KADAICHHASHRESULT string, which is present in line 1145.

It's also possible with line 1114.

How about search with "ProcessResultKeyword"?

[fox88]

ProcessResultKeyword was never found.
Broken code serves no purposes, and same for invented NAT rules.

[Enig123]

fox88, you're right.

Just took a closer look at these ips, and found they're using a fixed port different from advertised port to send the responses, possibly due to misconfigured firewall rules.

One potentially issue is that I found some of these nodes got into the routing zone, wonder if there's some way to avoid that other than ban these ips.

Edit: In my humble opinion, these responses are legit, and the condition finding pFromContact should be loosen to ip matching only, instead of matching both ip and udp port.

FYI, CSearch::ProcessResultFile and CSearch::ProcessResultNotes the pFromContact was even irrelevant, and the responses are accepted directly.

This post has been edited by Enig123: 16 March 2025 - 04:14 AM

[fox88]

Enig123, on 16 March 2025 - 03:47 AM, said:

Just took a closer look at these ips, and found they're using a fixed port different from advertised port to send the responses, possibly due to misconfigured firewall rules.

The sender port does not have to match listening port. Sender port does not have to stay fixed.

Matching contacts by IP only - is wrong.
Your reports for IP filtering as "Invalid KAD tags sender" require a more detailed explanation.

[Enig123]

fox88, on 02 April 2025 - 03:14 AM, said:

Your reports for IP filtering as "Invalid KAD tags sender" require a more detailed explanation.

Here's the story. I am using the following code to test tag validity when 0.51a or so has tag number issue:

//Enig123::This is prelimilary test if the tag that is going to be read is valid or not - Feb. 28, 2019
//	return false means it's definitly invalid, return true only means it's probably valid
bool CDataIO::isValidTag(byte byType, uint16 uLenName)
{
	switch (byType)
	{
		case TAGTYPE_HASH:	//0x01
			if (uLenName == 0)
				return true;
			break;
		case TAGTYPE_STRING:	//0x02
		case TAGTYPE_UINT64:	//0x0B
		case TAGTYPE_UINT32:	//0x03
		case TAGTYPE_UINT16:	//0x08
		case TAGTYPE_UINT8:	//0x09
		case TAGTYPE_FLOAT32:	//0x04
		case TAGTYPE_BSOB:	//0x0A
			if (uLenName == 1)
				return true;
			break;
	}
	return false;
}

since according to opcode.h, all tag name string eMule used has a length of 1, only exception is TAGTYPE_HASH.

With the codes, I am able to identify possibly invalid tags from a few specific ips, like the following:

Quote

3/23/2025 9:07:46 PM: Invalid Kad tag; type=0x09 lenName=4 name=0xb8
3/23/2025 9:07:46 PM: Client UDP socket: prot=0xe4 opcode=0x44 sizeaftercrypt=80 realsize=96 Invalid Tag Parameter detected: 46.44.35.59:56769

3/20/2025 12:28:00 PM: Invalid Kad tag; type=0x08 lenName=4 name=0x4c
3/20/2025 12:28:00 PM: Client UDP socket: prot=0xe4 opcode=0x19 sizeaftercrypt=31 realsize=47 Invalid Tag Parameter detected: 46.44.35.59:56769

Current eMule codes will tolerate these tags possibly considering potential future usage, yet these are very suspicious I am afraid no legit eMule or aMule clients would send similar tags.

[fox88]

lenName=4, but why only one byte printed for name?

[Enig123]

fox88, on 03 April 2025 - 07:13 AM, said:

lenName=4, but why only one byte printed for name?

As defined in opcodes.h, all tag name length currently used by eMule is 1 only.

In reality, no clients triggered the exception other than the very a few ips mentioned in the ipfilter thread.

[fox88]

I briefly checked your rather old code.
Theoretically, tags have no limits for name length.
If the name does not match, the tag would be mostly ignored by eMule.
Fixing the name buffer size to 4 is not good.
Printing name before it was read - means the value is garbage.

[Enig123]

I am aware of the theoretically no limits for name length, what I am talking about is in reality the legit name length is 1 as for now at least.

The validation testing is used in the following codes to cope with possibly wrong tag count in ReatTagList:

void CByteIO::ReadTagList(TagList* pTaglist, bool bOptACP)
{
	for (uint32 uCount = ReadByte(); uCount > 0; --uCount)
	{
		if (GetAvailable() == 0)	// uCount errornously writen with +1 by remote peer
			break;

		pTaglist->push_back(ReadTag(bOptACP));

		if (GetAvailable() > 3)
		{
			size_t cur_pos = GetUsed();

			byte byType = ReadByte();
			uint16 uLenName = ReadUInt16();

			Seek(cur_pos);

			if (CDataIO::isValidTag(byType, uLenName)) {
				if (uCount == 1)
					uCount++;
			}
			else
				break;
		}
	}
}

in this case, IMHO, a good validation test is necessary here.

Edit: It seems worked well with the following codes for validation testing:

bool CDataIO::isValidTag(byte byType, uint16 uLenName)
{
	switch (byType)
	{
		case TAGTYPE_HASH:	//0x01
			if (uLenName == 0)
				return true;
			break;
		case TAGTYPE_STRING:	//0x02
		case TAGTYPE_UINT64:	//0x0B
		case TAGTYPE_UINT32:	//0x03
		case TAGTYPE_UINT16:	//0x08
		case TAGTYPE_UINT8:	//0x09
		case TAGTYPE_FLOAT32:	//0x04
		case TAGTYPE_BSOB:	//0x0A
			if (uLenName >= 1 && uLenName <= 255)
				return true;
			break;
	}
	return false;
}

fox88, do you think it's acceptable, for detecting of tag count error?

This post has been edited by Enig123: 04 April 2025 - 08:48 PM

[fox88]

The tag list read method never was in ByteIO class, and should not be there. It had 2 lines of code only, and got bloated to a whole page.
The code again tries to "fix received data" based on voluntary assumptions. As said many times, this never was a good idea.
While attempting to "save" one more tag out of potentially invalid data, processing exits after the first unexpected name length - no logic in this behaviour.

KAD was written with enabled variable tag name length, and there is no reason to change this.
eMule does seemingly sufficient validation before using the tags.

Therefore, this addition in the current form is unsuitable.
Simplified and fixed version very well might exist in debug/test/research builds, but not in releases.