Re: [HACKERS] is_absolute_path incorrect on Windows
Bruce Momjian wrote: Bruce Momjian wrote: Tom Lane wrote: Bruce Momjian br...@momjian.us writes: Tom Lane wrote: Bruce Momjian br...@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. Applied, with a new mention of why we don't use GetFullPathName(): + /* +* 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. +*/ -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] is_absolute_path incorrect on Windows
Bruce Momjian wrote:
Tom Lane wrote:
Bruce Momjian br...@momjian.us writes:
Tom Lane wrote:
Bruce Momjian br...@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 br...@momjian.ushttp://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
Re: [HACKERS] is_absolute_path incorrect on Windows
Tom Lane wrote:
Bruce Momjian br...@momjian.us writes:
Tom Lane wrote:
Bruce Momjian br...@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 br...@momjian.ushttp://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
***
Re: [HACKERS] is_absolute_path incorrect on Windows
Bruce Momjian wrote:
However, it misses the case with for example E:foo, which is a perfectly
valid path on windows. Which isn't absolute *or* relative - it's relative
to the current directory on the E: drive. Which will be the same as the
current directory for the process *if* the process current directory is
on drive E:. In other cases, it's a different directory.
I would argue that E:foo is always relative (which matches
is_absolute_path()). If E: is the current drive of the process, it is
relative, and if the current drive is not E:, it is relative to the last
current drive on E: for that process, or the top level if there was no
current drive. (Tested on XP.)
There seem to be three states:
1. absolute - already tested by is_absolute_path()
2. relative to the current directory (current drive)
3. relative on a different drive
We could probably develop code to test all three, but keep in mind that
the path itself can't distinguish between 2 and 3, and while you can
test the current drive, if the current drive changes, a 2 could become a
3, and via versa.
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.
--
Bruce Momjian br...@momjian.ushttp://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..991affd 100644
*** a/contrib/adminpack/adminpack.c
--- b/contrib/adminpack/adminpack.c
*** convert_and_check_filename(text *arg, bo
*** 73,84
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 */
--- 73,78
*** convert_and_check_filename(text *arg, bo
*** 97,102
--- 91,100
}
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..9774b39 100644
*** a/src/backend/utils/adt/genfile.c
--- b/src/backend/utils/adt/genfile.c
*** convert_and_check_filename(text *arg)
*** 51,62
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 */
--- 51,56
*** convert_and_check_filename(text *arg)
*** 74,79
--- 68,77
}
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..9a14488 100644
*** a/src/include/port.h
--- b/src/include/port.h
*** extern void join_path_components(char *r
*** 41,47
const char *head, const char *tail);
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_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);
--- 41,47
const char *head, const char *tail);
extern void canonicalize_path(char *path);
extern void make_native_path(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)
Re: [HACKERS] is_absolute_path incorrect on Windows
Bruce Momjian br...@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? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] is_absolute_path incorrect on Windows
Tom Lane wrote: Bruce Momjian br...@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 I assume we can allow .. for an absolute path in these cases, i.e. it has to match the data or log path we defined, and I don't see a general reason to prevent .. in absolute paths, only relative ones. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] is_absolute_path incorrect on Windows
Bruce Momjian br...@momjian.us writes: Tom Lane wrote: Bruce Momjian br...@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' regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] is_absolute_path incorrect on Windows
Giles Lean wrote: Bruce Momjian br...@momjian.us wrote: is_relative_to_cwd()? ../../../../some/other/place/not/under/cwd Names are hard, but if I understood the original post, the revised function is intended to check that the directory is below the current working directory. We check for things like .. other places, though we could roll that into the macro if we wanted. Because we are adding a new function, that might make sense. If my understanding is wrong (always possible!) and it only has to be on the same drive, then your name is probably better although it doesn't mention 'drive' ... hrm. is_on_current_drive()? (Yuck.) is_on_current_filesystem()? (Yuck, but at least more general.) I think we (or at least I) need some clarification from the original poster about what the code is checking for in detail. I think you have to look at all the reference to is_absolute_path() in the C code. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + None of us is going to be here forever. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] is_absolute_path incorrect on Windows
Bruce Momjian br...@momjian.us writes: Giles Lean wrote: Names are hard, but if I understood the original post, the revised function is intended to check that the directory is below the current working directory. We check for things like .. other places, though we could roll that into the macro if we wanted. Because we are adding a new function, that might make sense. Yeah. If we were to go with Greg's suggestion of inventing a separate is_relative_to_cwd test function, I'd expect that to insist on no .. while it was at it. That seems like a fairly clean approach in the abstract, but I agree that somebody would have to look closely at each existing usage to be sure it works out well. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] is_absolute_path incorrect on Windows
Tom Lane t...@sss.pgh.pa.us wrote: Yeah. If we were to go with Greg's suggestion of inventing a separate is_relative_to_cwd test function, I'd expect that to insist on no .. while it was at it. So it's now two problems, and I think this is my final comment: 1. is_relative_to_cwd() I continue to think is a bad name for something concerned about .. (plus on Windows not having a drive letter other than the current one); the normal meaning of relative path is merely not absolute 2. if this proposed new function is to replace some uses of is_absolute_path() then I'm afraid I'd not picked up on that (as Bruce did) and have no opinion on whether it's a good idea or not, and am not qualified to be the one doing the code investigation (not enough knowledge of the code, it's beta time, and I'm frantically short of time just now as well, sorry) Giles -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] is_absolute_path incorrect on Windows
On Tue, Jun 1, 2010 at 3:20 PM, Giles Lean giles.l...@pobox.com wrote: 1. is_relative_to_cwd() I continue to think is a bad name for something concerned about .. (plus on Windows not having a drive letter other than the current one); the normal meaning of relative path is merely not absolute Maybe something like is_under_cwd()? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] is_absolute_path incorrect on Windows
Robert Haas wrote: On Tue, Jun 1, 2010 at 3:20 PM, Giles Lean giles.l...@pobox.com wrote: 1. is_relative_to_cwd() I continue to think is a bad name for something ? concerned about .. (plus on Windows not having a drive letter other ? than the current one); the normal meaning of relative path is ? merely not absolute Maybe something like is_under_cwd()? Yeah, is_below_cwd? -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + None of us is going to be here forever. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] is_absolute_path incorrect on Windows
Bruce Momjian br...@momjian.us writes: Robert Haas wrote: Maybe something like is_under_cwd()? Yeah, is_below_cwd? Hm. Neither of these obviously exclude the case of an absolute path that happens to lead to cwd. I'm not sure how important that is, but still ... regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] is_absolute_path incorrect on Windows
Tom Lane wrote: Bruce Momjian br...@momjian.us writes: Robert Haas wrote: Maybe something like is_under_cwd()? Yeah, is_below_cwd? Hm. Neither of these obviously exclude the case of an absolute path that happens to lead to cwd. I'm not sure how important that is, but still ... We currently do that with path_is_prefix_of_path(). Maybe that needs to be called as well. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + None of us is going to be here forever. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] is_absolute_path incorrect on Windows
Bruce Momjian br...@momjian.us writes: Tom Lane wrote: Hm. Neither of these obviously exclude the case of an absolute path that happens to lead to cwd. I'm not sure how important that is, but still ... We currently do that with path_is_prefix_of_path(). Maybe that needs to be called as well. I think you misunderstood my point: in the places where we're insisting on a relative path, I don't think we *want* an absolute path to be accepted. What I was trying to say is that these proposed function names don't obviously mean a relative path that does not try to break out of cwd. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] is_absolute_path incorrect on Windows
Tom Lane wrote: Bruce Momjian br...@momjian.us writes: Tom Lane wrote: Hm. Neither of these obviously exclude the case of an absolute path that happens to lead to cwd. I'm not sure how important that is, but still ... We currently do that with path_is_prefix_of_path(). Maybe that needs to be called as well. I think you misunderstood my point: in the places where we're insisting on a relative path, I don't think we *want* an absolute path to be accepted. What I was trying to say is that these proposed function names don't obviously mean a relative path that does not try to break out of cwd. Oh, OK. I know Magnus has a patch that he was working on and will send it out soon. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + None of us is going to be here forever. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] is_absolute_path incorrect on Windows
Magnus Hagander wrote: Here's a thread that incorrectly started on the security list, but really is more about functionality. Looking for comments: I looked into this and it seems to be a serious issue. The function is_absolute_path() is incorrect on Windows. As it's implemented, it considers the following to be an absolute path: * Anything that starts with / * Anything that starst with \ * Anything alphanumerical, followed by a colon, followed by either / or \ Everything else is treated as relative. However, it misses the case with for example E:foo, which is a perfectly valid path on windows. Which isn't absolute *or* relative - it's relative to the current directory on the E: drive. Which will be the same as the current directory for the process *if* the process current directory is on drive E:. In other cases, it's a different directory. I would argue that E:foo is always relative (which matches is_absolute_path()). If E: is the current drive of the process, it is relative, and if the current drive is not E:, it is relative to the last current drive on E: for that process, or the top level if there was no current drive. (Tested on XP.) There seem to be three states: 1. absolute - already tested by is_absolute_path() 2. relative to the current directory (current drive) 3. relative on a different drive We could probably develop code to test all three, but keep in mind that the path itself can't distinguish between 2 and 3, and while you can test the current drive, if the current drive changes, a 2 could become a 3, and via versa. This function is used in the genfile.c functions to read and list files by admin tools like pgadmin - to make sure we can only open files that are in our own data directory - by making sure they're either relative, or they're absolute but rooted in our own data directory. (It rejects anything with .. in it already). So it is currently broken because you can read other drives? The latest step in that thread is this comment from Tom: Yeah. I think the fundamental problem is that this code assumes there are two kinds of paths: absolute and relative to CWD. But on Windows there's really a third kind, relative with a drive letter. I believe that is_absolute_path is correct on its own terms, namely to identify a fully specified path. If we change it to allow cases that aren't really fully specified we will break other uses, such as in make_absolute_path. I'm inclined to propose adding an additional path test operator, along the lines of has_drive_specifier(path) (always false on non-Windows), and use that where needed to reject relative-with-drive-letter paths. I think I agree with this point, but we all agreed that we should throw the question out for the wider audience on -hackers for more comments. So, should this be implemented? -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + None of us is going to be here forever. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] is_absolute_path incorrect on Windows
On Fri, Apr 9, 2010 at 2:16 PM, Magnus Hagander mag...@hagander.net wrote: I'm inclined to propose adding an additional path test operator, along the lines of has_drive_specifier(path) (always false on non-Windows), and use that where needed to reject relative-with-drive-letter paths. I think I agree with this point, but we all agreed that we should throw the question out for the wider audience on -hackers for more comments. If you invert the sense then it might not be so windows-specific: /* NOTE: these two functions aren't complementary under windows, * be sure to use the right one */ /* Check path always means the same thing regardless of cwd */ is_absolute_path() /* Check that path is under cwd */ is_relative_path() -- greg -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] is_absolute_path incorrect on Windows
Greg Stark gsst...@mit.edu wrote: /* NOTE: these two functions aren't complementary under windows, * be sure to use the right one */ /* Check path always means the same thing regardless of cwd */ is_absolute_path() /* Check that path is under cwd */ is_relative_path() Um ... isn't that second function name pretty misleading, if what you want is what the comment above it says? Assuming the comment is what you want (presumably, else you'd just negate a test of is_absolute_path()) then my suggestions for (IMHO :-) clearer names would be is_subdir_path() if you still want path in the name, or just is_subdir() if the meaning will be clear enough from context. Giles -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] is_absolute_path incorrect on Windows
Giles Lean wrote: Greg Stark gsst...@mit.edu wrote: /* NOTE: these two functions aren't complementary under windows, * be sure to use the right one */ /* Check path always means the same thing regardless of cwd */ is_absolute_path() /* Check that path is under cwd */ is_relative_path() Um ... isn't that second function name pretty misleading, if what you want is what the comment above it says? Assuming the comment is what you want (presumably, else you'd just negate a test of is_absolute_path()) then my suggestions for (IMHO :-) clearer names would be is_subdir_path() if you still want path in the name, or just is_subdir() if the meaning will be clear enough from context. is_relative_to_cwd()? -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + None of us is going to be here forever. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] is_absolute_path incorrect on Windows
Bruce Momjian br...@momjian.us wrote: is_relative_to_cwd()? ../../../../some/other/place/not/under/cwd Names are hard, but if I understood the original post, the revised function is intended to check that the directory is below the current working directory. If my understanding is wrong (always possible!) and it only has to be on the same drive, then your name is probably better although it doesn't mention 'drive' ... hrm. is_on_current_drive()? (Yuck.) is_on_current_filesystem()? (Yuck, but at least more general.) I think we (or at least I) need some clarification from the original poster about what the code is checking for in detail. Cheers, Giles -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] is_absolute_path incorrect on Windows
Magnus Hagander mag...@hagander.net wrote: it considers the following to be an absolute path: * Anything that starts with / * Anything that starst with \ These aren't truly absolute, because the directory you find will be based on your current work directory's drive letter; however, if the point is to then check whether it falls under the current work directory, even when an absolute path is specified, it works. * Anything alphanumerical, followed by a colon, followed by either / or \ I assume we reject anything where what precedes the colon doesn't match the current drive's designation? However, it misses the case with for example E:foo, which is a perfectly valid path on windows. Which isn't absolute *or* relative - it's relative to the current directory on the E: drive. This function is used in the genfile.c functions to read and list files by admin tools like pgadmin - to make sure we can only open files that are in our own data directory - by making sure they're either relative, or they're absolute but rooted in our own data directory. (It rejects anything with .. in it already). Well, if that's a good idea, then you would need to reject anything specifying a drive which doesn't match the drive of the data directory. Barring the user from accessing directories on the current drive which aren't under the data directory on that drive, but allowing them to access any other drive they want, is just silly. It does raise the question of why we need to check this at all, rather than counting on OS security to limit access to things which shouldn't be seen. -Kevin -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] is_absolute_path incorrect on Windows
On Fri, Apr 9, 2010 at 16:02, Kevin Grittner kevin.gritt...@wicourts.gov wrote: Magnus Hagander mag...@hagander.net wrote: it considers the following to be an absolute path: * Anything that starts with / * Anything that starst with \ These aren't truly absolute, because the directory you find will be based on your current work directory's drive letter; however, if the point is to then check whether it falls under the current work directory, even when an absolute path is specified, it works. That is true. However, since we have chdir():ed into our data directory, we know which drive we are on. So I think we're safe. * Anything alphanumerical, followed by a colon, followed by either / or \ I assume we reject anything where what precedes the colon doesn't match the current drive's designation? Define reject? We're just answering the question is absolute path?. It's then up to the caller. For example, in the genfiles function, we will take the absolute path and compare it to the path specified for the data directory, to make sure we can't go outside it. However, it misses the case with for example E:foo, which is a perfectly valid path on windows. Which isn't absolute *or* relative - it's relative to the current directory on the E: drive. This function is used in the genfile.c functions to read and list files by admin tools like pgadmin - to make sure we can only open files that are in our own data directory - by making sure they're either relative, or they're absolute but rooted in our own data directory. (It rejects anything with .. in it already). Well, if that's a good idea, then you would need to reject anything specifying a drive which doesn't match the drive of the data directory. Barring the user from accessing directories on the current drive which aren't under the data directory on that drive, but allowing them to access any other drive they want, is just silly. Yes. That's what the code does - once it's determined that it's an absolute directory, it will compare the start of it to the data directory. This will obviously not match if the data directory is on a different drive. It does raise the question of why we need to check this at all, rather than counting on OS security to limit access to things which shouldn't be seen. That is a different question, of course. For reading, it really should. But there was strong opposition to that when the functions were added, so this was added as an extra security check. This is why we're not treating it as a security problem. The callpoints require you to have superuser, so this is really just a way to make it a bit harder to do things wrong. There are other ways to get to the information, so it's not a security issue. It's more about preventing you from doing the wrong thing by mistake. Say a \copy foo e:foo.csv instead of e:/foo.csv, that might overwrite the wrong file by mistake - since the path isn't fully specified. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] is_absolute_path incorrect on Windows
Magnus Hagander mag...@hagander.net wrote: On Fri, Apr 9, 2010 at 16:02, Kevin Grittner I assume we reject anything where what precedes the colon doesn't match the current drive's designation? Define reject? I guess I made that comment thinking about the example of usage farther down. We're just answering the question is absolute path?. It's then up to the caller. For example, in the genfiles function, we will take the absolute path and compare it to the path specified for the data directory, to make sure we can't go outside it. I would say that a function which tells you whether a path is absolute should, under Windows, return false if there isn't a leading slash or backslash after any drive specification. Whether lack of a drive specification should cause it to return false or whether that should be a separate test doesn't seem like it makes a big difference, as long as it's clear and documented. -Kevin -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
