Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update minimum value of *input script command. #2494

Merged
merged 1 commit into from Jul 28, 2019

Conversation

Emistry
Copy link
Member

@Emistry Emistry commented Jun 16, 2019

Pull Request Prelude

Changes Proposed

All input() shall have default values, especially old scripts.
Maybe consider to set all parameter for input() as mandatory?

How to reproduce:
https://github.com/HerculesWS/Hercules/blob/stable/conf/map/script.conf

 'input_min_value:  <any negative value> 

talk to any NPC that use input() to exchange item/zeny/points and enter negative values to cause the exploit. (Ex: points = (points - (-amount)); became points = (points + amount);

Issues addressed:

Other Issue:

  • Previously, input() doesn;t allow negative, so I am not sure why some NPC does/doesn't check for negative value. Hence it might affect minor NPC that has the if-else checking for negative value.

@Emistry Emistry added type:bug Issue is a bug or describes an incorrect behavior that should be fixed component:scripts Affecting the scripts and NPCs component:core:scriptengine Affecting the script engine or the script commands severity:5-critical Crashes, security issues and exploits labels Jun 16, 2019
@HerculesWSAPI
Copy link
Contributor

This change is Reviewable

@dastgirp
Copy link
Member

Didnt we had restriction for this before?
I think it would be better to not allow negative value Unless specified in input

@Emistry
Copy link
Member Author

Emistry commented Jun 21, 2019

But this issue only happen if input_min_value are set to negative.
so if anyone changed it into negative value, and use input(.@value); without define the min and max, then it might cause issue.
although this could be said as "user fault" but the old script that use input() are the culprit, those script mostly doesn't define the value of min and max.

I think a safety way to use input() would be change the script command into input(min, max) where min max are mandatory field.

@4144
Copy link
Contributor

4144 commented Jun 21, 2019

I think it was merged incomplete negative value implimentation.
Correct way is control negatives in input
As simple half fix probably need add optional parameters to input with allowed limits. And by default without negative values

@MishimaHaruna MishimaHaruna added this to the Release v2019.07.28 milestone Jun 30, 2019
- avoid potential hacks for old scripts that use `input()` script
commands.

Signed-off-by: Haru <haru@dotalux.com>
@MishimaHaruna
Copy link
Member

I edited the pull request and removed the minimum bound where it's not necessary (wherever a negative value is already explicitly or implicitly handled by the script).
In many cases I also changed the == 0 checks to be <= 0.

@MishimaHaruna MishimaHaruna merged commit afe0f13 into HerculesWS:master Jul 28, 2019
@Emistry Emistry deleted the npc_input branch July 29, 2019 08:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:core:scriptengine Affecting the script engine or the script commands component:scripts Affecting the scripts and NPCs severity:5-critical Crashes, security issues and exploits type:bug Issue is a bug or describes an incorrect behavior that should be fixed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants