Tom Lane wrote:
> Bruce Momjian <[email protected]> writes:
> > Tom Lane wrote:
> >> Bruce Momjian <[email protected]> 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 <[email protected]> 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
--
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
