Command injection

[Peter-Maguire]

Command injection

[Peter-Maguire]

Why is enabling shell a requirement? You could remove characters from the input like &;| and remove most of the problems.

[mondwan]

Hi all, I have just set shell to False as requested, and I have pushed that commit to master :D

so... let's see whether what will happen afterwards :D

[mondwan]

Patches are available on 0.4.0. Reopen this issue if there are things to follow up

[mondwan]

Yes. It is marked in the README. Since we haven't find a good way to deal with this one, and the need of argument 'other', we mark that risk in README first. Do you have any suggestions on that? 在 2020年10月17日週六 04:32，Peter Maguire <[email protected]> 寫道：

…

The ping input is not sanitised, so something like const response = await ping.promise.probe('localhost;id', {}); Will execute id and print the output in response — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#128>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA5YBIYMR6HXXXU3XDCDTI3SLCUWBANCNFSM4STZOZNQ> .

[eternalharvest]

@mondwan

I'm agree with @UnacceptableUse if there is no reason to enable shell option of child_process#spawn() call.
I wonder why shell option is needed, it seems working without enabling this option.
Please tell me if I am misunderstanding.

[m-rousse]

Enabling the shell option also prevents the error from being raised in lib/ping-promise.js#L64 as it is treated as output on stderr and not as a faulty exit code.
So I’m also in favor of disabling shell here.