From: Matthew Fernandez Date: Tue, 16 Aug 2022 02:19:29 +0000 (-0700) Subject: remove $GV_FILE_PATH support X-Git-Tag: 6.0.1~35^2 X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=1e45fd838a7308f0a8e9712d8dbc2e486c265455;p=graphviz remove $GV_FILE_PATH support The `$GV_FILE_PATH` environment variable could be set to sandbox Graphviz’ ability to read and write to the file system. This made sense once upon a time, but the world around Graphviz has shifted. Sandboxing yourself is no longer as valuable a proposition as an external sandboxer that can be more easily audited. Platforms’ ecosystems have matured to support this use case (Capsicum on FreeBSD, Seccomp on Linux, App Sandbox on macOS, Pledge on OpenBSD, …). This change makes any attempt to use `$GV_FILE_PATH` “fail-closed,” in the sense that execution will be aborted. This may be surprising and not what the user intended, but this conservatively guarantees safety: you can never think you have enabled `$GV_FILE_PATH`-based sandboxing and be instead running unguarded. Gitlab: closes #2257 --- diff --git a/CHANGELOG.md b/CHANGELOG.md index 6c38e5412..911511fec 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased (6.0.0)] +### Removed + +- The `$GV_FILE_PATH` sandboxing mechanism has been removed #2257 + ## [5.0.1] – 2022-08-20 ### Fixed diff --git a/lib/common/globals.h b/lib/common/globals.h index d4ca68b28..17fdaffc7 100644 --- a/lib/common/globals.h +++ b/lib/common/globals.h @@ -34,7 +34,6 @@ extern "C" { GLOBALS_API EXTERN char **Files; /* from command line */ GLOBALS_API EXTERN const char **Lib; /* from command line */ GLOBALS_API EXTERN char *CmdName; - GLOBALS_API EXTERN char *Gvfilepath; /* Per-process path of files allowed in image attributes (also ps libs) */ GLOBALS_API EXTERN char *Gvimagepath; /* Per-graph path of files allowed in image attributes (also ps libs) */ GLOBALS_API EXTERN unsigned char Verbose; diff --git a/lib/common/input.c b/lib/common/input.c index 11d7e8988..1778a5eca 100644 --- a/lib/common/input.c +++ b/lib/common/input.c @@ -234,8 +234,13 @@ int dotneato_args_initialize(GVC_t * gvc, int argc, char **argv) /* establish if we are running in a CGI environment */ HTTPServerEnVar = getenv("SERVER_NAME"); - /* establish Gvfilepath, if any */ - Gvfilepath = getenv("GV_FILE_PATH"); + // test `$GV_FILE_PATH`, a legacy knob, is not set + if (getenv("GV_FILE_PATH") != NULL) { + fprintf(stderr, "$GV_FILE_PATH environment variable set; exiting\n" + "\n" + "This sandboxing mechanism is no longer supported\n"); + graphviz_exit(EXIT_FAILURE); + } gvc->common.cmdname = dotneato_basename(argv[0]); if (gvc->common.verbose) { @@ -657,8 +662,6 @@ void graph_init(graph_t * g, bool use_rankdir) if (!HTTPServerEnVar) { Gvimagepath = agget (g, "imagepath"); - if (!Gvimagepath) - Gvimagepath = Gvfilepath; } GD_drawing(g)->quantum = diff --git a/lib/common/utils.c b/lib/common/utils.c index 15d857e87..dc3c9dd6b 100644 --- a/lib/common/utils.c +++ b/lib/common/utils.c @@ -295,17 +295,7 @@ char *Fgets(FILE * fp) * It returns NULL if the filename is trivial. * * If the application has set the SERVER_NAME environment variable, - * this indicates it is web-active. In this case, it requires that the GV_FILE_PATH - * environment variable be set. This gives the legal directories - * from which files may be read. safefile then derives the rightmost component - * of filename, where components are separated by a slash, backslash or colon, - * It then checks for the existence of a file consisting of a directory from - * GV_FILE_PATH followed by the rightmost component of filename. It returns the - * first such found, or NULL otherwise. - * The filename returned is thus - * Gvfilepath concatenated with the last component of filename, - * where a component is determined by a slash, backslash or colon - * character. + * this indicates it is web-active. * * If filename contains multiple components, the user is * warned, once, that everything to the left is ignored. @@ -360,48 +350,18 @@ const char *safefile(const char *filename) static char *pathlist = NULL; static size_t maxdirlen; static strview_t *dirs; - const char *str, *p; if (!filename || !filename[0]) return NULL; if (HTTPServerEnVar) { /* If used as a server */ - /* - * If we are running in an http server we allow - * files only from the directory specified in - * the GV_FILE_PATH environment variable. - */ - if (!Gvfilepath || (*Gvfilepath == '\0')) { - if (onetime) { - agerr(AGWARN, - "file loading is disabled because the environment contains SERVER_NAME=\"%s\"\n" - "and the GV_FILE_PATH variable is unset or empty.\n", + if (onetime) { + agerr(AGWARN, + "file loading is disabled because the environment contains SERVER_NAME=\"%s\"\n", HTTPServerEnVar); - onetime = false; - } - return NULL; - } - if (!pathlist) { - dirs = mkDirlist (Gvfilepath, &maxdirlen); - pathlist = Gvfilepath; - } - - str = filename; - if ((p = strrchr(str, '/'))) - str = ++p; - if ((p = strrchr(str, '\\'))) - str = ++p; - if ((p = strrchr(str, ':'))) - str = ++p; - - if (onetime && str != filename) { - agerr(AGWARN, "Path provided to file: \"%s\" has been ignored" - " because files are only permitted to be loaded from the directories in \"%s\"" - " when running in an http server.\n", filename, Gvfilepath); onetime = false; } - - return findPath (dirs, maxdirlen, str); + return NULL; } if (pathlist != Gvimagepath) { diff --git a/tests/test_regression.py b/tests/test_regression.py index 8941595c8..548e7bd6a 100644 --- a/tests/test_regression.py +++ b/tests/test_regression.py @@ -1902,7 +1902,6 @@ def test_2225(): p.check_returncode() -@pytest.mark.xfail(strict=True) def test_2257(): """ `$GV_FILE_PATH` being set should prevent Graphviz from running