Horton

Thread

Commits

  1. Only superuser can set sslcert/sslkey in postgres_fdw user mappings

  1. Allow 'sslkey' and 'sslcert' in postgres_fdw user mappings

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2019-10-31T23:54:41Z

    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

  2. Re: Allow 'sslkey' and 'sslcert' in postgres_fdw user mappings

    Michael Paquier <michael@paquier.xyz> — 2019-12-01T01:48:57Z

    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

  3. Re: Allow 'sslkey' and 'sslcert' in postgres_fdw user mappings

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2019-12-01T23:12:14Z

    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

  4. Re: Allow 'sslkey' and 'sslcert' in postgres_fdw user mappings

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2020-01-08T09:06:02Z

    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

  5. Re: Allow 'sslkey' and 'sslcert' in postgres_fdw user mappings

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2020-01-09T08:14:33Z

    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

  6. Re: Allow 'sslkey' and 'sslcert' in postgres_fdw user mappings

    Christoph Berg <myon@debian.org> — 2020-01-09T10:30:14Z

    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

  7. Re: Allow 'sslkey' and 'sslcert' in postgres_fdw user mappings

    Christoph Berg <myon@debian.org> — 2020-01-09T10:45:11Z

    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

  8. Re: Allow 'sslkey' and 'sslcert' in postgres_fdw user mappings

    Christoph Berg <myon@debian.org> — 2020-01-09T12:48:55Z

    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

  9. Re: Allow 'sslkey' and 'sslcert' in postgres_fdw user mappings

    Robert Haas <robertmhaas@gmail.com> — 2020-01-09T14:51:32Z

    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

  10. Re: Allow 'sslkey' and 'sslcert' in postgres_fdw user mappings

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2020-01-09T21:38:42Z

    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

  11. Re: Allow 'sslkey' and 'sslcert' in postgres_fdw user mappings

    Daniel Gustafsson <daniel@yesql.se> — 2020-01-09T22:00:59Z

    > 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

  12. Re: Allow 'sslkey' and 'sslcert' in postgres_fdw user mappings

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-01-09T22:02:18Z

    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

  13. Re: Allow 'sslkey' and 'sslcert' in postgres_fdw user mappings

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2020-01-09T22:16:11Z

    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

  14. Re: Allow 'sslkey' and 'sslcert' in postgres_fdw user mappings

    Craig Ringer <craig@2ndquadrant.com> — 2020-01-20T08:09:26Z

    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