Skip to content

Commit 8e9d910

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 8e9d910

8 files changed

Lines changed: 509 additions & 34 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 it `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/utils.R

Lines changed: 213 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,219 @@ cat_info <- function(...) {
9090
}
9191
}
9292

93+
#' Validate user-supplied strings that flow into a Dockerfile shell context.
94+
#'
95+
#' These helpers reject inputs that, if interpolated raw, would either
96+
#' break the generated Dockerfile / shell command or allow injection of
97+
#' arbitrary commands at `docker build` time. Each helper raises an
98+
#' error with a clear message naming the offending parameter. `NULL` is
99+
#' accepted where it has a documented meaning (e.g. `renv_paths_cache`,
100+
#' `renv_version`, `extra_sysreqs`, `repos`).
101+
#' @noRd
102+
.validate_FROM <- function(x) {
103+
if (!is.character(x) || length(x) != 1L || is.na(x)) {
104+
stop(
105+
"`FROM` must be a single string, got: ",
106+
deparse(x)
107+
)
108+
}
109+
if (
110+
!grepl(
111+
"^[a-zA-Z0-9][a-zA-Z0-9._/-]*(:[a-zA-Z0-9._-]+)?(@sha256:[a-fA-F0-9]+)?$",
112+
x
113+
)
114+
) {
115+
stop(
116+
"`FROM` must be a valid Docker image reference ",
117+
"(alphanumerics, dot, slash, dash, underscore; optional `:tag` ",
118+
"and / or `@sha256:<hex>`; no newlines or shell metacharacters), got: ",
119+
deparse(x)
120+
)
121+
}
122+
invisible()
123+
}
124+
125+
#' @noRd
126+
.validate_r_version <- function(x) {
127+
if (!is.character(x) || length(x) != 1L || is.na(x)) {
128+
stop(
129+
"`r_version` (read from the lockfile) must be a single string, got: ",
130+
deparse(x)
131+
)
132+
}
133+
if (!grepl("^[0-9]+\\.[0-9]+(\\.[0-9]+)?$", x)) {
134+
stop(
135+
"`r_version` (read from the lockfile) must look like a numeric ",
136+
"R version such as \"4.5\" or \"4.5.0\", got: ",
137+
deparse(x)
138+
)
139+
}
140+
invisible()
141+
}
142+
143+
#' @noRd
144+
.validate_repos <- function(x) {
145+
if (is.null(x)) {
146+
return(invisible())
147+
}
148+
if (!is.character(x)) {
149+
stop(
150+
"`repos` must be a character vector, got: ",
151+
deparse(x)
152+
)
153+
}
154+
bad <- !grepl(
155+
"^https?://[A-Za-z0-9._~:/?#@!$&()*+,;=%-]+$",
156+
x
157+
)
158+
if (any(bad)) {
159+
stop(
160+
"`repos` entries must be http(s) URLs without quotes, ",
161+
"spaces or newlines; invalid: ",
162+
paste(vapply(x[bad], deparse, character(1)), collapse = ", ")
163+
)
164+
}
165+
# Names are emitted via `dput(repos)`, which wraps R-syntax-unsafe
166+
# names in backticks. Backticks inside a Dockerfile RUN's outer
167+
# double-quoted shell context trigger command substitution. Tight
168+
# regex on names to keep them simple identifiers.
169+
nms <- names(x)
170+
if (!is.null(nms)) {
171+
bad_nms <- !grepl("^[A-Za-z][A-Za-z0-9._-]*$", nms)
172+
if (any(bad_nms)) {
173+
stop(
174+
"`names(repos)` must be simple identifiers ",
175+
"(`^[A-Za-z][A-Za-z0-9._-]*$`); invalid: ",
176+
paste(vapply(nms[bad_nms], deparse, character(1)), collapse = ", ")
177+
)
178+
}
179+
}
180+
invisible()
181+
}
182+
183+
#' @noRd
184+
.validate_AS <- function(x) {
185+
if (is.null(x)) {
186+
return(invisible())
187+
}
188+
if (!is.character(x) || length(x) != 1L || is.na(x)) {
189+
stop(
190+
"`AS` must be a single string or NULL, got: ",
191+
deparse(x)
192+
)
193+
}
194+
if (!grepl("^[a-zA-Z0-9][a-zA-Z0-9._-]*$", x)) {
195+
stop(
196+
"`AS` must be a simple build-stage name ",
197+
"(`^[a-zA-Z0-9][a-zA-Z0-9._-]*$`), got: ",
198+
deparse(x)
199+
)
200+
}
201+
invisible()
202+
}
203+
204+
#' @noRd
205+
.validate_scalar_logical <- function(x, name) {
206+
if (
207+
!is.logical(x) ||
208+
length(x) != 1L ||
209+
is.na(x)
210+
) {
211+
stop(
212+
sprintf("`%s` must be a single `TRUE` or `FALSE`, got: ", name),
213+
deparse(x)
214+
)
215+
}
216+
invisible()
217+
}
218+
219+
#' @noRd
220+
.validate_extra_sysreqs <- function(x) {
221+
if (is.null(x)) {
222+
return(invisible())
223+
}
224+
if (!is.character(x)) {
225+
stop(
226+
"`extra_sysreqs` must be a character vector, got: ",
227+
deparse(x)
228+
)
229+
}
230+
bad <- !grepl("^[a-z0-9][a-z0-9.+-]+$", x)
231+
if (any(bad)) {
232+
stop(
233+
"`extra_sysreqs` entries must be Debian package names ",
234+
"matching `^[a-z0-9][a-z0-9.+-]+$`; invalid: ",
235+
paste(vapply(x[bad], deparse, character(1)), collapse = ", ")
236+
)
237+
}
238+
invisible()
239+
}
240+
241+
#' @noRd
242+
.validate_renv_version <- function(x) {
243+
if (is.null(x)) {
244+
return(invisible())
245+
}
246+
if (!is.character(x) || length(x) != 1L || is.na(x)) {
247+
stop(
248+
"`renv_version` must be a single string or NULL, got: ",
249+
deparse(x)
250+
)
251+
}
252+
if (!grepl("^[0-9]+(\\.[0-9]+){0,3}([-.][a-zA-Z0-9]+)?$", x)) {
253+
stop(
254+
"`renv_version` must look like a version string such as ",
255+
"\"1.0.0\" or \"0.16.0-beta\", got: ",
256+
deparse(x)
257+
)
258+
}
259+
invisible()
260+
}
261+
262+
#' @noRd
263+
.validate_lockfile <- function(x) {
264+
if (!is.character(x) || length(x) != 1L || is.na(x)) {
265+
stop(
266+
"`lockfile` must be a single path string, got: ",
267+
deparse(x)
268+
)
269+
}
270+
bn <- basename(x)
271+
if (!grepl("^[A-Za-z0-9._-]+$", bn)) {
272+
stop(
273+
"`lockfile` basename must contain only alphanumerics, dots, ",
274+
"underscores or hyphens (no spaces or shell metacharacters); ",
275+
"the COPY directive in the generated Dockerfile would otherwise ",
276+
"be malformed. Got basename: ",
277+
deparse(bn)
278+
)
279+
}
280+
invisible()
281+
}
282+
283+
#' @noRd
284+
.validate_renv_paths_cache <- function(x) {
285+
if (is.null(x)) {
286+
return(invisible())
287+
}
288+
if (!is.character(x) || length(x) != 1L || is.na(x)) {
289+
stop(
290+
"`renv_paths_cache` must be a single path string or NULL, got: ",
291+
deparse(x)
292+
)
293+
}
294+
if (!grepl("^/[A-Za-z0-9._/-]*$", x)) {
295+
stop(
296+
"`renv_paths_cache` must be an absolute path containing only ",
297+
"alphanumerics, dots, slashes, underscores or hyphens ",
298+
"(`^/[A-Za-z0-9._/-]*$`); shell metacharacters or newlines ",
299+
"would break the ARG directive. Got: ",
300+
deparse(x)
301+
)
302+
}
303+
invisible()
304+
}
305+
93306
#' Emit a one-shot reminder describing how the PAT must be supplied at
94307
#' `docker build` time. No-op when mode is `"none"`.
95308
#' @noRd

0 commit comments

Comments
 (0)