Re: is_absolute_path incorrect on Windows

Bruce Momjian wrote:
> Tom Lane wrote:
> > Bruce Momjian <bruce@momjian.us> writes:
> > > Tom Lane wrote:
> > >> Bruce Momjian <bruce@momjian.us> writes:
> > >>> I have reviewed is_absolute_path() and have implemented
> > >>> path_is_relative_and_below_cwd() to cleanly handle cases like 'E:abc' on
> > >>> Win32;  patch attached.
> > >>
> > >> This patch appears to remove some security-critical restrictions.
> > >> Why did you delete the path_contains_parent_reference calls?
> >
> > > They are now in path_is_relative_and_below_cwd(),
> >
> > ... and thus not invoked in the absolute-path case.  This is a security
> > hole.
> >
> > >  I don't see a general reason to prevent
> > > ".." in absolute paths, only relative ones.
> >
> >     load '/path/to/database/../../../path/to/anywhere'
>
> Ah, good point. I was thinking about someone using ".." in the part of
> the path that is compared to /data or /log, but using it after that
> would clearly be a security problem.
>
> I have attached an updated patch that restructures the code to be
> clearer and adds the needed checks.

I decided that my convert_and_check_filename() usage was too intertwined
so I have developed a simplified version that is easier to understand;
patch attached.

--
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + It's impossible for everything to be true. +
diff --git a/contrib/adminpack/adminpack.c b/contrib/adminpack/adminpack.c
index 381554d..c149dd6 100644
*** a/contrib/adminpack/adminpack.c
--- b/contrib/adminpack/adminpack.c
*************** convert_and_check_filename(text *arg, bo
*** 73,104 ****

      canonicalize_path(filename);    /* filename can change length here */

-     /* Disallow ".." in the path */
-     if (path_contains_parent_reference(filename))
-         ereport(ERROR,
-                 (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
-             (errmsg("reference to parent directory (\"..\") not allowed"))));
-
      if (is_absolute_path(filename))
      {
!         /* Allow absolute references within DataDir */
!         if (path_is_prefix_of_path(DataDir, filename))
!             return filename;
!         /* The log directory might be outside our datadir, but allow it */
!         if (logAllowed &&
!             is_absolute_path(Log_directory) &&
!             path_is_prefix_of_path(Log_directory, filename))
!             return filename;
!
!         ereport(ERROR,
                  (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
                   (errmsg("absolute path not allowed"))));
-         return NULL;            /* keep compiler quiet */
-     }
-     else
-     {
-         return filename;
      }
  }


--- 73,102 ----

      canonicalize_path(filename);    /* filename can change length here */

      if (is_absolute_path(filename))
      {
!         /* Disallow '/a/b/data/..' */
!         if (path_contains_parent_reference(filename))
!             ereport(ERROR,
!                 (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
!                 (errmsg("reference to parent directory (\"..\") not allowed"))));
!         /*
!          *    Allow absolute paths if within DataDir or Log_directory, even
!          *    though Log_directory might be outside DataDir.
!          */
!         if (!path_is_prefix_of_path(DataDir, filename) &&
!             (!logAllowed || !is_absolute_path(Log_directory) ||
!              !path_is_prefix_of_path(Log_directory, filename)))
!             ereport(ERROR,
                  (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
                   (errmsg("absolute path not allowed"))));
      }
+     else if (!path_is_relative_and_below_cwd(filename))
+         ereport(ERROR,
+                 (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+                  (errmsg("path must be in or below the current directory"))));
+
+     return filename;
  }


diff --git a/src/backend/utils/adt/genfile.c b/src/backend/utils/adt/genfile.c
index 93bc401..bbcddfb 100644
*** a/src/backend/utils/adt/genfile.c
--- b/src/backend/utils/adt/genfile.c
*************** convert_and_check_filename(text *arg)
*** 51,81 ****
      filename = text_to_cstring(arg);
      canonicalize_path(filename);    /* filename can change length here */

-     /* Disallow ".." in the path */
-     if (path_contains_parent_reference(filename))
-         ereport(ERROR,
-                 (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
-             (errmsg("reference to parent directory (\"..\") not allowed"))));
-
      if (is_absolute_path(filename))
      {
!         /* Allow absolute references within DataDir */
!         if (path_is_prefix_of_path(DataDir, filename))
!             return filename;
!         /* The log directory might be outside our datadir, but allow it */
!         if (is_absolute_path(Log_directory) &&
!             path_is_prefix_of_path(Log_directory, filename))
!             return filename;
!
!         ereport(ERROR,
                  (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
                   (errmsg("absolute path not allowed"))));
-         return NULL;            /* keep compiler quiet */
-     }
-     else
-     {
-         return filename;
      }
  }


--- 51,80 ----
      filename = text_to_cstring(arg);
      canonicalize_path(filename);    /* filename can change length here */

      if (is_absolute_path(filename))
      {
!         /* Disallow '/a/b/data/..' */
!         if (path_contains_parent_reference(filename))
!             ereport(ERROR,
!                 (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
!                 (errmsg("reference to parent directory (\"..\") not allowed"))));
!         /*
!          *    Allow absolute paths if within DataDir or Log_directory, even
!          *    though Log_directory might be outside DataDir.
!          */
!         if (!path_is_prefix_of_path(DataDir, filename) &&
!             (!is_absolute_path(Log_directory) ||
!              !path_is_prefix_of_path(Log_directory, filename)))
!             ereport(ERROR,
                  (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
                   (errmsg("absolute path not allowed"))));
      }
+     else if (!path_is_relative_and_below_cwd(filename))
+         ereport(ERROR,
+                 (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+                  (errmsg("path must be in or below the current directory"))));
+
+     return filename;
  }


diff --git a/src/include/port.h b/src/include/port.h
index 2020a26..5be42f5 100644
*** a/src/include/port.h
--- b/src/include/port.h
*************** extern void join_path_components(char *r
*** 42,47 ****
--- 42,48 ----
  extern void canonicalize_path(char *path);
  extern void make_native_path(char *path);
  extern bool path_contains_parent_reference(const char *path);
+ extern bool path_is_relative_and_below_cwd(const char *path);
  extern bool path_is_prefix_of_path(const char *path1, const char *path2);
  extern const char *get_progname(const char *argv0);
  extern void get_share_path(const char *my_exec_path, char *ret_path);
*************** extern void pgfnames_cleanup(char **file
*** 77,89 ****
  #else
  #define IS_DIR_SEP(ch)    ((ch) == '/' || (ch) == '\\')

! /*
!  * On Win32, a drive letter _not_ followed by a slash, e.g. 'E:abc', is
!  * relative to the cwd on that drive, or the drive's root directory
!  * if that drive has no cwd.  Because the path itself cannot tell us
!  * which is the case, we have to assume the worst, i.e. that it is not
!  * absolute;  this check is done by IS_DIR_SEP(filename[2]).
!  */
  #define is_absolute_path(filename) \
  ( \
      IS_DIR_SEP((filename)[0]) || \
--- 78,84 ----
  #else
  #define IS_DIR_SEP(ch)    ((ch) == '/' || (ch) == '\\')

! /* See path_is_relative_and_below_cwd() for how we handle 'E:abc'. */
  #define is_absolute_path(filename) \
  ( \
      IS_DIR_SEP((filename)[0]) || \
diff --git a/src/port/path.c b/src/port/path.c
index 5b0056d..9a6a27a 100644
*** a/src/port/path.c
--- b/src/port/path.c
*************** path_contains_parent_reference(const cha
*** 359,364 ****
--- 359,395 ----
  }

  /*
+  * Detect whether a path is only in or below the current working directory.
+  * An absolute path that matches the current working directory should
+  * return false (we only want relative to the cwd).  We don't allow
+  * "/../" even if that would keep us under the cwd (it is too hard to
+  * track that).
+  */
+ bool
+ path_is_relative_and_below_cwd(const char *path)
+ {
+     if (!is_absolute_path(path))
+         return false;
+     /* don't allow anything above the cwd */
+     else if (path_contains_parent_reference(path))
+         return false;
+ #ifdef WIN32
+     /*
+      *    On Win32, a drive letter _not_ followed by a slash, e.g. 'E:abc', is
+      *    relative to the cwd on that drive, or the drive's root directory
+      *    if that drive has no cwd.  Because the path itself cannot tell us
+      *    which is the case, we have to assume the worst, i.e. that it is not
+      *    below the cwd.
+      */
+     else if (isalpha((unsigned char) path[0]) && path[1] == ':' &&
+             !IS_DIR_SEP(path[2]))
+         return false;
+ #endif
+     else
+         return true;
+ }
+
+ /*
   * Detect whether path1 is a prefix of path2 (including equality).
   *
   * This is pretty trivial, but it seems better to export a function than