voxpupuli / puppet-selinux

Puppet Module to manage SELinux on RHEL machines
https://forge.puppet.com/puppet/selinux
Apache License 2.0
48 stars 147 forks source link

selinux_build_module_simple.sh: improve quoting #375

Closed kenyon closed 1 year ago

kenyon commented 1 year ago

If module_name or module_dir had a space, this script would fail.

Also avoid existence test for the tmp dir and use mkdir -p instead.

kenyon commented 1 year ago

Yeah, that's another problem with using a shell script like this, it would have to be tested in acceptance tests.

I found more command lines where filenames should be quoted. Tomorrow I'll see if I can add an acceptance test that fails without this PR.

smortex commented 1 year ago

Thinking about it, the "best" is probably to rely on shellquote to escape shell special chars. I can't really think that somebody able to inject malicious subcommands in a control-repo without validation has no prior root access to the system, but for the sake of best practice it may make sense to prevent such misuse.

kenyon commented 1 year ago

Thinking about it, the "best" is probably to rely on shellquote to escape shell special chars. I can't really think that somebody able to inject malicious subcommands in a control-repo without validation has no prior root access to the system, but for the sake of best practice it may make sense to prevent such misuse.

Hmmmm, the command line is actually already shellquoted: https://github.com/voxpupuli/puppet-selinux/blob/37e1761d6df0a02439416bcc17e384a84249465d/manifests/module.pp#L83

I used a $title with a space in it, which is how I discovered this problem. So the shellquoting protects the exec command line, but I guess the problem is that usage of those arguments inside the script still needs proper quoting. What do you think?