Cinder utils SSHPool should allow customized ssh host keys and missing policy

Bug #1320056 reported by Jia Ming Zhang on 2014-05-16
8
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Cinder
High
Jia Ming Zhang
OpenStack Security Advisory
Undecided
Unassigned
OpenStack Security Notes
High
Tim Kelsey

Bug Description

In cinder/utils.py, SSHPool is using paramiko.AutoAddPolicy() as default. This may lead security issue without being notified. The utility should allow customized usage when create the pool or session. Also the host_keys file should be allowed to be customized so that any driver utilizing the SSHPool should have their customized security setting or delegate to customer's scenario & configuration to determine the policy and key files.

Jia Ming Zhang (jmzhang) on 2014-05-16
information type: Private Security → Public Security
Jia Ming Zhang (jmzhang) on 2014-05-16
Changed in cinder:
assignee: nobody → Jia Ming Zhang (jmzhang)

Fix proposed to branch: master
Review: https://review.openstack.org/93867

Changed in cinder:
status: New → In Progress
Jay Bryant (jsbryant) on 2014-05-19
Changed in cinder:
importance: Undecided → High
tags: added: icehouse-backport-potential

Fix proposed to branch: master
Review: https://review.openstack.org/94165

Mike Perez (thingee) on 2014-05-19
Changed in cinder:
milestone: none → juno-1

The OSSA task is incomplete pending additional details from security reviewers.
Can a cinder-coresec member confirm this one ? Would it be possible to enforce host key check or is it part of the design?

Changed in ossa:
status: New → Incomplete

@cinder-coresec What could be the consequences of someone spoofing a ssh server host for cinder ?

Jay Bryant (jsbryant) wrote :

@tristan ... It is possible for users to configure the connection to their backend storage to use a username and password so a person doing a MITM attack could collect username and password to gain access to the user's storage backend.

If that were to happen things could really go down hill. They would potentially be able to harvest sensitive data off the storage backend by making volumes available to compromised nodes or even worse they could plant malicious code in existing or spoofed volumes that could infect people's VMs.

I don't know how likely it is that something like that would happen but it seems that it would be best to not just ignore host key failures.

Changed in ossa:
status: Incomplete → Confirmed
assignee: nobody → Tristan Cacqueray (tristan-cacqueray)
Changed in cinder:
assignee: Jia Ming Zhang (jmzhang) → Jay Bryant (jsbryant)
Changed in cinder:
assignee: Jay Bryant (jsbryant) → Jia Ming Zhang (jmzhang)

Reviewed: https://review.openstack.org/94165
Committed: https://git.openstack.org/cgit/openstack/cinder/commit/?id=39081787963298be87c1941ab6cba5310f6261c6
Submitter: Jenkins
Branch: master

commit 39081787963298be87c1941ab6cba5310f6261c6
Author: Lynxzh <email address hidden>
Date: Mon May 19 18:47:16 2014 +0800

    SSHPool in utils should allow customized host key missing policy

    The cinder/utils SSHPool should allow missing key policy and host key
    file being customized so that any caller can determine by their own
    scenario if the host key file can be customized, or if an 'AutoAdd' is
    appropriate, or just reject the key when mismatch. This will give more
    flexible customization and also prevent any security issue as a middle
    man.

    Closes-Bug: #1320056
    Change-Id: I3c72b0d042de719ecd45429d376bd88d0aefb2cc

Changed in cinder:
status: In Progress → Fix Committed
Thierry Carrez (ttx) wrote :

I'm not totally convinced by OSSA need yet.

The only thing that patch does is to honor extra arguments if they are passed to the SSH pool function. But no driver actually passes those arguments, so the patch doesn't "fix" any vulnerability. It just exposes a new feature for driver authors.

The vulnerability fix would be in the calling drivers, and would make use of that new feature, most likely by introducing new configuration options. It would be good to consider case-by-case each driver to see if their use of SSH pools is actually vulnerable to anything.

Changed in ossa:
assignee: Tristan Cacqueray (tristan-cacqueray) → nobody
status: Confirmed → Incomplete
Jay Bryant (jsbryant) wrote :

@Thierry, this change was put in to support the following change: https://review.openstack.org/#/c/94258/ . So, one driver would be making use of this and I would like to get it in to the first icehouse fix pack. As far as the need for an OSSA. I am ok with waiting on that. I think the situation where this would happen is pretty unlikely, but should be, eventually, addressed.

Thierry Carrez (ttx) wrote :

OK, so this would be considered a missing security feature (weak security for internal communications). That needs to be fixed in future versions for sure. But we typically don't issue OSSA for those.

Changed in ossa:
status: Incomplete → Won't Fix
tags: added: security
information type: Public Security → Public
Robert Clark (robert-clark) wrote :

We should probably document this with an OSSN as deployers may have an incorrect understanding of the security offered with this feature

Change abandoned by Jay Bryant (<email address hidden>) on branch: stable/icehouse
Review: https://review.openstack.org/94711
Reason: It was decided that we will not backport this. A Blueprint is being created to get this fixed in Juno.

Thierry Carrez (ttx) on 2014-06-11
Changed in cinder:
status: Fix Committed → Fix Released
Changed in ossn:
assignee: nobody → Tim Kelsey (tim-kelsey)
Jay Bryant (jsbryant) on 2014-06-16
Changed in cinder:
milestone: juno-1 → juno-2
Changed in ossn:
status: New → In Progress
Tim Kelsey (tim-kelsey) wrote :

Hello all, so im working on the OSSN documenting this issue. I would just like to confirm that the plan for Juno is to update the relevant drivers so that by default they don't use the SSHPool auto-accept policy?

Duncan Thomas (duncan-thomas) wrote :

@Tim Kelsey: I think the plan is to make the policy configurable, with auto-add (but fail if changed) as the default, which is secure enough for most people but can be bumped up by sufficiently paranoid installed who do the work to collect the keys first.

Matthew Edmonds (edmondsw) wrote :

@duncan-thomas: The decision in IRC was that it would be ok to default to a special policy where we auto-add on first connect only and then reject thereafter. But that assumes it's possible to distinguish a first connect, and I'm not sure that's possible. Lacking that, the default needs to be a normal reject policy.

First connect means 'we haven't cached the key yet'.... that's the
only sane definition it the ssh world.

On 24 June 2014 14:34, Matthew Edmonds <email address hidden> wrote:
> @duncan-thomas: The decision in IRC was that it would be ok to default
> to a special policy where we auto-add on first connect only and then
> reject thereafter. But that assumes it's possible to distinguish a first
> connect, and I'm not sure that's possible. Lacking that, the default
> needs to be a normal reject policy.
>
> --
> You received this bug notification because you are a member of Cinder
> Bug Team, which is subscribed to Cinder.
> https://bugs.launchpad.net/bugs/1320056
>
> Title:
> Cinder utils SSHPool should allow customized ssh host keys and missing
> policy
>
> Status in Cinder:
> Fix Released
> Status in OpenStack Security Advisories:
> Won't Fix
> Status in OpenStack Security Notes:
> In Progress
>
> Bug description:
> In cinder/utils.py, SSHPool is using paramiko.AutoAddPolicy() as
> default. This may lead security issue without being notified. The
> utility should allow customized usage when create the pool or session.
> Also the host_keys file should be allowed to be customized so that any
> driver utilizing the SSHPool should have their customized security
> setting or delegate to customer's scenario & configuration to
> determine the policy and key files.
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/cinder/+bug/1320056/+subscriptions

--
Duncan Thomas

Jay Bryant (jsbryant) wrote :

Matt, Duncan and Tim,

Here is an excerpt from the spec I wrote for this change: https://review.openstack.org/#/c/100697/1/specs/juno/configurable-ssh-host-key-policy.rst

  41
   The solution would require two new configuration items as well as 42
   a change to the current default behavior. First, there would need 43
   to a 'strict_ssh_host_key_policy' configuration option with possible 44
   settings of 'false' (default) or 'true'. When this option is set to 45
   'false' it will automatically accept the host key on the first connection 46
   and then will throw an exception if the host key changes in the future. 47
   This is where the default behavior changes from the current functionality. 48
    49
   In the case that 'strict_ssh_host_key_policy' is set to 'true' then a 50
   second option 'ssh_host_keys_file' must be configured. When the strict 51
   configuration is used it is assumed that the administrator is going to 52
   have pre-configured ssh host keys and any deviation from those expected 53
   keys will be handled with an exception.

So, that was the plan that John approved though he noted that he is mixed on the default proposal.

The agreement in IRC was that we would reject a connection if it changed after the host key was added to known hosts. As far as first connection is concerned, it looks like I get get a list of known keys with get_host_keys . If we don't have the key yet we accept it. If we do have the key we do strict checking on it.

Duncan Thomas (duncan-thomas) wrote :

@Jay: Works for me

Tim Kelsey (tim-kelsey) wrote :

Thanks for the information all, @Jay: thanks for the detail and additional background as well.

Robert Clark (robert-clark) wrote :

OSSN issued, congratulations Tim!
https://wiki.openstack.org/wiki/OSSN/OSSN-0019

Changed in ossn:
importance: Undecided → High
status: In Progress → Fix Released
Thierry Carrez (ttx) on 2014-10-16
Changed in cinder:
milestone: juno-2 → 2014.2
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers