From 6a278a34fe1a849258f961cf8d158a2e1a55da68 Mon Sep 17 00:00:00 2001 From: Vincent Guyader Date: Mon, 11 May 2026 13:53:35 +0200 Subject: [PATCH] fix(security): validate the renv version resolved from the lockfile `dock_from_renv()` interpolates the `renv` package version into the generated `R -e 'remotes::install_version("renv", version = "")'` line, which runs as root at `docker build` time. `.validate_renv_version()` was only applied when the value came from a user-supplied `renv_version=` argument; the fallback path that reads `lock$Packages$renv$Version` from the (untrusted) lockfile skipped validation entirely. A crafted `renv.lock` carrying e.g. `1.0.0"); system("..."); ("` would break out of the inner R string literal and execute arbitrary code in the build container. Validate the resolved value inside the `if (missing(renv_version))` block, right after the lockfile read, so the lockfile-fallback path is guarded where it is resolved (the user-supplied path keeps its entry-point validation; no double-check on a known-good value). The validator's existing regex `^[0-9]+(\.[0-9]+){0,3}([-.][a-zA-Z0-9]+)?$` already rejects `"`, `;`, parens and whitespace, so a benign lockfile (`renv 1.0.0`) still produces `RUN R -e 'remotes::install_version("renv", version = "1.0.0")'`. Also tightened the `.validate_r_version()` "`X.Y.Z Patched`" branch to require a single literal space instead of `\s` (which in R matches a newline / tab too): a lockfile with an embedded newline in `R$Version` would otherwise pass validation and emit a two-line `FROM` directive. Not command injection, but it could silently break `docker build`. Tests added (red-first): a lockfile whose `Packages$renv$Version` carries a `system()` payload must raise the `renv_version` validation error rather than reaching the Dockerfile (verified red against the unpatched function, green after the fix); and `.validate_r_version("4.5.0\nPatched")` / `"...\tPatched"` must error (verified red against the `\s` regex). Full test suite: 0 failures, 155 passing in the touched file. R CMD check: 0/0/0 (one transient "unable to verify current time" host NOTE, unrelated). --- NEWS.md | 19 ++++++++++++ R/dock_from_renv.R | 8 +++++ R/utils.R | 2 +- tests/testthat/test-dock_from_renv.R | 45 ++++++++++++++++++++++++++++ 4 files changed, 73 insertions(+), 1 deletion(-) 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)