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 6c38e5412643c68614ba5769b4334736a317d414..911511fec938c81551c41c361deb3f0fb9b5d60d 100644 (file)
--- 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 d4ca68b289c52f935acb3529ef456042f3098e2b..17fdaffc7017455f43bb58793f78be2ea2f73c68 100644 (file)
--- 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 11d7e898894d4a96cd3ddc9c462fb601d4f380bf..1778a5eca959559ce190192e11c9846745aebafc 100644 (file)
--- 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 15d857e8784fef87f0e6e72cfc381418d32f2d1c..dc3c9dd6bcd1051878837d2dad2a6050578e2486 100644 (file)
--- 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 8941595c8c2e935607d858b1b091a6bb8c40692b..548e7bd6ad8551575a5a80128d6b12ed7d787669 100644 (file)
--- 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