diff --git a/NEWS.md b/NEWS.md index ca0cc8e..e84629e 100644 --- a/NEWS.md +++ b/NEWS.md @@ -56,6 +56,25 @@ executes attacker-controlled commands at `docker build` time. This closes the post-#106 audit follow-up for all five sites surfaced by Copilot review. +- Fixed a long-standing code-injection path in `dock_from_renv()`: + the `renv` package version resolved from the lockfile + (`lock$Packages$renv$Version`) was interpolated raw into the + generated `R -e 'remotes::install_version("renv", version = "")'` + line without passing through `.validate_renv_version()`. A crafted + `renv.lock` could break out of the inner R string and execute + arbitrary code as root at `docker build` time. The user-supplied + `renv_version=` argument has been validated since the 0.3.0 + shell-context hardening above, but the lockfile-fallback path was + missed; the bug itself predates 0.3.0 (it existed while the + vendored `{renv}` parser was in use). The validator is now applied + to the resolved value whatever its source. Found by an internal + security audit before release. +- Tightened the `.validate_r_version()` "`X.Y.Z Patched`" branch to + require a single literal space (it previously used `\s`, which in R + also matches a newline or tab): a lockfile whose `R$Version` carried + an embedded newline would pass validation and then emit a two-line + `FROM` directive. Not exploitable for command injection, but it + could silently break `docker build`. ## New features diff --git a/R/dock_from_renv.R b/R/dock_from_renv.R index 75db403..c45bcf8 100644 --- a/R/dock_from_renv.R +++ b/R/dock_from_renv.R @@ -248,7 +248,15 @@ dock_from_renv <- function( # get renv version if (missing(renv_version)) { + # The lockfile is untrusted input: its `Packages$renv$Version` is + # interpolated into the generated `R -e + # 'remotes::install_version(...)'` line, which runs as root at + # `docker build` time. Validate it with the same regex applied to a + # user-supplied `renv_version` (which is validated at function entry). renv_version <- lock$Packages$renv$Version + if (!is.null(renv_version)) { + .validate_renv_version(renv_version) + } } message("renv version = ", diff --git a/R/utils.R b/R/utils.R index cad437e..bd0c328 100644 --- a/R/utils.R +++ b/R/utils.R @@ -153,7 +153,7 @@ cat_info <- function(...) { ok <- ( grepl("^[0-9]+\\.[0-9]+(\\.[0-9]+)?(-[A-Za-z0-9._]+)?$", x) || identical(x, "r-devel") || - grepl("^[0-9]+\\.[0-9]+(\\.[0-9]+)?\\sPatched$", x) + grepl("^[0-9]+\\.[0-9]+(\\.[0-9]+)? Patched$", x) ) if (!ok) { stop( diff --git a/tests/testthat/test-dock_from_renv.R b/tests/testthat/test-dock_from_renv.R index de02f9b..81022fd 100644 --- a/tests/testthat/test-dock_from_renv.R +++ b/tests/testthat/test-dock_from_renv.R @@ -243,6 +243,18 @@ test_that(".validate_r_version accepts R-devel, release-candidate and Patched lo dockerfiler:::.validate_r_version(""), "r_version" ) + # The " Patched" branch matches a single literal space only: + # a newline or tab between the version and "Patched" would otherwise + # slip through (R's `\\s` matches `\n` / `\t`) and produce a + # two-line FROM directive in the generated Dockerfile. + expect_error( + dockerfiler:::.validate_r_version("4.5.0\nPatched"), + "r_version" + ) + expect_error( + dockerfiler:::.validate_r_version("4.5.0\tPatched"), + "r_version" + ) }) test_that(".patch_rprofile_for_ppm matches all three current PPM host shapes", { @@ -1153,5 +1165,38 @@ test_that("dock_from_renv rejects a lockfile path whose basename contains shell ) }) +test_that("dock_from_renv validates the renv version read from the lockfile, not only the user-supplied one", { + skip_if(is_rdevel, "skip on R-devel") + # `renv_version` is interpolated raw into the generated + # `R -e 'remotes::install_version("renv", version = "")'` line, which + # runs as root at `docker build` time. `.validate_renv_version()` must be + # applied to the value resolved from `lock$Packages$renv$Version` (an + # untrusted lockfile a user may have received from a colleague, a vendored + # project, or a CI cache), not only to a user-supplied `renv_version=`. + # A crafted lockfile carrying `'1.0.0"); system("..."); ("'` would + # otherwise break out of the inner R string and execute arbitrary code. + lock <- jsonlite::read_json( + system.file("renv_with_1.0.0.lock", package = "dockerfiler"), + simplifyVector = TRUE, + simplifyDataFrame = FALSE, + simplifyMatrix = FALSE + ) + lock$Packages$renv$Version <- '1.0.0"); system("touch /tmp/dockerfiler_pwned"); ("' + malicious_lf <- file.path(dir_build, "malicious-renv.lock") + jsonlite::write_json(lock, path = malicious_lf, auto_unbox = TRUE, pretty = TRUE) + on.exit(unlink(malicious_lf), add = TRUE) + + expect_error( + dock_from_renv( + lockfile = malicious_lf, + FROM = "rocker/verse", + user = NULL, + sysreqs = FALSE + # NOTE: no `renv_version =` -> the value comes from the lockfile. + ), + "renv_version" + ) +}) + unlink(dir_build, recursive = TRUE)