From 792a6809cd3b6e7c006af6221011bfbc2b376601 Mon Sep 17 00:00:00 2001 From: David Steele Date: Fri, 6 Apr 2018 09:29:15 -0400 Subject: [PATCH 1/2] Refactor file permissions in backend/frontend Adds a new header (file_perm.h) and makes all front-end utilities use the new constants for setting umask. Converts mkdir() calls in the backend to MakePGDirectory() if the original call used default permissions (always the case for regular PG directories). Adds tests to make sure permissions in PGDATA are set correctly by the front-end tools. Author: David Steele Reviewed-By: Michael Paquier, with discussion amongst many others. Discussion: https://postgr.es/m/ad346fe6-b23e-59f1-ecb7-0e08390ad629%40pgmasters.net --- src/backend/access/transam/xlog.c | 2 +- src/backend/commands/tablespace.c | 18 ++++--- src/backend/postmaster/postmaster.c | 7 +-- src/backend/postmaster/syslogger.c | 5 +- src/backend/replication/basebackup.c | 5 +- src/backend/replication/slot.c | 5 +- src/backend/storage/file/copydir.c | 2 +- src/backend/storage/file/fd.c | 38 +++++++++------ src/backend/storage/ipc/dsm_impl.c | 3 +- src/backend/storage/ipc/ipc.c | 4 ++ src/backend/utils/init/miscinit.c | 5 +- src/bin/initdb/initdb.c | 24 ++++----- src/bin/initdb/t/001_initdb.pl | 11 ++++- src/bin/pg_basebackup/pg_basebackup.c | 9 ++-- src/bin/pg_basebackup/t/010_pg_basebackup.pl | 14 +++++- src/bin/pg_basebackup/t/020_pg_receivewal.pl | 14 +++++- src/bin/pg_basebackup/walmethods.c | 6 ++- src/bin/pg_ctl/pg_ctl.c | 4 +- src/bin/pg_ctl/t/001_start_stop.pl | 18 ++++++- src/bin/pg_resetwal/pg_resetwal.c | 5 +- src/bin/pg_resetwal/t/001_basic.pl | 12 ++++- src/bin/pg_rewind/RewindTest.pm | 4 ++ src/bin/pg_rewind/file_ops.c | 7 +-- src/bin/pg_rewind/t/001_basic.pl | 11 ++++- src/bin/pg_upgrade/file.c | 5 +- src/bin/pg_upgrade/pg_upgrade.c | 3 +- src/bin/pg_upgrade/test.sh | 11 +++++ src/common/Makefile | 4 +- src/common/file_perm.c | 19 ++++++++ src/include/common/file_perm.h | 32 ++++++++++++ src/include/storage/fd.h | 3 ++ src/test/perl/PostgresNode.pm | 3 ++ src/test/perl/TestLib.pm | 73 ++++++++++++++++++++++++++++ src/tools/msvc/Mkvcbuild.pm | 4 +- 34 files changed, 316 insertions(+), 74 deletions(-) create mode 100644 src/common/file_perm.c create mode 100644 src/include/common/file_perm.h diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index 813b2afaac..4a47395174 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -4107,7 +4107,7 @@ ValidateXLOGDirectoryStructure(void) { ereport(LOG, (errmsg("creating missing WAL directory \"%s\"", path))); - if (mkdir(path, S_IRWXU) < 0) + if (MakePGDirectory(path) < 0) ereport(FATAL, (errmsg("could not create missing directory \"%s\": %m", path))); diff --git a/src/backend/commands/tablespace.c b/src/backend/commands/tablespace.c index 5c450caa4e..cfc9fe468c 100644 --- a/src/backend/commands/tablespace.c +++ b/src/backend/commands/tablespace.c @@ -68,6 +68,7 @@ #include "commands/seclabel.h" #include "commands/tablecmds.h" #include "commands/tablespace.h" +#include "common/file_perm.h" #include "miscadmin.h" #include "postmaster/bgwriter.h" #include "storage/fd.h" @@ -151,7 +152,7 @@ TablespaceCreateDbspace(Oid spcNode, Oid dbNode, bool isRedo) else { /* Directory creation failed? */ - if (mkdir(dir, S_IRWXU) < 0) + if (MakePGDirectory(dir) < 0) { char *parentdir; @@ -173,7 +174,7 @@ TablespaceCreateDbspace(Oid spcNode, Oid dbNode, bool isRedo) get_parent_directory(parentdir); get_parent_directory(parentdir); /* Can't create parent and it doesn't already exist? */ - if (mkdir(parentdir, S_IRWXU) < 0 && errno != EEXIST) + if (MakePGDirectory(parentdir) < 0 && errno != EEXIST) ereport(ERROR, (errcode_for_file_access(), errmsg("could not create directory \"%s\": %m", @@ -184,7 +185,7 @@ TablespaceCreateDbspace(Oid spcNode, Oid dbNode, bool isRedo) parentdir = pstrdup(dir); get_parent_directory(parentdir); /* Can't create parent and it doesn't already exist? */ - if (mkdir(parentdir, S_IRWXU) < 0 && errno != EEXIST) + if (MakePGDirectory(parentdir) < 0 && errno != EEXIST) ereport(ERROR, (errcode_for_file_access(), errmsg("could not create directory \"%s\": %m", @@ -192,7 +193,7 @@ TablespaceCreateDbspace(Oid spcNode, Oid dbNode, bool isRedo) pfree(parentdir); /* Create database directory */ - if (mkdir(dir, S_IRWXU) < 0) + if (MakePGDirectory(dir) < 0) ereport(ERROR, (errcode_for_file_access(), errmsg("could not create directory \"%s\": %m", @@ -279,7 +280,8 @@ CreateTableSpace(CreateTableSpaceStmt *stmt) /* * Check that location isn't too long. Remember that we're going to append * 'PG_XXX//_.'. FYI, we never actually - * reference the whole path here, but mkdir() uses the first two parts. + * reference the whole path here, but MakePGDirectory() uses the first + * two parts. */ if (strlen(location) + 1 + strlen(TABLESPACE_VERSION_DIRECTORY) + 1 + OIDCHARS + 1 + OIDCHARS + 1 + FORKNAMECHARS + 1 + OIDCHARS > MAXPGPATH) @@ -574,7 +576,7 @@ create_tablespace_directories(const char *location, const Oid tablespaceoid) * Attempt to coerce target directory to safe permissions. If this fails, * it doesn't exist or has the wrong owner. */ - if (chmod(location, S_IRWXU) != 0) + if (chmod(location, pg_dir_create_mode) != 0) { if (errno == ENOENT) ereport(ERROR, @@ -599,7 +601,7 @@ create_tablespace_directories(const char *location, const Oid tablespaceoid) if (stat(location_with_version_dir, &st) == 0 && S_ISDIR(st.st_mode)) { if (!rmtree(location_with_version_dir, true)) - /* If this failed, mkdir() below is going to error. */ + /* If this failed, MakePGDirectory() below is going to error. */ ereport(WARNING, (errmsg("some useless files may be left behind in old database directory \"%s\"", location_with_version_dir))); @@ -610,7 +612,7 @@ create_tablespace_directories(const char *location, const Oid tablespaceoid) * The creation of the version directory prevents more than one tablespace * in a single location. */ - if (mkdir(location_with_version_dir, S_IRWXU) < 0) + if (MakePGDirectory(location_with_version_dir) < 0) { if (errno == EEXIST) ereport(ERROR, diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c index 3dfb87d701..10afecffb3 100644 --- a/src/backend/postmaster/postmaster.c +++ b/src/backend/postmaster/postmaster.c @@ -97,6 +97,7 @@ #include "access/xlog.h" #include "bootstrap/bootstrap.h" #include "catalog/pg_control.h" +#include "common/file_perm.h" #include "common/ip.h" #include "lib/ilist.h" #include "libpq/auth.h" @@ -589,7 +590,7 @@ PostmasterMain(int argc, char *argv[]) /* * for security, no dir or file created can be group or other accessible */ - umask(S_IRWXG | S_IRWXO); + umask(PG_MODE_MASK_OWNER); /* * Initialize random(3) so we don't get the same values in every run. @@ -4490,9 +4491,9 @@ internal_forkexec(int argc, char *argv[], Port *port) { /* * As in OpenTemporaryFileInTablespace, try to make the temp-file - * directory + * directory, ignoring errors. */ - mkdir(PG_TEMP_FILES_DIR, S_IRWXU); + (void) MakePGDirectory(PG_TEMP_FILES_DIR); fp = AllocateFile(tmpfilename, PG_BINARY_W); if (!fp) diff --git a/src/backend/postmaster/syslogger.c b/src/backend/postmaster/syslogger.c index f70eea37df..58b759f305 100644 --- a/src/backend/postmaster/syslogger.c +++ b/src/backend/postmaster/syslogger.c @@ -41,6 +41,7 @@ #include "postmaster/postmaster.h" #include "postmaster/syslogger.h" #include "storage/dsm.h" +#include "storage/fd.h" #include "storage/ipc.h" #include "storage/latch.h" #include "storage/pg_shmem.h" @@ -322,7 +323,7 @@ SysLoggerMain(int argc, char *argv[]) /* * Also, create new directory if not present; ignore errors */ - mkdir(Log_directory, S_IRWXU); + (void) MakePGDirectory(Log_directory); } if (strcmp(Log_filename, currentLogFilename) != 0) { @@ -564,7 +565,7 @@ SysLogger_Start(void) /* * Create log directory if not present; ignore errors */ - mkdir(Log_directory, S_IRWXU); + (void) MakePGDirectory(Log_directory); /* * The initial logfile is created right in the postmaster, to verify that diff --git a/src/backend/replication/basebackup.c b/src/backend/replication/basebackup.c index 8ba29453b9..babf85a6ea 100644 --- a/src/backend/replication/basebackup.c +++ b/src/backend/replication/basebackup.c @@ -19,6 +19,7 @@ #include "access/xlog_internal.h" /* for pg_start/stop_backup */ #include "catalog/catalog.h" #include "catalog/pg_type.h" +#include "common/file_perm.h" #include "lib/stringinfo.h" #include "libpq/libpq.h" #include "libpq/pqformat.h" @@ -930,7 +931,7 @@ sendFileWithContent(const char *filename, const char *content) statbuf.st_gid = getegid(); #endif statbuf.st_mtime = time(NULL); - statbuf.st_mode = S_IRUSR | S_IWUSR; + statbuf.st_mode = pg_file_create_mode; statbuf.st_size = len; _tarWriteHeader(filename, NULL, &statbuf, false); @@ -1628,7 +1629,7 @@ _tarWriteDir(const char *pathbuf, int basepathlen, struct stat *statbuf, #else if (pgwin32_is_junction(pathbuf)) #endif - statbuf->st_mode = S_IFDIR | S_IRWXU; + statbuf->st_mode = S_IFDIR | pg_dir_create_mode; return _tarWriteHeader(pathbuf + basepathlen + 1, NULL, statbuf, sizeonly); } diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c index fc9ef22b0b..056628fe8e 100644 --- a/src/backend/replication/slot.c +++ b/src/backend/replication/slot.c @@ -1166,13 +1166,14 @@ CreateSlotOnDisk(ReplicationSlot *slot) * It's just barely possible that some previous effort to create or drop a * slot with this name left a temp directory lying around. If that seems * to be the case, try to remove it. If the rmtree() fails, we'll error - * out at the mkdir() below, so we don't bother checking success. + * out at the MakePGDirectory() below, so we don't bother checking + * success. */ if (stat(tmppath, &st) == 0 && S_ISDIR(st.st_mode)) rmtree(tmppath, true); /* Create and fsync the temporary slot directory. */ - if (mkdir(tmppath, S_IRWXU) < 0) + if (MakePGDirectory(tmppath) < 0) ereport(ERROR, (errcode_for_file_access(), errmsg("could not create directory \"%s\": %m", diff --git a/src/backend/storage/file/copydir.c b/src/backend/storage/file/copydir.c index ca6342db0d..4a0d23b11e 100644 --- a/src/backend/storage/file/copydir.c +++ b/src/backend/storage/file/copydir.c @@ -41,7 +41,7 @@ copydir(char *fromdir, char *todir, bool recurse) char fromfile[MAXPGPATH * 2]; char tofile[MAXPGPATH * 2]; - if (mkdir(todir, S_IRWXU) != 0) + if (MakePGDirectory(todir) != 0) ereport(ERROR, (errcode_for_file_access(), errmsg("could not create directory \"%s\": %m", todir))); diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c index d30a725f90..3231ffe2b9 100644 --- a/src/backend/storage/file/fd.c +++ b/src/backend/storage/file/fd.c @@ -84,6 +84,7 @@ #include "access/xlog.h" #include "catalog/catalog.h" #include "catalog/pg_tablespace.h" +#include "common/file_perm.h" #include "pgstat.h" #include "portability/mem.h" #include "storage/fd.h" @@ -124,12 +125,6 @@ */ #define FD_MINFREE 10 -/* - * Default mode for created files, unless something else is specified using - * the *Perm() function variants. - */ -#define PG_FILE_MODE_DEFAULT (S_IRUSR | S_IWUSR) - /* * A number of platforms allow individual processes to open many more files * than they can really support when *many* processes do the same thing. @@ -937,7 +932,7 @@ set_max_safe_fds(void) int BasicOpenFile(const char *fileName, int fileFlags) { - return BasicOpenFilePerm(fileName, fileFlags, PG_FILE_MODE_DEFAULT); + return BasicOpenFilePerm(fileName, fileFlags, pg_file_create_mode); } /* @@ -1356,7 +1351,7 @@ FileInvalidate(File file) File PathNameOpenFile(const char *fileName, int fileFlags) { - return PathNameOpenFilePerm(fileName, fileFlags, PG_FILE_MODE_DEFAULT); + return PathNameOpenFilePerm(fileName, fileFlags, pg_file_create_mode); } /* @@ -1434,7 +1429,7 @@ PathNameOpenFilePerm(const char *fileName, int fileFlags, mode_t fileMode) void PathNameCreateTemporaryDir(const char *basedir, const char *directory) { - if (mkdir(directory, S_IRWXU) < 0) + if (MakePGDirectory(directory) < 0) { if (errno == EEXIST) return; @@ -1444,14 +1439,14 @@ PathNameCreateTemporaryDir(const char *basedir, const char *directory) * EEXIST to close a race against another process following the same * algorithm. */ - if (mkdir(basedir, S_IRWXU) < 0 && errno != EEXIST) + if (MakePGDirectory(basedir) < 0 && errno != EEXIST) ereport(ERROR, (errcode_for_file_access(), errmsg("cannot create temporary directory \"%s\": %m", basedir))); /* Try again. */ - if (mkdir(directory, S_IRWXU) < 0 && errno != EEXIST) + if (MakePGDirectory(directory) < 0 && errno != EEXIST) ereport(ERROR, (errcode_for_file_access(), errmsg("cannot create temporary subdirectory \"%s\": %m", @@ -1601,11 +1596,11 @@ OpenTemporaryFileInTablespace(Oid tblspcOid, bool rejectError) * We might need to create the tablespace's tempfile directory, if no * one has yet done so. * - * Don't check for error from mkdir; it could fail if someone else - * just did the same thing. If it doesn't work then we'll bomb out on + * Don't check error from MakePGDirectory; it could fail if someone + * else just did the same thing. If it doesn't work then we'll bomb out on * the second create attempt, instead. */ - mkdir(tempdirpath, S_IRWXU); + (void) MakePGDirectory(tempdirpath); file = PathNameOpenFile(tempfilepath, O_RDWR | O_CREAT | O_TRUNC | PG_BINARY); @@ -2401,7 +2396,7 @@ TryAgain: int OpenTransientFile(const char *fileName, int fileFlags) { - return OpenTransientFilePerm(fileName, fileFlags, PG_FILE_MODE_DEFAULT); + return OpenTransientFilePerm(fileName, fileFlags, pg_file_create_mode); } /* @@ -3554,3 +3549,16 @@ fsync_parent_path(const char *fname, int elevel) return 0; } + +/* + * Create a directory using pg_dir_create_mode for permissions + * + * Directories in PGDATA should normally have specific permissions -- when + * using this function the caller does not need to know what they should be. + * For permissions other than the default use mkdir() directly. + */ +int +MakePGDirectory(const char *directoryName) +{ + return mkdir(directoryName, pg_dir_create_mode); +} diff --git a/src/backend/storage/ipc/dsm_impl.c b/src/backend/storage/ipc/dsm_impl.c index 67e76b98fe..2fca9fae51 100644 --- a/src/backend/storage/ipc/dsm_impl.c +++ b/src/backend/storage/ipc/dsm_impl.c @@ -60,6 +60,7 @@ #ifdef HAVE_SYS_SHM_H #include #endif +#include "common/file_perm.h" #include "pgstat.h" #include "portability/mem.h" @@ -285,7 +286,7 @@ dsm_impl_posix(dsm_op op, dsm_handle handle, Size request_size, * returning. */ flags = O_RDWR | (op == DSM_OP_CREATE ? O_CREAT | O_EXCL : 0); - if ((fd = shm_open(name, flags, 0600)) == -1) + if ((fd = shm_open(name, flags, PG_FILE_MODE_OWNER)) == -1) { if (errno != EEXIST) ereport(elevel, diff --git a/src/backend/storage/ipc/ipc.c b/src/backend/storage/ipc/ipc.c index fc0a9c0756..6cf69fe55b 100644 --- a/src/backend/storage/ipc/ipc.c +++ b/src/backend/storage/ipc/ipc.c @@ -137,6 +137,10 @@ proc_exit(int code) else snprintf(gprofDirName, 32, "gprof/%d", (int) getpid()); + /* + * Use mkdir() instead of MakePGDirectory() since we aren't making a PG + * directory here. + */ mkdir("gprof", S_IRWXU | S_IRWXG | S_IRWXO); mkdir(gprofDirName, S_IRWXU | S_IRWXG | S_IRWXO); chdir(gprofDirName); diff --git a/src/backend/utils/init/miscinit.c b/src/backend/utils/init/miscinit.c index 87ed7d3f71..f8f08f3f88 100644 --- a/src/backend/utils/init/miscinit.c +++ b/src/backend/utils/init/miscinit.c @@ -32,6 +32,7 @@ #include "access/htup_details.h" #include "catalog/pg_authid.h" +#include "common/file_perm.h" #include "libpq/libpq.h" #include "mb/pg_wchar.h" #include "miscadmin.h" @@ -831,7 +832,7 @@ CreateLockFile(const char *filename, bool amPostmaster, * Think not to make the file protection weaker than 0600. See * comments below. */ - fd = open(filename, O_RDWR | O_CREAT | O_EXCL, 0600); + fd = open(filename, O_RDWR | O_CREAT | O_EXCL, pg_file_create_mode); if (fd >= 0) break; /* Success; exit the retry loop */ @@ -848,7 +849,7 @@ CreateLockFile(const char *filename, bool amPostmaster, * Read the file to get the old owner's PID. Note race condition * here: file might have been deleted since we tried to create it. */ - fd = open(filename, O_RDONLY, 0600); + fd = open(filename, O_RDONLY, pg_file_create_mode); if (fd < 0) { if (errno == ENOENT) diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c index 78990f5a27..3765548a24 100644 --- a/src/bin/initdb/initdb.c +++ b/src/bin/initdb/initdb.c @@ -64,6 +64,7 @@ #include "catalog/pg_authid.h" #include "catalog/pg_class.h" #include "catalog/pg_collation.h" +#include "common/file_perm.h" #include "common/file_utils.h" #include "common/restricted_token.h" #include "common/username.h" @@ -1170,7 +1171,7 @@ setup_config(void) snprintf(path, sizeof(path), "%s/postgresql.conf", pg_data); writefile(path, conflines); - if (chmod(path, S_IRUSR | S_IWUSR) != 0) + if (chmod(path, pg_file_create_mode) != 0) { fprintf(stderr, _("%s: could not change permissions of \"%s\": %s\n"), progname, path, strerror(errno)); @@ -1190,7 +1191,7 @@ setup_config(void) sprintf(path, "%s/postgresql.auto.conf", pg_data); writefile(path, autoconflines); - if (chmod(path, S_IRUSR | S_IWUSR) != 0) + if (chmod(path, pg_file_create_mode) != 0) { fprintf(stderr, _("%s: could not change permissions of \"%s\": %s\n"), progname, path, strerror(errno)); @@ -1277,7 +1278,7 @@ setup_config(void) snprintf(path, sizeof(path), "%s/pg_hba.conf", pg_data); writefile(path, conflines); - if (chmod(path, S_IRUSR | S_IWUSR) != 0) + if (chmod(path, pg_file_create_mode) != 0) { fprintf(stderr, _("%s: could not change permissions of \"%s\": %s\n"), progname, path, strerror(errno)); @@ -1293,7 +1294,7 @@ setup_config(void) snprintf(path, sizeof(path), "%s/pg_ident.conf", pg_data); writefile(path, conflines); - if (chmod(path, S_IRUSR | S_IWUSR) != 0) + if (chmod(path, pg_file_create_mode) != 0) { fprintf(stderr, _("%s: could not change permissions of \"%s\": %s\n"), progname, path, strerror(errno)); @@ -2692,7 +2693,7 @@ create_data_directory(void) pg_data); fflush(stdout); - if (pg_mkdir_p(pg_data, S_IRWXU) != 0) + if (pg_mkdir_p(pg_data, pg_dir_create_mode) != 0) { fprintf(stderr, _("%s: could not create directory \"%s\": %s\n"), progname, pg_data, strerror(errno)); @@ -2710,7 +2711,7 @@ create_data_directory(void) pg_data); fflush(stdout); - if (chmod(pg_data, S_IRWXU) != 0) + if (chmod(pg_data, pg_dir_create_mode) != 0) { fprintf(stderr, _("%s: could not change permissions of directory \"%s\": %s\n"), progname, pg_data, strerror(errno)); @@ -2778,7 +2779,7 @@ create_xlog_or_symlink(void) xlog_dir); fflush(stdout); - if (pg_mkdir_p(xlog_dir, S_IRWXU) != 0) + if (pg_mkdir_p(xlog_dir, pg_dir_create_mode) != 0) { fprintf(stderr, _("%s: could not create directory \"%s\": %s\n"), progname, xlog_dir, strerror(errno)); @@ -2796,7 +2797,7 @@ create_xlog_or_symlink(void) xlog_dir); fflush(stdout); - if (chmod(xlog_dir, S_IRWXU) != 0) + if (chmod(xlog_dir, pg_dir_create_mode) != 0) { fprintf(stderr, _("%s: could not change permissions of directory \"%s\": %s\n"), progname, xlog_dir, strerror(errno)); @@ -2846,7 +2847,7 @@ create_xlog_or_symlink(void) else { /* Without -X option, just make the subdirectory normally */ - if (mkdir(subdirloc, S_IRWXU) < 0) + if (mkdir(subdirloc, pg_dir_create_mode) < 0) { fprintf(stderr, _("%s: could not create directory \"%s\": %s\n"), progname, subdirloc, strerror(errno)); @@ -2882,7 +2883,8 @@ initialize_data_directory(void) setup_signals(); - umask(S_IRWXG | S_IRWXO); + /* Set dir/file mode mask */ + umask(PG_MODE_MASK_OWNER); create_data_directory(); @@ -2902,7 +2904,7 @@ initialize_data_directory(void) * The parent directory already exists, so we only need mkdir() not * pg_mkdir_p() here, which avoids some failure modes; cf bug #13853. */ - if (mkdir(path, S_IRWXU) < 0) + if (mkdir(path, pg_dir_create_mode) < 0) { fprintf(stderr, _("%s: could not create directory \"%s\": %s\n"), progname, path, strerror(errno)); diff --git a/src/bin/initdb/t/001_initdb.pl b/src/bin/initdb/t/001_initdb.pl index c0cfa6e92c..9dfb2ed96f 100644 --- a/src/bin/initdb/t/001_initdb.pl +++ b/src/bin/initdb/t/001_initdb.pl @@ -6,7 +6,7 @@ use strict; use warnings; use PostgresNode; use TestLib; -use Test::More tests => 15; +use Test::More tests => 16; my $tempdir = TestLib::tempdir; my $xlogdir = "$tempdir/pgxlog"; @@ -45,6 +45,15 @@ mkdir $datadir; command_ok([ 'initdb', '-N', '-T', 'german', '-X', $xlogdir, $datadir ], 'successful creation'); + + # Permissions on PGDATA should be default + SKIP: + { + skip "unix-style permissions not supported on Windows", 1 if ($windows_os); + + ok(check_mode_recursive($datadir, 0700, 0600), + "check PGDATA permissions"); + } } command_ok([ 'initdb', '-S', $datadir ], 'sync only'); command_fails([ 'initdb', $datadir ], 'existing data directory'); diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c index 8610fe8a1a..32b41e313c 100644 --- a/src/bin/pg_basebackup/pg_basebackup.c +++ b/src/bin/pg_basebackup/pg_basebackup.c @@ -27,6 +27,7 @@ #endif #include "access/xlog_internal.h" +#include "common/file_perm.h" #include "common/file_utils.h" #include "common/string.h" #include "fe_utils/string_utils.h" @@ -629,7 +630,7 @@ StartLogStreamer(char *startpos, uint32 timeline, char *sysidentifier) PQserverVersion(conn) < MINIMUM_VERSION_FOR_PG_WAL ? "pg_xlog" : "pg_wal"); - if (pg_mkdir_p(statusdir, S_IRWXU) != 0 && errno != EEXIST) + if (pg_mkdir_p(statusdir, pg_dir_create_mode) != 0 && errno != EEXIST) { fprintf(stderr, _("%s: could not create directory \"%s\": %s\n"), @@ -685,7 +686,7 @@ verify_dir_is_empty_or_create(char *dirname, bool *created, bool *found) /* * Does not exist, so create */ - if (pg_mkdir_p(dirname, S_IRWXU) == -1) + if (pg_mkdir_p(dirname, pg_dir_create_mode) == -1) { fprintf(stderr, _("%s: could not create directory \"%s\": %s\n"), @@ -1129,7 +1130,7 @@ ReceiveTarFile(PGconn *conn, PGresult *res, int rownum) tarCreateHeader(header, "recovery.conf", NULL, recoveryconfcontents->len, - 0600, 04000, 02000, + pg_file_create_mode, 04000, 02000, time(NULL)); padding = ((recoveryconfcontents->len + 511) & ~511) - recoveryconfcontents->len; @@ -1441,7 +1442,7 @@ ReceiveAndUnpackTarFile(PGconn *conn, PGresult *res, int rownum) * Directory */ filename[strlen(filename) - 1] = '\0'; /* Remove trailing slash */ - if (mkdir(filename, S_IRWXU) != 0) + if (mkdir(filename, pg_dir_create_mode) != 0) { /* * When streaming WAL, pg_wal (or pg_xlog for pre-9.6 diff --git a/src/bin/pg_basebackup/t/010_pg_basebackup.pl b/src/bin/pg_basebackup/t/010_pg_basebackup.pl index e2f1465472..ec54b555ed 100644 --- a/src/bin/pg_basebackup/t/010_pg_basebackup.pl +++ b/src/bin/pg_basebackup/t/010_pg_basebackup.pl @@ -5,7 +5,7 @@ use Config; use File::Basename qw(basename dirname); use PostgresNode; use TestLib; -use Test::More tests => 104; +use Test::More tests => 105; program_help_ok('pg_basebackup'); program_version_ok('pg_basebackup'); @@ -15,6 +15,9 @@ my $tempdir = TestLib::tempdir; my $node = get_new_node('main'); +# Set umask so test directories and files are created with default permissions +umask(0077); + # Initialize node without replication settings $node->init(extra => [ '--data-checksums' ]); $node->start; @@ -93,6 +96,15 @@ $node->command_ok([ 'pg_basebackup', '-D', "$tempdir/backup", '-X', 'none' ], 'pg_basebackup runs'); ok(-f "$tempdir/backup/PG_VERSION", 'backup was created'); +# Permissions on backup should be default +SKIP: +{ + skip "unix-style permissions not supported on Windows", 1 if ($windows_os); + + ok(check_mode_recursive("$tempdir/backup", 0700, 0600), + "check backup dir permissions"); +} + # Only archive_status directory should be copied in pg_wal/. is_deeply( [ sort(slurp_dir("$tempdir/backup/pg_wal/")) ], diff --git a/src/bin/pg_basebackup/t/020_pg_receivewal.pl b/src/bin/pg_basebackup/t/020_pg_receivewal.pl index 64e3a35a87..19c106d9f5 100644 --- a/src/bin/pg_basebackup/t/020_pg_receivewal.pl +++ b/src/bin/pg_basebackup/t/020_pg_receivewal.pl @@ -2,12 +2,15 @@ use strict; use warnings; use TestLib; use PostgresNode; -use Test::More tests => 18; +use Test::More tests => 19; program_help_ok('pg_receivewal'); program_version_ok('pg_receivewal'); program_options_handling_ok('pg_receivewal'); +# Set umask so test directories and files are created with default permissions +umask(0077); + my $primary = get_new_node('primary'); $primary->init(allows_streaming => 1); $primary->start; @@ -56,3 +59,12 @@ $primary->command_ok( [ 'pg_receivewal', '-D', $stream_dir, '--verbose', '--endpos', $nextlsn, '--synchronous', '--no-loop' ], 'streaming some WAL with --synchronous'); + +# Permissions on WAL files should be default +SKIP: +{ + skip "unix-style permissions not supported on Windows", 1 if ($windows_os); + + ok(check_mode_recursive($stream_dir, 0700, 0600), + "check stream dir permissions"); +} diff --git a/src/bin/pg_basebackup/walmethods.c b/src/bin/pg_basebackup/walmethods.c index b4558a0184..267a40debb 100644 --- a/src/bin/pg_basebackup/walmethods.c +++ b/src/bin/pg_basebackup/walmethods.c @@ -22,6 +22,7 @@ #endif #include "pgtar.h" +#include "common/file_perm.h" #include "common/file_utils.h" #include "receivelog.h" @@ -89,7 +90,7 @@ dir_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_ * does not do any system calls to fsync() to make changes permanent on * disk. */ - fd = open(tmppath, O_WRONLY | O_CREAT | PG_BINARY, S_IRUSR | S_IWUSR); + fd = open(tmppath, O_WRONLY | O_CREAT | PG_BINARY, pg_file_create_mode); if (fd < 0) return NULL; @@ -534,7 +535,8 @@ tar_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_ * We open the tar file only when we first try to write to it. */ tar_data->fd = open(tar_data->tarfilename, - O_WRONLY | O_CREAT | PG_BINARY, S_IRUSR | S_IWUSR); + O_WRONLY | O_CREAT | PG_BINARY, + pg_file_create_mode); if (tar_data->fd < 0) return NULL; diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c index 9bc830b085..5ede385e6a 100644 --- a/src/bin/pg_ctl/pg_ctl.c +++ b/src/bin/pg_ctl/pg_ctl.c @@ -25,6 +25,7 @@ #include "catalog/pg_control.h" #include "common/controldata_utils.h" +#include "common/file_perm.h" #include "getopt_long.h" #include "utils/pidfile.h" @@ -2170,7 +2171,8 @@ main(int argc, char **argv) */ argv0 = argv[0]; - umask(S_IRWXG | S_IRWXO); + /* Set dir/file mode mask */ + umask(PG_MODE_MASK_OWNER); /* support --help and --version even if invoked as root */ if (argc > 1) diff --git a/src/bin/pg_ctl/t/001_start_stop.pl b/src/bin/pg_ctl/t/001_start_stop.pl index 5da4746cb4..067a084c87 100644 --- a/src/bin/pg_ctl/t/001_start_stop.pl +++ b/src/bin/pg_ctl/t/001_start_stop.pl @@ -4,7 +4,7 @@ use warnings; use Config; use PostgresNode; use TestLib; -use Test::More tests => 19; +use Test::More tests => 21; my $tempdir = TestLib::tempdir; my $tempdir_short = TestLib::tempdir_short; @@ -57,9 +57,23 @@ command_ok([ 'pg_ctl', 'stop', '-D', "$tempdir/data" ], 'pg_ctl stop'); command_fails([ 'pg_ctl', 'stop', '-D', "$tempdir/data" ], 'second pg_ctl stop fails'); +# Log file for default permission test. The permissions won't be checked on +# Windows but we still want to do the restart test. +my $logFileName = "$tempdir/data/perm-test-600.log"; + command_ok( - [ 'pg_ctl', 'restart', '-D', "$tempdir/data" ], + [ 'pg_ctl', 'restart', '-D', "$tempdir/data", '-l', $logFileName ], 'pg_ctl restart with server not running'); + +# Permissions on log file should be default +SKIP: +{ + skip "unix-style permissions not supported on Windows", 2 if ($windows_os); + + ok(-f $logFileName); + ok(check_mode_recursive("$tempdir/data", 0700, 0600)); +} + command_ok([ 'pg_ctl', 'restart', '-D', "$tempdir/data" ], 'pg_ctl restart with server running'); diff --git a/src/bin/pg_resetwal/pg_resetwal.c b/src/bin/pg_resetwal/pg_resetwal.c index eba7c5fdee..bdf71886ee 100644 --- a/src/bin/pg_resetwal/pg_resetwal.c +++ b/src/bin/pg_resetwal/pg_resetwal.c @@ -52,6 +52,7 @@ #include "catalog/catversion.h" #include "catalog/pg_control.h" #include "common/fe_memutils.h" +#include "common/file_perm.h" #include "common/restricted_token.h" #include "storage/large_object.h" #include "pg_getopt.h" @@ -967,7 +968,7 @@ RewriteControlFile(void) fd = open(XLOG_CONTROL_FILE, O_RDWR | O_CREAT | O_EXCL | PG_BINARY, - S_IRUSR | S_IWUSR); + pg_file_create_mode); if (fd < 0) { fprintf(stderr, _("%s: could not create pg_control file: %s\n"), @@ -1249,7 +1250,7 @@ WriteEmptyXLOG(void) unlink(path); fd = open(path, O_RDWR | O_CREAT | O_EXCL | PG_BINARY, - S_IRUSR | S_IWUSR); + pg_file_create_mode); if (fd < 0) { fprintf(stderr, _("%s: could not open file \"%s\": %s\n"), diff --git a/src/bin/pg_resetwal/t/001_basic.pl b/src/bin/pg_resetwal/t/001_basic.pl index 1b157cb555..0d6ab20073 100644 --- a/src/bin/pg_resetwal/t/001_basic.pl +++ b/src/bin/pg_resetwal/t/001_basic.pl @@ -3,7 +3,7 @@ use warnings; use PostgresNode; use TestLib; -use Test::More tests => 11; +use Test::More tests => 12; program_help_ok('pg_resetwal'); program_version_ok('pg_resetwal'); @@ -15,3 +15,13 @@ $node->init; command_like([ 'pg_resetwal', '-n', $node->data_dir ], qr/checkpoint/, 'pg_resetwal -n produces output'); + + +# Permissions on PGDATA should be default +SKIP: +{ + skip "unix-style permissions not supported on Windows", 1 if ($windows_os); + + ok(check_mode_recursive($node->data_dir, 0700, 0600), + 'check PGDATA permissions'); +} diff --git a/src/bin/pg_rewind/RewindTest.pm b/src/bin/pg_rewind/RewindTest.pm index 00b5b42dd7..7b632c7dcd 100644 --- a/src/bin/pg_rewind/RewindTest.pm +++ b/src/bin/pg_rewind/RewindTest.pm @@ -237,6 +237,10 @@ sub run_pg_rewind "$tmp_folder/master-postgresql.conf.tmp", "$master_pgdata/postgresql.conf"); + chmod(0600, "$master_pgdata/postgresql.conf") + or BAIL_OUT( + "unable to set permissions for $master_pgdata/postgresql.conf"); + # Plug-in rewound node to the now-promoted standby node my $port_standby = $node_standby->port; $node_master->append_conf( diff --git a/src/bin/pg_rewind/file_ops.c b/src/bin/pg_rewind/file_ops.c index f491ed7f5c..94bcc13ae8 100644 --- a/src/bin/pg_rewind/file_ops.c +++ b/src/bin/pg_rewind/file_ops.c @@ -18,6 +18,7 @@ #include #include +#include "common/file_perm.h" #include "file_ops.h" #include "filemap.h" #include "logging.h" @@ -57,7 +58,7 @@ open_target_file(const char *path, bool trunc) mode = O_WRONLY | O_CREAT | PG_BINARY; if (trunc) mode |= O_TRUNC; - dstfd = open(dstpath, mode, 0600); + dstfd = open(dstpath, mode, pg_file_create_mode); if (dstfd < 0) pg_fatal("could not open target file \"%s\": %s\n", dstpath, strerror(errno)); @@ -198,7 +199,7 @@ truncate_target_file(const char *path, off_t newsize) snprintf(dstpath, sizeof(dstpath), "%s/%s", datadir_target, path); - fd = open(dstpath, O_WRONLY, 0); + fd = open(dstpath, O_WRONLY, pg_file_create_mode); if (fd < 0) pg_fatal("could not open file \"%s\" for truncation: %s\n", dstpath, strerror(errno)); @@ -219,7 +220,7 @@ create_target_dir(const char *path) return; snprintf(dstpath, sizeof(dstpath), "%s/%s", datadir_target, path); - if (mkdir(dstpath, S_IRWXU) != 0) + if (mkdir(dstpath, pg_dir_create_mode) != 0) pg_fatal("could not create directory \"%s\": %s\n", dstpath, strerror(errno)); } diff --git a/src/bin/pg_rewind/t/001_basic.pl b/src/bin/pg_rewind/t/001_basic.pl index 736f34eae3..1b0f823b0c 100644 --- a/src/bin/pg_rewind/t/001_basic.pl +++ b/src/bin/pg_rewind/t/001_basic.pl @@ -1,7 +1,7 @@ use strict; use warnings; use TestLib; -use Test::More tests => 8; +use Test::More tests => 10; use RewindTest; @@ -86,6 +86,15 @@ in master, before promotion ), 'tail-copy'); + # Permissions on PGDATA should be default + SKIP: + { + skip "unix-style permissions not supported on Windows", 1 if ($windows_os); + + ok(check_mode_recursive($node_master->data_dir(), 0700, 0600), + 'check PGDATA permissions'); + } + RewindTest::clean_rewind_test(); } diff --git a/src/bin/pg_upgrade/file.c b/src/bin/pg_upgrade/file.c index f38bfacf02..f68211aa20 100644 --- a/src/bin/pg_upgrade/file.c +++ b/src/bin/pg_upgrade/file.c @@ -10,6 +10,7 @@ #include "postgres_fe.h" #include "access/visibilitymap.h" +#include "common/file_perm.h" #include "pg_upgrade.h" #include "storage/bufpage.h" #include "storage/checksum.h" @@ -44,7 +45,7 @@ copyFile(const char *src, const char *dst, schemaName, relName, src, strerror(errno)); if ((dest_fd = open(dst, O_RDWR | O_CREAT | O_EXCL | PG_BINARY, - S_IRUSR | S_IWUSR)) < 0) + pg_file_create_mode)) < 0) pg_fatal("error while copying relation \"%s.%s\": could not create file \"%s\": %s\n", schemaName, relName, dst, strerror(errno)); @@ -151,7 +152,7 @@ rewriteVisibilityMap(const char *fromfile, const char *tofile, schemaName, relName, fromfile, strerror(errno)); if ((dst_fd = open(tofile, O_RDWR | O_CREAT | O_EXCL | PG_BINARY, - S_IRUSR | S_IWUSR)) < 0) + pg_file_create_mode)) < 0) pg_fatal("error while copying relation \"%s.%s\": could not create file \"%s\": %s\n", schemaName, relName, tofile, strerror(errno)); diff --git a/src/bin/pg_upgrade/pg_upgrade.c b/src/bin/pg_upgrade/pg_upgrade.c index d12412799f..1d35188143 100644 --- a/src/bin/pg_upgrade/pg_upgrade.c +++ b/src/bin/pg_upgrade/pg_upgrade.c @@ -38,6 +38,7 @@ #include "pg_upgrade.h" #include "catalog/pg_class.h" +#include "common/file_perm.h" #include "common/restricted_token.h" #include "fe_utils/string_utils.h" @@ -79,7 +80,7 @@ main(int argc, char **argv) set_pglocale_pgservice(argv[0], PG_TEXTDOMAIN("pg_upgrade")); /* Ensure that all files created by pg_upgrade are non-world-readable */ - umask(S_IRWXG | S_IRWXO); + umask(PG_MODE_MASK_OWNER); parseCommandLine(argc, argv); diff --git a/src/bin/pg_upgrade/test.sh b/src/bin/pg_upgrade/test.sh index 39983abea1..574639d47e 100644 --- a/src/bin/pg_upgrade/test.sh +++ b/src/bin/pg_upgrade/test.sh @@ -230,6 +230,17 @@ standard_initdb 'initdb' pg_upgrade $PG_UPGRADE_OPTS -d "${PGDATA}.old" -D "${PGDATA}" -b "$oldbindir" -B "$bindir" -p "$PGPORT" -P "$PGPORT" +# make sure all directories and files have correct permissions +if [ $(find ${PGDATA} -type f ! -perm 600 | wc -l) -ne 0 ]; then + echo "files in PGDATA with permission != 600"; + exit 1; +fi + +if [ $(find ${PGDATA} -type d ! -perm 700 | wc -l) -ne 0 ]; then + echo "directories in PGDATA with permission != 700"; + exit 1; +fi + pg_ctl start -l "$logdir/postmaster2.log" -o "$POSTMASTER_OPTS" -w case $testhost in diff --git a/src/common/Makefile b/src/common/Makefile index 873fbb6438..e9e75867f3 100644 --- a/src/common/Makefile +++ b/src/common/Makefile @@ -40,8 +40,8 @@ override CPPFLAGS += -DVAL_LIBS="\"$(LIBS)\"" override CPPFLAGS := -DFRONTEND $(CPPFLAGS) LIBS += $(PTHREAD_LIBS) -OBJS_COMMON = base64.o config_info.o controldata_utils.o exec.o ip.o \ - keywords.o md5.o pg_lzcompress.o pgfnames.o psprintf.o relpath.o \ +OBJS_COMMON = base64.o config_info.o controldata_utils.o exec.o file_perm.o \ + ip.o keywords.o md5.o pg_lzcompress.o pgfnames.o psprintf.o relpath.o \ rmtree.o saslprep.o scram-common.o string.o unicode_norm.o \ username.o wait_error.o diff --git a/src/common/file_perm.c b/src/common/file_perm.c new file mode 100644 index 0000000000..fdfbb9a44c --- /dev/null +++ b/src/common/file_perm.c @@ -0,0 +1,19 @@ +/*------------------------------------------------------------------------- + * + * File and directory permission routines + * + * + * Portions Copyright (c) 1996-2018, PostgreSQL Global Development Group + * Portions Copyright (c) 1994, Regents of the University of California + * + * src/include/common/file_perm.c + * + *------------------------------------------------------------------------- + */ +#include + +#include "common/file_perm.h" + +/* Modes for creating directories and files in the data directory */ +int pg_dir_create_mode = PG_DIR_MODE_OWNER; +int pg_file_create_mode = PG_FILE_MODE_OWNER; diff --git a/src/include/common/file_perm.h b/src/include/common/file_perm.h new file mode 100644 index 0000000000..700f562582 --- /dev/null +++ b/src/include/common/file_perm.h @@ -0,0 +1,32 @@ +/*------------------------------------------------------------------------- + * + * File and directory permission constants + * + * + * Portions Copyright (c) 1996-2018, PostgreSQL Global Development Group + * Portions Copyright (c) 1994, Regents of the University of California + * + * src/include/common/file_perm.h + * + *------------------------------------------------------------------------- + */ +#ifndef FILE_PERM_H +#define FILE_PERM_H + +/* + * Default mode mask for data directory permissions only allows the owner to + * read/write directories and files. + */ +#define PG_MODE_MASK_OWNER (S_IRWXG | S_IRWXO) + +/* Default mode for creating directories */ +#define PG_DIR_MODE_OWNER S_IRWXU + +/* Default mode for creating files */ +#define PG_FILE_MODE_OWNER (S_IRUSR | S_IWUSR) + +/* Modes for creating directories and files in the data directory */ +extern int pg_dir_create_mode; +extern int pg_file_create_mode; + +#endif /* FILE_PERM_H */ diff --git a/src/include/storage/fd.h b/src/include/storage/fd.h index e49b42ce86..785d9058e9 100644 --- a/src/include/storage/fd.h +++ b/src/include/storage/fd.h @@ -112,6 +112,9 @@ extern int CloseTransientFile(int fd); extern int BasicOpenFile(const char *fileName, int fileFlags); extern int BasicOpenFilePerm(const char *fileName, int fileFlags, mode_t fileMode); + /* Make a directory with default permissions */ +extern int MakePGDirectory(const char *directoryName); + /* Miscellaneous support routines */ extern void InitFileAccess(void); extern void set_max_safe_fds(void); diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm index 80188315f1..76e571b98c 100644 --- a/src/test/perl/PostgresNode.pm +++ b/src/test/perl/PostgresNode.pm @@ -484,6 +484,9 @@ sub append_conf my $conffile = $self->data_dir . '/' . $filename; TestLib::append_to_file($conffile, $str . "\n"); + + chmod(0600, $conffile) + or die("unable to set permissions for $conffile"); } =pod diff --git a/src/test/perl/TestLib.pm b/src/test/perl/TestLib.pm index b6862688d4..93610e4bc4 100644 --- a/src/test/perl/TestLib.pm +++ b/src/test/perl/TestLib.pm @@ -13,8 +13,11 @@ use warnings; use Config; use Cwd; use Exporter 'import'; +use Fcntl qw(:mode); use File::Basename; +use File::Find; use File::Spec; +use File::stat qw(stat); use File::Temp (); use IPC::Run; use SimpleTee; @@ -27,6 +30,7 @@ our @EXPORT = qw( slurp_dir slurp_file append_to_file + check_mode_recursive check_pg_config system_or_bail system_log @@ -240,6 +244,75 @@ sub append_to_file close $fh; } +# Check that all file/dir modes in a directory match the expected values, +# ignoring the mode of any specified files. +sub check_mode_recursive +{ + my ($dir, $expected_dir_mode, $expected_file_mode, $ignore_list) = @_; + + # Result defaults to true + my $result = 1; + + find + ( + {follow_fast => 1, + wanted => + sub + { + my $file_stat = stat($File::Find::name); + + # Is file in the ignore list? + foreach my $ignore ($ignore_list ? @{$ignore_list} : []) + { + if ("$dir/$ignore" eq $File::Find::name) + { + return; + } + } + + defined($file_stat) + or die("unable to stat $File::Find::name"); + + my $file_mode = S_IMODE($file_stat->mode); + + # Is this a file? + if (S_ISREG($file_stat->mode)) + { + if ($file_mode != $expected_file_mode) + { + print(*STDERR, + sprintf("$File::Find::name mode must be %04o\n", + $expected_file_mode)); + + $result = 0; + return; + } + } + # Else a directory? + elsif (S_ISDIR($file_stat->mode)) + { + if ($file_mode != $expected_dir_mode) + { + print(*STDERR, + sprintf("$File::Find::name mode must be %04o\n", + $expected_dir_mode)); + + $result = 0; + return; + } + } + # Else something we can't handle + else + { + die "unknown file type for $File::Find::name"; + } + }}, + $dir + ); + + return $result; +} + # Check presence of a given regexp within pg_config.h for the installation # where tests are running, returning a match status result depending on # that. diff --git a/src/tools/msvc/Mkvcbuild.pm b/src/tools/msvc/Mkvcbuild.pm index 41d720880a..1d3ed6b0b1 100644 --- a/src/tools/msvc/Mkvcbuild.pm +++ b/src/tools/msvc/Mkvcbuild.pm @@ -111,8 +111,8 @@ sub mkvcbuild } our @pgcommonallfiles = qw( - base64.c config_info.c controldata_utils.c exec.c ip.c keywords.c - md5.c pg_lzcompress.c pgfnames.c psprintf.c relpath.c rmtree.c + base64.c config_info.c controldata_utils.c exec.c file_perm.c ip.c + keywords.c md5.c pg_lzcompress.c pgfnames.c psprintf.c relpath.c rmtree.c saslprep.c scram-common.c string.c unicode_norm.c username.c wait_error.c); -- 2.14.3 (Apple Git-98)