On Fri, 2020-04-24 at 10:37 -0700, Bradford Boyle wrote: > It looks like it currently is using `exec.Command` to call `/bin/sh` > that is being passed the result of the shellescaped string.
Thanks for providing the details of what is happening. > Instead of passing through `sh`, just use `exec.Command` on the > string directly? I was suggesting that you replace the string with an array of strings that contains the command being run and the arguments to the command, then pass that to `exec.Command`, bypassing `sh` entirely. > Assuming I've understood correctly, would you recommend including > this change as a patch in the package for now? Or should I try to get > upstream to incorporate the patch first? I think it would be best to include potential patches upstream. After reviewing the code to see why it needs `sh` I see that it loads the notifycommand parameter from ~/.barnard.yaml, replaces template parameters with their escaped values and then runs the result in sh. On reflection I think my suggestion isn't yet worth the time since: Switching this to not use shell is going to be potentially problematic for compatibility with existing config files. The YAML format for lists is quite a bit more verbose than just a string since it places each argument on one line. You could mitigate that by continuing to use a string but splitting it on spaces, but then that introduces further issues with how to add spaces to arguments. There is also the issue of how to escape the % template character. The systemd ini files have similar issues, so maybe their approach to splitting and escaping could be adopted but the approach is complex and is likely to be confusing to barnard users. https://manpages.debian.org/buster/systemd/systemd.service.5.en.html#COMMAND_LINES Right now there is no security issue with executing commands in barnard because there is no security boundary crossed at this time. If barnard ever adds support for loading config files from the current directory, then executing commands in barnard would become a security problem since someone could check out a potentially untrusted git repository, not check for a barnard config file and then run barnard, leading to barnard executing arbitrary commands provided by the git repo. To make the config file loading safe it could ignore the notify_command setting in anything other than the user's home dir or in dirs that the config in the user's home dir indicates are safe. The same issues apply whether or not shell is used though. -- bye, pabs https://wiki.debian.org/PaulWise
signature.asc
Description: This is a digitally signed message part
