search

ramnathv / htmlwidgets

HTML Widgets for R
http://htmlwidgets.org
Other
792 stars 205 forks source link

Use system2 to avoid shell injection #460

Open MichaelChirico opened 1 year ago

MichaelChirico commented 1 year ago

https://github.com/ramnathv/htmlwidgets/blob/7b9c1ea3d9fbf4736d84f1fd1178fce0af29f8e3/R/scaffold.R#L121

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

https://github.com/ramnathv/htmlwidgets/blob/7b9c1ea3d9fbf4736d84f1fd1178fce0af29f8e3/R/scaffold.R#L121

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 commented 1 year ago

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

zeehio commented 1 year ago

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.