Guest Agent - potential shell injection

[Tim Suter]

In troves guest agent backup it appears that unencrypted data is simply piped into openssl for encryption.

https://github.com/openstack/trove/blob/b3e3e6150394330d923d2fdf6a9b3c8f021ba412/trove/guestagent/strategies/backup/base.py#L124

- if the string substitution for -pass: is not correctly handled during shell escaping this could lead to shell injection. safest method is to remove substitution completely.

I recommend using pythons native ssl module (import ssl methods are bindings to system ssl libraries, usually openssl).

Acknowledgments:
Travis Scheponik <email address hidden>

[Tim Suter (tsuter)]

potential shell injection example

ie:
   -pass: derp;cat /etc/passwd
   -pass: derp&cat /etc/passwd

[Jeremy Stanley (fungi)]

Since this report concerns a possible security risk, an incomplete security advisory task has been added while the core security reviewers for the affected project or projects confirm the bug and discuss the scope of any vulnerability along with potential solutions.

This sounds like two individual bugs (which might get fixed together, but are still distinct issues):

1. Concerns over the underlying cryptographic transformation used for securing Trove's guest agent backups. Unless it can be shown that these choices definitely lead to a weakening of the at-rest security, fixing this can be handled as a hardening opportunity (VMT class D).

2. An assertion that untrusted user data used to populate the password field can lead to shell injection, which if true should be treated as a security vulnerability (VMT class A).

https://security.openstack.org/vmt-process.html#incident-report-taxonomy

[Morgan Fainberg (mdrnstm)]

I have split this bug into two separate bugs. I have left the shell-injection here and moved the concerns about the AES CBC cipher to bug #1606419

This was done since the concerns are separate and should be tracked independently for easier management of the vulnerabilities, even if the bugs can both be closed with a single patch series.

[Morgan Fainberg (mdrnstm)]

At first glance this looks like only the deployer can set the encrypt key, meaning only the deployer could cause shell injection.

I used the following searches:

https://github.com/openstack/trove/search?utf8=%E2%9C%93&q=backup_aes_cbc_key

https://github.com/openstack/trove/search?utf8=%E2%9C%93&q=encrypt_key

If this is true this is likely a Class D instead of a Class A. However, I definitely want the Trove Coresec team to make sure my research is in-fact correct.

https://security.openstack.org/vmt-process.html#incident-report-taxonomy

[Amrith Kumar (amrith)]

I believe that the vulnerability (if any) is even smaller than the one described by Morgan above. I will update with details.

[Amrith Kumar (amrith)]

Only the deployer (as Morgan points out) can set the encryption key, and it is in fact sent to the guest when backups are taken and therefore not susceptible to the kinds of injection that the reporter is concerned about. After all, the operator already has access to the data in plain text form if he or she so desires.

What should also be considered is that the execution of code on the guest instance in Trove (or for that matter, any managed service) is considered to be trusted code executing in a protected environment. The only code that can be executed there is code that the deployer has chosen to permit. It is therefore outside the realm of control of any party that is not in control of the deployer side of the OpenStack environment to inject these passwords and things into the guest environment.

I therefore concur with Morgan that this can in fact be treated as a Class D and not a Class A.

[Amrith Kumar (amrith)]

I would like to request a further change from Class D to Class E.

https://security.openstack.org/vmt-process.html#incident-report-taxonomy

I see no way in which one can actually perform encryption without a key, and since the execution environment is a secure one (should be; the operator can do that) and since the operator is the only one who can set this key, I believe that this should be reduced to a Class E.

[Amrith Kumar (amrith)]

I have marked this as Incomplete with the understanding that if Class E is not acceptable someone (the reporter) will provide additional reasons why this is in fact a bug that must be fixed.

[Jeremy Stanley (fungi)]

Amrith: The main distinction between class D and class E is that E is for things you consider an "invalid" or "won't fix" bug report in Trove, while D is for things where you wouldn't reject a fix if one were proposed in case things changed later to make this situation somehow exploitable, even if there's no way to exploit it to any useful end in the current implementation.

Regardless, only class A vulnerabilities by our taxonomy get security advisories published. Unfortunately this bug report refers to a second report which is still under evaluation, so we should leave it set to private security and not open it up nor switch our OSSA task to won't fix until the other bug is ready to be made public.

[Amrith Kumar (amrith)]

Jeremy: I was going by the distinction as I understood it in the taxonomy which describes Class E as, "Not a vulnerability at all", which is what I think this is. Is that wrong?

[Jeremy Stanley (fungi)]

I agree the wording is probably confusingly terse. Class D is meant to cover potential hardening/strengthening opportunity bugs (e.g., would you reject a patch to more safely escape the password parameter when passed to the system shell in case the design of this feature changed later in such a way that it might get passed a value by someone who does not already have root access to the system?), while Class E would be for things that are not bugs at all (intended behavior, mistaken report, et cetera) or are bugs but have no security implications whatsoever (wouldn't be likely to even constitute a contributing factor to a future vulnerability). To that end, I've proposed a clarification to our taxonomy wording: https://review.openstack.org/348979

[Tristan Cacqueray (tristan-cacqueray)]

Because the injection is only exposed to operators, I agree this isn't a class A and this bug could probably be fixed in the open. I've subscribed ossg-coresec to weight on this.

[Travis McPeak (travis-mcpeak)]

Well there's no question that this is at least a hardening opportunity - there is command injection in the password. Unless you want operators to be able to run commands in their passwords as a feature this is a bug.

At a minimum it's an example of insecure coding practices. I agree the impact is small because there's no ability to execute commands the operator couldn't otherwise execute directly.

On a side note command injection and other similar vulnerabilities are exactly what Bandit was created to find. In fact I personally found and filed the same issue two years ago:

https://bugs.launchpad.net/trove/+bug/1349939

I'd like to see these issues fixed or at least commented why they won't be fixed so we stop consuming cycles finding and triaging the same issues.

[Amrith Kumar (amrith)]

Travis, comments about why this is a low priority are above; see comment of 2016-07-26. Let's discuss, if the issue is more severe than I'm understanding, I'll most definitely fix it. The previous bug you referenced did result in changes at this very line, just not this one part; the provision of the password.

[Tristan Cacqueray (tristan-cacqueray)]

Closing the OSSA task and marking this bug public, reason: impact is small and it's a D type of bug according to VMT taxonomy ( https://security.openstack.org/vmt-process.html#incident-report-taxonomy ).

[Launchpad Janitor (janitor)]

[Expired for OpenStack DBaaS (Trove) because there has been no activity for 60 days.]