Skip to content

fix(core): removed eval from PHx and hardened @FILE - #2428

Open
elcreator wants to merge 1 commit into
evolution-cms:3.5.xfrom
elcreator:fix-vulnerabilities
Open

fix(core): removed eval from PHx and hardened @FILE#2428
elcreator wants to merge 1 commit into
evolution-cms:3.5.xfrom
elcreator:fix-vulnerabilities

Conversation

@elcreator

Copy link
Copy Markdown

The setup: what $key looks like when it arrives

By the time resolveSGVar() runs, _getSGVar() has already massaged the tag. For a template tag [[$_GET(id)]]:

  1. str_replace(['(', ')'], ["['", "']"]) rewrote $_GET(id) → $_GET['id']
  2. splitKeyAndFilter() peeled off any :modifier suffix

So $key is a string like $_GET['id'], $_SERVER['HTTP_HOST'], $_SESSION['user']['name'], or the bare form $_SERVER. The job is to turn that string into the actual value without letting PHP interpret it. The old code did
eval("return {$key};") — which is why $_SERVER . id executed a shell command. This code reads the value by hand instead.

resolveSGVar() — three steps

Step 1: identify and allowlist the superglobal

  if (!preg_match('@^\$_(GET|POST|SESSION|COOKIE|REQUEST|SERVER|FILES|ENV)@', $key, $matches)) {
      return '';
  }

The string must start with one of the eight known superglobal names. $matches[0] is the matched prefix (e.g. $_GET), $matches[1] is the bare name (e.g. GET). Anything else — $GLOBALS, $this, a bare expression — is refused
immediately. This is the allowlist: nothing outside these eight names can ever be reached.

Step 2: parse the ['key'] accessors into a path

  $path = [];
  $rest = substr($key, strlen($matches[0]));   // everything after "$_GET"
  while ($rest !== '' && $rest !== false) {
      if (!preg_match('@^\[\s*([\'"]?)([^\[\]\'"]*)\1\s*\]@', $rest, $accessor)) {
          return '';                            // trailing junk that isn't an accessor
      }
      $path[] = $accessor[2];                   // the key name
      $rest = substr($rest, strlen($accessor[0]));
  }

This walks the remaining string one [...] at a time, building a list of keys. For $_SESSION['user']['name'] it consumes ['user'] then ['name'], producing $path = ['user', 'name'].

The regex is the important guard — breaking it down:

  • ^[ — must start with [
  • \s*(['"]?) — optional whitespace, then capture an optional opening quote (', ", or none) into group 1
  • ([^\[\]\'"]*) — the key: any run of characters that are not brackets or quotes, captured into group 2
  • \1 — a backreference: the closing quote must match the opening one (both ', both ", or both absent)
  • \s*] — optional whitespace, then ]

The [^\[\]\'"] character class is the teeth: a key can't contain a bracket, quote, backtick, $, ., or any other character that could start an expression — because those characters would have to appear inside the class to be
allowed, and they aren't. Anything that isn't a clean [key] accessor makes the regex fail, and the whole tag is refused with ''. That's how $_SERVER . id dies: after matching $_SERVER, $rest is . id, which isn't [...], so
return ''.

Note: because the caller already turned (id) into ['id'], the unquoted accessor form ($accessor[1] empty) is what a (key) tag becomes, and the quoted form is what a literal ['key'] tag is — the regex accepts both via the
optional-quote group.

Step 3: walk the real array

  $container = $this->getSuperGlobal($matches[1]);

  if ($path === []) {
      return count($container) > 0 ? print_r($container, true) : '';
  }

  $cursor = $container;
  foreach ($path as $segment) {
      if (!is_array($cursor) || !array_key_exists($segment, $cursor)) {
          return '';
      }
      $cursor = $cursor[$segment];
  }
  return $cursor;
  • No accessors (bare $_SERVER) → dump the whole array via print_r, preserving the old behavior for that form (and '' if empty).
  • Otherwise descend the actual PHP array one key at a time. array_key_exists guards each hop, so a missing key returns '' cleanly instead of raising a notice, and a non-array cursor (you indexed too deep) also bails. The value
    returned is a genuine array element — never anything evaluated.

getSuperGlobal() — why a function instead of $$name

  switch ($name) {
      case 'GET': return $_GET;
      ...
      case 'SESSION':
          $session = isset($_SESSION) && is_array($_SESSION) ? $_SESSION : [];
          unset($session['mgrFormValues'], $session['token']);
          return $session;
  }

This maps the name string to the real superglobal explicitly. It matters that it's a hardcoded switch and not $GLOBALS[$name] or a variable-variable $$name — a dynamic lookup would reintroduce exactly the "attacker controls
which variable I read" problem the allowlist just closed. The switch can only ever return one of the eight.

The SESSION case is special: it returns a copy (PHP arrays are copy-on-assignment), then unset()s mgrFormValues and token from that copy. So a [[$_SESSION]] dump can't leak the CSRF token or stored form values, and because
it's a copy, the real $_SESSION is untouched. This preserves the redaction the original _getSGVar did.

The net effect

Every value that comes out is either a literal array element you named or a print_r of a redacted array. There is no code path where any part of the tag is executed — the string is only ever matched against regexes and used as
array keys. That's the whole point: same reads as before for legitimate $_GET(id)-style tags, zero reachability for id, ;phpinfo(), concatenation, or anything else.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant