Tom Lane wrote:
> Andres Freund <[email protected]> writes:
> > On 2013-08-19 14:28:28 -0400, Tom Lane wrote:
> >> One possibility is to do the initial check somewhere shortly after
> >> ChangeToDataDir(), and have the GUC check hook only attempt to make a
> >> check in SIGHUP context. Unfortunately we aren't passing the context to
> >> check hooks, only GucSource which isn't adequate for this. Not sure if we
> >> want to go so far as to change the check-hook API at this point. We could
> >> probably think of some other, klugy way to tell if it's initial startup.
>
> > Is it even actually safe to have stats_temp_directory PGC_SIGHUP after
> > the per DB splitup? I haven't audited the code for it, but it seems
> > somewhat likely that we would end up with some files in the old and some
> > in the new directory?
>
> That's a good point. For the moment we could just change it to
> PGC_POSTMASTER and eliminate this problem. Reducing it back to SIGHUP
> would be a future feature, which is evidently less than trivial.
Here's a patchset for this. The first patch is the same as upthread,
with the enum members renamed; the second is the actual pgstats change.
(I considered the idea that checkDirectoryPermissions returned a bitmask
of the failed checks, instead of just returning a code for the first
check that fails. This might be useful if some caller wants to ignore
certain problems. But at the moment I didn't see many other places
where this could be used, so it's probably pointless ATM.)
--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
*** a/src/backend/postmaster/postmaster.c
--- b/src/backend/postmaster/postmaster.c
***************
*** 1313,1385 **** getInstallationPaths(const char *argv0)
static void
checkDataDir(void)
{
char path[MAXPGPATH];
FILE *fp;
- struct stat stat_buf;
Assert(DataDir);
! if (stat(DataDir, &stat_buf) != 0)
{
! if (errno == ENOENT)
ereport(FATAL,
(errcode_for_file_access(),
errmsg("data directory \"%s\" does not exist",
DataDir)));
! else
ereport(FATAL,
(errcode_for_file_access(),
! errmsg("could not read permissions of directory \"%s\": %m",
! DataDir)));
}
- /* eventual chdir would fail anyway, but let's test ... */
- if (!S_ISDIR(stat_buf.st_mode))
- ereport(FATAL,
- (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
- errmsg("specified data directory \"%s\" is not a directory",
- DataDir)));
-
- /*
- * Check that the directory belongs to my userid; if not, reject.
- *
- * This check is an essential part of the interlock that prevents two
- * postmasters from starting in the same directory (see CreateLockFile()).
- * Do not remove or weaken it.
- *
- * XXX can we safely enable this check on Windows?
- */
- #if !defined(WIN32) && !defined(__CYGWIN__)
- if (stat_buf.st_uid != geteuid())
- ereport(FATAL,
- (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
- errmsg("data directory \"%s\" has wrong ownership",
- DataDir),
- errhint("The server must be started by the user that owns the data directory.")));
- #endif
-
- /*
- * Check if the directory has group or world access. If so, reject.
- *
- * It would be possible to allow weaker constraints (for example, allow
- * group access) but we cannot make a general assumption that that is
- * okay; for example there are platforms where nearly all users
- * customarily belong to the same group. Perhaps this test should be
- * configurable.
- *
- * XXX temporarily suppress check when on Windows, because there may not
- * be proper support for Unix-y file permissions. Need to think of a
- * reasonable check to apply on Windows.
- */
- #if !defined(WIN32) && !defined(__CYGWIN__)
- if (stat_buf.st_mode & (S_IRWXG | S_IRWXO))
- ereport(FATAL,
- (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
- errmsg("data directory \"%s\" has group or world access",
- DataDir),
- errdetail("Permissions should be u=rwx (0700).")));
- #endif
-
/* Look for PG_VERSION before looking for pg_control */
ValidatePgVersion(DataDir);
--- 1313,1363 ----
static void
checkDataDir(void)
{
+ CheckDirErrcode err;
char path[MAXPGPATH];
FILE *fp;
Assert(DataDir);
! err = checkDirectoryPermissions(DataDir);
! switch (err)
{
! case CKDIR_OK:
! break;
! case CKDIR_NOT_EXISTS:
ereport(FATAL,
(errcode_for_file_access(),
errmsg("data directory \"%s\" does not exist",
DataDir)));
! break;
! case CKDIR_CANT_READ_PERMS:
ereport(FATAL,
(errcode_for_file_access(),
! errmsg("could not read permissions of data directory \"%s\": %m",
! DataDir)));
! break;
! case CKDIR_NOT_DIR:
! ereport(FATAL,
! (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
! errmsg("specified data directory \"%s\" is not a directory",
! DataDir)));
! break;
! case CKDIR_WRONG_OWNER:
! ereport(FATAL,
! (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
! errmsg("data directory \"%s\" has wrong ownership",
! DataDir),
! errhint("The server must be started by the user that owns the data directory.")));
! break;
! case CKDIR_TOO_ACCESSIBLE:
! ereport(FATAL,
! (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
! errmsg("data directory \"%s\" has group or world access",
! DataDir),
! errdetail("Permissions should be u=rwx (0700).")));
! break;
}
/* Look for PG_VERSION before looking for pg_control */
ValidatePgVersion(DataDir);
*** a/src/include/port.h
--- b/src/include/port.h
***************
*** 463,468 **** extern char *inet_net_ntop(int af, const void *src, int bits,
--- 463,479 ----
/* port/pgcheckdir.c */
extern int pg_check_dir(const char *dir);
+ typedef enum
+ {
+ CKDIR_OK,
+ CKDIR_NOT_EXISTS,
+ CKDIR_CANT_READ_PERMS,
+ CKDIR_NOT_DIR,
+ CKDIR_WRONG_OWNER,
+ CKDIR_TOO_ACCESSIBLE
+ } CheckDirErrcode;
+ extern CheckDirErrcode checkDirectoryPermissions(char *directory);
+
/* port/pgmkdirp.c */
extern int pg_mkdir_p(char *path, int omode);
*** a/src/port/pgcheckdir.c
--- b/src/port/pgcheckdir.c
***************
*** 14,19 ****
--- 14,22 ----
#include "c.h"
#include <dirent.h>
+ #include <sys/types.h>
+ #include <sys/stat.h>
+ #include <unistd.h>
/*
***************
*** 88,90 **** pg_check_dir(const char *dir)
--- 91,147 ----
return result;
}
+
+ /*
+ * Verify permissions of a directory
+ */
+ CheckDirErrcode
+ checkDirectoryPermissions(char *directory)
+ {
+ struct stat stat_buf;
+
+ if (stat(directory, &stat_buf) != 0)
+ {
+ if (errno == ENOENT)
+ return CKDIR_NOT_EXISTS;
+ else
+ return CKDIR_CANT_READ_PERMS;
+ }
+
+ if (!S_ISDIR(stat_buf.st_mode))
+ return CKDIR_NOT_DIR;
+
+ /*
+ * Check that the directory belongs to my userid; if not, reject.
+ *
+ * This check is an essential part of the interlock that prevents two
+ * postmasters from starting in the same directory (see CreateLockFile()).
+ * Do not remove or weaken it.
+ *
+ * XXX can we safely enable this check on Windows?
+ */
+ #if !defined(WIN32) && !defined(__CYGWIN__)
+ if (stat_buf.st_uid != geteuid())
+ return CKDIR_WRONG_OWNER;
+ #endif
+
+ /*
+ * Check if the directory has group or world access. If so, reject.
+ *
+ * It would be possible to allow weaker constraints (for example, allow
+ * group access) but we cannot make a general assumption that that is
+ * okay; for example there are platforms where nearly all users
+ * customarily belong to the same group. Perhaps this test should be
+ * configurable.
+ *
+ * XXX temporarily suppress check when on Windows, because there may not
+ * be proper support for Unix-y file permissions. Need to think of a
+ * reasonable check to apply on Windows.
+ */
+ #if !defined(WIN32) && !defined(__CYGWIN__)
+ if (stat_buf.st_mode & (S_IRWXG | S_IRWXO))
+ return CKDIR_TOO_ACCESSIBLE;
+ #endif
+
+ return CKDIR_OK;
+ }
*** a/doc/src/sgml/config.sgml
--- b/doc/src/sgml/config.sgml
***************
*** 4446,4453 **** COPY postgres_log FROM '/full/path/to/logfile.csv' WITH csv;
is <filename>pg_stat_tmp</filename>. Pointing this at a RAM-based
file system will decrease physical I/O requirements and can lead to
improved performance.
! This parameter can only be set in the <filename>postgresql.conf</>
! file or on the server command line.
</para>
</listitem>
</varlistentry>
--- 4446,4452 ----
is <filename>pg_stat_tmp</filename>. Pointing this at a RAM-based
file system will decrease physical I/O requirements and can lead to
improved performance.
! This parameter can only be set at server start.
</para>
</listitem>
</varlistentry>
*** a/src/backend/postmaster/pgstat.c
--- b/src/backend/postmaster/pgstat.c
***************
*** 554,559 **** startup_failed:
--- 554,628 ----
}
/*
+ * Verify the permissions of stats_temp_directory.
+ *
+ * For security reasons, we want this directory to be tightly controlled;
+ * both so that we don't interfere with other running instances, and so that
+ * others can't inject fake stats.
+ *
+ * The obvious way to do this is to set this function as a GUC check hook; but
+ * this doesn't work because the first time those hooks are run, we have not
+ * changed the current directory to the data directory yet; so the check fails
+ * when it's set to a relative path, as the default value is. We'd have to
+ * find a way to skip this check in the first run of check hooks. To avoid this
+ * problem, the stats_temp_directory setting is PGC_POSTMASTER for now, and we
+ * run the check separately in PostmasterMain, after changing directory,
+ * instead.
+ *
+ * If we were to overcome that problem, we should probably also consider having
+ * the stats collector write a full set of files in the new temp directory if
+ * the setting is changed on SIGHUP.
+ */
+ bool
+ check_pgstat_temp_dir_perms(char *dir)
+ {
+ int elevel = FATAL;
+ CheckDirErrcode err;
+
+ err = checkDirectoryPermissions(dir);
+
+ switch (err)
+ {
+ case CKDIR_OK:
+ break;
+ case CKDIR_NOT_EXISTS:
+ ereport(elevel,
+ (errcode_for_file_access(),
+ errmsg("stats_temp_directory \"%s\" does not exist",
+ dir)));
+ break;
+ case CKDIR_CANT_READ_PERMS:
+ ereport(elevel,
+ (errcode_for_file_access(),
+ errmsg("could not read permissions of stats_temp_directory \"%s\": %m",
+ dir)));
+ break;
+ case CKDIR_NOT_DIR:
+ ereport(elevel,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("specified stats_temp_directory \"%s\" is not a directory",
+ dir)));
+ break;
+ case CKDIR_WRONG_OWNER:
+ ereport(elevel,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("stats_temp_directory \"%s\" has wrong ownership",
+ dir),
+ errhint("The server must be started by the user that owns the stats_temp_directory.")));
+ break;
+ case CKDIR_TOO_ACCESSIBLE:
+ ereport(elevel,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("stats_temp_directory \"%s\" has group or world access",
+ dir),
+ errdetail("Permissions should be u=rwx (0700).")));
+ break;
+ }
+
+ return err == CKDIR_OK;
+ }
+
+ /*
* subroutine for pgstat_reset_all
*/
static void
*** a/src/backend/postmaster/postmaster.c
--- b/src/backend/postmaster/postmaster.c
***************
*** 829,834 **** PostmasterMain(int argc, char *argv[])
--- 829,836 ----
ExitPostmaster(1);
}
+ (void) check_pgstat_temp_dir_perms(pgstat_stat_directory);
+
/*
* Now that we are done processing the postmaster arguments, reset
* getopt(3) library so that it will work correctly in subprocesses.
*** a/src/backend/utils/misc/guc.c
--- b/src/backend/utils/misc/guc.c
***************
*** 3081,3087 **** static struct config_string ConfigureNamesString[] =
},
{
! {"stats_temp_directory", PGC_SIGHUP, STATS_COLLECTOR,
gettext_noop("Writes temporary statistics files to the specified directory."),
NULL,
GUC_SUPERUSER_ONLY
--- 3081,3088 ----
},
{
! /* see check_pgstat_temp_dir_perms for why this is PGC_POSTMASTER */
! {"stats_temp_directory", PGC_POSTMASTER, STATS_COLLECTOR,
gettext_noop("Writes temporary statistics files to the specified directory."),
NULL,
GUC_SUPERUSER_ONLY
*** a/src/include/pgstat.h
--- b/src/include/pgstat.h
***************
*** 750,755 **** extern void pgstat_init(void);
--- 750,756 ----
extern int pgstat_start(void);
extern void pgstat_reset_all(void);
extern void allow_immediate_pgstat_restart(void);
+ extern bool check_pgstat_temp_dir_perms(char *dir);
#ifdef EXEC_BACKEND
extern void PgstatCollectorMain(int argc, char *argv[]) __attribute__((noreturn));
--
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
