search

zestsoftware / zest.releaser

Python software releasing made easy and repeatable
https://zestreleaser.readthedocs.io
GNU General Public License v2.0
198 stars 62 forks source link

Use list of arguments for execute_command, not a string #233

Closed htgoebel closed 6 years ago

htgoebel commented 7 years ago

In #228 the code was changed to quote the path of the git-repos. This is only necessary since the code uses strings as commands, instead of a list of command arguments.

This is bad practice! and may be insecure. Just imaging (in this use case) the path contains a quote-character ". Then the command will fail or even damage things.

Best-practice is to pass a list of arguments to Popen (and siblings) This avoids all quoting issues, since there is no command string which needs to be parsed to split it into arguments.

This may not be an issue in the case of cloning the repo, but it is an issue in general cases.

reinout commented 7 years ago

It isn't an issue in this specific case as we're just passing the current directory. There's no user input here. Quote characters in a directory name? Then you're really trying to break things :-)

Best practice: I think you're right. Fixing it everywhere in the code will probably be quite a lot of work, however.

I'm not inclined to do it as the only person that can break things is the one that's actually sitting behind its own keyboard. If you're experimenting with directory names with "rm -rf /" embedded/encoded in a directory name... I'd say that's a corner case.

I learned something: I never thought about Popen's lists as something that solves quoting issues. Makes sense. You've probably made my future code safer :-)

htgoebel commented 7 years ago

Then you're really trying to break things :-)

That's what attackers/hackers do :-)

I'd say that's a corner case.

It's always corner-cases, attackers and hackers are exploiting. Things where nobody thought they could ever happen. And then – peng – it happens. Just imagine somebody is using zest.releaser in an online-tool. The tool prohibits execution of commands. But due to some bug it allows to manipulate the repo's origin. Then the corner-case occurs. This is how all these (SQL-) injection vulnerabilities arise.

I have to admit that this has no top priority, thought :-)

You've probably made my future code safer :-)

You are welcome :-)