Safely pass user input to a CLI application on the server?

Question

My code will, from a web-gui, generate Vash images. This currently involves making a call to a local binary, java. I'm wondering, if I've taken reasonable measures against this being exploited by a malicious user, or not.

The code will be used in several different places, by several different administrators, meaning both that I want to retain as much flexibility as reasonably possible, and that different users will have different opportunities to subvert the content of the $arguments variable.

From the code has been striped some @todo comments, and completely hard coded error messages.

There are three functions involved.

_vash_call_local_vash(array $arguments); // Responsible for building the command to be executed.
_vash_proc_open($cmd, $context); // Executes the command built above.
_vash_defaults() // Provides a set of default arguments.

$base_dir Will be supplied by a program that I don't have any control over. Should I be doing extra validation on the sanity of that variable? At this point I don't know what the function will look like, that retrieves the exact path.

To avoid misuse, I mainly do two things. First, I hard code the parameter names, and then read only actual parameter values from keys in the hard coded list. I believe this will avoid the possibility of someone adding extra parameters. Second, I escapeshellarg() all the parameter values. I'm worrying a bit that it will be possible to somehow trick Java into executing the wrong jar file?

function _vash_call_local_vash($arguments) {
  if (empty($arguments['data'])) {
    return NULL;
  }
  elseif (empty($arguments['output'])) {
    return NULL;
  }

  if (isset($arguments['data'])) {
    $arguments += _vash_defaults();

    $base_dir = '/some/recieved/path/';

    // Collapse the arguments array into a single line.
    // Use hard-coded list of argument names to avoid the possibility of using
    // that as an attack vector.
    $names = array('format', 'width', 'height', 'algorithm', 'output', 'data');
    $cmd = 'java -jar ' . $vash_dir . 'Vash.jar ';
    foreach ($names as $name) {
      $cmd .= '--' . $name . ' ' . escapeshellarg($arguments[$name]) . ' ';
    }

    return _vash_proc_open($cmd);
  }
  return FALSE;
}

function _vash_proc_open($cmd, $context = NULL) {
  $descriptorspec = array(
    0 => array("pipe", "r"),
    1 => array("pipe", "w"),
    2 => array("pipe", "w"),
  );
  $process = proc_open($cmd, $descriptorspec, $pipes, NULL, NULL, array('context' => $context));
  if (is_resource($process)) {
    $info = stream_get_meta_data($pipes[1]);
    stream_set_blocking($pipes[1], TRUE);
    stream_set_timeout($pipes[1], 1);
    $string = '';
    while (!feof($pipes[1]) && !$info['timed_out']) {
      $string .= fgets($pipes[1], 4096);
      $info = stream_get_meta_data($pipes[1]);
    };

    $info = stream_get_meta_data($pipes[2]);
    stream_set_blocking($pipes[2], TRUE);
    stream_set_timeout($pipes[2], 1);
    while (!feof($pipes[2]) && !$info['timed_out']) {
      $string .= fgets($pipes[2], 4096);
      $info = stream_get_meta_data($pipes[2]);
    };

    fclose($pipes[0]);
    fclose($pipes[1]);
    fclose($pipes[2]);
    $code = proc_close($process);
    return array('cmd' => $cmd, 'output' => $string, 'code' => $code);
  }
  return FALSE;
}

function _vash_defaults() {
  $defaults = array(
    'format' => 'png',
    'width' => '512',
    'height' => '512',
    'algorithm' => '1.1',
    'output' => '/tmp/output.png',
    // 'salt-file' => 'salt-file',
  );
  return $defaults;
}                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                     }

Answer 1

You aren’t validating any user input in your code, while escapeshellarg protects you from malicious arguments nothing protects you from calling your JAR with wrong arguments. Properly validating everything is the best advice I can give you.

Example

<?php

class Vash {

  protected $args;

  protected $path;

  public function __construct(array $args, $path) {
    if (!empty($args["data"]) && !empty($args["output"])) {
      $this->args = $args;
      $this
        ->validateFormat()
        ->validateDimension("width")
        ->validateDimension("height")
        ->validateAlgorithm()
        ->validateOutput()
        ->validateData()
        ->setPath($path)
        ->execute()
      ;
    }
  }

  protected function setPath($path) {
    if (!is_string($path)) {
      throw new IllegalArgumentException;
    }
    $path = "{$path}Vash.jar";
    if (!is_file($path)) {
      throw new LogicException;
    }
    $this->path = $path;
    return $this;
  }

  protected function validateFormat() {
    if (empty($this->args["format"])) {
      $this->args["format"] = "png";
    }
    else {
      switch ($this->args["format"]) {
        case "jpg":
        case "png":
          break;

        default:
          throw new IllegalArgumentException;
      }
    }
    return $this;
  }

  protected function validateDimension($dimension, $default = 512) {
    if (empty($this->args[$dimension])) {
      $this->args[$dimension] = $default;
    }
    elseif (filter_var((integer) $this->args[$dimension], FILTER_VALIDATE_INT, array(
      "flags"   => FILTER_REQUIRE_SCALAR,
      "options" => array( "min_range" => 10, "max_range" => 2000 ),
    )) === false) {
      throw new IllegalArgumentException;
    }
    return $this;
  }

  protected function validateAlgorithm() {
    if (empty($this->args["algorithm"])) {
      $this->args["algorithm"];
    }
    elseif (filter_var((float) $this->args["algorithm"], FILTER_VALIDATE_FLOAT, array(
      "flags"   => FILTER_REQUIRE_SCALAR,
      "options" => array( "decimal" => 1 ),
    )) === false || $this->args["algorithm"] <= 0.0 || $this->args["algorithm"] > 1.0) {
      throw new IllegalArgumentException;
    }
    return $this;
  }

  //
  // Continue with validation!
  //

  protected function execute() {
    $cmd = "java -jar {$this->path}";
    foreach (array("format", "width", "height", "algorithm", "output", "data") as $name) {
      $arg  = escapeshellarg($this->args[$name]);
      $cmd .= " --name '{$arg}'";
    }
    return _vash_proc_open($cmd);
  }

}

?>