Exposure related to GUC value of ssl_passphrase_command
Deal Hackers.
The value of ssl_passphrase_command is set so that an external command
is called when the passphrase for decrypting an SSL file such as a
private key is obtained.
Therefore, easily set to work with echo "passphrase" or call to
another get of passphrase application.
I think that this GUC value doesn't contain very sensitive data,
but just in case, it's dangerous to be visible to all users.
I think do not possible these cases, but if a used echo external
commands or another external command, know what application used to
get the password, maybe we can't be convinced that there's the safety
of using abuse by backtracking on applications.
So I think to the need only superusers or users with the default role
of pg_read_all_settings should see these values.
Patch is very simple.
How do you think about my thoughts like this?
Best regards.
Moon.
Attachments:
Change-show-authority-of-ssl_passphrase_command.diffapplication/octet-stream; name=Change-show-authority-of-ssl_passphrase_command.diffDownload
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 31a5ef0474..174953cc41 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -4148,7 +4148,8 @@ static struct config_string ConfigureNamesString[] =
{
{"ssl_passphrase_command", PGC_SIGHUP, CONN_AUTH_SSL,
gettext_noop("Command to obtain passphrases for SSL."),
- NULL
+ NULL,
+ GUC_SUPERUSER_ONLY
},
&ssl_passphrase_command,
"",
Hello.
On Tue, Nov 5, 2019 at 5:15 PM Moon, Insung <tsukiwamoon.pgsql@gmail.com> wrote:
Deal Hackers.
The value of ssl_passphrase_command is set so that an external command
is called when the passphrase for decrypting an SSL file such as a
private key is obtained.
Therefore, easily set to work with echo "passphrase" or call to
another get of passphrase application.I think that this GUC value doesn't contain very sensitive data,
but just in case, it's dangerous to be visible to all users.
I think do not possible these cases, but if a used echo external
commands or another external command, know what application used to
get the password, maybe we can't be convinced that there's the safety
of using abuse by backtracking on applications.
So I think to the need only superusers or users with the default role
of pg_read_all_settings should see these values.Patch is very simple.
How do you think about my thoughts like this?
I'm hardly an expert on this topic, but reading this blog post about
ssl_passphrase_command:
https://www.2ndquadrant.com/en/blog/postgresql-passphrase-protected-ssl-keys-systemd/
which mentions that some users might go with the very naive
configuration such as:
ssl_passphrase_command = 'echo "secret"'
maybe it makes sense to protect its value from everyone but superusers.
So +1.
Thanks,
Amit
On Fri, Nov 8, 2019 at 4:24 PM Amit Langote <amitlangote09@gmail.com> wrote:
Hello.
On Tue, Nov 5, 2019 at 5:15 PM Moon, Insung <tsukiwamoon.pgsql@gmail.com> wrote:
Deal Hackers.
The value of ssl_passphrase_command is set so that an external command
is called when the passphrase for decrypting an SSL file such as a
private key is obtained.
Therefore, easily set to work with echo "passphrase" or call to
another get of passphrase application.I think that this GUC value doesn't contain very sensitive data,
but just in case, it's dangerous to be visible to all users.
I think do not possible these cases, but if a used echo external
commands or another external command, know what application used to
get the password, maybe we can't be convinced that there's the safety
of using abuse by backtracking on applications.
So I think to the need only superusers or users with the default role
of pg_read_all_settings should see these values.Patch is very simple.
How do you think about my thoughts like this?I'm hardly an expert on this topic, but reading this blog post about
ssl_passphrase_command:https://www.2ndquadrant.com/en/blog/postgresql-passphrase-protected-ssl-keys-systemd/
which mentions that some users might go with the very naive
configuration such as:ssl_passphrase_command = 'echo "secret"'
maybe it makes sense to protect its value from everyone but superusers.
So +1.
Seems this proposal is reasonable.
Regards,
--
Fujii Masao
At Thu, 13 Feb 2020 02:37:29 +0900, Fujii Masao <masao.fujii@gmail.com> wrote in
On Fri, Nov 8, 2019 at 4:24 PM Amit Langote <amitlangote09@gmail.com> wrote:
Hello.
On Tue, Nov 5, 2019 at 5:15 PM Moon, Insung <tsukiwamoon.pgsql@gmail.com> wrote:
Deal Hackers.
The value of ssl_passphrase_command is set so that an external command
is called when the passphrase for decrypting an SSL file such as a
private key is obtained.
Therefore, easily set to work with echo "passphrase" or call to
another get of passphrase application.I think that this GUC value doesn't contain very sensitive data,
but just in case, it's dangerous to be visible to all users.
I think do not possible these cases, but if a used echo external
commands or another external command, know what application used to
get the password, maybe we can't be convinced that there's the safety
of using abuse by backtracking on applications.
So I think to the need only superusers or users with the default role
of pg_read_all_settings should see these values.Patch is very simple.
How do you think about my thoughts like this?I'm hardly an expert on this topic, but reading this blog post about
ssl_passphrase_command:https://www.2ndquadrant.com/en/blog/postgresql-passphrase-protected-ssl-keys-systemd/
which mentions that some users might go with the very naive
configuration such as:ssl_passphrase_command = 'echo "secret"'
maybe it makes sense to protect its value from everyone but superusers.
So +1.
Seems this proposal is reasonable.
I think it is reasonable.
By the way, I'm not sure the criteria of setting a GUC variable as
GUC_SUPERUSER_ONLY, but for example, ssl_max/min_protocol_version,
dynamic_library_path, log_directory, krb_server_keyfile,
data_directory and config_file are GUC_SUPERUSER_ONLY. So, it seems to
me very strange that ssl_*_file are not. Don't we need to mark them
maybe and some of the other ssl_* as the same?
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
On Thu, Feb 13, 2020 at 11:28:05AM +0900, Kyotaro Horiguchi wrote:
I think it is reasonable.
Indeed, that makes sense to me as well. I am adding Peter Eisentraut
in CC as the author/committer of 8a3d942 to comment on that.
By the way, I'm not sure the criteria of setting a GUC variable as
GUC_SUPERUSER_ONLY, but for example, ssl_max/min_protocol_version,
dynamic_library_path, log_directory, krb_server_keyfile,
data_directory and config_file are GUC_SUPERUSER_ONLY. So, it seems to
me very strange that ssl_*_file are not. Don't we need to mark them
maybe and some of the other ssl_* as the same?
This should be a separate discussion IMO. Perhaps there is a point in
softening or hardening some of them.
--
Michael
On 2020-02-13 04:38, Michael Paquier wrote:
On Thu, Feb 13, 2020 at 11:28:05AM +0900, Kyotaro Horiguchi wrote:
I think it is reasonable.
Indeed, that makes sense to me as well. I am adding Peter Eisentraut
in CC as the author/committer of 8a3d942 to comment on that.
I'm OK with changing that.
By the way, I'm not sure the criteria of setting a GUC variable as
GUC_SUPERUSER_ONLY, but for example, ssl_max/min_protocol_version,
dynamic_library_path, log_directory, krb_server_keyfile,
data_directory and config_file are GUC_SUPERUSER_ONLY. So, it seems to
me very strange that ssl_*_file are not. Don't we need to mark them
maybe and some of the other ssl_* as the same?This should be a separate discussion IMO. Perhaps there is a point in
softening or hardening some of them.
I think some of this makes sense, and we should have a discussion about it.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Dear Hackers.
Thank you for an response.
I registered this entry in commifest of 2020-03.
# I registered in the security part, but if it is wrong, sincerely
apologize for this.
And I'd like to review show authority to ssl_ * later and discuss it
in a separate thread.
Best regards.
Moon.
On Thu, Feb 13, 2020 at 6:11 PM Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
Show quoted text
On 2020-02-13 04:38, Michael Paquier wrote:
On Thu, Feb 13, 2020 at 11:28:05AM +0900, Kyotaro Horiguchi wrote:
I think it is reasonable.
Indeed, that makes sense to me as well. I am adding Peter Eisentraut
in CC as the author/committer of 8a3d942 to comment on that.I'm OK with changing that.
By the way, I'm not sure the criteria of setting a GUC variable as
GUC_SUPERUSER_ONLY, but for example, ssl_max/min_protocol_version,
dynamic_library_path, log_directory, krb_server_keyfile,
data_directory and config_file are GUC_SUPERUSER_ONLY. So, it seems to
me very strange that ssl_*_file are not. Don't we need to mark them
maybe and some of the other ssl_* as the same?This should be a separate discussion IMO. Perhaps there is a point in
softening or hardening some of them.I think some of this makes sense, and we should have a discussion about it.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: tested, passed
Documentation: not tested
I tested the patch on the master branch (addd034) and it works fine.
I think that test case which non-superuser can't see this parameter is unnecessary.
There is a similar test for pg_read_all_settings role.
The new status of this patch is: Ready for Committer
On 2020/02/14 10:31, Moon, Insung wrote:
Dear Hackers.
Thank you for an response.
I registered this entry in commifest of 2020-03.
# I registered in the security part, but if it is wrong, sincerely
apologize for this.And I'd like to review show authority to ssl_ * later and discuss it
in a separate thread.
So, you are planning to start new discussion about this?
Regards,
--
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters
On 2020/03/06 16:20, keisuke kuroda wrote:
The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: tested, passed
Documentation: not testedI tested the patch on the master branch (addd034) and it works fine.
I think that test case which non-superuser can't see this parameter is unnecessary.
There is a similar test for pg_read_all_settings role.The new status of this patch is: Ready for Committer
Pushed! Thanks!
Regards,
--
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters
Dear Kuroda-san, Fujii-san
Thank you for review and commit!
#Oops.. Sorry..This mail thread has been spammed in Gmail.
I'll go to submit a new discussion after found which case could leak
about the GUC parameters related to ssl_*.
Please wait a bit.
Best regards.
Moon.
Show quoted text
On Mon, Mar 9, 2020 at 11:43 AM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
On 2020/02/14 10:31, Moon, Insung wrote:
Dear Hackers.
Thank you for an response.
I registered this entry in commifest of 2020-03.
# I registered in the security part, but if it is wrong, sincerely
apologize for this.And I'd like to review show authority to ssl_ * later and discuss it
in a separate thread.So, you are planning to start new discussion about this?
Regards,
--
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters