[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

[hartwork]

so no particular security issues that didn't already exist

@henry2cox I guess it would need an environment where (1) the user can influence the filename and (2) cannot normally execute commands on the local box. I'm imagining some web application that uses LCOV in the backend to offer code coverage services to developers. Signalling this as a vulnerability with a CVE would increase the chance to wake up those parties that indeed operate services like that (if any). In case you do support that view, I can take over the https://cveform.mitre.org/ filing to obtain a CVE with question Has vendor confirmed or acknowledged the vulnerability? answered as yes.

CC @hannob

[henry2cox]

The other remaining issue is that the various callbacks are allowed to be either a module or a script.
If a module: the callback is invoked as a list - so no interpretation of names or keys: safe enough, I think - but you do need to verify your callback.
If a script: then the callback is invoked as a string - because there is no way to know what might be passed. In this case: yeah. Carefully crafted name or key could cause trouble.

The workaround would be for the service provider to use only module-based callbacks.

There may be other issues that I haven't thought of.