Multiple drivers set insecure file permissions

[Dirk Mueller]

GPFS from various places calls "chmod 666" as root:

./cinder/volume/drivers/gpfs.py: self._execute('chmod', '666', path, run_as_root=True)
./cinder/volume/drivers/gpfs.py: self._execute('chmod', '666', vol_path, run_as_root=True)

the Huawei driver sets 777 permissions as root on some files:

./cinder/volume/drivers/huawei/ssh_common.py: utils.execute('chmod', '777', filepath, run_as_root=True)
./cinder/volume/drivers/huawei/rest_common.py: utils.execute('chmod', '777', filepath, run_as_root=True)

the Scality driver sets 666 permissions on all volumes:

cinder/volume/drivers/scality.py:

    def _create_file(self, path, size):
        with open(path, "ab") as f:
            f.truncate(size)
        os.chmod(path, 0o666)

Similarly, the NFS and NEXENTA driver have an implementation of

def _set_rw_permissions_for_all()

that is being called on all newly created volumes.

[Thierry Carrez (ttx)]

Sounds legit, unless those backends actually rely on 666/777 files to work properly (!?)
Probably needs a common OSSA with bug 1260680.

[John Griffith (john-griffith)]

Checking with the driver maintainers on both of these to see if this is a requirement or not. I'd say it's a valid security issue, but I'll need input from IBM and Huawei in terms of their capabilities and what our options are here.

[John Griffith (john-griffith)]

Propose merging this and https://bugs.launchpad.net/ossa/+bug/1260680 into a single bug if there are no objections?

[John Griffith (john-griffith)]

Proposed patch removes some of the perm changes and uses 660 instead of 666. This has been tested against GPFS functional tests as well.

[John Griffith (john-griffith)]

Any feedback WRT to the proposed patch?

[Thierry Carrez (ttx)]

Added cinder-coresec so that they can +2 the patch by commenting on this bug

[Jay Bryant (jsbryant)]

Thanks for adding me to this bug Thierry. Let me know if there is anything I can do to help get this through.

[Thierry Carrez (ttx)]

@jay: you can review the proposed patch for correctness and say +2 as a comment here :) Thanks!

[Jay Bryant (jsbryant)]

@thierry: I helped get the patch initially put together so I am happy to give a +2 !

[John Griffith (john-griffith)]

Confirmed valid settings from Huawei, and generated a patch with the submitted gpfs fix and the huaweii fix.

[John Griffith (john-griffith)]

Bahh... attachment didn't take.

[Thierry Carrez (ttx)]

Merging bugs

[Thierry Carrez (ttx)]

@Dirk: should we credit a company for your discovery work ?
@John: is Grizzly and/or Havana also affected ?

[John Griffith (john-griffith)]

@Thierry: Yes, Havana as well... Huawei only for Grizzly.

[Thierry Carrez (ttx)]

@John: any chance you can propose backports and attach them here ?

[Dirk Mueller (dmllr)]

@Thierry, Yes looks like I reported this during work time. Please credit 'SUSE' then

[Dirk Mueller (dmllr)]

Please note that the Scality driver is also affected..

[John Griffith (john-griffith)]

As does nexenta... those are the only two others I've found. If there's one I'm missing let me know.

[Dirk Mueller (dmllr)]

NFS seems to do the same (thanks for the hint, the ugo+rw was missed by my initial grep!)

[Thierry Carrez (ttx)]

Waiting for a patch refresh :)

[John Griffith (john-griffith)]

There's some additional complexities here as I try and fix up the other items. I think we need to look at changing these drivers and nova to use a common UID to fix this properly. Either that or I'm considering if we can just change the set operation to not be done with root privs.

In other words, perhaps if we just set "run_as_root=False" perhaps that would be sufficient? I need to tweak a number of things that are creating as root, but I think this might be the best answer for the short term.

[Thierry Carrez (ttx)]

A few remarks...

If the files are all owned by the current UID, there is no need for running as root to change file permissions. Also the setup is not really more secure by having those files owned by root instead of the cinder UID. You still need to avoid 777s though.

It's always good to have less stuff running as root. Ideally you'd have nothing at all.

Using common UIDs is a more significant change that we probably won't be able to backport. So if running without root privs here solves the issue, I see only benefits to that approach. i would even question why that was run with root privileges in the first place...

[Thierry Carrez (ttx)]

Proposed impact description:

-----------------------
Title: Multiple Cinder drivers set insecure file permissions
Reporter: Dirk Mueller (SUSE)
Products: Cinder
Affects: All supported versions

Description:
Dirk Mueller from SUSE reported that GPFS, Huawei, Scality and Nexenta Cinder drivers were setting insecure file permissions for various files. A local attacker with shell access to the Cinder host could read and write those files, which may result in disclosure or corruption of block storage users information. Only Cinder setups using the GPFS, Huawei, Scality or Nexenta drivers are affected.

[Jeremy Stanley (fungi)]

Ignore my English grammar nits if you like... The mix of verb tenses in there makes this a little hard to follow. Also, the possessive of a plural common noun ending in "s" gets its apostrophe at the end of the word. How about:

Dirk Mueller from SUSE reported that GPFS, Huawei, Scality and Nexenta Cinder drivers set insecure file permissions for various files. A local attacker with shell access to the Cinder host can read and write those files, which may result in disclosure or corruption of block storage users' information. Only Cinder setups using the GPFS, Huawei, Scality or Nexenta drivers are affected.

[Thierry Carrez (ttx)]

Proposed impact description based on Jeremy's suggestion:

-----------------------
Title: Multiple Cinder drivers set insecure file permissions
Reporter: Dirk Mueller (SUSE)
Products: Cinder
Affects: All supported versions

Description:
Dirk Mueller from SUSE reported that GPFS, Huawei, Scality and Nexenta Cinder drivers set insecure file permissions for various files. A local attacker with shell access to the Cinder host can read and write those files, which may result in disclosure or corruption of block storage users' information. Only Cinder setups using the GPFS, Huawei, Scality or Nexenta drivers are affected.

[Dirk Mueller (dmllr)]

I don't think the impact description is fully accurate:

- you don't need shell access.. any user access on the host is fine (also via exploits of potential other daemons)
- by default, the nfs mounts (netapp and NFS driver, others not checked) are mounted under $service_state_path/mnt, which is for proper packaged systems something like /var/lib/nova/mnt. /var/lib/nova is 0755 nova:nova for me, as is /mnt. That is probably part of the issue: that dir should be 0750 service:group-shared-with-other-services-that-need -access
- the problem is not restricted to GPFS, Huawei, Scality and Nexenta. I can reproduce it with plan-NFS and NetApp/NFS driver as well. Basically anything that derives from the NFS driver.

[Dirk Mueller (dmllr)]

I think the main advantage of running chmod as root is that it always works. as non-root, you can only do that when you own the file (which might not be the case in certain problematic configurations, like files being created on one host with uid1 and mounted again on another server with uid2).

I was originally scared by allowing rootwrap to run "chmod" on anything, thats how I looked into this issue. but not running chmod as root but as normal user doesn't solve the problem fully.

I'd say removing the rootwrapping for chmod is a hardening issue and lesser a security issue in itself.

[John Griffith (john-griffith)]

This has unfortunately turned out to be a bit of a bear, as Dirk pointed out every NFS/Share based driver has this problem. The backporting is a bit of a mess to say the least.

I had initially thought that using a common UID was the correct answer but that's proving to be difficult. Removing root is another option, but the trouble I'm running in to is combing through and verifying that the the volumes are created properly in order to allow this.

At this point I'm sort of stuck determining the best way to proceed. Thoughts from the rest of the security team?

[Thierry Carrez (ttx)]

Let's work on impact description when we actually know the full scope of this ;)

[Thierry Carrez (ttx)]

Added Rob and Bryan from the OSSG so that they can suggest ways to fix this mess.
Rob, Bryan, feel free to selectively subscrobe other OSSG members who would have relevant expertise here.

[Robert Clark (robert-clark)]

Thanks for the add, not the easiest issue to work out.

A common UUID makes a lot of sense but as pointed out, backporting is difficult. I'll need to chew this over a little, it may well be this is one of those things that it's going to hurt to fix. Sad that so many people made the same mistake in their drivers.

[John Griffith (john-griffith)]

IMO backporting isn't just difficult, it's a bit unrealistic. Looking at a number of things here it's a significantly heavy lift to fix all of these particularly since a number of the vendors have zero activity in the community.

I'm wondering if it would be worth proposing a public security advisory stating "use the shares drivers at your own risk" and of course trim that down as vendors implement an updated/secure version of their driver?

Either that or my other preference is just deprecate all the shares based drivers :) (ok, I'm only kidding... kinda)

[Thierry Carrez (ttx)]

That's one option. We would issue an OSSN stating that those specific drivers have flaws in their design that make them vulnerable to local attacks, and encourage people to use different drivers.

So 3 options, basically:
1- Fix in master and somehow backport the fix in a stable-branch-update compatoible and upgradeable way (?): issue OSSA
2- Fix in master but don't backport the fix: issue OSSN stating we fixed it in icehouse but the flawed design persists in <=Havana
3- Don't fix it but acknowledge the ongoing weakness in an OSSN (like what we did for internal cleartext communications between nodes)

Option 3 could come with some deprecation plan for lousy drivers.

[Dirk Mueller (dmllr)]

I'm not sure if it is realistic to deprecate NFS based drivers.

Is it realistic to expect POSIX ACL support to be available? This might avoid the shared uid/gid issue with nova. We can simply set an acl on the compute node to allow rw for the local nova user.

[Duncan Thomas (duncan-thomas)]

For sensibly set up systems with common uids, you can just change the mask
to be 500 can't you? Adding that as a config option would enable many
people to secure their systems I think...
On Jan 29, 2014 6:01 PM, "Thierry Carrez" <email address hidden>
wrote:

> That's one option. We would issue an OSSN stating that those specific
> drivers have flaws in their design that make them vulnerable to local
> attacks, and encourage people to use different drivers.
>
> So 3 options, basically:
> 1- Fix in master and somehow backport the fix in a stable-branch-update
> compatoible and upgradeable way (?): issue OSSA
> 2- Fix in master but don't backport the fix: issue OSSN stating we fixed
> it in icehouse but the flawed design persists in <=Havana
> 3- Don't fix it but acknowledge the ongoing weakness in an OSSN (like what
> we did for internal cleartext communications between nodes)
>
> Option 3 could come with some deprecation plan for lousy drivers.
>
> --
> You received this bug notification because you are a member of Cinder
> Core security contacts, which is subscribed to the bug report.
> https://bugs.launchpad.net/bugs/1260679
>
> Title:
> Multiple drivers set insecure file permissions
>
> Status in Cinder:
> In Progress
> Status in Cinder grizzly series:
> New
> Status in Cinder havana series:
> New
> Status in OpenStack Security Advisories:
> Confirmed
>
> Bug description:
> GPFS from various places calls "chmod 666" as root:
>
> ./cinder/volume/drivers/gpfs.py: self._execute('chmod', '666',
> path, run_as_root=True)
> ./cinder/volume/drivers/gpfs.py: self._execute('chmod',
> '666', vol_path, run_as_root=True)
>
> the Huawei driver sets 777 permissions as root on some files:
>
> ./cinder/volume/drivers/huawei/ssh_common.py: utils.execute('chmod',
> '777', filepath, run_as_root=True)
> ./cinder/volume/drivers/huawei/rest_common.py: utils.execute('chmod',
> '777', filepath, run_as_root=True)
>
> the Scality driver sets 666 permissions on all volumes:
>
> cinder/volume/drivers/scality.py:
>
> def _create_file(self, path, size):
> with open(path, "ab") as f:
> f.truncate(size)
> os.chmod(path, 0o666)
>
> Similarly, the NFS and NEXENTA driver have an implementation of
>
> def _set_rw_permissions_for_all()
>
> that is being called on all newly created volumes.
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/cinder/+bug/1260679/+subscriptions
>

[Bryan D. Payne (bdpayne)]

Looking at Thierry's options... I'm leaning towards something like #2. But that does create three additional questions:

1) How should this be fixed in master?

2) What specific / actionable recommendations can be made in the OSSN to help people improve their systems?

3) What can be done to prevent similar problems from happening in the future?

A few thoughts...

On (1), I think that the program that creates a file should do so with proper permissions and be careful to avoid a race in the permissions setting. So the fix should be in the code around the creation of the file. The fix should not be in the packaging or as deployment guidance, etc.

On (2), I think that it may be useful to mention things like SELinux and ACLs and how they can help. But I think we should also provide guidance for people working on systems without those options. Can it be as easy as validating / correcting file permissions through a configuration management system like chef, puppet, etc? Or am I missing some complexities here?

On (3), I think that this is a broader topic. But I'd like to see thought around how we could detect something like this in testing.

[Robert Clark (robert-clark)]

+1 for an OSSN. Fits the bill.

We can point to the already reasonably good documentation on MAC in the openstack security guide.

[John Griffith (john-griffith)]

@Robert Clark
Which option of the OSSN? Number 2 or 3?

My vote is probably number 2, that's the better case but willing to punt to 3 and still not heart broken. It seems that it's not just as easy as "go in and change the permissions" apparently some devices are expecting specific settings here that may not match with others.

[Jay Bryant (jsbryant)]

I would vote for option 2. I think we need to fix Master at least. Security advisory for Havana should be fine.

[Robert Clark (robert-clark)]

Sorry, I should have been clearer.

I vote for option 2. This is something that has to be fixed moving forward. It's too difficult to fix previous releases which is where an OSSN comes into play, providing guidance on how to secure previous releases.

[Thierry Carrez (ttx)]

OK, unless someone complains soon, I think we should open this bug and let the OSSG group work on a OSSN for this.

This issue shall join the "unencrypted internal communications" in the pile of known faulty design with security implications.

[Jeremy Stanley (fungi)]

Agreed, this is better handled in the open. The sooner we make the bug report public the sooner the OSSG can get started on any necessary documentation around it.

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

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

commit b0ac8294dd9d8dbddfbb320c62d7264daf55be26
Author: Bill Owen <email address hidden>
Date: Wed Feb 12 17:37:36 2014 -0700

    Update gpfs driver volume creation process

    Modify gpfs driver to set file permissions in
    a more consistent way.

    Modify image_utils.resize_image to allow caller
    to request it be run as root.

    SecurityImpact
    Change-Id: Ic01d91c0d660c74095e8d2b212279b39b9b9dc05
    Partial-Bug: #1260679

[Jay Bryant (jsbryant)]

Removing Bill Owen as the assignee given that this is now resolved for GPFS.

[Nathan Kinder (nkinder)]

Published as OSSN-0014 on the wiki and the openstack and openstack-dev mailing lists:

https://wiki.openstack.org/wiki/OSSN/OSSN-0014

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/107693
Committed: https://git.openstack.org/cgit/openstack/cinder/commit/?id=6879bd0720b2c4c5ef4d2f2c42fe0e4e436ba998
Submitter: Jenkins
Branch: master

commit 6879bd0720b2c4c5ef4d2f2c42fe0e4e436ba998
Author: Glenn M. Gobeli <email address hidden>
Date: Thu Jun 12 09:31:25 2014 -0400

    NFS Security Enhancements: allows secure NFS environment setup

    This patch allows an OpenStack environment to run as a secure NAS
    environment from the client and server perspective, including having
    root squash enabled and not running file operations as the 'root'
    user. This also sets Cinder file permissions as 660: removing
    other/world file access.

    The "nas_secure_file_permissions" option controls the setting of file
    permissions when Cinder volumes are created. The option defaults to
    "auto" to gracefully handle upgrade scenarios. When set to "auto",
    a check is done during Cinder startup to determine if there are
    existing Cinder volumes: no volumes will set the option to 'true',
    and use secure file permissions. The detection of existing volumes will
    set the option to 'false', and use the current insecure method of
    handling file permissions.

    The "nas_secure_file_operations" option controls whether file
    operations are run as the 'root' user or the current OpenStack
    'process' user. The option defaults to "auto" to gracefully handle
    upgrade scenarios. When set to "auto", a check is done during Cinder
    startup to determine if there are existing Cinder volumes: no volumes
    will set the option to 'true', be secure and do NOT run as the 'root'
    user. The detection of existing volumes will set the option to 'false',
    and use the current method of running operations as the 'root' user.
    For new installations, a 'marker file' is written so that subsequent
    restarts of Cinder will know what the original determination had been.

    This patch enables this functionality only for the NFS driver.
    Other similar drivers can use this code to enable the same
    functionality with the same config options.

    DocImpact
    Change-Id: I3d25f593beab7f5462576b14ab62d13d8c53e7c6
    Implements: blueprint secure-nfs
    Partial-Bug: 1260679