Use system2 to avoid shell injection

[MichaelChirico]

htmlwidgets/R/scaffold.R

Line 121 in 7b9c1ea

cmd <- sprintf('%s install %s', findBower(), pkg)

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

[wch]

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.

[MichaelChirico]

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

[zeehio]

htmlwidgets/R/scaffold.R

Line 121 in 7b9c1ea

cmd <- sprintf('%s install %s', findBower(), pkg)

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.

https://github.com/wch/r-source/blob/0987da15dd567ad07f91745238588c7873844d4c/src/library/base/R/unix/system.unix.R#L56

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'

[MichaelChirico]

Indeed that's right. Should we just run shQuote() on pkg ourselves, in that case?

[zeehio]

Indeed that's right. Should we just run shQuote() on pkg 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.