Re: 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 <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ None of us is going to be here forever. +