Multiple command injection vulns in schema_diff tool

[Travis McPeak]

These lines in the latest Nova (as of May 1, 2015) are vulnerable to command injection

https://github.com/openstack/nova/blob/master/tools/db/schema_diff.py#L86
https://github.com/openstack/nova/blob/master/tools/db/schema_diff.py#L103
https://github.com/openstack/nova/blob/master/tools/db/schema_diff.py#L117

In this case (https://github.com/openstack/nova/blob/master/tools/db/schema_diff.py#L86 ), if a malicious filename such as "; rm -rf /etc" is provided, the /etc directory will be removed with the privileges of the user running this script.

In this case (https://github.com/openstack/nova/blob/master/tools/db/schema_diff.py#L103), if either a malicious name or filename are provided, the command will be executed with the privileges of the running user.

In this case(https://github.com/openstack/nova/blob/master/tools/db/schema_diff.py#L117), if either a malicious name or filename are provided, the command will be executed with the privileges of the running user.

I'm not familiar enough with the usage of this module to know all of the places these inputs can come from, but presumably it can be used in automation, potentially with elevated privileges. I'm sure the idea of this script is to allow certain functionality, not unrestricted commands. The way this has been developed allows unrestricted command execution by tampering with any of the above mentioned inputs.

[Tristan Cacqueray (tristan-cacqueray)]

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.

[Thierry Carrez (ttx)]

This sounds like a strengthement opportunity. While I agree there is a bug (which should be solved), since this is an admin tool I can't picture any way to reasonably exploit it.

I suggest we open this bug and get it fixed ASAP.

[Travis McPeak (travis-mcpeak)]

Yeah, it really depends on how this is used. If it is only used by admins on trusted data, I'd treat it as a strengthening opportunity. If it is used in automation and any of the data is fed in from an untrusted or less trusted source, there could be a vulnerability.

In absence of more concrete information about its use, the best thing is probably to open it up and fix it ASAP as Thierry suggests.

[Michael Still (mikal)]

I'm not sure I agree here. None of these methods take filenames constructed from user input. The filenames they are passed are based on a timestamp. Do we really think there's a defence in depth option here? If so, then I'd assert that you could say the same thing of https://github.com/openstack/oslo.concurrency/blob/master/oslo_concurrency/processutils.py#L99

[Travis McPeak (travis-mcpeak)]

Michael - the code you mention in processutils is a wrapper around Popen, and one of its parameters is "shell". Setting shell to True will cause the command to run in a shell with shell capabilities, including the ability to "escape" one command and start another with the semicolon character.

When the following is provided as input to to a shell=True call, it executes two commands (ls and rm)

"ls /my/dir; rm /etc/passwd". The issue is that if an input is constructed taking the second part of this "/my/dir; rm /etc/passwd" it will execute a new command. The safe way to use this is to call without shell and parameterize the call. So you call:

POpen(mycommand, my_argument, my_argument2) without running on a shell.

If we're 100% sure that none of the inputs come from an untrusted or less trusted user, then I agree with you - there is no risk. However I'd still claim that we should fix this as:

1) it isn't best secure development practice
2) you never know when the assumption of clean user input might change and a new threat vector is created
3) shelling this command doesn't appear to be required

[Tristan Cacqueray (tristan-cacqueray)]

Unless someone find a usecase where these inputs could be user controlled, then we better remove the OSSA task and open this for proper bug triage (e.g. code hardening).

According to the module documentation, this is unlikely, can a nova-coresec please confirm that ?

[Michael Still (mikal)]

@Travis -- I still don't see the problem here. We have a method in oslo whose purpose is to execute commands. It takes a command line, and if you ask for a shell uses a shell to execute it. So if the caller passes in something with semicolons, they get what it says on the box.

@Tristan -- as a nova-coresec, my reading of the schema diff code is that it is not currently vulnerable. Note as well that there is no reason for the schema diff tool to run as anything other than a normal user. So, I think we should remove the private label from this and make it a wishlist item.

I'm not opposed to someone having a go at making these things safer, but I do think this is not a security emergency.

[Travis McPeak (travis-mcpeak)]

Michael - ok, given that it's designed to run as a normal user with no untrusted input, I'd agree: there is no security emergency.

[Jeremy Stanley (fungi)]

Based on the above discussion, I'm reclassifying this as "E: not a vulnerability" http://security.openstack.org/vmt-process.html#incident-report-taxonomy but have tagged it "security" since it might present a strengthening opportunity (albeit a very minimal one).

[John Garbutt (johngarbutt)]

I see in progress here, can you please link the review to this bug?

[OpenStack Infra (hudson-openstack)]

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

[Sean Dague (sdague)]

really?

Please actually look at the code a little bit before filing security bugs. This is a very specific tool used to compare the state of migrations. It's not installed as part of normal nova process.

[Travis McPeak (travis-mcpeak)]

Hmm, ok. It's still being called with shell=True needlessly, as there are documented safe ways of doing what you're trying to do here: https://security.openstack.org/guidelines/dg_avoid-shell-true.html .

Is it possible that it will be used as part of an automated process that takes input from somewhere that might be user controlled? If so this can lead to code being run on that box.

If nothing else when poor examples like this begin to make their way out of the code at least new developers won't copy these patterns to places that are more mission critical.

That being said, if the idea is that "it isn't run in production so we don't care", I'll certainly file with that in mind in the future.

[OpenStack Infra (hudson-openstack)]

Change abandoned by Gleb Stepanov (<email address hidden>) on branch: master
Review: https://review.openstack.org/276751