Обсуждение: is_absolute_path incorrect on Windows
Here's a thread that incorrectly started on the security list, but really is more about functionality. Looking for comments: 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. 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). 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 - comments? -- Magnus HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/
Magnus Hagander <magnus@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
On Fri, Apr 9, 2010 at 16:02, Kevin Grittner <Kevin.Grittner@wicourts.gov> wrote: > Magnus Hagander <magnus@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 HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/
Magnus Hagander <magnus@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
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 differentdrive 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 <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + None of us is going to be here forever. +
On Fri, Apr 9, 2010 at 2:16 PM, Magnus Hagander <magnus@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
Greg Stark <gsstark@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
Giles Lean wrote: > > Greg Stark <gsstark@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 <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + None of us is going to be here forever. +
Bruce Momjian <bruce@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
Giles Lean wrote: > > Bruce Momjian <bruce@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 <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + None of us is going to be here forever. +
Bruce Momjian <bruce@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
Tom Lane <tgl@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 havinga 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 thecode investigation (not enough knowledge of the code, it's beta time, and I'm frantically short of time just now aswell, sorry) Giles
On Tue, Jun 1, 2010 at 3:20 PM, Giles Lean <giles.lean@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
Robert Haas wrote: > On Tue, Jun 1, 2010 at 3:20 PM, Giles Lean <giles.lean@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 <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + None of us is going to be here forever. +
Bruce Momjian <bruce@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
Tom Lane wrote: > Bruce Momjian <bruce@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 <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + None of us is going to be here forever. +
Bruce Momjian <bruce@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
Tom Lane wrote: > Bruce Momjian <bruce@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 <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + None of us is going to be here forever. +
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 <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..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) \ ( \ IS_DIR_SEP((filename)[0]) || \ --- 77,83 ---- #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..d3d1249 100644 *** a/src/port/path.c --- b/src/port/path.c *************** *** 40,45 **** --- 40,46 ---- #define IS_PATH_VAR_SEP(ch) ((ch) == ';') #endif + static bool path_contains_parent_reference(const char *path); static void make_relative_path(char *ret_path, const char *target_path, const char *bin_path, const char *my_exec_path); static void trim_directory(char *path); *************** canonicalize_path(char *path) *** 336,342 **** * This is a bit tricky because we mustn't be fooled by "..a.." (legal) * nor "C:.." (legal on Unix but not Windows). */ ! bool path_contains_parent_reference(const char *path) { int path_len; --- 337,343 ---- * This is a bit tricky because we mustn't be fooled by "..a.." (legal) * nor "C:.." (legal on Unix but not Windows). */ ! static bool path_contains_parent_reference(const char *path) { int path_len; *************** path_contains_parent_reference(const cha *** 359,364 **** --- 360,396 ---- } /* + * 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
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?
regards, tom lane
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 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 <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
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'
regards, tom lane
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
Bruce Momjian wrote: > 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. 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 <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..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
Bruce Momjian wrote: > Bruce Momjian wrote: > > 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. > > 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 <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +