Robustifying R_CleanTempDir a bit more

Hello,

This is probably a very minor point, but R_CleanTempDir may still have
a shell injection in it. I couldn't find a way to shoot the user in the
foot in a significant way (by, say, accidentally removing ~), thanks to
R disallowing spaces in the path, but if Sys_TempDir somehow acquires a
value of "/tmp/';echo;'", R_CleanTempDir() will remove /tmp instead of
its aptly-named subdirectory.

While adding the single-quote symbol to the list of special symbols
should suffice (it and the backslash being the only allowed ways to
"un-quote" a single-quoted string), I would like to suggest solving the
problem without the use of quoting:

#include <spawn.h>

char ** argv = { "rm", "-Rf", Sys_TempDir, NULL };
posix_spawnp(NULL, "rm", NULL, NULL, argv, NULL);

Are there Unix-like platforms on which R is intended to work that don't
have posix_spawn()? Circa-2014 versions of both Solaris and OpenBSD
seem to have it. Spawning the process manually by means of [v]fork()
and exec() is probably not worth the maintainer effort required to
perform it correctly.

On 2/16/23 15:09, Ivan Krylov wrote:

Hello,

This is probably a very minor point, but R_CleanTempDir may still have
a shell injection in it. I couldn't find a way to shoot the user in the
foot in a significant way (by, say, accidentally removing ~), thanks to
R disallowing spaces in the path, but if Sys_TempDir somehow acquires a
value of "/tmp/';echo;'", R_CleanTempDir() will remove /tmp instead of
its aptly-named subdirectory.

Please see 83851 from earlier today which does a bit more of 
robustification, and if you find any problem in it, please let me know.

While adding the single-quote symbol to the list of special symbols
should suffice (it and the backslash being the only allowed ways to
"un-quote" a single-quoted string), 

I would like to suggest solving the
problem without the use of quoting:

#include <spawn.h>

char ** argv = { "rm", "-Rf", Sys_TempDir, NULL };
posix_spawnp(NULL, "rm", NULL, NULL, argv, NULL);

Are there Unix-like platforms on which R is intended to work that don't
have posix_spawn()? Circa-2014 versions of both Solaris and OpenBSD
seem to have it. Spawning the process manually by means of [v]fork()
and exec() is probably not worth the maintainer effort required to
perform it correctly.

Yes, this is a good point and we have been thinking about spawn() as 
well, and we are considering that. Re implementing, I also fear the 
cost may be too high, thinking about the timeout support in system() I 
implemented earlier, so essentially a system() replacement for Unix. 
The details are complicated on Unix as well as on Windows. And re 
reusing existing implementations, we will have to check they do 
exactly what we need about signals, terminal, process groups, 
termination, input and output, etc. It may also be that improving 
performance of R_unlink() would be easier, as it is rather 
un-optimized now. So I just wanted to buy time with (possibly 
temporary) fix in 83851.

Thanks
Tomas

Please see 83851 from earlier today which does a bit more of 
robustification, and if you find any problem in it, please let me
know.