Multiple vulnerabilities in Couchbase implementation of restore strategy

[Travis McPeak]

In the Couchbase implementation of restore in Trove, the following takes place:

1) pre-restore removes previous temp directory contents here: https://github.com/openstack/trove/blob/master/trove/guestagent/strategies/restore/couchbase_impl.py#L45

2) run_restore of the parent class is executed which puts together and runs the following command:
"openssl enc -d -aes-256-cbc -salt -pass pass:%s | gzip -d -c | sudo tar xPf -"

3) post-restore reads the secret key from an extracted file here:
https://github.com/openstack/trove/blob/master/trove/guestagent/strategies/restore/couchbase_impl.py#L54

4) a command is built from inputs from a JSON file in the directory created above starting here:
https://github.com/openstack/trove/blob/master/trove/guestagent/strategies/restore/couchbase_impl.py#L67

5) the command built in step 4 is executed

At no point in this process are permissions explicitly set. Presumably the files will be created based on the umask of the user that Trove runs as. This opens up the possibility for a couple of vulnerabilities:

1) malicious user can read the secret key if the default permissions are world readable (which is pretty common for many system's default umask)

2) malicious user can write arbitrary commands to be executed in step 5 with a shell injection vulnerability if the default permissions are world writeable. An example of this vulnerability would be something like setting bucket type in this line:

https://github.com/openstack/trove/blob/master/trove/guestagent/strategies/restore/couchbase_impl.py#L75

to something like "; rm -rf /etc". The possibilities for exploits are nearly endless as any command that the Trove user can run, the attacker can run.

[Jeremy Stanley (fungi)]

I've added an advisory task in an incomplete state pending feedback from the trove-coresec team, whom I've also just subscribed to this bug.

[Thierry Carrez (ttx)]

It looks like this is a new feature in Juno. If that's confirmed to be the case, then we would just fix this in master before final release, and not issue an advisory about it.

[Travis McPeak (travis-mcpeak)]

Any word on this?

[Nikhil Manchanda (slicknik)]

This is a new feature in Juno, so I think doing what ttx says above makes sense. I'm going to follow up with the author of this feature to figure out a timeline for getting this fixed.

[Jeremy Stanley (fungi)]

In that case, we should also switch this bug to public security since there's no need to embargo a vulnerability only on the master branch, and for which we're not going to be publishing an advisory. If no one objects within the next couple days, I'll open up the bug report.

[Amrith Kumar (amrith)]

Per conversation with Nikhil

[OpenStack Infra (hudson-openstack)]

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

[Amrith Kumar (amrith)]

Found a problem with my proposed fix. This ain't likely to make it for j3.

[Travis McPeak (travis-mcpeak)]

I really think this needs to be fixed prior to release. This could really be a major security issue.

[Nikhil Manchanda (slicknik)]

Yes - it still has time to make it in for the Juno release between J3, and the RC.

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/117174
Committed: https://git.openstack.org/cgit/openstack/trove/commit/?id=89e9a429f4b59801b9acca01bd7dabbeaf91269d
Submitter: Jenkins
Branch: master

commit 89e9a429f4b59801b9acca01bd7dabbeaf91269d
Author: Amrith Kumar <email address hidden>
Date: Wed Aug 27 06:06:21 2014 -0400

    Partially address concerns in Couchbase restore strategy

    This is a preliminary fix for community review. I expect to be making
    some changes as well.

    This change addresses some concerns about the Couchbase restore
    strategy. In particular, the concerns that are addressed include
    string injection in some execute calls, and issues around the
    permissions on the secret file used to store the password.

    This change set aims to fully address the issue around the password
    file but is a partial solution to the string injection
    problem. Additional changes are required to completely address
    that. Some additional bugs have also been raised to address issues
    related to other concerns discovered while fixing this bug. More to
    come.

    Change-Id: Icd8033caea4866f57f8cb4c5861d21797136ff90
    Partial-Bug: #1349939