Allow 'sslkey' and 'sslcert' in postgres_fdw user mappings
This patch achieves $SUBJECT and also provides some testing of the
sslpassword setting.
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
postgres_fdw_sslparms-1.patchtext/x-patch; name=postgres_fdw_sslparms-1.patchDownload+45-2
On Thu, Oct 31, 2019 at 07:54:41PM -0400, Andrew Dunstan wrote:
This patch achieves $SUBJECT and also provides some testing of the
sslpassword setting.
The patch does not apply anymore, so a rebase is needed. As it has
not been reviewed, I am moving it to next CF, waiting on author.
--
Michael
On 11/30/19 8:48 PM, Michael Paquier wrote:
On Thu, Oct 31, 2019 at 07:54:41PM -0400, Andrew Dunstan wrote:
This patch achieves $SUBJECT and also provides some testing of the
sslpassword setting.The patch does not apply anymore, so a rebase is needed. As it has
not been reviewed, I am moving it to next CF, waiting on author.
That's OK. This patch is dependent, as it always has been, on the patch
to allow passwordless user mappings for postgres_fdw. I hope to commit
that soon, but I'd prefer some signoff from prominent hackers, as I
don't want to go too far down this road and then encounter a bunch of
objections.
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2019-12-02 00:12, Andrew Dunstan wrote:
On 11/30/19 8:48 PM, Michael Paquier wrote:
On Thu, Oct 31, 2019 at 07:54:41PM -0400, Andrew Dunstan wrote:
This patch achieves $SUBJECT and also provides some testing of the
sslpassword setting.The patch does not apply anymore, so a rebase is needed. As it has
not been reviewed, I am moving it to next CF, waiting on author.That's OK. This patch is dependent, as it always has been, on the patch
to allow passwordless user mappings for postgres_fdw. I hope to commit
that soon, but I'd prefer some signoff from prominent hackers, as I
don't want to go too far down this road and then encounter a bunch of
objections.
The prerequisite patch has been committed, so please see about getting
this patch moving forward.
The patch is very small, of course, but I don't understand the "bit of a
hack" comment. Could you explain that?
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Wed, Jan 8, 2020 at 7:36 PM Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
On 2019-12-02 00:12, Andrew Dunstan wrote:
On 11/30/19 8:48 PM, Michael Paquier wrote:
On Thu, Oct 31, 2019 at 07:54:41PM -0400, Andrew Dunstan wrote:
This patch achieves $SUBJECT and also provides some testing of the
sslpassword setting.The patch does not apply anymore, so a rebase is needed. As it has
not been reviewed, I am moving it to next CF, waiting on author.That's OK. This patch is dependent, as it always has been, on the patch
to allow passwordless user mappings for postgres_fdw. I hope to commit
that soon, but I'd prefer some signoff from prominent hackers, as I
don't want to go too far down this road and then encounter a bunch of
objections.The prerequisite patch has been committed, so please see about getting
this patch moving forward.The patch is very small, of course, but I don't understand the "bit of a
hack" comment. Could you explain that?
I have rewritten the comment, and committed the feature.
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Andrew Dunstan 2019-11-01 <f941b95e-27ad-cb5c-2495-13c44f90b1bc@2ndQuadrant.com>
{"password_required", UserMappingRelationId, false}, + /* + * Extra room for the user mapping copies of sslcert and sslkey. These + * are really libpq options but we repeat them here to allow them to + * appear in both foreign server context (when we generate libpq + * options) and user mapping context (from here). Bit of a hack + * putting this in "non_libpq_options". + */ + {"sslcert", UserMappingRelationId, true}, + {"sslkey", UserMappingRelationId, true},
Nice feature, we were actually looking for exactly this yesterday.
I have some concerns about security, though. It's true that the
sslcert/sslkey options can only be set/modified by superusers when
"password_required" is set. But when password_required is not set, any
user and create user mappings that reference arbitrary files on the
server filesystem. I believe the options are still used in that case
for creating connections, even when that means the remote server isn't
set up for cert auth, which needs password_required=false to succeed.
In short, I believe these options need explicit superuser checks.
Christoph
Re: To Andrew Dunstan 2020-01-09 <20200109103014.GA4192@msg.df7cb.de>
sslcert/sslkey options can only be set/modified by superusers when
"password_required" is set. But when password_required is not set, any
user and create user mappings that reference arbitrary files on the
server filesystem.
(A nice addition here which would avoid the problems would be the
possibility to pass in the certificates as strings, but that needs
support in libpq.)
Christoph
Re: To Andrew Dunstan 2020-01-09 <20200109103014.GA4192@msg.df7cb.de>
I believe the options are still used in that case
for creating connections, even when that means the remote server isn't
set up for cert auth, which needs password_required=false to succeed.
They are indeed:
stat("/var/lib/postgresql/.postgresql/root.crt", 0x7ffcff3e2bb0) = -1 ENOENT (Datei oder Verzeichnis nicht gefunden)
stat("/foo", 0x7ffcff3e2bb0) = -1 ENOENT (Datei oder Verzeichnis nicht gefunden)
^^^^ sslcert
I'm not sure if that could be exploited in any way, but let's just
forbid it.
Christoph
On Thu, Jan 9, 2020 at 5:30 AM Christoph Berg <myon@debian.org> wrote:
I have some concerns about security, though. It's true that the
sslcert/sslkey options can only be set/modified by superusers when
"password_required" is set. But when password_required is not set, any
user and create user mappings that reference arbitrary files on the
server filesystem. I believe the options are still used in that case
for creating connections, even when that means the remote server isn't
set up for cert auth, which needs password_required=false to succeed.In short, I believe these options need explicit superuser checks.
I share the concern about the security issue here. I can't testify to
whether Christoph's whole analysis is here, but as a general point,
non-superusers can't be allowed to do things that cause the server to
access arbitrary local files.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Fri, Jan 10, 2020 at 1:21 AM Robert Haas <robertmhaas@gmail.com> wrote:
On Thu, Jan 9, 2020 at 5:30 AM Christoph Berg <myon@debian.org> wrote:
I have some concerns about security, though. It's true that the
sslcert/sslkey options can only be set/modified by superusers when
"password_required" is set. But when password_required is not set, any
user and create user mappings that reference arbitrary files on the
server filesystem. I believe the options are still used in that case
for creating connections, even when that means the remote server isn't
set up for cert auth, which needs password_required=false to succeed.In short, I believe these options need explicit superuser checks.
I share the concern about the security issue here. I can't testify to
whether Christoph's whole analysis is here, but as a general point,
non-superusers can't be allowed to do things that cause the server to
access arbitrary local files.
It's probably fairly easy to do (c.f. 6136e94dcb). I'm not (yet)
convinced that there is any significant security threat here. This
doesn't give the user or indeed any postgres code any access to the
contents of these files. But if there is a consensus to restrict this
I'll do it.
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 9 Jan 2020, at 22:38, Andrew Dunstan <andrew.dunstan@2ndquadrant.com> wrote:
I'm not (yet)
convinced that there is any significant security threat here. This
doesn't give the user or indeed any postgres code any access to the
contents of these files. But if there is a consensus to restrict this
I'll do it.
I've seen successful exploits made from parts that I in my wildest imagination
couldn't think be useful, so FWIW +1 for adding belts to suspenders and
restricting this.
cheers ./daniel
Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
On Fri, Jan 10, 2020 at 1:21 AM Robert Haas <robertmhaas@gmail.com> wrote:
I share the concern about the security issue here. I can't testify to
whether Christoph's whole analysis is here, but as a general point,
non-superusers can't be allowed to do things that cause the server to
access arbitrary local files.
It's probably fairly easy to do (c.f. 6136e94dcb). I'm not (yet)
convinced that there is any significant security threat here. This
doesn't give the user or indeed any postgres code any access to the
contents of these files. But if there is a consensus to restrict this
I'll do it.
Well, even without access to the file contents, the mere ability to
probe the existence of a file is something we don't want unprivileged
users to have. And (I suppose) this is enough for that, by looking
at what error you get back from trying it.
regards, tom lane
On Fri, Jan 10, 2020 at 8:32 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
On Fri, Jan 10, 2020 at 1:21 AM Robert Haas <robertmhaas@gmail.com> wrote:
I share the concern about the security issue here. I can't testify to
whether Christoph's whole analysis is here, but as a general point,
non-superusers can't be allowed to do things that cause the server to
access arbitrary local files.It's probably fairly easy to do (c.f. 6136e94dcb). I'm not (yet)
convinced that there is any significant security threat here. This
doesn't give the user or indeed any postgres code any access to the
contents of these files. But if there is a consensus to restrict this
I'll do it.Well, even without access to the file contents, the mere ability to
probe the existence of a file is something we don't want unprivileged
users to have. And (I suppose) this is enough for that, by looking
at what error you get back from trying it.
OK, that's convincing enough. Will do it before long.
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, 10 Jan 2020 at 06:16, Andrew Dunstan <andrew.dunstan@2ndquadrant.com>
wrote:
On Fri, Jan 10, 2020 at 8:32 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
On Fri, Jan 10, 2020 at 1:21 AM Robert Haas <robertmhaas@gmail.com>
wrote:
I share the concern about the security issue here. I can't testify to
whether Christoph's whole analysis is here, but as a general point,
non-superusers can't be allowed to do things that cause the server to
access arbitrary local files.It's probably fairly easy to do (c.f. 6136e94dcb). I'm not (yet)
convinced that there is any significant security threat here. This
doesn't give the user or indeed any postgres code any access to the
contents of these files. But if there is a consensus to restrict this
I'll do it.Well, even without access to the file contents, the mere ability to
probe the existence of a file is something we don't want unprivileged
users to have. And (I suppose) this is enough for that, by looking
at what error you get back from trying it.OK, that's convincing enough. Will do it before long.
Thanks. I'm 100% convinced the superuser restriction should be imposed. I
can imagine there being a risk of leaking file contents in error output
such as parse errors from OpenSSL that we pass on for example. Tricking Pg
into reading from a fifo could be problematic too.
I should've applied that restriction from the start, the same way as
passwordless connections are restricted.
--
Craig Ringer http://www.2ndQuadrant.com/
2ndQuadrant - PostgreSQL Solutions for the Enterprise