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