Thread

is_absolute_path incorrect on Windows

Magnus Hagander <magnus@hagander.net> — 2010-04-09T13:16:58Z

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 Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Re: is_absolute_path incorrect on Windows

Kevin Grittner <kevin.grittner@wicourts.gov> — 2010-04-09T14:02:32Z

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

Re: is_absolute_path incorrect on Windows

Magnus Hagander <magnus@hagander.net> — 2010-04-09T14:35:01Z

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 Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Re: is_absolute_path incorrect on Windows

Kevin Grittner <kevin.grittner@wicourts.gov> — 2010-04-09T17:28:06Z

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

Re: is_absolute_path incorrect on Windows

Bruce Momjian <bruce@momjian.us> — 2010-06-01T00:22:47Z

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@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

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

Re: is_absolute_path incorrect on Windows

Greg Stark <gsstark@mit.edu> — 2010-06-01T00:50:35Z

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

Re: is_absolute_path incorrect on Windows

Giles Lean <giles.lean@pobox.com> — 2010-06-01T02:58:28Z

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

Re: is_absolute_path incorrect on Windows

Bruce Momjian <bruce@momjian.us> — 2010-06-01T03:02:21Z

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. +

Re: is_absolute_path incorrect on Windows

Giles Lean <giles.lean@pobox.com> — 2010-06-01T04:35:50Z

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

Re: is_absolute_path incorrect on Windows

Bruce Momjian <bruce@momjian.us> — 2010-06-01T13:37:30Z

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. +

Re: is_absolute_path incorrect on Windows

Tom Lane <tgl@sss.pgh.pa.us> — 2010-06-01T14:21:12Z

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

Re: is_absolute_path incorrect on Windows

Giles Lean <giles.lean@pobox.com> — 2010-06-01T19:20:18Z

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 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

Re: is_absolute_path incorrect on Windows

Robert Haas <robertmhaas@gmail.com> — 2010-06-01T20:04:02Z

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

Re: is_absolute_path incorrect on Windows

Bruce Momjian <bruce@momjian.us> — 2010-06-01T22:04:41Z

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. +

Re: is_absolute_path incorrect on Windows

Tom Lane <tgl@sss.pgh.pa.us> — 2010-06-01T22:09:51Z

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

Re: is_absolute_path incorrect on Windows

Bruce Momjian <bruce@momjian.us> — 2010-06-01T22:19:49Z

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. +

Re: is_absolute_path incorrect on Windows

Tom Lane <tgl@sss.pgh.pa.us> — 2010-06-01T22:34:38Z

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

Re: is_absolute_path incorrect on Windows

Bruce Momjian <bruce@momjian.us> — 2010-06-01T22:38:05Z

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. +

Re: is_absolute_path incorrect on Windows

Bruce Momjian <bruce@momjian.us> — 2011-02-03T16:50:47Z

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. +

Re: is_absolute_path incorrect on Windows

Tom Lane <tgl@sss.pgh.pa.us> — 2011-02-03T17:26:00Z

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

Re: is_absolute_path incorrect on Windows

Bruce Momjian <bruce@momjian.us> — 2011-02-03T18:20:26Z

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. +

Re: is_absolute_path incorrect on Windows

Tom Lane <tgl@sss.pgh.pa.us> — 2011-02-03T18:32:31Z

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

Re: is_absolute_path incorrect on Windows

Bruce Momjian <bruce@momjian.us> — 2011-02-04T21:53:17Z

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. +

Re: is_absolute_path incorrect on Windows

Bruce Momjian <bruce@momjian.us> — 2011-02-05T18:10:12Z

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. +

Re: is_absolute_path incorrect on Windows

Bruce Momjian <bruce@momjian.us> — 2011-02-12T14:49:36Z

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. +