Sanitize dangerous SubCommands and Argument names

[KiNgMaR]

Hi,

we would like to use your excellently designed shell wrapper, but are a bit concerned that injections cannot be completely ruled out if the average programmer is not aware that SubCommands and Arguments are not escaped at all.

Therefore, I would like to suggest the attached changes. The duplicate code in SubCommand and Value will be cleaned up, but I would like to hear your general opinion on this first.

Thanks & Best Regards Ingmar

[adambrett]

I am open to these changes, but need to spend a bit more time digging in to what they do, and what impact they will have, as I don't want to break backwards compatibility! I'll get back to you over the weekend :)

[adambrett]

Just thinking out loud (feedback welcome):

I originally designed the library so that if you were passing in user data to a shell command (which is NOT recommended), you had to escape it yourself, and it's not automagic, I wanted to make it harder work, because I wanted to discourage people from doing it.

That said, I don't object to adding them to the library, but I don't want to add them as the default behaviour.

I don't want to break backwards compatibility, either (even though a < 1.0 API deliberately doesn't imply stability), and I'm pretty sure sub-command is being used for glob patterns like this: https://github.com/adambrett/php-shell-wrapper/issues/10. I'm quite happy with how that works, so I would be apprehensive enforcing this as the default as it would break that use-case.

My immediate thinking would suggest something like "EscapedSubCommand" and "EscapedFlag" classes, for cases where you need to sanitise user input, but that could lead to a lot of duplication, something I'd also like to avoid.

So what about a "SanitisedBuilder" class. It won't change the default behaviour, but it could simply override each of the builder methods, then escape the input before passing it to the parent method.

I do believe that the most secure method should be the default, so could look at making this the default in 1.0 (at some unspecified point in the future).

Let me know your thoughts!

[KiNgMaR]

Hey there,

thanks for the feedback! I pretty much agree with what you said. One amendment is that shell wild cards *?{}, are still allowed in SubCommands (not in Arguments). There's also a test for it. So at least that use case should not be affected :)

I'm still thinking about a better class structure to add the escape functionality into, possibly along the lines of what you proposed.

[adambrett]

Cool, I'll look forward to seeing what you come up with :)

Just wanted to add, when I said:

I do believe that the most secure method should be the default, so could look at making this the default in 1.0 (at some unspecified point in the future).

I believe that the most secure method should be the default, but in this instance, even though the escaped version is more secure in code terms, I think the thing that will lead to the most secure code might be to not escape anything by default, and force developers to think about what they're sending to shell.

I'm still torn on that one!