Page MenuHomePhabricator

What awful and unforetold security problems am I unleashing by allowing users to proxy arbitrary commands to drydock machines?
OpenPublic

Asked by yelirekim on Sep 12 2016, 7:05 PM.

Details

Below are two extension classes which allow our users to lease and execute commands on drydock machines. This is highly useful for scripting, and looks basically like:

  • arc lease --blueprint 14 (assuming you are authorized to use the blueprint already)
  • cat myscript.sh | ssh phgit@phabricator.uberspaceships.com bash --lease 99

We do this to allow external systems to interface with drydock machines, especially in the case of extremely long running commands.

We only let people lease stuff from blueprints which create brand new virtual machines on each request, so they are never reusing a machine from a previous user.

The only obvious security "problem" I see so far is that the virtual machine is authenticated against phabricator as a bot user when it's created (it must be in order to clone the repository) , but we already have this problem in harbormaster anyways.

I'm more interested in discovering if there are potentially other ways that my methodology could have unintended consequences.

This is still an experimental service, but people are liking it quite a bit.

<?php

final class CIDrydockBashSSHWorkflow extends PhabricatorSSHWorkflow {

  protected function didConstruct() {
    $this->setName('bash');
    $this->setArguments(
      array(
        array(
          'name'      => 'lease',
          'param'     => 'id',
        ),
      ));
  }

  public function execute(PhutilArgumentParser $args) {
    $user = $this->getUser();
    $lease = (new DrydockLeaseQuery())
      ->setViewer($user)
      ->withIDs([$args->getArg('lease')])
      ->executeOne();
    if ($lease->getOwnerPHID() !== $user->getPHID()) {
      throw new Exception('You do not own this lease.');
    }
    if (!$lease->isActive()) {
      throw new Exception('Lease is not active.');
    }
    $interface = $lease->getInterface(DrydockCommandInterface::INTERFACE_TYPE);
    $future = $interface->getExecFuture('bash');

    $err = $this->newPassthruCommand()
      ->setIOChannel($this->getIOChannel())
      ->setCommandChannelFromExecFuture($future)
      ->execute();

    return $err;
  }
}
<?php

final class ICLeaseWorkflow extends ICDrydockWorkflow {

  public function getWorkflowName() {
    return 'lease';
  }

  public function getCommandSynopses() {
    return phutil_console_format(<<<EOTEXT
      **lease** [__options__]
EOTEXT
      );
  }

  public function getCommandHelp() {
    return phutil_console_format(<<<EOTEXT

          Lease resources from drydock.
EOTEXT
      );
  }

  public function getArguments() {
    return [
      'blueprint' => [
        'help' => pht('ID of the blueprint to use when acquiring a lease.'),
        'param' => 'id',
      ],
      '*' => 'attributes',
    ];
  }

  public function requiresAuthentication() {
    return true;
  }

  public function run() {
    if (!$this->getPassedArguments()) {
      $this->listBlueprints();
      $this->listLeases();
      return 0;
    }

    $attributes = $this->getArgument('attributes');
    if (count($attributes) != 1) {
      throw new ArcanistUsageException('You must provide lease attributes.');
    }
    $attributes = phutil_json_decode(Filesystem::readFile(head($attributes)));

    if (!$blueprint_id = $this->getArgument('blueprint')) {
      throw new ArcanistUsageException('You must provide a blueprint ID.');
    }
    $blueprint = $this->searchMethodForID(
      'drydock.blueprint.search',
      $blueprint_id);
    try {
      $lease = $this->getConduit()->callMethodSynchronous('drydock.lease.create', [
        'blueprintPHID' => $blueprint['phid'],
        'attributes' => $attributes,
      ]);
    } catch (ConduitClientException $e) {
      if ($e->getErrorCode() === 'ERR_NOT_AUTHORIZED') {
        if (phutil_console_confirm(pht(
          'You are not authorized to use this blueprint, request authorization?'))) {
          $this->requestAuthorization($blueprint['phid']);
          return 1;
        }
      }

    }

    return 0;
  }

  private function requestAuthorization($blueprint_phid) {
    $console = PhutilConsole::getConsole();
    $authorization = $this->getConduit()->callMethodSynchronous('drydock.authorization.request', [
      'blueprintPHID' => $blueprint_phid,
    ]);
    echo phutil_console_format(
      "%s\n        **%s** __%s__\n\n",
      pht('Authorization request issued.'),
      pht('Request URI:'),
      rtrim($this->getConfigFromAnySource('phabricator.uri'), '/').'/'.
      'drydock/authorization/'.$authorization['id'].'/');
  }

  private function listBlueprints() {
    $console = PhutilConsole::getConsole();
    $console->writeOut("<bg:blue> %s </bg>\n", pht('BLUEPRINTS'));
    $blueprints = $this->getConduit()->callMethodSynchronous('drydock.blueprint.search', [
      'constraints' => [
        'isDisabled' => false,
      ],
      'attachments' => [
        'authorizations' => true,
      ],
    ]);

    if (!$blueprints = idx($blueprints, 'data')) {
      $console->writeOut("\t%s\n", pht('No blueprints found.'));
    }

    $table = (new PhutilConsoleTable())
      ->addColumn('id', ['title' => 'ID'])
      ->addColumn('phid', ['title' => 'PHID'])
      ->addColumn('name', ['title' => 'Name'])
      ->addColumn('type', ['title' => 'Type'])
      ->addColumn('authorized', ['title' => 'Authorized'])
      ->setBorders(true);

    foreach ($blueprints as $blueprint) {
      $is_authorized = in_array(
        $this->getUserPHID(),
        idxv($blueprint, ['attachments', 'authorizations']));
      $table->addRow([
        'id' => idx($blueprint, 'id'),
        'phid' => idx($blueprint, 'phid'),
        'name' => idxv($blueprint, ['fields', 'name']),
        'type' => idxv($blueprint, ['fields', 'type']),
        'authorized' => $is_authorized ? pht('Yes') : pht('No'),
      ]);
    }

    $table->draw();
  }

}

Answers

epriestley
Updated 2,753 Days Ago

This is basically fine, subject to the concerns in Drydock User Guide: Security.

The equivalent "attack" here is:

$ nano tests/unit/whatever.py
$ # add "execute lots of arbitrary evil code"
$ arc diff # causes evil code to execute when the revision builds

...so it's not really doing anything more than users already could, particularly with the authorization scheme it looks like you've put in place.

T6193 discusses formalizing it upstream, we just don't really have any great use cases other than arc unit --on-a-remote-server which should probably be less low-level. It's conceptually fine and something I'm happy to bring upstream if there are reasonable, non-crazy things that you can do with it.

In practice I'd expect this to mostly cause UI/UX/management/administration/operational problems (one user allocating all the VMs and there not being a great way to see who did it; then wanting to quota each user; etc), not security problems.

New Answer