From 0de0cc150af46122238f2fe03605bf14e1a7c276 Mon Sep 17 00:00:00 2001 From: Bruce Momjian Date: Sat, 12 Feb 2011 09:47:51 -0500 Subject: [PATCH] Properly handle Win32 paths of 'E:abc', which can be either absolute or relative, by creating a function path_is_relative_and_below_cwd() to check for specific requirements. It is unclear if this fixes a security problem or not but the new code is more robust. --- contrib/adminpack/adminpack.c | 40 ++++++++++++++++----------------- src/backend/utils/adt/genfile.c | 39 ++++++++++++++++---------------- src/include/port.h | 9 ++------ src/port/path.c | 33 +++++++++++++++++++++++++++ 4 files changed, 73 insertions(+), 48 deletions(-) diff --git a/contrib/adminpack/adminpack.c b/contrib/adminpack/adminpack.c index 381554d114..c149dd6c63 100644 --- a/contrib/adminpack/adminpack.c +++ b/contrib/adminpack/adminpack.c @@ -73,32 +73,30 @@ convert_and_check_filename(text *arg, bool logAllowed) 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, + /* 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")))); - return NULL; /* keep compiler quiet */ - } - else - { - return filename; } + 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 c3ec98aa5e..7c59e9a20e 100644 --- a/src/backend/utils/adt/genfile.c +++ b/src/backend/utils/adt/genfile.c @@ -51,31 +51,30 @@ convert_and_check_filename(text *arg) 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, + /* 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")))); - return NULL; /* keep compiler quiet */ - } - else - { - return filename; } + 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 f6d6d2e444..9d08b392ce 100644 --- a/src/include/port.h +++ b/src/include/port.h @@ -42,6 +42,7 @@ extern void join_path_components(char *ret_path, 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); @@ -77,13 +78,7 @@ extern void pgfnames_cleanup(char **filenames); #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]). - */ +/* 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 5b0056dfe5..e01c7b8c0e 100644 --- a/src/port/path.c +++ b/src/port/path.c @@ -358,6 +358,39 @@ path_contains_parent_reference(const char *path) return false; } +/* + * 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. We could use GetFullPathName() to find the full path + * but that could change if the current directory for the drive changes + * underneath us, so we just disallow it. + */ + 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). * -- 2.40.0