Use system2 to avoid shell injection
MichaelChirico opened this issue · 5 comments
Line 121 in 7b9c1ea
IINM this could be subject to shell injection if pkg
can be passed as an arbitrary input.
system2() should have roughly the same level of code complexity & avoids this issue
Could you elaborate on the threat model here? If someone's in a position to call this function, I'd expect that they would already be able to run arbitrary R code, which means they could call system()
and do what ever they want.
What's preventing someone from leaving an API into this function from e.g. a shiny app?
is there a reason to prefer system() here? it seems the output is not captured anyway so the behavior AIUI should be the same
Line 121 in 7b9c1ea
IINM this could be subject to shell injection if
pkg
can be passed as an arbitrary input.system2() should have roughly the same level of code complexity & avoids this issue
Are you sure system2()
is any safer in this aspect? I would say it may be more convenient, but it seems to me its implementation uses shQuote()
on the command, pastes all args together and calls an internal system()
call.
Reading that code makes me believe that system2()
also creates a shell to run the given command, and therefore it is vulnerable to shell parsing as well.
I could try to write a system2()
call to prove this once I'm not on my phone if you need it, but the source linked above seems quite straightforward to follow.
Please correct me if I am wrong but it seems to me that injecting code in system2()
is perfectly doable through the 'args'
Indeed that's right. Should we just run shQuote()
on pkg
ourselves, in that case?
Indeed that's right. Should we just run
shQuote()
onpkg
ourselves, in that case?
If I had to run an external process without a shell I would use processx::run()
which does not use a shell and by definition avoids shell injection issues.