Thread: 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.
Attachment
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
Attachment
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: > > 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 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 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. 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