Unsufficient check in credentials.pm

[salben21]

Hi,
I recognize that one can request the ssh_root_key's of every zone from every client independently of its zone affiliation by a simple script using getcredentials.awk and allowcred.awk. This is a security issue if you use multiple zones with sshbetweenodes=no to avoid a privilege escalation to other nodes.

It seems that in credentials.pm of the xcatd following checks are missing:

If a ssh_root_key was requested check

• if the client is in the zone of the requested key's zone otherwise reject the request.
• if the sshbetweennodes is set to yes in the requested zone, otherwise reject the request

best

[salben21]

Workaround for xCAT-server/lib/xcat/plugins/credentials.pm from my side

    if ((($parm =~ /^ssh_root_key/) || ($parm =~ /^ssh_root_pub_key/)) && ($foundkeys == 0)) {
        my ($rootkeyparm, $zonename) = split(/:/, $parm);
        my $client_zonename=xCAT::Zone->getmyzonename($client);
        
        if ($zonename) {
            $parm = $rootkeyparm;           # take the zone off
            xCAT::MsgUtils->trace(0, 'I', "credentials: The node ($client) is asking for sshkeys of zone: $zonename.");
            if ($client_zonename eq $zonename) {
              my $sshbetweenodes_allowed = xCAT::Zone->enableSSHbetweennodes($client);
              if (($sshbetweenodes_allowed == 1) || ($parm =~ /^ssh_root_pub_key/)) { # check if sshbetweennodes is allowed or pub key is requested
                $sshrootkeydir = xCAT::Zone->getzonekeydir($zonename);
                if ($sshrootkeydir == 1) {      # error return
                    xCAT::MsgUtils->trace(0, 'W', "credentials: The zone: $zonename is not defined.");
                } else {
                    $foundkeys = 1;    # don't want to read the zone data twice
                }
              } else {
                  xCAT::MsgUtils->trace(0, 'E', "credentials: Not allowed to read root's private ssh key because sshbetweennodes is disabled.");
                  $sshrootkeydir = 1;
              }
            } else {
                xCAT::MsgUtils->trace(0, 'E', "credentials: Not allowed to read root's private ssh key of different zone.");
                $sshrootkeydir = 1;
            }
        } elsif ($client_zonename) { # check if no zonename is submitted but node is in a zone
            xCAT::MsgUtils->trace(0, 'E', "credentials: Not allowed to read root's private ssh key of default zone.");
            $sshrootkeydir = 1;
        }
    }

instead of

    if ((($parm =~ /^ssh_root_key/) || ($parm =~ /^ssh_root_pub_key/)) && ($foundkeys == 0)) {
        my ($rootkeyparm, $zonename) = split(/:/, $parm);
        if ($zonename) {
            $parm = $rootkeyparm;           # take the zone off
            xCAT::MsgUtils->trace(0, 'I', "credentials: The node ($client) is asking for sshkeys of zone: $zonename.");
            $sshrootkeydir = xCAT::Zone->getzonekeydir($zonename);
            if ($sshrootkeydir == 1) {      # error return
                xCAT::MsgUtils->trace(0, 'W', "credentials: The zone: $zonename is not defined.");
            } else {
                $foundkeys = 1;    # don't want to read the zone data twice
            }
        }
    }

[salben21]

By the way: The argument in the description of the function enableSSHbetweennodes in the file [https://github.com/xcat2/xcat-core/tree/master/perl-xCAT/xCAT/Zone.pm] is nodename and not zonename

=head3 enableSSHbetweennodes
Arguments:
zonenamenodename
Returns:
1 if the sshbetweennodes attribute is yes/1 or undefined
0 if the sshbetweennodes attribute is no/0
Example:
xCAT::Zone->enableSSHbetweennodes($zonename$nodename);
=cut

[jjohnson42]

BTW, I would be interested on your feedback on the confluent ssh design.

In confluent, no user private keys for root or any other user are ever shared. Same for private host keys, nodes generate their own.

Instead, confluent grants host certificates, sets up host based authentication based on the host certificates, and uses shosts.equiv to implement a variant of 'zones' (in confluent, it's "trustnodes", meaning that you can have asymettric zoning where, for example, the compute could trust the storage without storage trusting the compute).

Further, it implements a context aware where once a node security token is granted, it locks down the interface so that it can't be used, and getting ssh certs requires the initial token.

[jjohnson42]

Oh, and you may want to make this a pull request, it seems straightforward enough to me.

[salben21]

thanks for the fast reply. If you can make a pull request it would be fine (otherwise I have to fill out the papers). Attached is the latest version of my patch for credetials.pm (I've improved the check when the zone is empty) in a zip file. have a nice weekend,

[salben21]

Here the latest release of the changes:

    if ((($parm =~ /^ssh_root_key/) || ($parm =~ /^ssh_root_pub_key/)) && ($foundkeys == 0)) {
        my ($rootkeyparm, $zonename) = split(/:/, $parm);
        my $client_zonename = xCAT::Zone->getmyzonename($client);
        my $default_zonename = xCAT::Zone->getdefaultzone();
        
        if ($zonename) {
            $parm = $rootkeyparm;           # take the zone off
            xCAT::MsgUtils->trace(0, 'I', "credentials: The node ($client) is asking for sshkeys of zone: $zonename.");
            if ($client_zonename eq $zonename) {
                my $sshbetweenodes_allowed = xCAT::Zone->enableSSHbetweennodes($client);
                if (($sshbetweenodes_allowed == 1) || ($parm =~ /^ssh_root_pub_key/)) { # check if sshbetweennodes is allowed or pub key is requested
                    $sshrootkeydir = xCAT::Zone->getzonekeydir($zonename);
                    if ($sshrootkeydir == 1) {    # error return
                        xCAT::MsgUtils->trace(0, 'W', "credentials: The zone: $zonename is not defined.");
                    } else {
                        $foundkeys = 1; # don't want to read the zone data twice
                    }
                } else {
                    xCAT::MsgUtils->trace(0, 'E', "credentials: Not allowed to read root's private ssh key because sshbetweennodes is disabled.");
                    $sshrootkeydir = 1;
                }
            } else {
                xCAT::MsgUtils->trace(0, 'E', "credentials: Not allowed to read root's private ssh key of different zone.");
                $sshrootkeydir = 1;
            }
        } elsif ($client_zonename ne $default_zonename) { # check if no zonename is submitted but node is not in default zone
            xCAT::MsgUtils->trace(0, 'E', "credentials: Not allowed to read root's private ssh key of default zone.");
            $sshrootkeydir = 1;
        }
    }

[salben21]

Regarding confluent: I didn't dig in the code of confluent. Sorry.

[jjohnson42]

Ok, my attempt of incorporating changes as a pull request:
#7247

Broadly speaking, at least for my part I'm working towards a broadly more hardened design in confluent for these features, and if there is interest in discussion of those security design points, let me know. I hope it is found to be a broadly more secure approach.

[besawn]

This issue is fixed in xCAT 2.16.5.