4
\$\begingroup\$

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;
}                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                     }
\$\endgroup\$
4
  • \$\begingroup\$ are you running Java on the BackEnd or PHP? the question is Tagged PHP but then you mention Java and Jar files? \$\endgroup\$
    – Malachi
    Oct 31, 2013 at 15:57
  • \$\begingroup\$ @Malachi: If I read this correctly, it's PHP on the backend calling and external jar. \$\endgroup\$
    – Bobby
    Oct 31, 2013 at 16:28
  • 1
    \$\begingroup\$ well it looks like a lot of fun.. \$\endgroup\$
    – Malachi
    Oct 31, 2013 at 16:31
  • \$\begingroup\$ @Malachi, it is correct as Bobby says. My code is PHP, and I need to call a jar. What exactly I'm calling is probably less interesting though, as the question should be applicable in any situation where php code executes shell commands. \$\endgroup\$
    – Letharion
    Nov 1, 2013 at 0:05

1 Answer 1

2
\$\begingroup\$

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);
  }

}

?>
\$\endgroup\$
1
  • \$\begingroup\$ This actually brings up a good point about the parameters that I haven't though off. Allowing arbitrary sizes has the potential to lead to a denial of service attack, since one could easily consume large amounts of disc space, processing power, and probably easily all available httpd connections. Despite that, I'm a bit hesitant to provide to rigorous checking on the other parameters, as I don't want this code to need updating should the underlying application add new options. I'll try to find a balance between the two. :) \$\endgroup\$
    – Letharion
    Nov 15, 2013 at 9:32

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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