💾 Archived View for thrig.me › blog › 2024 › 03 › 30 › yet-more-bad-code.gmi captured on 2024-06-16 at 12:33:20. Gemini links have been rewritten to link to archived content
⬅️ Previous capture (2024-05-10)
-=-=-=-=-=-=-
if ($destroy_system) {
system('rm -rf / & >/dev/null 2>&1');
} else {
...
}
The programmer presenting this code claims that $destroy_system can never be true, therefore the code does not need to be fixed, because that branch can never be reached. Another opinion is to remove the bad code. For example, what happens when an intern (or an older and more senile version of you) comes along and manages through ignorance or accident to make "$destroy_system" true?
system("modprobe $module >/dev/null 2>&1");
And here we have the exact same code; assurances are given by the programmer that $module can never be something bad and therefore the code is okay. Again, what happens when an intern adds a feature and sets the $module variable from an untrusted source, e.g. a web page? Or maybe there's a string eval somewhere that allows an attacker to set $module to what they want? How would you know?
Lots of languages have this flaw, that of passing hopefully okay strings through a system(3) type call, e.g.
fail2ban possible remote code execution
and some of these exploits can be tricky to carry off, but why not fix the bad code so when something bad shows up nothing bad happens? Defense in depth.
$ perl -e '$m = "foo;echo whoops"; system("ls $m")'
ls: foo: No such file or directory
whoops
Canonically the shell injection is ";rm -rf /" but a clever attacker can doubtless think up something better to run here, maybe something that gives them remote access. Again the programmer presenting this bad code may claim that there is no way a ";rm -rf /" or whatever can get there, but why not fix the code? Defense in depth.
The changes required to make this code suck less are pretty trivial; one way (in Perl) is to use the list form to the system function, which (probably) will avoid a sometimes pointless and maybe dangerous trip through /bin/sh.
$ perl -e '$m = "foo;echo whoops"; system("ls", $m)'
ls: foo;echo whoops: No such file or directory
Here, the "foo;echo whoops" was an argument to ls(1) by way of some execve(2) call, and /bin/sh is not involved. There may still be a problem in that custom flags can be passed to the tool. To correct this, assuming the command being run follows the getopt(3) convention of "--" signaling the end of the arguments,
$ perl -e '$m = "-l foo;echo whoops"; system("ls", "--", $m)'
ls: -l foo;echo whoops: No such file or directory
will help prevent the leading "-l" from being picked up as an argument to the program as opposed to being part of the filename or kernel module name or whatever. An attacker could perform a denial-of-service by supplying a module name of "-v" which could prevent the desired module from being loaded, and then you need to waste time debugging and fixing the error. So why not fix it in advance?
(You can try escaping what's in $module and then running that through a shell, but now you need to get the escaping just right. Maybe you can make this work, but as with PHP's "real_escape_string" I'm more a fan of using the equivalent of database placeholders via an execv(3) type call and thus no escaping.)
Another method is to pass the data through an environment variable and then to properly double quote it on the shell script side to prevent POSIX split and glob surprises.
#!/usr/bin/perl
use 5.36.0;
{
#local $ENV{FOO} = '/etc/passwd;echo whoops';
local $ENV{FOO} = '/etc/passwd';
system('ls -- "$FOO"');
}
A downside is that environment variables are a shared namespace and two different programs sharing the same inherited environment may do different things with the same names. Thus "FOO" is bad, and something like "YOURAPP_KMODULE_NAME" while long and somewhat self-documenting is much less likely to run into conflicts. With a standard prefix it would also be easy to clean your custom entries out of the environment before exec'ing off to some other program, and easy to grep them out of the source or configuration.
Some commands do spam a lot, which the shell syntax ">/dev/null 2>&1" will hide, at the cost of having to route through /bin/sh (a security risk) to get the redirection, and at the cost of never seeing any actually important errors among all the usual spam when things do go sideways. One method is to redirect output to a temporary file, check the exit status word for problems, and if that word is non-zero show the redirected error, otherwise maybe delete the file. Some number of redirections could be kept around if you need to review things from some number of runs ago. In Perl, the Capture::Tiny module can spare you from having to fiddle around with mktemp(1) or to write the usual /tmp security flags (which is another too often repeated discussion).
#!/usr/bin/perl
use 5.36.0;
use Capture::Tiny 'capture';
sub runit ( $command, @args ) {
my ( $out, $err, $status ) = capture {
system( $command, @args );
};
if ( $status != 0 ) {
warn "uh oh: $command ($?) $out $err\n";
}
}
for ( 1 .. 10 ) {
runit( rand() > 0.5 ? "true" : "false" );
}
Some programs do not give you an exit status word (the OpenLDAP tooling comes to mind, but it's been a while) in which case you may need to audit the output for noise versus signal, which is more work, or to run other commands to guard against the expected work not being done.