Re: is_absolute_path incorrect on Windows

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.

--
  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..afae9cb 100644
*** a/contrib/adminpack/adminpack.c
--- b/contrib/adminpack/adminpack.c
*************** convert_and_check_filename(text *arg, bo
*** 73,85 ****

      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))
--- 73,84 ----

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

!     /* Disallow '/a/b/data/..' and 'a/b/..' */
      if (path_contains_parent_reference(filename))
          ereport(ERROR,
!             (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
              (errmsg("reference to parent directory (\"..\") not allowed"))));
!     else if (is_absolute_path(filename))
      {
          /* Allow absolute references within DataDir */
          if (path_is_prefix_of_path(DataDir, filename))
*************** convert_and_check_filename(text *arg, bo
*** 93,104 ****
          ereport(ERROR,
                  (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
                   (errmsg("absolute path not allowed"))));
-         return NULL;            /* keep compiler quiet */
-     }
-     else
-     {
-         return filename;
      }
  }


--- 92,104 ----
          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..63fc517 100644
*** a/src/backend/utils/adt/genfile.c
--- b/src/backend/utils/adt/genfile.c
*************** convert_and_check_filename(text *arg)
*** 51,63 ****
      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))
--- 51,62 ----
      filename = text_to_cstring(arg);
      canonicalize_path(filename);    /* filename can change length here */

!     /* Disallow '/a/b/data/..' and 'a/b/..' */
      if (path_contains_parent_reference(filename))
          ereport(ERROR,
!             (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
              (errmsg("reference to parent directory (\"..\") not allowed"))));
!     else if (is_absolute_path(filename))
      {
          /* Allow absolute references within DataDir */
          if (path_is_prefix_of_path(DataDir, filename))
*************** convert_and_check_filename(text *arg)
*** 70,81 ****
          ereport(ERROR,
                  (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
                   (errmsg("absolute path not allowed"))));
-         return NULL;            /* keep compiler quiet */
-     }
-     else
-     {
-         return filename;
      }
  }


--- 69,81 ----
          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