Обсуждение: BUG #15120: use of getcwd(3)/chdir(2) during path resolution (exec.c)
BUG #15120: use of getcwd(3)/chdir(2) during path resolution (exec.c)
От
PG Bug reporting form
Дата:
The following bug has been logged on the website:
Bug reference: 15120
Logged by: Petar Bogdanovic
Email address: petar@smokva.net
PostgreSQL version: 9.4.17
Operating system: NetBSD 8.0
Description:
When trying to deduct their absolute, symlink-free path from argv0,
pg_ctl, postgres (and probably others) use:
a) getcwd(3) in find_my_exec in order to complement a relative path
containing at least one directory separator
b) getcwd(3) and chdir(2) in resolve_symlinks in order to prune
symlinks from given absolute path
In both cases, the utility/daemon will fail (and exit) if the current
directory is either not readable or not searchable, even though said
utility/daemon has permission to access each component of its absolute
path in argv0.
That scenario is not uncommon:
$ id
uid=1000(petar)
$ pwd
/home/petar
$ ls -lad .
drwx------ 5 petar petar 512 Mar 2 23:41 .
$ sudo -u pgsql /usr/bin/pg_ctl start (...)
could not identify current directory: Permission denied
could not identify current directory: Permission denied
could not identify current directory: Permission denied
The program "postgres" is needed by pg_ctl but was not found in the same
directory as "pg_ctl".
Check your installation.
There is an easy workaround for (a): Only do getcwd if argv0 is not an
absolute path. The variable cwd is not relevant for absolute paths.
(b) is not as straightforward; resolve_symlinks only trusts getcwd to
provide symlink-free paths (see comment for rationale) so, during path
resolution, it will change the current directory one or more times and
finally, before returing, chdir back to the original wd.
The last step is, however, not possible if the original wd is not read-
and/or searchable.
In the patch below, path resolution is skipped if getcwd returns EACCES.
The other option would have been ignoring the initial getcwd / final
chdir and just keeping the most recent wd, which, at that point, could
be pretty much any component of the absolute path. That seemed too
unpredictable.
This was tested with pgsql94 but that part of exec.c doesn't seem to
have changed in any more recent version.
--- src/common/exec.c.orig
+++ src/common/exec.c
@@ -103,45 +103,45 @@
/*
* find_my_exec -- find an absolute path to a valid executable
*
* argv0 is the name passed on the command line
* retpath is the output area (must be of size MAXPGPATH)
* Returns 0 if OK, -1 if error.
*
* The reason we have to work so hard to find an absolute path is that
* on some platforms we can't do dynamic loading unless we know the
* executable's location. Also, we need a full path not a relative
* path because we will later change working directory. Finally, we want
* a true path not a symlink location, so that we can locate other files
* that are part of our installation relative to the executable.
*/
int
find_my_exec(const char *argv0, char *retpath)
{
char cwd[MAXPGPATH],
test_path[MAXPGPATH];
char *path;
- if (!getcwd(cwd, MAXPGPATH))
+ if (!is_absolute_path(argv0) && !getcwd(cwd, MAXPGPATH))
{
log_error(_("could not identify current directory: %s"),
strerror(errno));
return -1;
}
/*
* If argv0 contains a separator, then PATH wasn't used.
*/
if (first_dir_separator(argv0) != NULL)
{
if (is_absolute_path(argv0))
StrNCpy(retpath, argv0, MAXPGPATH);
else
join_path_components(retpath, cwd, argv0);
canonicalize_path(retpath);
if (validate_exec(retpath) == 0)
return resolve_symlinks(retpath);
log_error(_("invalid binary \"%s\""), retpath);
return -1;
@@ -219,44 +219,50 @@
resolve_symlinks(char *path)
{
#ifdef HAVE_READLINK
struct stat buf;
char orig_wd[MAXPGPATH],
link_buf[MAXPGPATH];
char *fname;
/*
* To resolve a symlink properly, we have to chdir into its directory
and
* then chdir to where the symlink points; otherwise we may fail to
* resolve relative links correctly (consider cases involving mount
* points, for example). After following the final symlink, we use
* getcwd() to figure out where the heck we're at.
*
* One might think we could skip all this if path doesn't point to a
* symlink to start with, but that's wrong. We also want to get rid of
* any directory symlinks that are present in the given path. We expect
* getcwd() to give us an accurate, symlink-free path.
*/
if (!getcwd(orig_wd, MAXPGPATH))
{
+ if (errno == EACCES)
+ {
+ log_error(_("access to current directory denied, "
+ "skipping path resolution of \"%s\""), path);
+ return 0;
+ }
log_error(_("could not identify current directory: %s"),
strerror(errno));
return -1;
}
for (;;)
{
char *lsep;
int rllen;
lsep = last_dir_separator(path);
if (lsep)
{
*lsep = '\0';
if (chdir(path) == -1)
{
log_error4(_("could not change directory to \"%s\": %s"), path,
strerror(errno));
return -1;
}
fname = lsep + 1;
}
else
=?utf-8?q?PG_Bug_reporting_form?= <noreply@postgresql.org> writes:
> In both cases, the utility/daemon will fail (and exit) if the current
> directory is either not readable or not searchable, even though said
> utility/daemon has permission to access each component of its absolute
> path in argv0.
TBH, I don't have a whole lot of sympathy for the premise that we need to
support such cases. If we can do it without giving anything else up,
then OK, but ...
> In the patch below, path resolution is skipped if getcwd returns EACCES.
... I find that quite an unacceptable answer. We need to resolve the
symlink correctly, or we risk malfunctioning later, for the reasons
recited in the comment for find_my_exec().
The idea of pre-checking to see if the initial path is already absolute
seems safe enough, but I'm not sure how much of the use-case it'd cover.
I think your example of "sudo /usr/bin/pg_ctl" is pretty artificial;
who'd bother spelling that out?
While I've not thought about it very hard, it might be possible to rewrite
find_my_exec() and resolve_symlinks() "from the ground up" so that they
don't do getcwd() except in cases where there's really no alternative,
such as the executable having been invoked using a relative path.
Also, I realize that this patch is just a POC, but we don't much like
patches that make significant logic changes without appropriate
comment updates.
regards, tom lane
>>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes: Tom> ... I find that quite an unacceptable answer. We need to resolve Tom> the symlink correctly, or we risk malfunctioning later, for the Tom> reasons recited in the comment for find_my_exec(). On systems with openat(), would it not be possible to resolve symlinks without ever needing chdir? Tom> I think your example of "sudo /usr/bin/pg_ctl" is pretty Tom> artificial; who'd bother spelling that out? Not necessarily exactly like that, but we do occasionally get this issue coming up on the IRC channel. Tom> While I've not thought about it very hard, it might be possible to Tom> rewrite find_my_exec() and resolve_symlinks() "from the ground up" Tom> so that they don't do getcwd() except in cases where there's Tom> really no alternative, such as the executable having been invoked Tom> using a relative path. I think getcwd is avoidable even then, if openat() etc. are available. -- Andrew (irc:RhodiumToad)
Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:
> Tom> ... I find that quite an unacceptable answer. We need to resolve
> Tom> the symlink correctly, or we risk malfunctioning later, for the
> Tom> reasons recited in the comment for find_my_exec().
> On systems with openat(), would it not be possible to resolve symlinks
> without ever needing chdir?
Um ... AFAICS, openat() just opens a file, it doesn't give back a
resolved path. Did you mean readlinkat()?
In any case, we'd still need to support logic that doesn't rely on
non-portable functions. The (vague) idea I had was to try a little harder
to build absolute paths for ourselves instead of letting the system do it.
I believe for instance that canonicalize_path() has more smarts now about
shortening paths like /foo/bar/baz/../../quux than it did when these
functions were written, so we might be able to rely on just concatenating
paths and then canonicalizing the result. (OTOH, I think there are
filesystems where this wouldn't necessarily yield a nice answer, due to
multiple mounts and suchlike.)
regards, tom lane
On Sat, Mar 17, 2018 at 12:28:53PM -0400, Tom Lane wrote:
> PG Bug reporting form <noreply@postgresql.org> writes:
> > In the patch below, path resolution is skipped if getcwd returns EACCES.
>
> ... I find that quite an unacceptable answer.
The patch was just a POC, yes. I was out of better ideas and not really
qualified to rewrite find_my_exec/resolve_symlinks without breaking path
resolution for some other supported platform.
> The idea of pre-checking to see if the initial path is already absolute
> seems safe enough, but I'm not sure how much of the use-case it'd cover.
> I think your example of "sudo /usr/bin/pg_ctl" is pretty artificial;
> who'd bother spelling that out?
rc-scripts on some BSDs spell it out:
$ uname
NetBSD
$ egrep 'command=|doit=' /etc/rc.d/pgsql
command="/usr/pkg/bin/pg_ctl"
doit="/usr/bin/su -m ${pgsql_user} -c '${command} init ${command_args}'"
doit="/usr/bin/su -m ${pgsql_user} -c '${command} start ${command_args}'"
doit="/usr/bin/su -m ${pgsql_user} -c '${command} restart ${command_args}'"
doit="/usr/bin/su -m ${pgsql_user} -c '${command} stop ${command_args}'"
doit="/usr/bin/su -m ${pgsql_user} -c '${command} reload ${command_args}'"
But you are right, testing for an absolute path is not a complete
solution either. Running "sudo pg_ctl" will still fail.
>>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes: >> On systems with openat(), would it not be possible to resolve >> symlinks without ever needing chdir? Tom> Um ... AFAICS, openat() just opens a file, it doesn't give back a Tom> resolved path. Did you mean readlinkat()? No, what I meant was that if you have openat() (and fstatat() and fdopendir()), you can do what getcwd does except starting with some arbitrary directory other than the current one. So the current logic of "chdir through all symlinks, then do getcwd to find where we landed" would not be necessary. -- Andrew (irc:RhodiumToad)
Re: Andrew Gierth 2018-03-17 <87in9uhbco.fsf@news-spur.riddles.org.uk> > Tom> I think your example of "sudo /usr/bin/pg_ctl" is pretty > Tom> artificial; who'd bother spelling that out? > > Not necessarily exactly like that, but we do occasionally get this issue > coming up on the IRC channel. It's absolutely not artificial. I can easily trigger the message from my home directory: $ chmod 700 . $ sudo -u postgres psql could not change directory to "/home/cbe": Permission denied I never observed any negative side effects of it, though. (Except for the obvious fact that "\i somethinginmyhome.sql" won't work.) Christoph
Christoph Berg <myon@debian.org> writes:
> Re: Andrew Gierth 2018-03-17 <87in9uhbco.fsf@news-spur.riddles.org.uk>
>> Tom> I think your example of "sudo /usr/bin/pg_ctl" is pretty
>> Tom> artificial; who'd bother spelling that out?
>>
>> Not necessarily exactly like that, but we do occasionally get this issue
>> coming up on the IRC channel.
> It's absolutely not artificial. I can easily trigger the message from
> my home directory:
> $ chmod 700 .
> $ sudo -u postgres psql
> could not change directory to "/home/cbe": Permission denied
No, I think you missed my point: I thought typing "sudo ... /usr/bin/pg_ctl"
rather than just "sudo ... pg_ctl" seemed artificial. Your example
doesn't exactly contradict that. It's relevant here because it affects
whether or not argv[0] is already an absolute path.
regards, tom lane
On Sat, Mar 17, 2018 at 09:40:51PM +0100, Christoph Berg wrote: > $ chmod 700 . > $ sudo -u postgres psql > could not change directory to "/home/cbe": Permission denied > > I never observed any negative side effects of it, though. (Except for > the obvious fact that "\i somethinginmyhome.sql" won't work.) At least on NetBSD, shutting down the system with sudo while being in a 0700 directory will make the pgsql rc-script fail (because it relies on pg_ctl), i.e. pull the plug on a running pgsql instance. One could argue that the NetBSD /etc/rc script should, in general, chdir before doing anything but relying on getcwd/chdir still seems suboptimal.