10

Consider a front-end JavaScript application where menu items needed to be shown or hidden based on somewhat simple logic (roles that user has and some other logical state).

A simple language was introduced to define this logic in a concise human-readable way and every menu item was assigned a string condition, which looks like this: "isLoggedIn() AND NOT role(PARTNER)".

In order to actually check this condition, a simple compiler was implemented, which translates this language into a valid JavaScript code string and executes it using eval() returning the boolean result in the end.

The compiler works by replacing some constructs, like AND, OR and NOT with valid JavaScript equivalents like &&, || and ! using simple regular expressions:

private compileExpression(expression: string) {

  // Adding commas around function arguments (to make them strings)
  expression = expression.replace(/(\w+)\((.*?)\)/g, `$1('$2')`);

  // Replacing "AND"
  expression = expression.replace(/\sAND\s/g, ' && ');

  // Replacing "OR"
  expression = expression.replace(/\sOR\s/g, ' || ');

  // Replacing "NOT"
  expression = expression.replace(/(\s|^)NOT\s/g, ' !');

  // Prefixing predicates
  expression = expression.replace(/(\w+)\((.*?)\)/g, 'Π.$1($2)');

  return expression;

}

This turns "isLoggedIn() AND NOT role(PARTNER)" into "Π.isLoggedIn() && !Π.role('PARTNER)", which is then executed by eval():

public matchCondition = (condition: string): boolean => {

  // Using greek letter "P" for predicate (for shortness and uniqueness)
  // noinspection NonAsciiCharacters
  const Π = this.predicates;

  const expression = this.compileExpression(condition);

  const result = eval(expression);

  return result;

}

The Π is an object with simple predicate functions, which return boolean values like isLoggedIn(): boolean and role(roleName: string): boolean.

The expressions, that are being translated and executed are stored statically in local object as strings and are not accessible from global context. Also, all expressions are written by the developers and are not coming from users of the application in any way.

Is it safe to use eval() this way or should it be avoided at all costs (e.g. "eval is evil", "never use eval", etc)?

What are the possible attack vectors, that could be used to compromise such eval usage?

If expression strings will be loaded from the HTTPS server using XHR/Fetch, will it change the situation security-wise?

The reason to introduce such language and not defining the rules in code directly is that it required that these conditions could be defined in string values, e.g. in a JSON file. The other reason is that such language is easier to read at a glance.

  • 24
    It's not just that eval is potentially insecure (which it is), it's also that solutions relying on eval are typically hard-to-follow, confusing, and impossible to debug when things break. There are nearly always solutions that are, in all ways, better. – Conor Mancone Aug 13 at 12:52
  • 21
    I mean, I don't know your exact use-case, but I really doubt you have to go so far as building an entire language to make this happen. I suspect you're over thinking it. I've built a rules engines that allow the user to fully configure detailed conditions that have to be met before taking arbitrary actions, and did so without requiring either eval or a meta-language. In fact my rules engine sounds much more complicated. Things like: "If this is the customer id, and the status is one of these, and it was created more than X days but less than Y days ago, then send this email and this text – Conor Mancone Aug 13 at 13:53
  • 4
    I suppose I can't exactly break down a new system for you in comments, but effectively my answer is "I think you're using the wrong approach and should come up with a different solution". Eval is rarely the answer. – Conor Mancone Aug 13 at 13:59
  • 18
    I do wonder why your developers are not writing javascript straight away. Your syntax doesn't seem to have that much of a conciseness and readability advantage, but the regular expression compiler is very limited. – Bergi Aug 13 at 20:56
  • 9
    If all you are doing is to hide and show menus, why not use CSS selectors instead of Javascript? – Ángel Aug 13 at 22:24
35

Using eval in this context doesn't create any vulnerability, as long as an attacker can't interfere with the arguments passed to matchCondition.

If you find it easier to read / program it this way, and you're confident that no untrusted input will ever go into your expression compiler, then go for it.

eval isn't evil, untrusted data is.


Please note that it's entirely possible to avoid eval, by extracting the predicates then handling them with your custom functions, for example:

if (predicate === 'isLoggedIn()') {
    return Π.isLoggedIn();
}
  • The code in my question is not even trying to somehow sanitize the passed expression, I guess this is obvious. I believe the main question is how would attacker put alert`1` (or any other malicious code) into the expression in the first place? – Slava Fomin II Aug 13 at 13:13
  • Oh, I missed the "all expressions are written by the developers" part. Edited my answer. – Benoit Esnard Aug 13 at 13:25
  • 3
    Another alternative for replacing it return Π[predicate](); where the passed in predicate is the string "isLoggedIn". You can validate it, of course, to avoid a TypeError when running the code or to avoid any potential other situations but there is very rarely a good reason to use eval especially if all you need is to call some function dynamically. In JS you can also pass functions as parameters, so it's entirely possible to do that instead of a string and execute it at the end. – VLAZ Aug 14 at 6:45
  • 2
    Although I realize this is infosec.SE and what the OP is doing isn't necessarily insecure, eval is still evil for plenty of other reasons. – Jared Smith Aug 15 at 12:35
31

Today, everything is written by developers. Next month or next year, someone will say "hey, why not let the users write those themselves?" Bam.

Also, even if the rules are written by the developers only, do they or will they include any user-originated data? Something like titles, names, categories, for instance? This could quickly lead to an XSS attack.

Your regular expressions are so "open" (using lots of .* without any validation) that if anything untoward gets in, it will slip through directly to the eval in a minute.

At the very least, if you want to keep eval, you should have a lot stricter expressions instead of .*. But those can quickly become either difficult to understand, or a hindrance for many practical cases.

  • 1
    The point regarding the future proofing makes sense, thank you. – Slava Fomin II Aug 14 at 1:08
  • 1
    It doesn't matter whether the rules are written by the users, or by professional devs, all that matters is whether you offer unsanitised data/code to other users. In the majority of cases, eval is fine, because a malicious actor can only affect code that he is running himself. – Mike Brockington Aug 14 at 16:04
  • I think you give developers too much credit here! :) – gbjbaanb Aug 15 at 13:32
5

Appearances and expectations

If something looks like a safe expression, people will probably treat it like one. If a field looks like any other data-field, people (even developers) will probably put untrusted data in there. If something is evaluated with full level application access, it should look and feel like code.

Another problem are subtle bugs in your pre-compiler, which could introduce unwanted bugs/security flaws. Most vulnerabilities start with unwanted bugs, before a malicious attacker can exploit something. And a new meta-language without proper vetting/tests and strongly defined syntax is just another layer of confusion and bugs waiting to happen.

If only developers write code for your conditions, why not just use plain JavaScript? The meta-language brings hardly any benefit. And code is handled by people like code.

3

Front-end javascript itself is completely at the will of the client running the code. If you are depending on front-end javascript for security, you've already failed to secure your application. Forget eval. The client can replace your entire website with their own implementation if they desire. Thus, your server should validate everything that the client asks it to do.

In your case, you should ask yourself if it's a security violation for users to see menu items that they're not privileged enough to see. If so, then the server should not deliver those menu items to the client, because users can look at any javascript you deliver to them. If not, then you don't have any security concerns at all -- as others have mentioned, eval is only "bad" if it is run against unsanitized code. Since your eval'd code is currently sanitized, then I don't believe you have security concerns.

1

As others have said, as long as your rules really are coming only from trusted developers, there shouldn't be any security holes from using eval.

However, eval has plenty of other disadvantages, in terms of complexity, maintainability, debuggability, etc. And using regular expressions plus eval could easily result in problems down the road, depending on how your application grows.

I also think it's possible that you're overestimating the difficulty of writing your own interpreter; open-source libraries are mature enough that you can create a pretty nice interpreter even if (like me) you're not well-versed in parser and compiler implementation. Here's a grammar for PEG.js that I put together in about 15 minutes based on your problem description. For demonstration purposes, it just returns the predicate names; to use it, you'd change it to return a function that takes the predicates object and invokes the appropriate predicate, but hopefully this is enough to give you an idea of one possible approach.

OrExpression
  = head:AndExpression tail:(_ ("OR") _ AndExpression)* {
      return tail.reduce(function(result, element) {
        return result || element[3];
      }, head);
    }

AndExpression
  = head:NotExpression tail:(_ ("AND") _ NotExpression)* {
      return tail.reduce(function(result, element) {
        return result && element[3];
      }, head);
    }

NotExpression
  = "NOT" _ expr:NotExpression { return !expr; }
  / "(" _ expr:OrExpression _ ")" { return expr; }
  / Predicate;

Predicate
  = _ predicate:[A-Z]+ _ "(" _ arg:[A-Z]* _ ")" {
        return predicate.join('') + '(' + arg.join('') + ')';
      }

_ "whitespace"
  = [ \t\n\r]*

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.