-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Description
I think that to goal of poUseShell was to allow PATH expansion. Right way to do this is to use execvp, not shell. While docs say that it is unsafe, even standard library doesn't respect that, i.e.:
import browsers
"http://$(echo HACKED > hacked; echo google).com".openDefaultBrowser
The only reasonable thing poUseShell could do is to assert args == nil and pass command straight to /bin/sh, so user can be blamed when he creates shell-injection using '&' operator. If we don't want to introduce backward incompatibility we may alias poUseShell to new flag poUsePath, which will use execvp instead of execv.
By the way, quoteIfContainsWhite also should be removed, because currently it just adds ", which is not safe. Theoretically it could be made to use proper quoting, but I don't think it's possible to do that on Windows.
If what I have written makes sense, I will investigate if secure quoteIfContainsWhite is possible on Windows. If it is I will change its implementation and keep poUseShell. If not quoteIfContainsWhite needs to be removed, because security is more important than backward compatibility.