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