mkschreder
commented
8 years ago Sounds fair. I do not like the whole idea of passing userdata to shell commands myself. It is just so useful at times to be able to write shell oneliners in lua scripts before processing output that this is allowed using this shell method but I want all such things to go through this single function where we do proper escaping of all the special symbols.
Also, this core lua library is now part of the rpc server backend here: https://github.com/mkschreder/orangerpcd/blob/master/lualib/orange/core.lua so I'm removing obsolete copies of these files from this repo.
As I'm adding more unit tests right now, it would be great to do proper coverage of all possible test cases. I will be adding tests for lua part shortly. Unit tests are currently run by travis: https://travis-ci.org/mkschreder/orangerpcd and coverage is uploaded to coveralls.io: https://coveralls.io/github/mkschreder/orangerpcd.
On 3 Sep 2016 11:24, "nelicacs" notifications@github.com wrote:
As the comment on the line 80 says, the currently used blacklist approach is dangerous. There are many other symbols (e.g. backtick, ampersand etc) and symbol sequences (e.g "$()") that can be used to perform OS command injection attacks. For example using "127.0.0.1 && /bin/cat /etc/shadow" as the host parameter value in /plugins/juci-diagnostics/rpc/diagnostics.lua causes displaying the shadow file contents.
Recommendation: consider using whitelist based approach instead of a blacklist. In case any of the arguments contain a symbol that is not in a specified whitelist, drop all arguments and raise an error.
— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/mkschreder/juci/issues/58, or mute the thread https://github.com/notifications/unsubscribe-auth/AERqiWJFpMdPptwKEniFfTn9hznMTZOxks5qmTzMgaJpZM4J0Q8j .
As the comment on the line 80 says, the currently used blacklist approach is dangerous. There are many other symbols (e.g. backtick, ampersand etc) and symbol sequences (e.g "$()") that can be used to perform OS command injection attacks. For example using "127.0.0.1 && /bin/cat /etc/shadow" as the host parameter value in /plugins/juci-diagnostics/rpc/diagnostics.lua causes displaying the shadow file contents.
Recommendation: consider using whitelist based approach instead of a blacklist. In case any of the arguments contain a symbol that is not in a specified whitelist, drop all arguments and raise an error.