Author Topic: What code vulnerabilities arise from using eval() somewhere?  (Read 1612 times)

I've seen it mentioned several times that using eval() in the code is generally bad and should be avoided, especially in Add-On releases, but I've never actually known why, what exactly occurs?

Arbitrary code injection by malicious people if it's done wrong. An example would be a servercmd that has an argument that is evaled; this was the case with the master server spam months ago.
Ex: (forgive me I'm on mobile)
Servercmdxyz(%client, %arg) {
(some code here)
Eval(%arg); //someone malicious can run any code they want here
}

eval(%script); executes %script. If %script is fed from, for example, the server, then the server is able to execute whatever it wants on the client; or visa versa, if the client can execute any script they want on the server, they could easily do findclientbyname(jincux).isSuperAdmin = 1;. I'm assuming that you can see why that would be problematic. Eval itself isn't bad, but it's very easy to accidentally leave it open to code injection.

Arbitrary code injection by malicious people if it's done wrong. An example would be a servercmd that has an argument that is evaled; this was the case with the master server spam months ago.
Ex: (forgive me I'm on mobile)
Servercmdxyz(%client, %arg) {
(some code here)
Eval(%arg); //someone malicious can run any code they want here
}
eval(%script); executes %script. If %script is fed from, for example, the server, then the server is able to execute whatever it wants on the client; or visa versa, if the client can execute any script they want on the server, they could easily do findclientbyname(jincux).isSuperAdmin = 1;. I'm assuming that you can see why that would be problematic. Eval itself isn't bad, but it's very easy to accidentally leave it open to code injection.
Ah, it's only vulnerable if accepting client input then, I just wanted to be sure nothing problematic would arise from using it.

My CityRPG uses a weird method with eval, but I doubt it will cause problems because it's impossible for the client to get to it. (Example of using eval)

Took me a while, but then I realized I should have used a different method.

yeah it's only malicious if the input can be in any way modified by a client
think of it like XSS attacks if you have an idea of what those are, never trust the end-user with it
« Last Edit: December 05, 2015, 02:16:58 AM by TheBlackParrot »

It opens up a threat of eval injection. For example:

%arg is available for user input.

eval("talk(" @ %arg @ ");");
So whatever they put as %arg gets talked.
But they can also make arg "code that gets arbitarily executed" and then finish the code so there is no syntax error.

It opens up a threat of eval injection. For example:

%arg is available for user input.

eval("talk(" @ %arg @ ");");
So whatever they put as %arg gets talked.
But they can also make arg "code that gets arbitarily executed" and then finish the code so there is no syntax error.

Well, that kind of thing gets caught pretty easily because
Code: [Select]
talk(two words);is a syntax error.  Bigger issues crop up when people do something like
Code: [Select]
function servercmdsaything(%client, %text) {
    eval("talk(\"" @ %text @ "\");");
}
Because that appears to work consistently.  However, if %talk contains any quotation marks in it, then you run into problems.

For example, if we have
Code: [Select]
%text = "he said \"some words\" to me";Then the function above will result in a syntax error since the text that gets evaluated becomes
Quote
talk("he said "some words" to me");

Even worse, a malicious input along the lines of
Code: [Select]
%text = "word\");deleteAllSaveFiles();stealbankinginfo();quit();//";Which would get evaluated as
Quote
talk("word");deleteAllSaveFiles();stealbankinginfo();quit();//");
Which is valid code.

I was just outlining the basic principle of eval injection.

In general, you're always safe passing constant strings only to eval. Issues only arise once anything external passes unmodified into the code string.
However, you almost never actually need to use eval. If there's a specific use case you have in mind that you're wondering about, do tell—there may be a way to avoid it.

The few cases where eval is actually necessary are things such as calling methods with the name as an expression (%x.%y(...)), defining functions (code generation tends to help with TS performance), defining packages (you really shouldn't need to do this), etc.

When you actually end up needing to use it, however, try your best to never introduce external data into the code string (for example, eval(%x @ "...")). This is necessary for certain parts of the AST (such as identifiers when calling methods or defining functions), but isn't in the vast majority of cases, as you can use variables from the local scope directly.

Example:

function SimObject::call(%this, %name, %a, %b, %c, %d, %e, %f, %g, %h, %i, %j, %k, %l, %m, %n, %o, %p, %q, %r) {
    return eval("return %this." @ %name @ "(%a, %b, %c, %d, %e, %f, %g, %h, %i, %j, %k, %l, %m, %n, %o, %p, %q, %r);");
}


Do note that even the above function allows code injection (through %name). This is why when you do end up using eval like this, make sure you keep track of what variables are passed to it unmodified. In this case, since %name is passed to it directly, the same rules for normal eval (not passing external data directly) would apply to any user of the above method (not passing external data to %name directly, however if you do the chain continues).

Notable things you don't need eval for:

  • Calling functions (see the default call function)
  • Accessing fields on objects by full name (use a switch + the x.y[z] expression)
  • Inheriting from objects stored in a variable (temporarily rename the base object)
  • Creating datablocks dynamically (this one is debatable as changing the name of a datablock is a bit hacky, but even if you do end up using eval here, make sure you use variables/expressions directly when assigning the various fields rather than passing them in with string concatenation). I'm not actually sure about this one; it's been a while, and I really don't remember if you can use datablock statements anywhere but the top level. I distinctly remember doing so, though. Try it first before eval regardless.

If you must concatenate external strings, consider identifier sanitizing for identifiers and always use expandEscape on arbitrary strings.

I'm so tired

I really don't remember if you can use datablock statements anywhere but the top level.
I've tried this before and it appeared that you could not. You can use the new keyword to instantiate a datablock and it won't throw any errors, but it the object is seemingly broken on creation. It might be that it's changed into a different type, I never tested for that.

Yes, you can create datablocks within functions.

If you must concatenate external strings, consider identifier sanitizing for identifiers and always use expandEscape on arbitrary strings.
Under what circumstances would concatenating strings require eval?

Also, I thought you could use call on simobjects. call(functionname, object)?

Under what circumstances would concatenating strings require eval?

By that I meant actually concatenating in external strings when building the string to pass to eval itself.

Also, I thought you could use call on simobjects. call(functionname, object)?

Show me how you'd call %methodName in the Player:: namespace.

Show me how you'd call %methodName in the Player:: namespace.
Okay so I discussed this with you a bit in steam. If there are players on the server then you can use one of them to call the function with (Using the method below). If there aren't, why have you defined a method in the player namepace in the first place that's required to be called when there are no players? Doesn't make much sense to me, the problem seems entirely avoidable through good coding practices.

For non-default namespaces you can create a scriptobject like so:
$ClassInstance = new ScriptObject()
{
    class = "classname";
};

$ClassInstance.schedule(0, functionname);


It won't let you use return values obviously, however you can make that function do everything you want it to do on its own instead of relying on its return value. You can also just avoid declaring custom functions in namespaces needlessly altogether and eliminate that problem.

I've always been a firm believer that eval should never ever appear in a finished addon, except one meant to allow for arbitrary code execution by trusted individuals. No matter what the problem it is that you're trying to solve by eval, there always appears to be an alternative method for getting that problem solved without using eval.

Seriously, the eval function is A. Ridiculously slow  B. Dangerous if you make even one little screwup  and C. Encourages lazy coding practices. I don't like it one bit.
« Last Edit: December 05, 2015, 02:19:42 PM by Ipquarx »