Skip to content

Commit ec7c9bb

Browse files
feat(security): validate user-supplied strings flowing into shell context
Closes the post-#106 security audit follow-up. Adds 9 .validate_* helpers in R/utils.R (@nord) that reject inputs which would otherwise be interpolated raw into the generated Dockerfile's shell commands. Each helper raises stop() with a clear message naming the offending parameter. Validators added: - .validate_FROM (image reference: `^[a-zA-Z0-9][a-zA-Z0-9._/-]*(:tag)?(@sha256:hex)?$`) - .validate_AS (build-stage name: `^[a-zA-Z0-9][a-zA-Z0-9._-]*$`) - .validate_repos (https URLs + simple-identifier names; closes the `dput()`-backtick injection vector on names) - .validate_extra_sysreqs (Debian package name: `^[a-z0-9][a-z0-9.+-]+$`) - .validate_renv_version (semver-like, NULL allowed) - .validate_lockfile (basename: alphanumerics + `._-`) - .validate_renv_paths_cache (absolute path, no metacharacters) - .validate_r_version (defensive depth on lockfile-derived value) - .validate_scalar_logical (factored out from strict_install; used for use_pak too, closing a real injection vector via `echo "options(renv.config.pak.enabled = %s, ...)"`) dock_from_desc() and dock_from_renv() call the relevant validators at function entry, just after match.arg(github_pat). The pr-reviewer agent flagged two additional vectors I missed in the first pass: use_pak (B1, blocked) and names(repos) (B2, blocked via the dput() backtick path). Both are closed by this commit. Tests: 9 new expect_error() blocks across test-dock_from_desc.R and test-dock_from_renv.R, each TDD red-against-buggy. One existing test patched: renv_version = "banana" -> "1.2.3" since the new validator rejects non-semver strings (the test was exercising the install_version code path, which "1.2.3" still covers). devtools::test() -> [ FAIL 0 | WARN 3 | SKIP 0 | PASS 302 ]. R CMD check --as-cran: 0 errors / 0 warnings / 1 note (the "unable to verify current time" host note, unrelated). Out of scope for this PR (per the threat model agreed with the maintainer): - R6 helpers in R/add.R (dock$RUN, dock$COPY, ...) are author-controlled API; if the author writes a malicious RUN it is self-foot-shooting, not injection. - distro is deprecated and ignored at runtime. - github_pat is match.arg'd against a fixed enum. NEWS.md gains a `## Security` section above `## New features`.
1 parent 859273a commit ec7c9bb

9 files changed

Lines changed: 643 additions & 35 deletions

File tree

NEWS.md

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,20 @@
2424
behaviour. debian/ubuntu only; for alpine-based images you must
2525
pass `user = NULL` and create the user yourself. Closes #100.
2626

27+
## Security
28+
29+
- `dock_from_desc()` and `dock_from_renv()` now validate every
30+
user-supplied parameter that flows into a Dockerfile shell context
31+
(`FROM`, `AS`, `repos` values and names, `extra_sysreqs`,
32+
`renv_version`, `renv_paths_cache`, `lockfile` basename, `use_pak`,
33+
`strict_install`, plus `r_version` read from the lockfile). Inputs
34+
that contain shell metacharacters, newlines, or do not match the
35+
documented format raise an explicit error at function entry,
36+
rather than silently producing a malformed Dockerfile or one that
37+
executes attacker-controlled commands at `docker build` time.
38+
This closes the post-#106 audit follow-up for all five sites
39+
surfaced by Copilot review.
40+
2741
## New features
2842

2943
- `dock$ARG()` and the internal `add_arg()` helper gain a `default`

R/dock_from_desc.R

Lines changed: 21 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -42,17 +42,29 @@ quote_not_na <- function(x){
4242
#
4343
#' @param path path to the DESCRIPTION file to use as an input.
4444
#' @param FROM The FROM of the Dockerfile. Default is
45-
#' FROM rocker/r-ver:`R.Version()$major`.`R.Version()$minor`.
46-
#' @param AS The AS of the Dockerfile. Default it NULL.
45+
#' `paste0("rocker/r-ver:", R.Version()$major, ".", R.Version()$minor)`.
46+
#' Validated as a Docker image reference (alphanumerics, dot, slash,
47+
#' dash, underscore, optional `:tag` and / or `@sha256:<hex>`); other
48+
#' values raise an error to prevent shell-metacharacter injection
49+
#' into the generated FROM directive.
50+
#' @param AS The AS of the Dockerfile. Default it NULL. When non-NULL,
51+
#' validated as a simple build-stage name (`^[a-zA-Z0-9][a-zA-Z0-9._-]*$`).
4752
#' @param sysreqs boolean. If TRUE, the Dockerfile will contain sysreq installation.
48-
#' @param repos character. The URL(s) of the repositories to use for `options("repos")`.
53+
#' @param repos character. The URL(s) of the repositories to use for
54+
#' `options("repos")`. Each value must look like an http(s) URL (no
55+
#' quotes, spaces or newlines); each name (when set) must be a simple
56+
#' identifier (`^[A-Za-z][A-Za-z0-9._-]*$`). Other values raise an
57+
#' error to prevent injection into the generated `echo "options(...)"`
58+
#' shell command.
4959
#' @param expand boolean. If `TRUE` each system requirement will have its own `RUN` line.
5060
#' @param build_from_source boolean. If `TRUE` no tar.gz is created and
5161
#' the Dockerfile directly mount the source folder.
5262
#' @param update_tar_gz boolean. If `TRUE` and `build_from_source` is also `TRUE`,
5363
#' an updated tar.gz is created.
5464
#' @param extra_sysreqs character vector. Extra debian system requirements.
55-
#' Will be installed with apt-get install.
65+
#' Will be installed with apt-get install. Each entry must be a Debian
66+
#' package name (`^[a-z0-9][a-z0-9.+-]+$`); other values raise an error
67+
#' to prevent injection into the generated apt-get RUN.
5668
#' @param github_pat character. How to provide a GitHub PAT to
5769
#' `remotes::install_github()` for private dependency repositories.
5870
#' One of `"none"` (default; the generated Dockerfile does not
@@ -103,16 +115,11 @@ dock_from_desc <- function(
103115
strict_install = TRUE
104116
) {
105117
github_pat <- match.arg(github_pat)
106-
if (
107-
!is.logical(strict_install) ||
108-
length(strict_install) != 1L ||
109-
is.na(strict_install)
110-
) {
111-
stop(
112-
"`strict_install` must be a single `TRUE` or `FALSE`, got: ",
113-
deparse(strict_install)
114-
)
115-
}
118+
.validate_scalar_logical(strict_install, "strict_install")
119+
.validate_FROM(FROM)
120+
.validate_AS(AS)
121+
.validate_repos(repos)
122+
.validate_extra_sysreqs(extra_sysreqs)
116123
path <- fs::path_abs(path)
117124

118125
packages <- desc_get_deps(path)$package

R/dock_from_renv.R

Lines changed: 33 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -9,14 +9,26 @@ pkg_sysreqs_mem <- memoise::memoise(
99
#' Create a Dockerfile from an `renv.lock` file
1010
#'
1111
#' @param lockfile Path to an `renv.lock` file to use as an input..
12-
#' @param FROM Docker image to start FROM Default is FROM rocker/r-base
13-
#' @param AS The AS of the Dockerfile. Default it `NULL`.
12+
#' @param FROM Docker image to start FROM. Default is `"rocker/r-base"`.
13+
#' Validated as a Docker image reference (alphanumerics, dot, slash,
14+
#' dash, underscore, optional `:tag` and / or `@sha256:<hex>`); other
15+
#' values raise an error to prevent shell-metacharacter injection
16+
#' into the generated FROM directive.
17+
#' @param AS The AS of the Dockerfile. Default is `NULL`. When non-NULL,
18+
#' validated as a simple build-stage name (`^[a-zA-Z0-9][a-zA-Z0-9._-]*$`).
1419
#' @param distro - deprecated - only debian/ubuntu based images are supported
1520
#' @param sysreqs boolean. If `TRUE`, the Dockerfile will contain sysreq installation.
1621
#' @param expand boolean. If `TRUE` each system requirement will have its own `RUN` line.
17-
#' @param repos character. The URL(s) of the repositories to use for `options("repos")`.
22+
#' @param repos character. The URL(s) of the repositories to use for
23+
#' `options("repos")`. Each value must look like an http(s) URL
24+
#' (no quotes, spaces or newlines); each name (when set) must be a
25+
#' simple identifier (`^[A-Za-z][A-Za-z0-9._-]*$`). Other values raise
26+
#' an error to prevent injection into the generated `echo "options(...)"`
27+
#' shell command.
1828
#' @param extra_sysreqs character vector. Extra debian system requirements.
19-
#' Will be installed with apt-get install.
29+
#' Will be installed with apt-get install. Each entry must be a Debian
30+
#' package name (`^[a-z0-9][a-z0-9.+-]+$`); other values raise an error
31+
#' to prevent injection into the generated apt-get RUN.
2032
#' @param renv_version character or `NULL`. The renv version to install.
2133
#' The argument has three distinct modes, deliberately encoded with
2234
#' the missing-vs-`NULL` distinction:
@@ -28,7 +40,12 @@ pkg_sysreqs_mem <- memoise::memoise(
2840
#' version.
2941
#' - a character string such as `"1.0.0"`: install that specific
3042
#' version regardless of what the lockfile says.
31-
#' @param use_pak boolean. If `TRUE` use pak to deal with dependencies during `renv::restore()`. FALSE by default
43+
#'
44+
#' When supplied as a string, validated as a version-like token
45+
#' (`^[0-9]+(\.[0-9]+){0,3}([-.][a-zA-Z0-9]+)?$`).
46+
#' @param use_pak boolean. If `TRUE` use pak to deal with dependencies
47+
#' during `renv::restore()`. FALSE by default. Must be a single
48+
#' `TRUE` or `FALSE` (no `NA`, no vector).
3249
#' @param user Name of the user the runtime container drops privilege
3350
#' to before the `renv::restore()` step (and therefore at runtime).
3451
#' Default is `"rstudio"` so the generated image runs as a non-root
@@ -139,6 +156,16 @@ dock_from_renv <- function(
139156
renv_paths_cache = NULL
140157
) {
141158
github_pat <- match.arg(github_pat)
159+
.validate_lockfile(lockfile)
160+
.validate_FROM(FROM)
161+
.validate_AS(AS)
162+
.validate_repos(repos)
163+
.validate_extra_sysreqs(extra_sysreqs)
164+
.validate_scalar_logical(use_pak, "use_pak")
165+
if (!missing(renv_version)) {
166+
.validate_renv_version(renv_version)
167+
}
168+
.validate_renv_paths_cache(renv_paths_cache)
142169
if (!is.null(user)) {
143170
# `user` is interpolated into shell commands (id, useradd, chown, USER).
144171
# Reject anything that is not a strict POSIX username so a caller passing
@@ -173,6 +200,7 @@ dock_from_renv <- function(
173200

174201
# start the dockerfile
175202
R_major_minor <- lock$R$Version
203+
.validate_r_version(R_major_minor)
176204
dock <- Dockerfile$new(
177205
FROM = gen_base_image(
178206
r_version = R_major_minor,

R/gen_base_image.R

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,13 @@ if (!is.null(distro)){
1515
warning("the `distro` parameter is not used anymore, only debian/ubuntu based images are supported")
1616
}
1717

18-
glue::glue("{FROM}:{r_version}")
18+
# If the caller already pinned a tag (`:something`) or digest
19+
# (`@sha256:...`), do not append `:r_version` on top of it -- that
20+
# would yield an invalid image reference like `repo:tag:r_version`.
21+
if (grepl("(:[^/]+|@sha256:[a-fA-F0-9]+)$", FROM)) {
22+
glue::glue("{FROM}")
23+
} else {
24+
glue::glue("{FROM}:{r_version}")
25+
}
1926

2027
}

0 commit comments

Comments
 (0)