Potential remote code execution vulnerability in multiplayer.

[gabfv]

Description

Hey! I was curious about how you were doing a math expression parser in your mod but noticed you were using pcall with user input (not really directly). If the non-math pattern is somehow breached, this enable remote code execution in multiplayer. My guts says the vulnerability is of low impact but I've worked on a math expression parser in lua the past week to fix this (see possible fix).

Expected behaviour

Ideally, user input shouldn't be executed with pcall, even if it's sanitized by a pattern.

Actual behaviour

No response

Steps to reproduce

No steps to reproduce.

Context

No response

Possible fix

See my example in my repo. It supports basic math right now (-, +, *, /, parenthesis, negative numbers) but I suppose I could update it if modulo and exponentiation operations are really needed.

Anyhow, I see two ways of integrating this, either by making a Factorio mod library that would be a dependency for your mod or integrating it directly to your mod. What do you think?

Mod version

v0.4.2

Factorio version

1.1.80

Operating system

Windows 10 2H22 19045.2846

[Sharparam]

Good catch! I was about to say this should be a non-issue because I pass in an empty environment to load, but upon checking it I actually forgot to add that.

See: https://www.lua.org/manual/5.2/manual.html#pdf-load

Passing in an empty environment as the env parameter should mean if it breaks out of the pattern it shouldn't be able to call any kind of function at all, so I'll add that now to add some sandboxing (which should've been there all along).

Of course an actual math expression parser would be ideal.

If there's none available on the mod portal at the moment, perhaps it would be ideal if you make it as a library and publish there so other mods can make use of it as well. Or adding it to flib.

Given that it's not a huge amount of code it could also be a good fit to just add directly in the mod, I'm a bit unsure what would be best. My vote would probably be to either add it to flib if its maintainers think it's a good fit, otherwise directly in the mod. (Or perhaps a new generic lib is better, since flib is more focused on Factorio-related functions.)

[gabfv]

Thanks! I opened an issue asking about whether it should be integrated with flib or not. I'll wait for their answer.

[raiguard]

I would recommend using game.evaluate_expression instead of building in a fully custom parser. Not only do you have to do no additional work, but it's pretty much guaranteed to be safe!

[Sharparam]

Oh wow that's indeed perfect. The only operations it's missing is division and modulo, but I doubt they're necessary here anyway.

I'll update it to use that API instead.