SECURE_NPCTIMEOUT behavior review.

[gustavobrigo]

• rAthena Hash: HEAD

• Client Date: 2015

• Server Mode: Pre RE

• Description of Issue:
  • Result: Possible exploits on checks and calculations based on select's() menus
  • Expected Result:
  • How to Reproduce:
  • Official Information:

Today we discovered some players abusing on Mercenarys Rental system, it basically waits for the timeout of SECURE_NPCTIMEOUT on the scroll selection menu. When it hits close, it returns to the menu and due how the script do the check, it can get the last item of the meny (mercenary lv10) withou any cost or check:

			for (.@i = 1; .@i <= 9; .@i++)
				.@menu$ = .@menu$ + callfunc("F_GetNumSuffix",.@i) + " Grade " + .@name$[.@type] + " Mercenary:";
			.@Grade = select(.@menu$);
***WAIT THE TIMEOUT HERE, AFTERWARDS IT KEEPS EXECUTING WITH .@Grade set to 0 *****
			.@BaseLevel = 5 + (.@Grade * 10);
			.@BaseLevel = (.@val > 90)? 90 : .@BaseLevel;
			.@ZenyCost = 7 * .@Grade;
			setarray .@FaithCost[7], 50,100,300;
			.@FaithCost = .@FaithCost[.@Grade];
			mes "[Mercenary Manager]";
			mes "So you want to hire a " + callfunc("F_GetNumSuffix",.@Grade);
			mes "Grade " + .@name$[.@type] + " Mercenary?";
			mes "You need to have attained";
			mes "Base Level " + .@BaseLevel + " or higher, and";
			mes "must pay the " + .@ZenyCost + ",000 zeny fee.";
			next;
			if(select("Yes:No") == 2) {
				mes "[Mercenary Manager]";
				mes "Oh, really? Well, now";
				mes "might not be a good time";
				mes "for you to consider hiring";
				mes "a Mercenary, but please feel";
				mes "free to come back if your";
				mes "needs change. Thank you~";
			}
			else if(.@FaithCost && .@faith_merc < .@FaithCost) {
				mes "[Mercenary Manager]";
				mes "Oh... Your Loyalty rating";
				mes "with the " + .@name$[.@type] + " Mercenary";
				mes "Guild isn't high enough to";
				mes "hire this Mercenary. Please";
				mes "come back after you earn";
				mes "" + .@FaithCost + " or more Loyalty with us.";
			}
			else if (BaseLevel < .@BaseLevel) {
				mes "[Mercenary Manager]";
				mes "I'm sorry, but your Base";
				mes "Level isn't high enough";
				mes "to hire this Mercenary.";
				mes "Please come back to me";
				mes "once you reach Base Level " + .@BaseLevel + ".";
			}
			else if (Zeny < .@ZenyCost * 1000) {
				mes "[Mercenary Manager]";
				mes "I'm sorry, but you";
				mes "don't have enough zeny";
				mes "to hire this Mercenary.";
				mes "The hiring fee is " + .@ZenyCost + ",000 zeny.";
			}
			else {
				mes "[Mercenary Manager]";
				mes "Great! Our Mercenaries";
				mes "are sincere and devoted";
				mes "to protecting their clients.";
				mes "Summoned Mercenaries will";
				mes "offer their support to you for";
				mes "30 minutes. Take care now.";
				Zeny = Zeny - (.@ZenyCost * 1000);
				getitem .@item[.@type] - 10 + .@Grade, 1;
			}
			close;

This is just an example of exploit created by the timeout, in this case, we can hire a 99 mercenary without any zeny or afinity check because it bypass all the checks in this case.

This is not a problem with the mercenary scritpt itself, but because when SECURE_NPCTIMEOUT hits the menu timeout it keeps executing with a select() return value of 0 (which should never happen).

[sader1992]

please add your hash

this maybe related https://rathena.org/board/topic/116508-how-to-privent-this-hacker/

however i can't Reproduce both

[gustavobrigo]

@sader1992 This affects until head revision.

My example, you can go to the Mercenary Manager > Select HIRE > Wait on this screen til the close button appears (60sec ~):

When the CLOSE button appears, click on it, and the menu will show up again.
Afterwards, for ex, scroll til the end and select the 10th grade mercenary.

This is not a bug on the mercenary NPC, but with the timeout of the menu. it should end the script, and not reset it.

Att

[sader1992]

ok i could reproduce it with this option [Hire Mercenary]

[gustavobrigo]

@sader1992 yes, and the way this specific script do the checking, you can get the item for "free".

The fix should not be on the script, this is a problematic behavior in the select()/Menu timeout that could lead to a LOT of exploits/problems.

i am disabling SECURE_NPCTIMEOUT for now on my server.

[gustavobrigo]

@aleos89 just a follow up, i've disabled SECURE_NPCTIMEOUT on my server, it hurts a bit the performance, i got 10% lower load average on map-server after disabling it, i know it is a protection to poor written npc scripts, but, i don't know if the performance trade off worth it.

[sader1992]

@gustavobrigo do you mean after you install the PR above ? (i don't see anything wrong with it xD)

if not , test the pr the problem has been fixed with it
as soon as you close a window with timeout the selection is closed too

[gustavobrigo]

@sader1992 no no, i know the fix is working.

Instead i've chosen to disable SECURE_NPCTIMEOUT entirely. And i saw a bit of performance improvement after it.

[aleos89]

That's because when a player is talking to a NPC, there's a timer that's attached to the player for that NPC to check timeouts. And this feature is not to check for poorly written NPC. It's an official feature that clears NPC from players that just idle out.

I would assume your load difference has to do with a custom script you're using at login that's continuously attached to logged in players.

[gustavobrigo]

@aleos89 yes it could be, we have a OnPCLoginEvent script and 2k players rs.

But this official behavior could be configurable at battle config in that case, its just a suggestion because in that case i don't see any harm on disabling it to increase performance.

=D

[mazvi]

helo @gustavobrigo after 4befcf7 can you test this bug again? can player got free mercenary again?

[gustavobrigo]

@mazvi im unable to test, i opted to disable the entire timeout on my server.

[sader1992]

@mazvi i did test it , and the bug have been fixed with this