is_absolute_path incorrect on Windows
in [PgSql]
|
Prev: bitmap-index-scan faster than seq-scan on full-table-scan(gin index)
Next: [BUGS] BUG #5487: dblink failed with 63 bytes connection names
From: Bruce Momjian on 31 May 2010 20:22 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 <bruce(a)momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + None of us is going to be here forever. + -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Greg Stark on 31 May 2010 20:50 On Fri, Apr 9, 2010 at 2:16 PM, Magnus Hagander <magnus(a)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(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Giles Lean on 1 Jun 2010 00:35 Bruce Momjian <bruce(a)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(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Bruce Momjian on 1 Jun 2010 09:37 Giles Lean wrote: > > Bruce Momjian <bruce(a)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(a)momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + None of us is going to be here forever. + -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Tom Lane on 1 Jun 2010 10:21
Bruce Momjian <bruce(a)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(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers |