Command injection

[Peter-Maguire]

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

[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 notifications@github.com 寫道：

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 https://github.com/danielzzz/node-ping/issues/128, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA5YBIYMR6HXXXU3XDCDTI3SLCUWBANCNFSM4STZOZNQ .

[Peter-Maguire]

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

[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.

[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