[Git `HEAD`] `py2lcov` and `xml2lcov`: Use of `subprocess.run([..], shell=True, [..])` allows command injection

[hartwork]

For example:

# ./bin/py2lcov 'with spaces'
/bin/sh: line 1: spaces: command not found
Error:  error during XML conversion of with spaces: Command 'COVERAGE_FILE=with spaces coverage xml -o with spaces.xml' returned non-zero exit status 127.

The two places with this problem are:

lcov/bin/py2lcov

Lines 179 to 182 in 34f05f5

	cmd = 'COVERAGE_FILE=%s coverage xml -o %s' % (f, xml)
	try:
	#x = subprocess.run(cmd, capture_output=True, shell=True, check=True)
	x = subprocess.run(cmd, shell=True, check=True, stdout=True, stderr=True)

and

lcov/bin/xml2lcovutil.py

Lines 124 to 131 in 34f05f5

	cmd = "%(lcov)s -a %(info)s -o %(info)s --version-script '%(vers)s' %(checksum)s--rc compute_file_version=1 --branch-coverage --ignore inconsistent" % {
	'lcov': os.path.join(os.path.split(sys.argv[0])[0], 'lcov'),
	'checksum': "--checksum " if self._args.checksum else '',
	'info': self._args.output,
	'vers' : self._args.version,
	}
	try:
	x = subprocess.run(cmd, shell=True, check=True, stdout=True, stderr=True)

Both cases can and should avoid shell=True and be good.

[hartwork]

@henry2cox pull request #355 did not solve the underlying problem. I was intentionally not saying that we're dealing with command injection here but maybe I should have said that from the beginning. Short article https://security.openstack.org/guidelines/dg_use-subprocess-securely.html#correct is our case with Python. Would you like me to try a pull request myself and for you to be in the review seat this time?

[hartwork]

@henry2cox PS: For proof that the problem persists in similar form (even with #355 merged):

# ./bin/py2lcov "with' 'quotes"
/bin/sh: line 1: quotes: command not found
Error:  error during XML conversion of with' 'quotes: Command 'COVERAGE_FILE='with' 'quotes' 'coverage' xml -o 'with' 'quotes.xml'' returned non-zero exit status 127.

[henry2cox]

I am of the opinion that a user who deliberately puts quotes and other shell-active characters in paths, is pretty much asking to lose.
I don't worry much about them.

But sure - if you have a portable fix in mind, feel free.

BTW: what timezone are you in?
Do you never sleep?

[hartwork]

I am of the opinion that a user who deliberately puts quotes and other shell-active characters in paths, is pretty much asking to lose. I don't worry much about them.

But sure - if you have a portable fix in mind, feel free.

@henry2cox thanks! I'll give it a shot.

BTW: what timezone are you in? Do you never sleep?

😄 I'm in UTC+1 and I do sleep but not yet 😄 What's your timezone?

[henry2cox]

UTC-5, currently.

I think the other aspect is that I'm mostly thinking about (internal) users and various jenkins jobs - which run with the permissions of the user in question, so no particular security issues that didn't already exist. But yeah - I am permitting you to do something nefarious, if you really want to.

[hartwork]

@henry2cox let me first try the pull request and than try answer to that. Starting now…

[hartwork]

@henry2cox ready to review now: #356