From 15f1173b1d2cf0d9bbc1da35ac01b6c49507fc83 Mon Sep 17 00:00:00 2001 From: Jelte Fennema Date: Mon, 17 Feb 2020 12:58:40 +0100 Subject: [PATCH] Semmle: Ensure permissions of private keys are 0600 (#3506) When using --allow-group-access option from initdb our keys and certificates would be created with 0640 permissions. Which is a pretty serious security issue: This changes that. This would not be exploitable though, since postgres would not actually enable SSL and would output the following message in the logs: ``` DETAIL: File must have permissions u=rw (0600) or less if owned by the database user, or permissions u=rw,g=r (0640) or less if owned by root. ``` Since citus still expected the cluster to have SSL enabled handshakes between workers and coordinator would fail. So instead of a security issue the cluster would simply be unusable. --- src/backend/distributed/utils/enable_ssl.c | 18 ++++++++++++++++-- src/test/regress/pg_regress_multi.pl | 6 ++++-- 2 files changed, 20 insertions(+), 4 deletions(-) diff --git a/src/backend/distributed/utils/enable_ssl.c b/src/backend/distributed/utils/enable_ssl.c index feff78a5c..8f4dcf7a4 100644 --- a/src/backend/distributed/utils/enable_ssl.c +++ b/src/backend/distributed/utils/enable_ssl.c @@ -392,7 +392,14 @@ StoreCertificate(EVP_PKEY *privateKey, X509 *certificate) /* Open the private key file and write the private key in PEM format to it */ - FILE *privateKeyFile = fopen(privateKeyFilename, "wb"); + int privateKeyFileDescriptor = open(privateKeyFilename, O_WRONLY | O_CREAT, 0600); + if (privateKeyFileDescriptor == -1) + { + ereport(ERROR, (errmsg("unable to open private key file '%s' for writing", + privateKeyFilename))); + } + + FILE *privateKeyFile = fdopen(privateKeyFileDescriptor, "wb"); if (!privateKeyFile) { ereport(ERROR, (errmsg("unable to open private key file '%s' for writing", @@ -407,8 +414,15 @@ StoreCertificate(EVP_PKEY *privateKey, X509 *certificate) ereport(ERROR, (errmsg("unable to store private key"))); } + int certificateFileDescriptor = open(certificateFilename, O_WRONLY | O_CREAT, 0600); + if (certificateFileDescriptor == -1) + { + ereport(ERROR, (errmsg("unable to open private key file '%s' for writing", + privateKeyFilename))); + } + /* Open the certificate file and write the certificate in the PEM format to it */ - FILE *certificateFile = fopen(certificateFilename, "wb"); + FILE *certificateFile = fdopen(certificateFileDescriptor, "wb"); if (!certificateFile) { ereport(ERROR, (errmsg("unable to open certificate file '%s' for writing", diff --git a/src/test/regress/pg_regress_multi.pl b/src/test/regress/pg_regress_multi.pl index 0530548e5..5febeca56 100755 --- a/src/test/regress/pg_regress_multi.pl +++ b/src/test/regress/pg_regress_multi.pl @@ -506,14 +506,16 @@ if ($followercluster) } # Create new data directories, copy workers for speed -system(catfile("$bindir", "initdb"), ("--nosync", "-U", $user, "--encoding", "UTF8", catfile($TMP_CHECKDIR, $MASTERDIR, "data"))) == 0 +# --allow-group-access is used to ensure we set permissions on private keys +# correctly +system(catfile("$bindir", "initdb"), ("--nosync", "--allow-group-access", "-U", $user, "--encoding", "UTF8", catfile($TMP_CHECKDIR, $MASTERDIR, "data"))) == 0 or die "Could not create $MASTERDIR data directory"; if ($usingWindows) { for my $port (@workerPorts) { - system(catfile("$bindir", "initdb"), ("--nosync", "-U", $user, "--encoding", "UTF8", catfile($TMP_CHECKDIR, "worker.$port", "data"))) == 0 + system(catfile("$bindir", "initdb"), ("--nosync", "--allow-group-access", "-U", $user, "--encoding", "UTF8", catfile($TMP_CHECKDIR, "worker.$port", "data"))) == 0 or die "Could not create worker data directory"; } }