Tom Lane wrote:
> I think we should change 9.3 to be restrictive about ownership/permissions
> on the stats_temp_directory (ie, require owner = postgres user,
> permissions = 0700, same as for the $PGDATA directory).
Not an easy thing to do, this. It should be done as a GUC check hook,
ISTM, but this doesn't work because the first time those are run we
haven't yet changed to the data directory, and so any relative path
(which the default value is) will cause the check to fail (I *assume*
setting an absolute path would work, but I haven't tried). We could
skip the check on the first run, and verify the directory separately in
PostmasterMain() after changing CWD, but I don't see any way to detect
that we're in the initial run of GUC processing. Any thoughts? Maybe
the idea of using a GUC check hook is flawed, but I don't think so
because we also need to verify a directory when the setting changes on
SIGHUP.
The implementation I chose for the actual check was to separate the
permission checks from checkDataDir() into src/port/pgcheckdir.c that
returns different error codes for each case; see first attachment.
This part seems pretty reasonable, except that the code should be in
src/common rather than src/port, but I believe the entire pgcheckdir.c
file should be moved.
> In addition to that, it might be a good idea to do what the comment in the
> code suggests, namely do more than zero checking on each file name to try
> to make sure it looks like a stats temp file name that we'd generate
> before we delete it. The ownership/permissions test wouldn't be enough
> to prevent you from pointing at, say, ~postgres and thereby losing some
> files you'd rather not.
This seems pretty simple to do; see second attachment. (It would delete
files named, "db_1234.tmpfoobar", that is, valid names with suffixes,
but I can't see that being a problem). (I haven't really tested this
part at all.)
--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
*** a/src/backend/postmaster/postmaster.c
--- b/src/backend/postmaster/postmaster.c
***************
*** 1333,1405 **** 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);
--- 1333,1383 ----
static void
checkDataDir(void)
{
+ CheckDirErrcode err;
char path[MAXPGPATH];
FILE *fp;
Assert(DataDir);
! err = checkDirectoryPermissions(DataDir);
! switch (err)
{
! case CKDIR_OK:
! break;
! case CKDIR_NOTEXISTS:
ereport(FATAL,
(errcode_for_file_access(),
errmsg("data directory \"%s\" does not exist",
DataDir)));
! break;
! case CKDIR_CANTREADPERMS:
ereport(FATAL,
(errcode_for_file_access(),
! errmsg("could not read permissions of data directory \"%s\": %m",
! DataDir)));
! break;
! case CKDIR_NOTADIR:
! ereport(FATAL,
! (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
! errmsg("specified data directory \"%s\" is not a directory",
! DataDir)));
! break;
! case CKDIR_WRONGOWNER:
! 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_TOOACCESIBLE:
! 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
***************
*** 459,464 **** extern char *inet_net_ntop(int af, const void *src, int bits,
--- 459,475 ----
/* port/pgcheckdir.c */
extern int pg_check_dir(const char *dir);
+ typedef enum
+ {
+ CKDIR_OK,
+ CKDIR_NOTEXISTS,
+ CKDIR_CANTREADPERMS,
+ CKDIR_NOTADIR,
+ CKDIR_WRONGOWNER,
+ CKDIR_TOOACCESIBLE
+ } 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_NOTEXISTS;
+ else
+ return CKDIR_CANTREADPERMS;
+ }
+
+ if (!S_ISDIR(stat_buf.st_mode))
+ return CKDIR_NOTADIR;
+
+ /*
+ * 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_WRONGOWNER;
+ #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_TOOACCESIBLE;
+ #endif
+
+ return CKDIR_OK;
+ }
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
