Skip to content
This repository has been archived by the owner on Oct 16, 2024. It is now read-only.

added shell=True to subprocess.check_return call #1855

Merged
merged 2 commits into from
Jul 9, 2015

Conversation

Babbleshack
Copy link
Contributor

SIMON
when running the subprocess without a shell, python fails to launch the ceph process.
I believe this is because python fails to a shared object which is part of the 'rados' library.

I did some digging and found
rochaporto/collectd-ceph#20

this issue is almost the same as what we have here, only the error code is differnt OSError, instead of IOError.

In the collectd-python docs I found
collectd handles SIGCHLD. This means that Python won't be able to determine the return code of spawned processes with system(), popen() and subprocess. This will result in Python not using external programs like less to display help texts. You can override this behavior with the PAGER environment variable, e.g. export PAGER=less before starting collectd. Depending on your version of Python this might or might not result in an OSError exception which can be ignored.

The problem is egnoring the output results in a null string.

access("/etc/ld.so.nohwcap") = -1 ENOENT (No such file or directory)` appears multiple time in the strace, 'ld' is a linker program.

So unless some linking occurs when collectd runs the python script, which would make sense, am not sure what is happening here.

Finally from the python docs.

Warning Executing shell commands that incorporate unsanitized input from an untrusted source makes a program vulnerable to shell injection, a serious security flaw which can result in arbitrary command execution. For this reason, the use of shell=True is strongly discouraged in cases where the command string is constructed from external input:

Executing shell commands that incorporate unsanitized input from an untrusted source makes a program vulnerable to shell injection

Our source input source is a split string hard coded by me...

And so I dont see this as a true 'vulnerability'

Please come over and advice me, if I am just being silly :)

@spjmurray
Copy link
Member

fine sigh

@seanhandley
Copy link
Member

👍 Seems legit

@bilco105
Copy link
Contributor

bilco105 commented Jul 9, 2015

tl;dr 👍

Babbleshack added a commit that referenced this pull request Jul 9, 2015
…ugin

added shell=True to subprocess.check_return call
@Babbleshack Babbleshack merged commit 573d819 into master Jul 9, 2015
@spjmurray spjmurray deleted the adjust_ceph_call_in_iops_plugin branch August 18, 2015 12:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants