Enhance security permissions
Hi.
I noticed this while checking the source
(src/interfaces/libpq/fe-connect.c).
It seems that S_IRWXU permission is harmful too.
In accord with [1]https://docs.aws.amazon.com/codeguru/detector-library/cpp/loose-file-permissions/ and [2]https://www.exploit-db.com/exploits/33145 this should also be checked.
Also, all other places in the source, S_IRWXU are checked.
So, I propose adding this check to enhance the security.
Maybe the error messages, do they need improvement as well?
patchs attached.
best regards,
Ranier Vilela
[1]: https://docs.aws.amazon.com/codeguru/detector-library/cpp/loose-file-permissions/
https://docs.aws.amazon.com/codeguru/detector-library/cpp/loose-file-permissions/
[2]: https://www.exploit-db.com/exploits/33145
Attachments:
enhance-security-file-permissions-be-secure-common.patchapplication/octet-stream; name=enhance-security-file-permissions-be-secure-common.patchDownload
diff --git a/src/backend/libpq/be-secure-common.c b/src/backend/libpq/be-secure-common.c
index e8b837d1fa..fed399ad03 100644
--- a/src/backend/libpq/be-secure-common.c
+++ b/src/backend/libpq/be-secure-common.c
@@ -161,7 +161,7 @@ check_ssl_key_file_permissions(const char *ssl_key_file, bool isServerStart)
return false;
}
- if ((buf.st_uid == geteuid() && buf.st_mode & (S_IRWXG | S_IRWXO)) ||
+ if ((buf.st_uid == geteuid() && buf.st_mode & (S_IRWXU | S_IRWXG | S_IRWXO)) ||
(buf.st_uid == 0 && buf.st_mode & (S_IWGRP | S_IXGRP | S_IRWXO)))
{
ereport(loglevel,
enhance-security-file-permissions-fe-connect.patchapplication/octet-stream; name=enhance-security-file-permissions-fe-connect.patchDownload
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 19bbc00666..0d0ebf3631 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -8014,7 +8014,7 @@ passwordFromFile(const char *hostname, const char *port,
}
/* If password file is insecure, alert the user and ignore it. */
- if (stat_buf.st_mode & (S_IRWXG | S_IRWXO))
+ if (stat_buf.st_mode & (S_IRWXU | S_IRWXG | S_IRWXO))
{
fprintf(stderr,
libpq_gettext("WARNING: password file \"%s\" has group or world access; permissions should be u=rw (0600) or less\n"),
enhance-security-file-permissions-fe-secure-openssl.patchapplication/octet-stream; name=enhance-security-file-permissions-fe-secure-openssl.patchDownload
diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c
index 51dd7b9fec..c8d99a3c3f 100644
--- a/src/interfaces/libpq/fe-secure-openssl.c
+++ b/src/interfaces/libpq/fe-secure-openssl.c
@@ -1264,7 +1264,7 @@ initialize_SSL(PGconn *conn)
#if !defined(WIN32) && !defined(__CYGWIN__)
if (buf.st_uid == 0 ?
buf.st_mode & (S_IWGRP | S_IXGRP | S_IRWXO) :
- buf.st_mode & (S_IRWXG | S_IRWXO))
+ buf.st_mode & (S_IRWXU | S_IRWXG | S_IRWXO))
{
libpq_append_conn_error(conn,
"private key file \"%s\" has group or world access; file must have permissions u=rw (0600) or less if owned by the current user, or permissions u=rw,g=r (0640) or less if owned by root",
enhance-security-file-permissions-pg_backup_tar.patchapplication/octet-stream; name=enhance-security-file-permissions-pg_backup_tar.patchDownload
diff --git a/src/bin/pg_dump/pg_backup_tar.c b/src/bin/pg_dump/pg_backup_tar.c
index b5ba3b46dd..73ac5b82b7 100644
--- a/src/bin/pg_dump/pg_backup_tar.c
+++ b/src/bin/pg_dump/pg_backup_tar.c
@@ -341,7 +341,7 @@ tarOpen(ArchiveHandle *AH, const char *filename, char mode)
* might retain the data but forget tmpfile()'s unlink(). If so, the
* file mode protects confidentiality of the data written.
*/
- old_umask = umask(S_IRWXG | S_IRWXO);
+ old_umask = umask(S_IRWXU | S_IRWXG | S_IRWXO);
#ifndef WIN32
tm->tmpFH = tmpfile();
On 11/4/2025 6:20 AM, Ranier Vilela wrote:
Hi.
I noticed this while checking the source (src/interfaces/libpq/fe-
connect.c).
It seems that S_IRWXU permission is harmful too.In accord with [1] and [2] this should also be checked.
Also, all other places in the source, S_IRWXU are checked.So, I propose adding this check to enhance the security.
Maybe the error messages, do they need improvement as well?
patchs attached.
best regards,
Ranier Vilela[1] https://docs.aws.amazon.com/codeguru/detector-library/cpp/loose-
file-permissions/ <https://docs.aws.amazon.com/codeguru/detector-
library/cpp/loose-file-permissions/>
[2] https://www.exploit-db.com/exploits/33145 <https://www.exploit-
db.com/exploits/33145>
I just took a glance an you
enhance-security-file-permissions-be-secure-common.patch file...
I may be misunderstanding either your intent or what this code actually
does, but it seems to me that the check rejects files if any of the
tested bits are set. Doesn't adding S_IRWXU means rejecting files with
any owner permissions, including S_IRUSR (owner read). That would reject
mode 0600, which is the documented and required permission for SSL key
files.
Mode 0000 would be the only thing that passes this check and we can't
read that.
I believe your [1] reference is about overly permissive roles in
creating files. We are validating existing ones.
Please correct my understanding as needed.
Thanks,
Bryan
Hi.
Em ter., 4 de nov. de 2025 às 09:44, Bryan Green <dbryan.green@gmail.com>
escreveu:
On 11/4/2025 6:20 AM, Ranier Vilela wrote:
Hi.
I noticed this while checking the source (src/interfaces/libpq/fe-
connect.c).
It seems that S_IRWXU permission is harmful too.In accord with [1] and [2] this should also be checked.
Also, all other places in the source, S_IRWXU are checked.So, I propose adding this check to enhance the security.
Maybe the error messages, do they need improvement as well?
patchs attached.
best regards,
Ranier Vilela[1] https://docs.aws.amazon.com/codeguru/detector-library/cpp/loose-
file-permissions/ <https://docs.aws.amazon.com/codeguru/detector-
library/cpp/loose-file-permissions/>
[2] https://www.exploit-db.com/exploits/33145 <https://www.exploit-
db.com/exploits/33145>I just took a glance an you
enhance-security-file-permissions-be-secure-common.patch file...I may be misunderstanding either your intent or what this code actually
does, but it seems to me that the check rejects files if any of the
tested bits are set.
Correct.
Doesn't adding S_IRWXU means rejecting files with
any owner permissions, including S_IRUSR (owner read).
S_IRWXU on stat is "Mask for file owner permissions".
That would reject
mode 0600, which is the documented and required permission for SSL key
files.
I think no.
Mode 0000 would be the only thing that passes this check and we can't
read that.I believe your [1] reference is about overly permissive roles in
creating files. We are validating existing ones.
Sorry, I think that [1] has wrong examples of this.
[2]: has a more correct example.
We are validating files existing, created by others.
S_IRWXU file mode indicating readable, writable and executable by owner.
I think if the file is executable by the owner, He should be rejected,
shouldn't he?
best regards,
Ranier Vilela
On 11/4/2025 7:17 AM, Ranier Vilela wrote:
Hi.
Em ter., 4 de nov. de 2025 às 09:44, Bryan Green <dbryan.green@gmail.com
<mailto:dbryan.green@gmail.com>> escreveu:On 11/4/2025 6:20 AM, Ranier Vilela wrote:
Hi.
I noticed this while checking the source (src/interfaces/libpq/fe-
connect.c).
It seems that S_IRWXU permission is harmful too.In accord with [1] and [2] this should also be checked.
Also, all other places in the source, S_IRWXU are checked.So, I propose adding this check to enhance the security.
Maybe the error messages, do they need improvement as well?
patchs attached.
best regards,
Ranier Vilela[1] https://docs.aws.amazon.com/codeguru/detector-library/cpp/
loose- <https://docs.aws.amazon.com/codeguru/detector-library/cpp/
loose->file-permissions/ <https://docs.aws.amazon.com/codeguru/detector-
<https://docs.aws.amazon.com/codeguru/detector->
library/cpp/loose-file-permissions/>
[2] https://www.exploit-db.com/exploits/33145 <https://www.exploit-db.com/exploits/33145> <https://www.exploit- <https://
www.exploit->db.com/exploits/33145 <http://db.com/exploits/33145>>
I just took a glance an you
enhance-security-file-permissions-be-secure-common.patch file...I may be misunderstanding either your intent or what this code actually
does, but it seems to me that the check rejects files if any of the
tested bits are set.Correct.
Doesn't adding S_IRWXU means rejecting files with
any owner permissions, including S_IRUSR (owner read).S_IRWXU on stat is "Mask for file owner permissions".
That would reject
mode 0600, which is the documented and required permission for SSL key
files.I think no.
(S_IRWXU | S_IRWXG | S_IRWXO) produces a mask of O777, because
S_IRWXU = 0700 (user read, write, execute)
S_IRWXG = 0070 (group read, write, execute)
S_IRWXO = 0007 (other read, write, execute)
mode 0600 & 0777 = 0600 (non-zero)
mode 0400 & 0777 = 0400 (non-zero)
mode 0700 & 0777 = 0700 (non-zero)
mode 0000 & 0777 = 0000 (zero)
The test was originally constructed to reject group and other
permissions being set-- allowing any user(file-owner) permissions to be
accepted. You are now rejecting everything but 0000.
Mode 0000 would be the only thing that passes this check and we can't
read that.I believe your [1] reference is about overly permissive roles in
creating files. We are validating existing ones.Sorry, I think that [1] has wrong examples of this.
[2] has a more correct example.
We are validating files existing, created by others.
S_IRWXU file mode indicating readable, writable and executable by owner.I think if the file is executable by the owner, He should be rejected,
shouldn't he?best regards,
Ranier Vilela
If your goal is to reject ONLY executable files, you need to check
S_IXUSR.
However, I question whether even this change is necessary. Private key
files having the execute bit set is not a security vulnerability - the
file cannot actually be executed as a program. The real security concern
is group/world access, which the current code already checks correctly.
BG