Skip to content

Commit

Permalink
Provide mitigation options for SSH redirection vulnerability
Browse files Browse the repository at this point in the history
The less intrusive options that give most immediate benefit for least
cost are enabled by default:
* Prevent server admins resetting SSH host key
* Block sync if multiple servers have the same SSH host key

An additional option for improved security is included to provide
hostname verification, either based on `hostname -f` or on an explicitly
defined '.hostnames' file.

Resolves: SSH redirection security issue reported by Tobias Josefowitz
of Opera Software
  • Loading branch information
Thomas Pike committed Nov 8, 2017
1 parent ca1562c commit c184b03
Show file tree
Hide file tree
Showing 8 changed files with 125 additions and 6 deletions.
35 changes: 35 additions & 0 deletions config/config-sample.ini
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,41 @@ logo = /logo-header-opera.png
; " < $gt;
footer = 'Developed by <a href="https://www.opera.com/">Opera Software</a>.'

[security]
; It is important that SKA is able to verify that it has connected to the
; server that it expected to connect to (otherwise it could be tricked into
; syncing the wrong keys to a server). The simplest way to accomplish this is
; through SSH host key verification. Setting either of the 2 options below to
; '0' can weaken the protection that SSH host key verification provides.

; Determine who can reset a server's SSH host key in SKA:
; 0: Allow server admins to reset the SSH host key for servers that they
; administer
; 1: Full SKA admin access is required to reset a server's host key
host_key_reset_restriction = 1

; Determine what happens if multiple servers have the same SSH host key:
; 0: Allow sync to proceed
; 1: Abort sync of affected servers and report an error
; It is not recommended to leave this set to '0' indefinitely
host_key_collision_protection = 1


; Hostname verification is a supplement to SSH host key verification for
; making sure that the sync process has connected to the server that it
; expected to.

; Determine how hostname verification is performed:
; 0: Do not perform hostname verification
; 1: Compare with the result of `hostname -f`
; 2: Compare with /var/local/keys-sync/.hostnames, fall back to `hostname -f`
; if the file does not exist
; 3: Compare with /var/local/keys-sync/.hostnames, abort sync if the file
; does not exist
; The last option provides the most solid verification, as a server will only
; be synced to if it has been explicitly allowed on the server itself.
hostname_verification = 0

[defaults]
; This setting will cause new servers to always have a managed account called
; "root" and for that account to be automatically added into the
Expand Down
1 change: 1 addition & 0 deletions model/server.php
Original file line number Diff line number Diff line change
Expand Up @@ -308,6 +308,7 @@ public function add_account(ServerAccount &$account) {
if(is_null($this->id)) throw new BadMethodCallException('Server must be in directory before accounts can be added');
$account_name = $account->name;
if($account_name === '') throw new AccountNameInvalid('Account name cannot be empty');
if(substr($account_name, 0, 1) === '.') throw new AccountNameInvalid('Account name cannot begin with .');
$sync_status = is_null($account->sync_status) ? 'not synced yet' : $account->sync_status;
$this->database->begin_transaction();
$stmt = $this->database->prepare("INSERT INTO entity SET type = 'server account'");
Expand Down
1 change: 1 addition & 0 deletions model/serverdirectory.php
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,7 @@ public function list_servers($include = array(), $filter = array()) {
$where[] = "hostname REGEXP '".$this->database->escape_string($value)."'";
break;
case 'ip_address':
case 'rsa_key_fingerprint':
$where[] = "server.$field = '".$this->database->escape_string($value)."'";
break;
case 'admin':
Expand Down
47 changes: 46 additions & 1 deletion scripts/sync.php
Original file line number Diff line number Diff line change
Expand Up @@ -285,6 +285,16 @@ function sync_server($id, $only_username = null, $preview = false) {
return;
}
}
if(!isset($config['security']) || !isset($config['security']['host_key_collision_protection']) || $config['security']['host_key_collision_protection'] == 1) {
$matching_servers = $server_dir->list_servers(array(), array('rsa_key_fingerprint' => $server->rsa_key_fingerprint, 'key_management' => array('keys')));
if(count($matching_servers) > 1) {
echo date('c')." {$hostname}: Multiple hosts with same host key.\n";
$server->sync_report('sync failure', 'Multiple hosts with same host key');
$server->delete_all_sync_requests();
report_all_accounts_failed($keyfiles);
return;
}
}
try {
ssh2_auth_pubkey_file($connection, $attempt, 'config/keys-sync.pub', 'config/keys-sync');
echo date('c')." {$hostname}: Logged in as $attempt.\n";
Expand Down Expand Up @@ -327,6 +337,41 @@ function sync_server($id, $only_username = null, $preview = false) {
$account_errors = 0;
$cleanup_errors = 0;

if(isset($config['security']) && isset($config['security']['hostname_verification']) && $config['security']['hostname_verification'] >= 1) {
// Verify that we have mutual agreement with the server that we sync to it with this hostname
$allowed_hostnames = null;
if($config['security']['hostname_verification'] >= 2) {
// 2+ = Compare with /var/local/keys-sync/.hostnames
try {
$allowed_hostnames = array_map('trim', file("ssh2.sftp://$sftp/var/local/keys-sync/.hostnames"));
} catch(ErrorException $e) {
if($config['security']['hostname_verification'] >= 3) {
// 3+ = Abort if file does not exist
echo date('c')." {$hostname}: Hostnames file missing.\n";
$server->sync_report('sync failure', 'Hostnames file missing');
$server->delete_all_sync_requests();
report_all_accounts_failed($keyfiles);
return;
} else {
$allowed_hostnames = null;
}
}
}
if(is_null($allowed_hostnames)) {
$stream = ssh2_exec($connection, '/bin/hostname -f');
stream_set_blocking($stream, true);
$allowed_hostnames = array(trim(stream_get_contents($stream)));
fclose($stream);
}
if(!in_array($hostname, $allowed_hostnames)) {
echo date('c')." {$hostname}: Hostname check failed (allowed: ".implode(", ", $allowed_hostnames).").\n";
$server->sync_report('sync failure', 'Hostname check failed');
$server->delete_all_sync_requests();
report_all_accounts_failed($keyfiles);
return;
}
}

if($legacy && isset($keyfiles['root'])) {
// Legacy sync (only if using root account)
$keyfile = $keyfiles['root'];
Expand Down Expand Up @@ -408,7 +453,7 @@ function sync_server($id, $only_username = null, $preview = false) {
if(is_null($only_username)) {
// Clean up directory
foreach($sha1sums as $file => $sha1sum) {
if($file != '' && $file != 'keys-sync') {
if($file != '' && $file != 'keys-sync' && $file != '.hostnames') {
try {
if(ssh2_sftp_unlink($sftp, "$keydir/$file")) {
echo date('c')." {$hostname}: Removed unknown file: {$file}\n";
Expand Down
14 changes: 14 additions & 0 deletions templates/help.php
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
##
$admin_mail = $this->get('admin_mail');
$baseurl = $this->get('baseurl');
$security_config = $this->get('security_config');
?>
<div class="panel-group" id="help">
<h1>Help</h1>
Expand Down Expand Up @@ -106,6 +107,10 @@
<dd>SSH key authority was unable to establish an SSH connection to your server. This could indicate that the server is offline or otherwise unreachable, or that the SSH server is not running.</dd>
<dt>SSH host key verification failed</dt>
<dd>SSH key authority was able to open an SSH connection to your server, but the host key no longer matches the one that is on record for your server. If this is expected (eg. your server has been migrated to a new host), you can reset the host key on the "Settings" page of your server. Press the "Clear" button for the host key fingerprint and then "Save changes".</dd>
<?php if(!isset($security_config['host_key_collision_protection']) || $security_config['host_key_collision_protection'] == 1) { ?>
<dt>SSH host key collision</dt>
<dd>Your server has the same SSH host key as another server. This should be corrected by regenerating the SSH host keys on one or both of the affected servers.</dd>
<?php } ?>
<dt>SSH authentication failed</dt>
<dd>Although SSH key authority was able to connect to your server via SSH, it failed to log in. See the guides for setting up <a data-toggle="collapse" data-parent="#help" href="#sync_setup">full account syncing</a> or <a data-toggle="collapse" data-parent="#help" href="#legacy_sync_setup">legacy root account syncing</a>.</dd>
<dt>SFTP subsystem failed</dt>
Expand All @@ -122,6 +127,12 @@
</dd>
<dt>Multiple hosts with same IP address</dt>
<dd>At least one other host managed by SSH Key Authority resolves to the same IP address as your server. SSH Key Authority will refuse to sync to either server until this is resolved.</dd>
<?php if(isset($security_config['hostname_verification']) && $security_config['hostname_verification'] >= 3) { ?>
<dt>Hostnames file missing</dt>
<dd>The <code>/var/local/keys-sync/.hostnames</code> file does not exist on the server. SSH Key Authority uses the contents of this file to verify that it is allowed to sync to your server.</dd>
<dt>Hostname check failed</dt>
<dd>The server name was not found in <code>/var/local/keys-sync/.hostnames</code> when SSH Key Authority tried to sync to your server.</dd>
<?php } ?>
</dl>
</div>
</div>
Expand Down Expand Up @@ -163,6 +174,9 @@
<li>Create <code>/var/local/keys-sync/keys-sync</code> file (owned by keys-sync, permissions 0644) with the following SSH key in it:
<pre><?php out($this->get('keys-sync-pubkey'))?></pre>
</li>
<?php if(isset($security_config['hostname_verification']) && $security_config['hostname_verification'] >= 3) { ?>
<li>Create <code>/var/local/keys-sync/.hostnames</code> text file (owned by keys-sync, permissions 0644) with the server's hostname in it</li>
<?php } ?>
</ol>
<h5>Verify Stage 1 success</h5>
<p>Once Stage 1 has been deployed to your server, trigger a resync from SSH Key Authority. The server should no longer have the "Key directory does not exist" warning after syncing (the "Using legacy sync method" warning is expected at this point instead). You can check the contents of the <code>/var/local/keys-sync</code> directory to make sure that the access looks right.</p>
Expand Down
25 changes: 22 additions & 3 deletions templates/server.php
Original file line number Diff line number Diff line change
Expand Up @@ -42,11 +42,23 @@
</dd>
</dl>
</form>
<?php if($this->get('server')->ip_address && count($this->get('matching_servers')) > 1) { ?>
<?php if($this->get('server')->ip_address && count($this->get('matching_servers_by_ip')) > 1) { ?>
<div class="alert alert-danger">
<p>The hostname <?php out($this->get('server')->hostname)?> resolves to the same IP address as the following:</p>
<ul>
<?php foreach($this->get('matching_servers') as $matched_server) { ?>
<?php foreach($this->get('matching_servers_by_ip') as $matched_server) { ?>
<?php if($matched_server->hostname != $this->get('server')->hostname) { ?>
<li><a href="/servers/<?php out($matched_server->hostname, ESC_URL)?>" class="server alert-link"><?php out($matched_server->hostname)?></a></li>
<?php } ?>
<?php } ?>
</ul>
</div>
<?php } ?>
<?php if($this->get('server')->rsa_key_fingerprint && count($this->get('matching_servers_by_host_key')) > 1) { ?>
<div class="alert alert-danger">
<p>The server has the same SSH host key as the following:</p>
<ul>
<?php foreach($this->get('matching_servers_by_host_key') as $matched_server) { ?>
<?php if($matched_server->hostname != $this->get('server')->hostname) { ?>
<li><a href="/servers/<?php out($matched_server->hostname, ESC_URL)?>" class="server alert-link"><?php out($matched_server->hostname)?></a></li>
<?php } ?>
Expand Down Expand Up @@ -332,6 +344,11 @@
</div>
</div>
</div>
<div class="form-group">
<div class="col-sm-offset-2 col-sm-10">
<button type="submit" name="edit_server" value="1" class="btn btn-primary">Change settings</button>
</div>
</div>
<?php } else { ?>
<dl>
<dt>Key management</dt>
Expand Down Expand Up @@ -374,6 +391,7 @@
</dd>
<?php } ?>
</dl>
<?php if($this->get('server_admin_can_reset_host_key')) { ?>
<div class="form-group">
<label for="rsa_key_fingerprint" class="col-sm-2 control-label">Host key fingerprint</label>
<div class="col-sm-4">
Expand All @@ -383,12 +401,13 @@
<button type="button" class="btn btn-default" data-clear="rsa_key_fingerprint">Clear</button>
</div>
</div>
<?php } ?>
<div class="form-group">
<div class="col-sm-offset-2 col-sm-10">
<button type="submit" name="edit_server" value="1" class="btn btn-primary">Change settings</button>
</div>
</div>
<?php } ?>
<?php } ?>
</form>
</div>
<div class="tab-pane fade" id="log">
Expand Down
1 change: 1 addition & 0 deletions views/help.php
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
}
$content->set('admin_mail', $config['email']['admin_address']);
$content->set('baseurl', $config['web']['baseurl']);
$content->set('security_config', isset($config['security']) ? $config['security'] : array());

$page = new PageSection('base');
$page->set('title', 'Help');
Expand Down
7 changes: 5 additions & 2 deletions views/server.php
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
$all_servers = $active_user->list_admined_servers();
$all_accounts = $server->list_accounts();
$ldap_access_options = $server->list_ldap_access_options();
$server_admin_can_reset_host_key = (isset($config['security']) && isset($config['security']['host_key_reset_restriction']) && $config['security']['host_key_reset_restriction'] == 0);

if(isset($_POST['sync']) && ($server_admin || $active_user->admin)) {
$server->sync_access();
Expand Down Expand Up @@ -123,7 +124,7 @@
$content->set('exception', $e);
}
}
} elseif(isset($_POST['edit_server']) && $server_admin) {
} elseif(isset($_POST['edit_server']) && $server_admin && $server_admin_can_reset_host_key) {
if($_POST['rsa_key_fingerprint'] == '') $server->rsa_key_fingerprint = null;
$server->update();
redirect('#settings');
Expand Down Expand Up @@ -291,7 +292,8 @@
$content->set('all_users', $all_users);
$content->set('last_sync', $server->get_last_sync_event());
$content->set('sync_requests', $server->list_sync_requests());
$content->set('matching_servers', $server_dir->list_servers(array(), array('ip_address' => $server->ip_address, 'key_management' => array('keys'))));
$content->set('matching_servers_by_ip', $server_dir->list_servers(array(), array('ip_address' => $server->ip_address, 'key_management' => array('keys'))));
$content->set('matching_servers_by_host_key', $server_dir->list_servers(array(), array('rsa_key_fingerprint' => $server->rsa_key_fingerprint, 'key_management' => array('keys'))));
$content->set('all_groups', $all_groups);
$content->set('all_servers', $all_servers);
$content->set('all_accounts', $all_accounts);
Expand All @@ -300,6 +302,7 @@
$content->set('email_config', $config['email']);
$content->set('inventory_config', $config['inventory']);
$content->set('default_accounts', isset($config['defaults']['account_groups']) ? $config['defaults']['account_groups'] : array());
$content->set('server_admin_can_reset_host_key', $server_admin_can_reset_host_key);
switch($server->sync_status) {
case 'sync success': $content->set('sync_class', 'success'); break;
case 'sync warning': $content->set('sync_class', 'warning'); break;
Expand Down

0 comments on commit c184b03

Please sign in to comment.