Thread: dblink connection security
Patch based on recent -hackers discussions, it removes usage from public, and
adds a note to the documentation about why this is neccessary.
--
Robert Treat
Build A Brighter LAMP :: Linux Apache {middleware} PostgreSQL
Attachment
Robert Treat wrote:
> Patch based on recent -hackers discussions, it removes usage from public, and
> adds a note to the documentation about why this is neccessary.
>
I agree with the fix as the simplest and most sensible approach, and in
general with the doc change, but I'm not inclined to reference the
security paper. Maybe something like:
As a security precaution, dblink revokes access from PUBLIC role
usage for the dblink_connect functions. It is not safe to allow
remote users to execute dblink from a database in a PostgreSQL
installation that allows local account access using the "trust"
authentication method. In that case, remote users could gain
access to other accounts via dblink. If "trust" authentication
is disabled, this is no longer an issue.
I suppose this ought to be applied back through the 7.3 branch?
Joe
"Joe Conway" <mail@joeconway.com> writes: > Robert Treat wrote: >> Patch based on recent -hackers discussions, it removes usage from public, and >> adds a note to the documentation about why this is neccessary. >> > > I agree with the fix as the simplest and most sensible approach, and in general > with the doc change, but I'm not inclined to reference the security paper. > Maybe something like: > > As a security precaution, dblink revokes access from PUBLIC role > usage for the dblink_connect functions. It is not safe to allow > remote users to execute dblink from a database in a PostgreSQL > installation that allows local account access using the "trust" > authentication method. In that case, remote users could gain > access to other accounts via dblink. If "trust" authentication > is disabled, this is no longer an issue. I think putting the emphasis on Postgresql trust authentication is missing the broader point. I would suggest two paragraphs such as: dblink allows any connected user to attempt to connect to TCP/IP or Unix sockets from the database server as the user the database system is running. This could allow users to circumvent access control policies based on the connecting user or the connecting host. In particular Postgres's "trust" authentication is one such system. It authenticates connecting users based on the unix userid of the process forming the connection. In typical configurations any user who is granted execute access to dblink can form connections as the "postgres" user which is the database super-user. If "trust" authentication is disabled this is no longer an issue. Therefore dblink requires you to explicitly grant execute privileges to users or roles you wish to allow to form connections. It does not grant these privileges to the PUBLIC role by default as other packages typically do. -- Gregory Stark EnterpriseDB http://www.enterprisedb.com
On Sunday 01 July 2007 13:15, Gregory Stark wrote:
> "Joe Conway" <mail@joeconway.com> writes:
> > Robert Treat wrote:
> >> Patch based on recent -hackers discussions, it removes usage from
> >> public, and adds a note to the documentation about why this is
> >> neccessary.
> >
> > I agree with the fix as the simplest and most sensible approach, and in
> > general with the doc change, but I'm not inclined to reference the
> > security paper. Maybe something like:
> >
> > As a security precaution, dblink revokes access from PUBLIC role
> > usage for the dblink_connect functions. It is not safe to allow
> > remote users to execute dblink from a database in a PostgreSQL
> > installation that allows local account access using the "trust"
> > authentication method. In that case, remote users could gain
> > access to other accounts via dblink. If "trust" authentication
> > is disabled, this is no longer an issue.
>
> I think putting the emphasis on Postgresql trust authentication is missing
> the broader point. I would suggest two paragraphs such as:
>
> dblink allows any connected user to attempt to connect to TCP/IP or Unix
> sockets from the database server as the user the database system is
> running. This could allow users to circumvent access control policies based
> on the connecting user or the connecting host.
>
> In particular Postgres's "trust" authentication is one such system. It
> authenticates connecting users based on the unix userid of the process
> forming the connection. In typical configurations any user who is granted
> execute access to dblink can form connections as the "postgres" user which
> is the database super-user. If "trust" authentication is disabled this is
> no longer an issue.
>
Did you mean s/trust/ident/g, otherwise I don't think I understand the
above... granted the combination of trust for localhost does open a door
for remote users if they have access to dblink, but I don't think that's what
you were trying to say.
> Therefore dblink requires you to explicitly grant execute privileges to
> users or roles you wish to allow to form connections. It does not grant
> these privileges to the PUBLIC role by default as other packages typically
> do.
--
Robert Treat
Build A Brighter LAMP :: Linux Apache {middleware} PostgreSQL
Robert Treat <xzilla@users.sourceforge.net> writes:
> Did you mean s/trust/ident/g, otherwise I don't think I understand the
> above...
Both trust and ident local auth are sources of risk for this, although
ident is particularly nasty since the DBA probably thinks he's being
secure.
For that matter, I'm not sure that *any* auth method except password
offers much security against the problem; don't LDAP and Kerberos
likewise rely mostly on process-level identity? And possibly PAM
depending on which PAM plugin you're using?
I'm not sure whether this is something to back-patch, though, since
a back-patch will accomplish zero for existing installations.
regards, tom lane
Tom Lane wrote: > Robert Treat <xzilla@users.sourceforge.net> writes: >> Did you mean s/trust/ident/g, otherwise I don't think I understand the >> above... > > Both trust and ident local auth are sources of risk for this, although > ident is particularly nasty since the DBA probably thinks he's being > secure. > > For that matter, I'm not sure that *any* auth method except password > offers much security against the problem; don't LDAP and Kerberos > likewise rely mostly on process-level identity? And possibly PAM > depending on which PAM plugin you're using? LDAP is not affected - it requires the user to enter a password. Same would be for any PAM plugins that actually require the user to enter a password, I think. Kerberos is not affected either, because the server does not get a copy of the ticket. In theory it could be affected if the server requested a delegation enabled ticket, and exported it so it could be used, but none of these are done. //Magnus
"Robert Treat" <xzilla@users.sourceforge.net> writes: >> In particular Postgres's "trust" authentication is one such system. It >> authenticates connecting users based on the unix userid of the process >> forming the connection. In typical configurations any user who is granted >> execute access to dblink can form connections as the "postgres" user which >> is the database super-user. If "trust" authentication is disabled this is >> no longer an issue. > > Did you mean s/trust/ident/g, otherwise I don't think I understand the > above... granted the combination of trust for localhost does open a door > for remote users if they have access to dblink, but I don't think that's what > you were trying to say. Er quite right. Moreover it's not even true that ``"if "ident" authentication is disabled this is no longer an issue''. It's still possible to have other restrictions in pg_hba which dblink would allow you to circumvent. That sentence is too generous of a promise. -- Gregory Stark EnterpriseDB http://www.enterprisedb.com
* Magnus Hagander (magnus@hagander.net) wrote:
> LDAP is not affected - it requires the user to enter a password. Same
> would be for any PAM plugins that actually require the user to enter a
> password, I think.
Agreed.
> Kerberos is not affected either, because the server does not get a copy
> of the ticket. In theory it could be affected if the server requested a
> delegation enabled ticket, and exported it so it could be used, but none
> of these are done.
That's quite a stretch even there, imv anyway... It'd have to be put
somewhere a backend connecting would think to look for it, given that
the user can't change the environment variables and whatnot (I don't
think) of the backend process... Regardless, strong wording against
allowing users to issue arbitrary connect's from a backend process is
appropriate, regardless of what's affected exactly (as that could
possibly change over time anyway too...).
Thanks,
Stephen
Attachment
Stephen Frost <sfrost@snowman.net> writes:
> * Magnus Hagander (magnus@hagander.net) wrote:
>> Kerberos is not affected either, because the server does not get a copy
>> of the ticket. In theory it could be affected if the server requested a
>> delegation enabled ticket, and exported it so it could be used, but none
>> of these are done.
> That's quite a stretch even there, imv anyway... It'd have to be put
> somewhere a backend connecting would think to look for it, given that
> the user can't change the environment variables and whatnot (I don't
> think) of the backend process...
Hmm. I think what you are both saying is that if the remote end wants
Kerberos auth then you would expect a dblink connection to always fail.
If so, then we still seem to be down to the conclusion that there
are only three kinds of dblink connection:
* those that require a password;
* those that don't work;
* those that are insecure.
Would it be sensible to change dblink so that unless invoked by a
superuser, it fails any connection attempt in which no password is
demanded? I am not sure that this is possible without changes to libpq;
but ignoring implementation difficulties, is this a sane idea from
the standpoint of security and usability?
regards, tom lane
Tom Lane wrote: > Stephen Frost <sfrost@snowman.net> writes: >> * Magnus Hagander (magnus@hagander.net) wrote: >>> Kerberos is not affected either, because the server does not get a copy >>> of the ticket. In theory it could be affected if the server requested a >>> delegation enabled ticket, and exported it so it could be used, but none >>> of these are done. > >> That's quite a stretch even there, imv anyway... It'd have to be put >> somewhere a backend connecting would think to look for it, given that >> the user can't change the environment variables and whatnot (I don't >> think) of the backend process... > > Hmm. I think what you are both saying is that if the remote end wants > Kerberos auth then you would expect a dblink connection to always fail. > If so, then we still seem to be down to the conclusion that there > are only three kinds of dblink connection: > * those that require a password; > * those that don't work; > * those that are insecure. > > Would it be sensible to change dblink so that unless invoked by a > superuser, it fails any connection attempt in which no password is > demanded? I am not sure that this is possible without changes to libpq; > but ignoring implementation difficulties, is this a sane idea from > the standpoint of security and usability? Not sure. That would break any attempts of implementing delegation in Kerberos for dblink, but I don't know if we're interested in doing that anyway. BTW, what I wrote about delegation before is wrong, of course. If delegation worked, in that pg requested a delegation enabled ticket and exported it through the dblink connection, it would authenticate as the user that authenticated to the original database, not as the superuser or anything like that. So delegation would actually be perfectly secure. If implemented properly of course. //Magnus
Tom Lane wrote:
> Robert Treat <xzilla@users.sourceforge.net> writes:
>> Did you mean s/trust/ident/g, otherwise I don't think I understand the
>> above...
>
> Both trust and ident local auth are sources of risk for this, although
> ident is particularly nasty since the DBA probably thinks he's being
> secure.
>
> For that matter, I'm not sure that *any* auth method except password
> offers much security against the problem; don't LDAP and Kerberos
> likewise rely mostly on process-level identity? And possibly PAM
> depending on which PAM plugin you're using?
OK, so following that line of thought, how about:
As a security precaution, dblink revokes access from PUBLIC role
usage for the dblink_connect functions. It is not safe to allow
ordinary users to execute dblink from a database in a PostgreSQL
installation that allows account access using any authentication
method which does not require a password. In that case, ordinary
users could gain access to other accounts via dblink as if they
had the privileges of the database superuser.
If the allowed authentication methods require a password, this is no
longer an issue.
> I'm not sure whether this is something to back-patch, though, since
> a back-patch will accomplish zero for existing installations.
OK. But it might still be worth doing, along with something in the
release notes.
Joe
Tom Lane wrote: > Stephen Frost <sfrost@snowman.net> writes: >> * Magnus Hagander (magnus@hagander.net) wrote: >>> Kerberos is not affected either, because the server does not get a copy >>> of the ticket. In theory it could be affected if the server requested a >>> delegation enabled ticket, and exported it so it could be used, but none >>> of these are done. > >> That's quite a stretch even there, imv anyway... It'd have to be put >> somewhere a backend connecting would think to look for it, given that >> the user can't change the environment variables and whatnot (I don't >> think) of the backend process... > > Hmm. I think what you are both saying is that if the remote end wants > Kerberos auth then you would expect a dblink connection to always fail. > If so, then we still seem to be down to the conclusion that there > are only three kinds of dblink connection: > * those that require a password; > * those that don't work; > * those that are insecure. > > Would it be sensible to change dblink so that unless invoked by a > superuser, it fails any connection attempt in which no password is > demanded? I am not sure that this is possible without changes to libpq; > but ignoring implementation difficulties, is this a sane idea from > the standpoint of security and usability? Possibly so. Remember that dblink is simply a libpq client. Doesn't that mean that similar (although likely less severe) issues affect other libpq clients executing locally, such as php or perl-dbi clients? Joe
Joe Conway <mail@joeconway.com> writes:
> Tom Lane wrote:
>> Would it be sensible to change dblink so that unless invoked by a
>> superuser, it fails any connection attempt in which no password is
>> demanded? I am not sure that this is possible without changes to libpq;
>> but ignoring implementation difficulties, is this a sane idea from
>> the standpoint of security and usability?
> Possibly so. Remember that dblink is simply a libpq client. Doesn't that
> mean that similar (although likely less severe) issues affect other
> libpq clients executing locally, such as php or perl-dbi clients?
Yeah, in principle this issue applies to any process performing a
Postgres connection on behalf of someone else. (Whether there are any
programs doing that, other than dblink, is debatable; but someday there
may be.)
The point about Kerberos delegation is interesting, but given that it
doesn't work anyway, I'm not sure we need a solution for it right now.
Possibly, when and if we get around to implementing it, we can somehow
treat use of a delegated ticket as equivalent to use of a password.
The general point is that we'd like to know whether the connection was
authorized by means of some data supplied by the client, or on the basis
of our own process identity (the latter being the case we wish to
reject). Right now the only kind of "data supplied by the client" here
is a password.
Here's a straw-man proposal that we could perhaps do for 8.3:
1. Invent a libpq connection-status function
bool PQconnectionUsedPassword(const PGconn *conn);
This returns true if the server had demanded a password during the
authentication phase. Aside from solving the immediate problem, this
can be useful for regular clients such as psql: it could be applied to a
failed connection object to decide whether to prompt for a password
(replacing the current egregious hack of strcmp'ing the error message).
2. Make dblink close the connection and throw error if called by a
non-superuser and PQconnectionUsedPassword returns false.
This idea isn't usable as a back-patch, however, because adding
functions to existing libpq versions is too chancy. What we could
possibly do in back versions is, if dblink_connect is called by a
non-superuser, first issue the connection attempt without any password
and reject if that doesn't fail. (This'd involve parsing the connect
string well enough to remove the password, which is tedious, but
certainly doable.)
I like this approach better than removing public execute privileges
on the functions, for two reasons:
* A routine minor version update would install the security fix into
existing installations, without need for any DBA intervention.
* It does not take away functionality that has perfectly legitimate uses.
regards, tom lane
Tom Lane wrote: > Joe Conway <mail@joeconway.com> writes: >> Tom Lane wrote: >>> Would it be sensible to change dblink so that unless invoked by a >>> superuser, it fails any connection attempt in which no password is >>> demanded? I am not sure that this is possible without changes to libpq; >>> but ignoring implementation difficulties, is this a sane idea from >>> the standpoint of security and usability? > >> Possibly so. Remember that dblink is simply a libpq client. Doesn't that >> mean that similar (although likely less severe) issues affect other >> libpq clients executing locally, such as php or perl-dbi clients? > > Yeah, in principle this issue applies to any process performing a > Postgres connection on behalf of someone else. (Whether there are any > programs doing that, other than dblink, is debatable; but someday there > may be.) Well certainly dbi-link has the exact same issue. And a local php-apache instance connecting to Postgres would allow Postgres connections as the apache user, no? Not that it is likely to be a problem, but if for some reason there was an apache user in Postgres, and even worse, if that user was given superuser status, you would have the exact same problem. > The point about Kerberos delegation is interesting, but given that it > doesn't work anyway, I'm not sure we need a solution for it right now. > Possibly, when and if we get around to implementing it, we can somehow > treat use of a delegated ticket as equivalent to use of a password. > The general point is that we'd like to know whether the connection was > authorized by means of some data supplied by the client, or on the basis > of our own process identity (the latter being the case we wish to > reject). Right now the only kind of "data supplied by the client" here > is a password. > > Here's a straw-man proposal that we could perhaps do for 8.3: > > 1. Invent a libpq connection-status function > > bool PQconnectionUsedPassword(const PGconn *conn); Maybe PQconnectionUsedAuthToken() to mean "data supplied by the client", including other potential future mechanisms? > 2. Make dblink close the connection and throw error if called by a > non-superuser and PQconnectionUsedPassword returns false. Sounds good to me. > This idea isn't usable as a back-patch, however, because adding > functions to existing libpq versions is too chancy. What we could > possibly do in back versions is, if dblink_connect is called by a > non-superuser, first issue the connection attempt without any password > and reject if that doesn't fail. (This'd involve parsing the connect > string well enough to remove the password, which is tedious, but > certainly doable.) Why not just require the connect string to contain a password for non-superusers? > I like this approach better than removing public execute privileges > on the functions, for two reasons: > > * A routine minor version update would install the security fix into > existing installations, without need for any DBA intervention. > > * It does not take away functionality that has perfectly legitimate uses. Agreed. I won't have time to work on this until the end of the coming week -- tomorrow is the last day of my current business trip which tends to be busy. Tuesday I spend all day getting from Germany to San Diego. If it can wait that long, I'll look into it starting on the 5th, unless someone beats me to it. Joe
Tom Lane wrote: > Joe Conway <mail@joeconway.com> writes: >> Tom Lane wrote: >>> Would it be sensible to change dblink so that unless invoked by a >>> superuser, it fails any connection attempt in which no password is >>> demanded? I am not sure that this is possible without changes to libpq; >>> but ignoring implementation difficulties, is this a sane idea from >>> the standpoint of security and usability? > >> Possibly so. Remember that dblink is simply a libpq client. Doesn't that >> mean that similar (although likely less severe) issues affect other >> libpq clients executing locally, such as php or perl-dbi clients? > > Yeah, in principle this issue applies to any process performing a > Postgres connection on behalf of someone else. (Whether there are any > programs doing that, other than dblink, is debatable; but someday there > may be.) > > The point about Kerberos delegation is interesting, but given that it > doesn't work anyway, I'm not sure we need a solution for it right now. Agreed. > Here's a straw-man proposal that we could perhaps do for 8.3: > > 1. Invent a libpq connection-status function > > bool PQconnectionUsedPassword(const PGconn *conn); > > This returns true if the server had demanded a password during the > authentication phase. Aside from solving the immediate problem, this > can be useful for regular clients such as psql: it could be applied to a > failed connection object to decide whether to prompt for a password > (replacing the current egregious hack of strcmp'ing the error message). Hmm. It would be better if it never actually completed an authentication in the first place, but I don't see how we can do that given how the protocol works. We could add a connection string parameter that disables it, but that doesn't really help since the backend moves into authenticated mode before you can abort anyway. And it's probably not a big issue anyway. //Magnus
Joe Conway <mail@joeconway.com> writes:
> Tom Lane wrote:
>> bool PQconnectionUsedPassword(const PGconn *conn);
> Maybe PQconnectionUsedAuthToken() to mean "data supplied by the client",
> including other potential future mechanisms?
Well, that'd not solve the pre-existing problem of how to tell whether
to request a password. If we had a fairly clear idea of what other
sorts of auth tokens might be involved, I'd be willing to go that way,
but I distrust our ability to design it today.
>> This idea isn't usable as a back-patch, however, because adding
>> functions to existing libpq versions is too chancy. What we could
>> possibly do in back versions is, if dblink_connect is called by a
>> non-superuser, first issue the connection attempt without any password
>> and reject if that doesn't fail.
> Why not just require the connect string to contain a password for
> non-superusers?
Doesn't fix the problem, because you don't know whether libpq actually
used the password or not.
> I won't have time to work on this until the end of the coming week --
No hurry, I don't think there are any short-term plans for a release.
regards, tom lane
Magnus Hagander <magnus@hagander.net> writes:
> Hmm. It would be better if it never actually completed an authentication
> in the first place, but I don't see how we can do that given how the
> protocol works.
> We could add a connection string parameter that disables it, but that
> doesn't really help since the backend moves into authenticated mode
> before you can abort anyway.
Yeah. Since this is really a question of client-side code protecting
itself from misuse of its credentials, I don't think it's a very severe
problem --- it can certainly make the check before allowing any use of
the new PGconn object.
regards, tom lane
On Sunday 01 July 2007 16:03, Joe Conway wrote:
> Tom Lane wrote:
> > Joe Conway <mail@joeconway.com> writes:
> >> Tom Lane wrote:
> >>> Would it be sensible to change dblink so that unless invoked by a
> >>> superuser, it fails any connection attempt in which no password is
> >>> demanded? I am not sure that this is possible without changes to
> >>> libpq; but ignoring implementation difficulties, is this a sane idea
> >>> from the standpoint of security and usability?
> >>
> >> Possibly so. Remember that dblink is simply a libpq client. Doesn't that
> >> mean that similar (although likely less severe) issues affect other
> >> libpq clients executing locally, such as php or perl-dbi clients?
> >
> > Yeah, in principle this issue applies to any process performing a
> > Postgres connection on behalf of someone else. (Whether there are any
> > programs doing that, other than dblink, is debatable; but someday there
> > may be.)
>
> Well certainly dbi-link has the exact same issue.
dbi-link only works in plperlu, so you've already decided your superuser only.
> And a local php-apache
> instance connecting to Postgres would allow Postgres connections as the
> apache user, no? Not that it is likely to be a problem, but if for some
> reason there was an apache user in Postgres, and even worse, if that
> user was given superuser status, you would have the exact same problem.
>
That doesnt seem like the same problem to me, since those connections are
still from an external source. The issue with dblink is that because you are
making a connection from a postgres process, you are connecting as the
postgres user when you weren't that user before.
> > The point about Kerberos delegation is interesting, but given that it
> > doesn't work anyway, I'm not sure we need a solution for it right now.
> > Possibly, when and if we get around to implementing it, we can somehow
> > treat use of a delegated ticket as equivalent to use of a password.
> > The general point is that we'd like to know whether the connection was
> > authorized by means of some data supplied by the client, or on the basis
> > of our own process identity (the latter being the case we wish to
> > reject). Right now the only kind of "data supplied by the client" here
> > is a password.
> >
> > Here's a straw-man proposal that we could perhaps do for 8.3:
> >
> > 1. Invent a libpq connection-status function
> >
> > bool PQconnectionUsedPassword(const PGconn *conn);
>
> Maybe PQconnectionUsedAuthToken() to mean "data supplied by the client",
> including other potential future mechanisms?
>
> > 2. Make dblink close the connection and throw error if called by a
> > non-superuser and PQconnectionUsedPassword returns false.
>
> Sounds good to me.
>
> > This idea isn't usable as a back-patch, however, because adding
> > functions to existing libpq versions is too chancy. What we could
> > possibly do in back versions is, if dblink_connect is called by a
> > non-superuser, first issue the connection attempt without any password
> > and reject if that doesn't fail. (This'd involve parsing the connect
> > string well enough to remove the password, which is tedious, but
> > certainly doable.)
>
> Why not just require the connect string to contain a password for
> non-superusers?
>
> > I like this approach better than removing public execute privileges
> > on the functions, for two reasons:
> >
> > * A routine minor version update would install the security fix into
> > existing installations, without need for any DBA intervention.
> >
> > * It does not take away functionality that has perfectly legitimate uses.
>
> Agreed.
>
I think this will break backwards compatability though. ie. non-superusers
may be calling functions that are relying on dblink to connect sans password,
which would be broken by the above changes. I'm pretty sure it can be worked
around (though the work around is ugly, you cant hardcode passwords in the
functions or else people can look them up in pgproc) but people may need to
make changes to thier code to live with this.
--
Robert Treat
Build A Brighter LAMP :: Linux Apache {middleware} PostgreSQL
Robert Treat <xzilla@users.sourceforge.net> writes:
> Tom Lane wrote:
>> I like this approach better than removing public execute privileges
>> on the functions, for two reasons:
> I think this will break backwards compatability though.
Well, revoking public execute will break backwards compatibility too.
If you have a situation where you think it's safe to allow a
non-superuser to get at passwordless connections, you could wrap the
dblink_connect function in a postgres-owned SECURITY DEFINER function.
So either change can be worked around to get the old behavior if necessary.
regards, tom lane
Robert Treat wrote: >>> Joe Conway <mail@joeconway.com> writes: >> Well certainly dbi-link has the exact same issue. > > dbi-link only works in plperlu, so you've already decided your superuser only. How so -- it is fundamentally no different than dblink, which is C language (also untrusted). I think the issue is that once the superuser creates said functions, usage of the functions is automatically granted to PUBLIC, no? Being an untrusted language just means that it takes a superuser to create the functions using that language, not to use the functions themselves. Joe
"Tom Lane" <tgl@sss.pgh.pa.us> writes: > I like this approach better than removing public execute privileges > on the functions, for two reasons: > > * A routine minor version update would install the security fix into > existing installations, without need for any DBA intervention. > > * It does not take away functionality that has perfectly legitimate uses. I think there are two problems with this: a) dblink still shouldn't allow arbitrary users to open arbitrary tcp/ip sockets or unix sockets from the server. That's still a security threat even if we close Postgres's vulnerability to it. Even if libpq prevents you from doing much because it looks for the libpq protocol messages it would still allow, for example, an attacker to do a port probe and see what services are running on other hosts on the internal network. b) For a situation like a homebrew replication system someone may want to have set up a second server which allows passwordless access from the first server. In which case it is entirely sane (though it doesn't seem to be the best idea imho) to use ident and requiring a password is removing functionality that has a perfectly legitimate use. -- Gregory Stark EnterpriseDB http://www.enterprisedb.com
Joe Conway wrote:
> Robert Treat wrote:
>>>> Joe Conway <mail@joeconway.com> writes:
>>> Well certainly dbi-link has the exact same issue.
>> dbi-link only works in plperlu, so you've already decided your superuser only.
>
> How so -- it is fundamentally no different than dblink, which is C
> language (also untrusted).
>
> I think the issue is that once the superuser creates said functions,
> usage of the functions is automatically granted to PUBLIC, no? Being an
> untrusted language just means that it takes a superuser to create the
> functions using that language, not to use the functions themselves.
In fact, this misconception can prove dangerous in other ways. From the
docs:
CREATE FUNCTION badfunc() RETURNS integer AS $$
my $tmpfile = "/tmp/badfile";
open my $fh, '>', $tmpfile
or elog(ERROR, qq{could not open the file "$tmpfile": $!});
print $fh "Testing writing to a file\n";
close $fh or elog(ERROR, qq{could not close the file "$tmpfile": $!});
return 1;
$$ LANGUAGE plperlu;
select usename, usesuper from pg_shadow;
usename | usesuper
----------+----------
postgres | t
foo | f
(2 rows)
contrib_regression=# \c - foo
You are now connected to database "contrib_regression" as user "foo".
contrib_regression=> select badfunc();
badfunc
---------
1
(1 row)
So anyone thinking that just because a language is untrusted means that
they don't need to be careful, is mistaken.
Joe
Gregory Stark <stark@enterprisedb.com> writes:
> I think there are two problems with this:
> a) dblink still shouldn't allow arbitrary users to open arbitrary tcp/ip
> sockets or unix sockets from the server. That's still a security threat
> even if we close Postgres's vulnerability to it.
The only way we could enforce that would be to completely disallow
non-superuser use of dblink; ie, "if (!superuser()) elog(ERROR)",
nothing so weak as revoking public execute access. That's a good deal
further than I'm prepared to go, as it really does take away
functionality. And it does it in order to close someone else's security
problem, so I think it's a pretty bad tradeoff.
> b) For a situation like a homebrew replication system someone may want
> to have set up a second server which allows passwordless access
> from the first server. In which case it is entirely sane (though it
> doesn't seem to be the best idea imho) to use ident and requiring a
> password is removing functionality that has a perfectly legitimate
> use.
Neither of the proposed fixes prevent that; you can either grant execute
access to appropriate people in the original suggestion, or wrap
dblink_connect in a SECURITY DEFINER function in my new suggestion.
regards, tom lane
On Sunday 01 July 2007 17:59, Joe Conway wrote:
> Joe Conway wrote:
> > Robert Treat wrote:
> >>>> Joe Conway <mail@joeconway.com> writes:
> >>>
> >>> Well certainly dbi-link has the exact same issue.
> >>
> >> dbi-link only works in plperlu, so you've already decided your superuser
> >> only.
> >
> > How so -- it is fundamentally no different than dblink, which is C
> > language (also untrusted).
> >
> > I think the issue is that once the superuser creates said functions,
> > usage of the functions is automatically granted to PUBLIC, no? Being an
> > untrusted language just means that it takes a superuser to create the
> > functions using that language, not to use the functions themselves.
>
> In fact, this misconception can prove dangerous in other ways. From the
> docs:
>
> CREATE FUNCTION badfunc() RETURNS integer AS $$
> my $tmpfile = "/tmp/badfile";
> open my $fh, '>', $tmpfile
> or elog(ERROR, qq{could not open the file "$tmpfile": $!});
> print $fh "Testing writing to a file\n";
> close $fh or elog(ERROR, qq{could not close the file "$tmpfile": $!});
> return 1;
> $$ LANGUAGE plperlu;
>
> select usename, usesuper from pg_shadow;
> usename | usesuper
> ----------+----------
> postgres | t
> foo | f
> (2 rows)
>
> contrib_regression=# \c - foo
> You are now connected to database "contrib_regression" as user "foo".
> contrib_regression=> select badfunc();
> badfunc
> ---------
> 1
> (1 row)
>
> So anyone thinking that just because a language is untrusted means that
> they don't need to be careful, is mistaken.
>
lol... you're absolutly correct, I wasn't thinking clearly earlier. I took a
3-hour nap shortly after my last email, I probably should have taken it
before :-)
--
Robert Treat
Build A Brighter LAMP :: Linux Apache {middleware} PostgreSQL
"Tom Lane" <tgl@sss.pgh.pa.us> writes: > Gregory Stark <stark@enterprisedb.com> writes: >> I think there are two problems with this: > >> a) dblink still shouldn't allow arbitrary users to open arbitrary tcp/ip >> sockets or unix sockets from the server. That's still a security threat >> even if we close Postgres's vulnerability to it. > > The only way we could enforce that would be to completely disallow > non-superuser use of dblink; ie, "if (!superuser()) elog(ERROR)", > nothing so weak as revoking public execute access. That's a good deal > further than I'm prepared to go, as it really does take away > functionality. And it does it in order to close someone else's security > problem, so I think it's a pretty bad tradeoff. Well first of all this isn't someone else's security problem. dblink is the one allowing the privilege escalation: it allows user stark on box A to create sockets on box B as user postgres. The other services are not at fault for trusting that a connection from box B as user postgres was made by someone with the privilege to do so. While restricting dblink_connect() to superuser would guarantee nobody ever have a problem merely removing the privilege from non-superusers would at least require the sysadmin to consider which users should have the privilege of creating connections as the postgres user and grant only those users the ability. As long as it's the sysadmin doing the granting and not the install script that's fine. Fwiw I don't see much distinction between restricting a function to superuser only and only granting initial privileges to superuser. In either case a sysadmin can grant access to non-superusers, it's just more convenient in the latter case as the former case requires security definer functions. -- Gregory Stark EnterpriseDB http://www.enterprisedb.com
Tom Lane wrote:
>
> Here's a straw-man proposal that we could perhaps do for 8.3:
What about using the attached for 8.3, as well as earlier?
It simply does not allow the local database user to become someone else
on the libpq remote connection unless they are a superuser. As Tom
noted, a simple SECURITY DEFINER function created as a superuser could
allow backward compatible behavior.
CREATE OR REPLACE FUNCTION dblink_connect_u(connstr TEXT)
RETURNS TEXT AS $$
DECLARE passed TEXT;
BEGIN
SELECT dblink_connect(connstr) INTO passed;
RETURN passed;
END;
$$ LANGUAGE plpgsql SECURITY DEFINER;
contrib_regression=# \c - foo
You are now connected to database "contrib_regression" as user "foo".
contrib_regression=> select dblink_connect('dbname=contrib_regression');
ERROR: switching user not allowed
DETAIL: failed to connect local user "foo" as remote user "postgres"
HINT: only superuser may switch user name
contrib_regression=> select dblink_connect_u('dbname=contrib_regression');
dblink_connect_u
------------------
OK
(1 row)
Comments?
Thanks,
Joe
Index: dblink.c
===================================================================
RCS file: /opt/src/cvs/pgsql/contrib/dblink/dblink.c,v
retrieving revision 1.63
diff -c -r1.63 dblink.c
*** dblink.c 6 Apr 2007 04:21:41 -0000 1.63
--- dblink.c 7 Jul 2007 04:39:49 -0000
***************
*** 37,42 ****
--- 37,43 ----
#include "libpq-fe.h"
#include "fmgr.h"
#include "funcapi.h"
+ #include "miscadmin.h"
#include "access/heapam.h"
#include "access/tupdesc.h"
#include "catalog/namespace.h"
***************
*** 230,235 ****
--- 231,249 ----
rconn = (remoteConn *) palloc(sizeof(remoteConn));
conn = PQconnectdb(connstr);
+ if (!superuser())
+ {
+ char *luser = PQuser(conn);
+ char *cuser = GetUserNameFromId(GetUserId());
+
+ if (strcmp(luser, cuser) != 0)
+ ereport(ERROR,
+ (errcode(ERRCODE_S_R_E_PROHIBITED_SQL_STATEMENT_ATTEMPTED),
+ errmsg("switching user not allowed"),
+ errdetail("failed to connect local user \"%s\" as remote user \"%s\"", cuser, luser),
+ errhint("only superuser may switch user name")));
+ }
+
MemoryContextSwitchTo(oldcontext);
if (PQstatus(conn) == CONNECTION_BAD)
Joe Conway <mail@joeconway.com> writes:
> What about using the attached for 8.3, as well as earlier?
> It simply does not allow the local database user to become someone else
> on the libpq remote connection unless they are a superuser.
This assumes that usernames on the remote site are equivalent to those
locally. Which is helpful for the sort of local-loop scenarios we've
been thinking about, but is hardly watertight even then (consider
multiple postmasters on one machine). For remote connections it seems
counterproductive; you might as well say "you must be superuser" and
keep it simple.
regards, tom lane
Tom Lane wrote: > Joe Conway <mail@joeconway.com> writes: >> What about using the attached for 8.3, as well as earlier? > >> It simply does not allow the local database user to become someone else >> on the libpq remote connection unless they are a superuser. > > This assumes that usernames on the remote site are equivalent to those > locally. Which is helpful for the sort of local-loop scenarios we've > been thinking about, but is hardly watertight even then (consider > multiple postmasters on one machine). For remote connections it seems > counterproductive; you might as well say "you must be superuser" and > keep it simple. I see your point. OK, I'm back to implementing your proposal... One question: should we provide the SECURITY DEFINER functions with revoked privileges or just mention that in the docs? I was thinking something along the lines of the following even for the backpatched version: CREATE OR REPLACE FUNCTION dblink_connect_u (text) RETURNS text AS 'MODULE_PATHNAME','dblink_connect' LANGUAGE C STRICT SECURITY DEFINER; CREATE OR REPLACE FUNCTION dblink_connect_u (text, text) RETURNS text AS 'MODULE_PATHNAME','dblink_connect' LANGUAGE C STRICT SECURITY DEFINER; REVOKE execute ON FUNCTION dblink_connect_u (text) FROM public; REVOKE execute ON FUNCTION dblink_connect_u (text, text) FROM public; Joe
Joe Conway <mail@joeconway.com> writes:
> One question: should we provide the SECURITY DEFINER functions with
> revoked privileges or just mention that in the docs? I was thinking
> something along the lines of the following even for the backpatched version:
Hmm. I guess the advantage of providing these pre-made is that it would
standardize the names to use for them, which seems like a good thing.
I'm not sure about the point of back-patching, though, since again
you're not going to be affecting the content of existing installations.
> REVOKE execute ON FUNCTION dblink_connect_u (text) FROM public;
> REVOKE execute ON FUNCTION dblink_connect_u (text, text) FROM public;
I'd write that as REVOKE ALL just to be future-proof.
regards, tom lane
Tom Lane wrote:
> Here's a straw-man proposal that we could perhaps do for 8.3:
>
> 1. Invent a libpq connection-status function
>
> bool PQconnectionUsedPassword(const PGconn *conn);
>
> This returns true if the server had demanded a password during the
> authentication phase. Aside from solving the immediate problem, this
> can be useful for regular clients such as psql: it could be applied to a
> failed connection object to decide whether to prompt for a password
> (replacing the current egregious hack of strcmp'ing the error message).
>
> 2. Make dblink close the connection and throw error if called by a
> non-superuser and PQconnectionUsedPassword returns false.
Attached patch implements this proposal, including documentation
changes. I'll work separately on the back-branch version.
Any comments/objections?
Joe
Index: contrib/dblink/dblink.c
===================================================================
RCS file: /opt/src/cvs/pgsql/contrib/dblink/dblink.c,v
retrieving revision 1.63
diff -c -r1.63 dblink.c
*** contrib/dblink/dblink.c 6 Apr 2007 04:21:41 -0000 1.63
--- contrib/dblink/dblink.c 7 Jul 2007 22:49:05 -0000
***************
*** 37,42 ****
--- 37,43 ----
#include "libpq-fe.h"
#include "fmgr.h"
#include "funcapi.h"
+ #include "miscadmin.h"
#include "access/heapam.h"
#include "access/tupdesc.h"
#include "catalog/namespace.h"
***************
*** 245,250 ****
--- 246,261 ----
errdetail("%s", msg)));
}
+ if (!superuser())
+ {
+ if (!PQconnectionUsedPassword(conn))
+ ereport(ERROR,
+ (errcode(ERRCODE_S_R_E_PROHIBITED_SQL_STATEMENT_ATTEMPTED),
+ errmsg("connection without password not allowed"),
+ errdetail("non-superuser cannot connect if server does not request password"),
+ errhint("target server authentication method must be changed")));
+ }
+
if (connname)
{
rconn->conn = conn;
Index: contrib/dblink/dblink.sql.in
===================================================================
RCS file: /opt/src/cvs/pgsql/contrib/dblink/dblink.sql.in,v
retrieving revision 1.11
diff -c -r1.11 dblink.sql.in
*** contrib/dblink/dblink.sql.in 2 Sep 2006 21:11:15 -0000 1.11
--- contrib/dblink/dblink.sql.in 8 Jul 2007 01:22:13 -0000
***************
*** 1,3 ****
--- 1,5 ----
+ -- dblink_connect now restricts non-superusers to password
+ -- authenticated connections
CREATE OR REPLACE FUNCTION dblink_connect (text)
RETURNS text
AS 'MODULE_PATHNAME','dblink_connect'
***************
*** 8,13 ****
--- 10,31 ----
AS 'MODULE_PATHNAME','dblink_connect'
LANGUAGE C STRICT;
+ -- dblink_connect_u allows non-superusers to use
+ -- non-password authenticated connections, but initially
+ -- privileges are revoked from public
+ CREATE OR REPLACE FUNCTION dblink_connect_u (text)
+ RETURNS text
+ AS 'MODULE_PATHNAME','dblink_connect'
+ LANGUAGE C STRICT SECURITY DEFINER;
+
+ CREATE OR REPLACE FUNCTION dblink_connect_u (text, text)
+ RETURNS text
+ AS 'MODULE_PATHNAME','dblink_connect'
+ LANGUAGE C STRICT SECURITY DEFINER;
+
+ REVOKE ALL ON FUNCTION dblink_connect_u (text) FROM public;
+ REVOKE ALL ON FUNCTION dblink_connect_u (text, text) FROM public;
+
CREATE OR REPLACE FUNCTION dblink_disconnect ()
RETURNS text
AS 'MODULE_PATHNAME','dblink_disconnect'
Index: contrib/dblink/doc/connection
===================================================================
RCS file: /opt/src/cvs/pgsql/contrib/dblink/doc/connection,v
retrieving revision 1.4
diff -c -r1.4 connection
*** contrib/dblink/doc/connection 11 Mar 2006 04:38:29 -0000 1.4
--- contrib/dblink/doc/connection 8 Jul 2007 01:51:07 -0000
***************
*** 27,32 ****
--- 27,38 ----
Returns status = "OK"
+ Notes
+
+ Only superusers may use dblink_connect to create non-password
+ authenticated connections. If non-superusers need this capability,
+ use dblink_connect_u instead.
+
Example usage
select dblink_connect('dbname=postgres');
***************
*** 44,49 ****
--- 50,95 ----
==================================================================
Name
+ dblink_connect_u -- Opens a persistent connection to a remote database
+
+ Synopsis
+
+ dblink_connect_u(text connstr)
+ dblink_connect_u(text connname, text connstr)
+
+ Inputs
+
+ connname
+ if 2 arguments are given, the first is used as a name for a persistent
+ connection
+
+ connstr
+
+ standard libpq format connection string,
+ e.g. "hostaddr=127.0.0.1 port=5432 dbname=mydb user=postgres password=mypasswd"
+
+ if only one argument is given, the connection is unnamed; only one unnamed
+ connection can exist at a time
+
+ Outputs
+
+ Returns status = "OK"
+
+ Notes
+
+ With dblink_connect_u, a non-superuser may connect to any database server
+ using any authentication method. If the authentication method specified
+ for a particular user does not require a password, impersonation and
+ therefore escalation of privileges may occur. For this reason,
+ dblink_connect_u is initially installed with all privileges revoked from
+ public. Privilege to these functions should be granted with care.
+
+ Example usage
+
+
+ ==================================================================
+ Name
+
dblink_disconnect -- Closes a persistent connection to a remote database
Synopsis
Index: doc/src/sgml/libpq.sgml
===================================================================
RCS file: /opt/src/cvs/pgsql/doc/src/sgml/libpq.sgml,v
retrieving revision 1.235
diff -c -r1.235 libpq.sgml
*** doc/src/sgml/libpq.sgml 30 Mar 2007 03:19:02 -0000 1.235
--- doc/src/sgml/libpq.sgml 8 Jul 2007 02:06:36 -0000
***************
*** 1059,1064 ****
--- 1059,1078 ----
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><function>PQconnectionUsedPassword</function><indexterm><primary>PQconnectionUsedPassword</></></term>
+ <listitem>
+ <para>
+ Returns true (1) if the connection authentication method
+ required a password to be supplied. Returns false (0)
+ otherwise.
+ <synopsis>
+ bool PQconnectionUsedPassword(const PGconn *conn);
+ </synopsis>
+ </para>
+ </listitem>
+ </varlistentry>
+
</variablelist>
</para>
Index: src/include/libpq/pqcomm.h
===================================================================
RCS file: /opt/src/cvs/pgsql/src/include/libpq/pqcomm.h,v
retrieving revision 1.102
diff -c -r1.102 pqcomm.h
*** src/include/libpq/pqcomm.h 5 Jan 2007 22:19:55 -0000 1.102
--- src/include/libpq/pqcomm.h 8 Jul 2007 01:16:29 -0000
***************
*** 156,161 ****
--- 156,162 ----
#define AUTH_REQ_CRYPT 4 /* crypt password */
#define AUTH_REQ_MD5 5 /* md5 password */
#define AUTH_REQ_SCM_CREDS 6 /* transfer SCM credentials */
+ #define AUTH_REQ_UNK 7 /* User has not yet attempted to authenticate */
typedef uint32 AuthRequest;
Index: src/interfaces/libpq/exports.txt
===================================================================
RCS file: /opt/src/cvs/pgsql/src/interfaces/libpq/exports.txt,v
retrieving revision 1.15
diff -c -r1.15 exports.txt
*** src/interfaces/libpq/exports.txt 3 Mar 2007 19:52:46 -0000 1.15
--- src/interfaces/libpq/exports.txt 7 Jul 2007 22:07:48 -0000
***************
*** 137,139 ****
--- 137,140 ----
PQsendDescribePrepared 135
PQsendDescribePortal 136
lo_truncate 137
+ PQconnectionUsedPassword 138
Index: src/interfaces/libpq/fe-connect.c
===================================================================
RCS file: /opt/src/cvs/pgsql/src/interfaces/libpq/fe-connect.c,v
retrieving revision 1.345
diff -c -r1.345 fe-connect.c
*** src/interfaces/libpq/fe-connect.c 8 Mar 2007 19:27:28 -0000 1.345
--- src/interfaces/libpq/fe-connect.c 7 Jul 2007 23:07:02 -0000
***************
*** 1641,1646 ****
--- 1641,1650 ----
return PGRES_POLLING_READING;
}
+ /* save the authentication request type */
+ if (conn->areq == AUTH_REQ_UNK)
+ conn->areq = areq;
+
/* Get the password salt if there is one. */
if (areq == AUTH_REQ_MD5)
{
***************
*** 1873,1878 ****
--- 1877,1883 ----
conn->std_strings = false; /* unless server says differently */
conn->verbosity = PQERRORS_DEFAULT;
conn->sock = -1;
+ conn->areq = AUTH_REQ_UNK;
#ifdef USE_SSL
conn->allow_ssl_try = true;
conn->wait_ssl_try = false;
***************
*** 3441,3446 ****
--- 3446,3462 ----
return status;
}
+ bool
+ PQconnectionUsedPassword(const PGconn *conn)
+ {
+ if (conn->areq == AUTH_REQ_MD5 ||
+ conn->areq == AUTH_REQ_CRYPT ||
+ conn->areq == AUTH_REQ_PASSWORD)
+ return true;
+ else
+ return false;
+ }
+
PGVerbosity
PQsetErrorVerbosity(PGconn *conn, PGVerbosity verbosity)
{
Index: src/interfaces/libpq/libpq-fe.h
===================================================================
RCS file: /opt/src/cvs/pgsql/src/interfaces/libpq/libpq-fe.h,v
retrieving revision 1.136
diff -c -r1.136 libpq-fe.h
*** src/interfaces/libpq/libpq-fe.h 3 Mar 2007 19:52:46 -0000 1.136
--- src/interfaces/libpq/libpq-fe.h 7 Jul 2007 21:55:12 -0000
***************
*** 23,32 ****
#include <stdio.h>
/*
! * postgres_ext.h defines the backend's externally visible types,
* such as Oid.
*/
#include "postgres_ext.h"
/* Application-visible enum types */
--- 23,33 ----
#include <stdio.h>
/*
! * defines the backend's externally visible types,
* such as Oid.
*/
#include "postgres_ext.h"
+ #include "postgres_fe.h"
/* Application-visible enum types */
***************
*** 265,270 ****
--- 266,272 ----
extern int PQbackendPID(const PGconn *conn);
extern int PQclientEncoding(const PGconn *conn);
extern int PQsetClientEncoding(PGconn *conn, const char *encoding);
+ extern bool PQconnectionUsedPassword(const PGconn *conn);
/* Get the OpenSSL structure associated with a connection. Returns NULL for
* unencrypted connections or if any other TLS library is in use. */
Index: src/interfaces/libpq/libpq-int.h
===================================================================
RCS file: /opt/src/cvs/pgsql/src/interfaces/libpq/libpq-int.h,v
retrieving revision 1.119
diff -c -r1.119 libpq-int.h
*** src/interfaces/libpq/libpq-int.h 3 Mar 2007 19:52:47 -0000 1.119
--- src/interfaces/libpq/libpq-int.h 7 Jul 2007 21:09:40 -0000
***************
*** 299,304 ****
--- 299,305 ----
SockAddr raddr; /* Remote address */
ProtocolVersion pversion; /* FE/BE protocol version in use */
int sversion; /* server version, e.g. 70401 for 7.4.1 */
+ AuthRequest areq; /* server demanded password during auth */
/* Transient state needed while establishing connection */
struct addrinfo *addrlist; /* list of possible backend addresses */
Joe Conway <mail@joeconway.com> writes:
> Attached patch implements this proposal, including documentation
> changes. I'll work separately on the back-branch version.
> Any comments/objections?
Looks OK in a fast scan, except that you are not following the message
style guidelines here:
> + ereport(ERROR,
> + (errcode(ERRCODE_S_R_E_PROHIBITED_SQL_STATEMENT_ATTEMPTED),
> + errmsg("connection without password not allowed"),
> + errdetail("non-superuser cannot connect if server does not request password"),
> + errhint("target server authentication method must be changed")));
The guidelines say errdetail and errhint messages should be full
sentences (with initial cap and trailing period). Also possibly
"Target server's auth..." would read better. Also, I'd personally not
leave the "is" out of the errmsg, though that part is surely a judgment
call. Or maybe it should be just errmsg("password is required")?
regards, tom lane
Tom Lane wrote:
> Joe Conway <mail@joeconway.com> writes:
>> Attached patch implements this proposal, including documentation
>> changes. I'll work separately on the back-branch version.
>
>> Any comments/objections?
>
> Looks OK in a fast scan, except that you are not following the message
> style guidelines here:
>
Thanks for the corrections. Final version committed to HEAD attached.
I'm working on the back branch solution now.
Joe
Index: contrib/dblink/dblink.c
===================================================================
RCS file: /cvsroot/pgsql/contrib/dblink/dblink.c,v
retrieving revision 1.63
diff -c -r1.63 dblink.c
*** contrib/dblink/dblink.c 6 Apr 2007 04:21:41 -0000 1.63
--- contrib/dblink/dblink.c 8 Jul 2007 16:52:53 -0000
***************
*** 37,42 ****
--- 37,43 ----
#include "libpq-fe.h"
#include "fmgr.h"
#include "funcapi.h"
+ #include "miscadmin.h"
#include "access/heapam.h"
#include "access/tupdesc.h"
#include "catalog/namespace.h"
***************
*** 245,250 ****
--- 246,267 ----
errdetail("%s", msg)));
}
+ if (!superuser())
+ {
+ if (!PQconnectionUsedPassword(conn))
+ {
+ PQfinish(conn);
+ if (rconn)
+ pfree(rconn);
+
+ ereport(ERROR,
+ (errcode(ERRCODE_S_R_E_PROHIBITED_SQL_STATEMENT_ATTEMPTED),
+ errmsg("password is required"),
+ errdetail("Non-superuser cannot connect if the server does not request a password."),
+ errhint("Target server's authentication method must be changed.")));
+ }
+ }
+
if (connname)
{
rconn->conn = conn;
Index: contrib/dblink/dblink.sql.in
===================================================================
RCS file: /cvsroot/pgsql/contrib/dblink/dblink.sql.in,v
retrieving revision 1.11
diff -c -r1.11 dblink.sql.in
*** contrib/dblink/dblink.sql.in 2 Sep 2006 21:11:15 -0000 1.11
--- contrib/dblink/dblink.sql.in 8 Jul 2007 16:52:53 -0000
***************
*** 1,3 ****
--- 1,5 ----
+ -- dblink_connect now restricts non-superusers to password
+ -- authenticated connections
CREATE OR REPLACE FUNCTION dblink_connect (text)
RETURNS text
AS 'MODULE_PATHNAME','dblink_connect'
***************
*** 8,13 ****
--- 10,31 ----
AS 'MODULE_PATHNAME','dblink_connect'
LANGUAGE C STRICT;
+ -- dblink_connect_u allows non-superusers to use
+ -- non-password authenticated connections, but initially
+ -- privileges are revoked from public
+ CREATE OR REPLACE FUNCTION dblink_connect_u (text)
+ RETURNS text
+ AS 'MODULE_PATHNAME','dblink_connect'
+ LANGUAGE C STRICT SECURITY DEFINER;
+
+ CREATE OR REPLACE FUNCTION dblink_connect_u (text, text)
+ RETURNS text
+ AS 'MODULE_PATHNAME','dblink_connect'
+ LANGUAGE C STRICT SECURITY DEFINER;
+
+ REVOKE ALL ON FUNCTION dblink_connect_u (text) FROM public;
+ REVOKE ALL ON FUNCTION dblink_connect_u (text, text) FROM public;
+
CREATE OR REPLACE FUNCTION dblink_disconnect ()
RETURNS text
AS 'MODULE_PATHNAME','dblink_disconnect'
Index: contrib/dblink/doc/connection
===================================================================
RCS file: /cvsroot/pgsql/contrib/dblink/doc/connection,v
retrieving revision 1.4
diff -c -r1.4 connection
*** contrib/dblink/doc/connection 11 Mar 2006 04:38:29 -0000 1.4
--- contrib/dblink/doc/connection 8 Jul 2007 16:52:53 -0000
***************
*** 27,32 ****
--- 27,38 ----
Returns status = "OK"
+ Notes
+
+ Only superusers may use dblink_connect to create non-password
+ authenticated connections. If non-superusers need this capability,
+ use dblink_connect_u instead.
+
Example usage
select dblink_connect('dbname=postgres');
***************
*** 44,49 ****
--- 50,95 ----
==================================================================
Name
+ dblink_connect_u -- Opens a persistent connection to a remote database
+
+ Synopsis
+
+ dblink_connect_u(text connstr)
+ dblink_connect_u(text connname, text connstr)
+
+ Inputs
+
+ connname
+ if 2 arguments are given, the first is used as a name for a persistent
+ connection
+
+ connstr
+
+ standard libpq format connection string,
+ e.g. "hostaddr=127.0.0.1 port=5432 dbname=mydb user=postgres password=mypasswd"
+
+ if only one argument is given, the connection is unnamed; only one unnamed
+ connection can exist at a time
+
+ Outputs
+
+ Returns status = "OK"
+
+ Notes
+
+ With dblink_connect_u, a non-superuser may connect to any database server
+ using any authentication method. If the authentication method specified
+ for a particular user does not require a password, impersonation and
+ therefore escalation of privileges may occur. For this reason,
+ dblink_connect_u is initially installed with all privileges revoked from
+ public. Privilege to these functions should be granted with care.
+
+ Example usage
+
+
+ ==================================================================
+ Name
+
dblink_disconnect -- Closes a persistent connection to a remote database
Synopsis
Index: doc/src/sgml/libpq.sgml
===================================================================
RCS file: /cvsroot/pgsql/doc/src/sgml/libpq.sgml,v
retrieving revision 1.235
diff -c -r1.235 libpq.sgml
*** doc/src/sgml/libpq.sgml 30 Mar 2007 03:19:02 -0000 1.235
--- doc/src/sgml/libpq.sgml 8 Jul 2007 16:53:03 -0000
***************
*** 1059,1064 ****
--- 1059,1078 ----
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><function>PQconnectionUsedPassword</function><indexterm><primary>PQconnectionUsedPassword</></></term>
+ <listitem>
+ <para>
+ Returns true (1) if the connection authentication method
+ required a password to be supplied. Returns false (0)
+ otherwise.
+ <synopsis>
+ bool PQconnectionUsedPassword(const PGconn *conn);
+ </synopsis>
+ </para>
+ </listitem>
+ </varlistentry>
+
</variablelist>
</para>
Index: src/include/libpq/pqcomm.h
===================================================================
RCS file: /cvsroot/pgsql/src/include/libpq/pqcomm.h,v
retrieving revision 1.102
diff -c -r1.102 pqcomm.h
*** src/include/libpq/pqcomm.h 5 Jan 2007 22:19:55 -0000 1.102
--- src/include/libpq/pqcomm.h 8 Jul 2007 16:53:05 -0000
***************
*** 156,161 ****
--- 156,162 ----
#define AUTH_REQ_CRYPT 4 /* crypt password */
#define AUTH_REQ_MD5 5 /* md5 password */
#define AUTH_REQ_SCM_CREDS 6 /* transfer SCM credentials */
+ #define AUTH_REQ_UNK 7 /* User has not yet attempted to authenticate */
typedef uint32 AuthRequest;
Index: src/interfaces/libpq/exports.txt
===================================================================
RCS file: /cvsroot/pgsql/src/interfaces/libpq/exports.txt,v
retrieving revision 1.15
diff -c -r1.15 exports.txt
*** src/interfaces/libpq/exports.txt 3 Mar 2007 19:52:46 -0000 1.15
--- src/interfaces/libpq/exports.txt 8 Jul 2007 16:53:05 -0000
***************
*** 137,139 ****
--- 137,140 ----
PQsendDescribePrepared 135
PQsendDescribePortal 136
lo_truncate 137
+ PQconnectionUsedPassword 138
Index: src/interfaces/libpq/fe-connect.c
===================================================================
RCS file: /cvsroot/pgsql/src/interfaces/libpq/fe-connect.c,v
retrieving revision 1.345
diff -c -r1.345 fe-connect.c
*** src/interfaces/libpq/fe-connect.c 8 Mar 2007 19:27:28 -0000 1.345
--- src/interfaces/libpq/fe-connect.c 8 Jul 2007 16:53:05 -0000
***************
*** 1641,1646 ****
--- 1641,1650 ----
return PGRES_POLLING_READING;
}
+ /* save the authentication request type */
+ if (conn->areq == AUTH_REQ_UNK)
+ conn->areq = areq;
+
/* Get the password salt if there is one. */
if (areq == AUTH_REQ_MD5)
{
***************
*** 1873,1878 ****
--- 1877,1883 ----
conn->std_strings = false; /* unless server says differently */
conn->verbosity = PQERRORS_DEFAULT;
conn->sock = -1;
+ conn->areq = AUTH_REQ_UNK;
#ifdef USE_SSL
conn->allow_ssl_try = true;
conn->wait_ssl_try = false;
***************
*** 3441,3446 ****
--- 3446,3462 ----
return status;
}
+ bool
+ PQconnectionUsedPassword(const PGconn *conn)
+ {
+ if (conn->areq == AUTH_REQ_MD5 ||
+ conn->areq == AUTH_REQ_CRYPT ||
+ conn->areq == AUTH_REQ_PASSWORD)
+ return true;
+ else
+ return false;
+ }
+
PGVerbosity
PQsetErrorVerbosity(PGconn *conn, PGVerbosity verbosity)
{
Index: src/interfaces/libpq/libpq-fe.h
===================================================================
RCS file: /cvsroot/pgsql/src/interfaces/libpq/libpq-fe.h,v
retrieving revision 1.136
diff -c -r1.136 libpq-fe.h
*** src/interfaces/libpq/libpq-fe.h 3 Mar 2007 19:52:46 -0000 1.136
--- src/interfaces/libpq/libpq-fe.h 8 Jul 2007 16:53:05 -0000
***************
*** 23,32 ****
#include <stdio.h>
/*
! * postgres_ext.h defines the backend's externally visible types,
* such as Oid.
*/
#include "postgres_ext.h"
/* Application-visible enum types */
--- 23,33 ----
#include <stdio.h>
/*
! * defines the backend's externally visible types,
* such as Oid.
*/
#include "postgres_ext.h"
+ #include "postgres_fe.h"
/* Application-visible enum types */
***************
*** 265,270 ****
--- 266,272 ----
extern int PQbackendPID(const PGconn *conn);
extern int PQclientEncoding(const PGconn *conn);
extern int PQsetClientEncoding(PGconn *conn, const char *encoding);
+ extern bool PQconnectionUsedPassword(const PGconn *conn);
/* Get the OpenSSL structure associated with a connection. Returns NULL for
* unencrypted connections or if any other TLS library is in use. */
Index: src/interfaces/libpq/libpq-int.h
===================================================================
RCS file: /cvsroot/pgsql/src/interfaces/libpq/libpq-int.h,v
retrieving revision 1.119
diff -c -r1.119 libpq-int.h
*** src/interfaces/libpq/libpq-int.h 3 Mar 2007 19:52:47 -0000 1.119
--- src/interfaces/libpq/libpq-int.h 8 Jul 2007 16:53:05 -0000
***************
*** 299,304 ****
--- 299,305 ----
SockAddr raddr; /* Remote address */
ProtocolVersion pversion; /* FE/BE protocol version in use */
int sversion; /* server version, e.g. 70401 for 7.4.1 */
+ AuthRequest areq; /* server demanded password during auth */
/* Transient state needed while establishing connection */
struct addrinfo *addrlist; /* list of possible backend addresses */
Joe Conway wrote:
> I'm working on the back branch solution now.
>
Attached is for back-patching. I left in the SECURITY DEFINER functions
and dblink doc changes -- even though they may not do existing
installations much good, I think there will still be enough new 8.2.x
installations for a while to make it worthwhile. And even for older
branches, at least it gives us something handy to point to as a
workaround when people complain we broke their apps.
If there are no objections I'll commit this later today.
Thanks,
Joe
Index: contrib/dblink/dblink.c
===================================================================
RCS file: /cvsroot/pgsql/contrib/dblink/dblink.c,v
retrieving revision 1.60
diff -c -r1.60 dblink.c
*** contrib/dblink/dblink.c 19 Oct 2006 19:53:03 -0000 1.60
--- contrib/dblink/dblink.c 8 Jul 2007 17:38:00 -0000
***************
*** 37,42 ****
--- 37,43 ----
#include "libpq-fe.h"
#include "fmgr.h"
#include "funcapi.h"
+ #include "miscadmin.h"
#include "access/heapam.h"
#include "access/tupdesc.h"
#include "catalog/namespace.h"
***************
*** 89,94 ****
--- 90,96 ----
static HeapTuple get_tuple_of_interest(Oid relid, int2vector *pkattnums, int16 pknumatts, char **src_pkattvals);
static Oid get_relid_from_relname(text *relname_text);
static char *generate_relation_name(Oid relid);
+ static char *connstr_strip_password(const char *connstr);
/* Global */
static remoteConn *pconn = NULL;
***************
*** 228,233 ****
--- 230,257 ----
if (connname)
rconn = (remoteConn *) palloc(sizeof(remoteConn));
+
+ /* for non-superusers, check that server requires a password */
+ if (!superuser())
+ {
+ /* this attempt must fail */
+ conn = PQconnectdb(connstr_strip_password(connstr));
+
+ if (PQstatus(conn) == CONNECTION_OK)
+ {
+ PQfinish(conn);
+ if (rconn)
+ pfree(rconn);
+
+ ereport(ERROR,
+ (errcode(ERRCODE_S_R_E_PROHIBITED_SQL_STATEMENT_ATTEMPTED),
+ errmsg("password is required"),
+ errdetail("Non-superuser cannot connect if the server does not request a password."),
+ errhint("Target server's authentication method must be changed.")));
+ }
+ else
+ PQfinish(conn);
+ }
conn = PQconnectdb(connstr);
MemoryContextSwitchTo(oldcontext);
***************
*** 2273,2275 ****
--- 2297,2430 ----
errmsg("undefined connection name")));
}
+
+
+ /*
+ * Modified version of conninfo_parse() from fe-connect.c
+ * Used to remove any password from the connection string
+ * in order to test whether the server auth method will
+ * require it.
+ */
+ static char *
+ connstr_strip_password(const char *connstr)
+ {
+ char *pname;
+ char *pval;
+ char *buf;
+ char *cp;
+ char *cp2;
+ StringInfoData result;
+
+ /* initialize return value */
+ initStringInfo(&result);
+
+ /* Need a modifiable copy of the input string */
+ buf = pstrdup(connstr);
+ cp = buf;
+
+ while (*cp)
+ {
+ /* Skip blanks before the parameter name */
+ if (isspace((unsigned char) *cp))
+ {
+ cp++;
+ continue;
+ }
+
+ /* Get the parameter name */
+ pname = cp;
+ while (*cp)
+ {
+ if (*cp == '=')
+ break;
+ if (isspace((unsigned char) *cp))
+ {
+ *cp++ = '\0';
+ while (*cp)
+ {
+ if (!isspace((unsigned char) *cp))
+ break;
+ cp++;
+ }
+ break;
+ }
+ cp++;
+ }
+
+ /* Check that there is a following '=' */
+ if (*cp != '=')
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("missing \"=\" after \"%s\" in connection string", pname)));
+ *cp++ = '\0';
+
+ /* Skip blanks after the '=' */
+ while (*cp)
+ {
+ if (!isspace((unsigned char) *cp))
+ break;
+ cp++;
+ }
+
+ /* Get the parameter value */
+ pval = cp;
+
+ if (*cp != '\'')
+ {
+ cp2 = pval;
+ while (*cp)
+ {
+ if (isspace((unsigned char) *cp))
+ {
+ *cp++ = '\0';
+ break;
+ }
+ if (*cp == '\\')
+ {
+ cp++;
+ if (*cp != '\0')
+ *cp2++ = *cp++;
+ }
+ else
+ *cp2++ = *cp++;
+ }
+ *cp2 = '\0';
+ }
+ else
+ {
+ cp2 = pval;
+ cp++;
+ for (;;)
+ {
+ if (*cp == '\0')
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("unterminated quoted string in connection string")));
+ if (*cp == '\\')
+ {
+ cp++;
+ if (*cp != '\0')
+ *cp2++ = *cp++;
+ continue;
+ }
+ if (*cp == '\'')
+ {
+ *cp2 = '\0';
+ cp++;
+ break;
+ }
+ *cp2++ = *cp++;
+ }
+ }
+
+ /*
+ * Now we have the name and the value. If it is not a password,
+ * append to the return connstr.
+ */
+ if (strcmp("password", pname) != 0)
+ /* append the value */
+ appendStringInfo(&result, " %s='%s'", pname, pval);
+ }
+
+ return result.data;
+ }
Index: contrib/dblink/dblink.sql.in
===================================================================
RCS file: /cvsroot/pgsql/contrib/dblink/dblink.sql.in,v
retrieving revision 1.11
diff -c -r1.11 dblink.sql.in
*** contrib/dblink/dblink.sql.in 2 Sep 2006 21:11:15 -0000 1.11
--- contrib/dblink/dblink.sql.in 8 Jul 2007 17:38:00 -0000
***************
*** 1,3 ****
--- 1,5 ----
+ -- dblink_connect now restricts non-superusers to password
+ -- authenticated connections
CREATE OR REPLACE FUNCTION dblink_connect (text)
RETURNS text
AS 'MODULE_PATHNAME','dblink_connect'
***************
*** 8,13 ****
--- 10,31 ----
AS 'MODULE_PATHNAME','dblink_connect'
LANGUAGE C STRICT;
+ -- dblink_connect_u allows non-superusers to use
+ -- non-password authenticated connections, but initially
+ -- privileges are revoked from public
+ CREATE OR REPLACE FUNCTION dblink_connect_u (text)
+ RETURNS text
+ AS 'MODULE_PATHNAME','dblink_connect'
+ LANGUAGE C STRICT SECURITY DEFINER;
+
+ CREATE OR REPLACE FUNCTION dblink_connect_u (text, text)
+ RETURNS text
+ AS 'MODULE_PATHNAME','dblink_connect'
+ LANGUAGE C STRICT SECURITY DEFINER;
+
+ REVOKE ALL ON FUNCTION dblink_connect_u (text) FROM public;
+ REVOKE ALL ON FUNCTION dblink_connect_u (text, text) FROM public;
+
CREATE OR REPLACE FUNCTION dblink_disconnect ()
RETURNS text
AS 'MODULE_PATHNAME','dblink_disconnect'
Index: contrib/dblink/doc/connection
===================================================================
RCS file: /cvsroot/pgsql/contrib/dblink/doc/connection,v
retrieving revision 1.4
diff -c -r1.4 connection
*** contrib/dblink/doc/connection 11 Mar 2006 04:38:29 -0000 1.4
--- contrib/dblink/doc/connection 8 Jul 2007 17:38:00 -0000
***************
*** 27,32 ****
--- 27,38 ----
Returns status = "OK"
+ Notes
+
+ Only superusers may use dblink_connect to create non-password
+ authenticated connections. If non-superusers need this capability,
+ use dblink_connect_u instead.
+
Example usage
select dblink_connect('dbname=postgres');
***************
*** 44,49 ****
--- 50,95 ----
==================================================================
Name
+ dblink_connect_u -- Opens a persistent connection to a remote database
+
+ Synopsis
+
+ dblink_connect_u(text connstr)
+ dblink_connect_u(text connname, text connstr)
+
+ Inputs
+
+ connname
+ if 2 arguments are given, the first is used as a name for a persistent
+ connection
+
+ connstr
+
+ standard libpq format connection string,
+ e.g. "hostaddr=127.0.0.1 port=5432 dbname=mydb user=postgres password=mypasswd"
+
+ if only one argument is given, the connection is unnamed; only one unnamed
+ connection can exist at a time
+
+ Outputs
+
+ Returns status = "OK"
+
+ Notes
+
+ With dblink_connect_u, a non-superuser may connect to any database server
+ using any authentication method. If the authentication method specified
+ for a particular user does not require a password, impersonation and
+ therefore escalation of privileges may occur. For this reason,
+ dblink_connect_u is initially installed with all privileges revoked from
+ public. Privilege to these functions should be granted with care.
+
+ Example usage
+
+
+ ==================================================================
+ Name
+
dblink_disconnect -- Closes a persistent connection to a remote database
Synopsis
"Joe Conway" <mail@joeconway.com> writes: > If there are no objections I'll commit this later today. My objection is that I think we should still revoke access for non-superuser by default. The patch makes granting execute reasonable for most users but nonetheless it shouldn't be the default. Being able to connect to a postgres server shouldn't mean being able to open tcp connections *from* that server to arbitrary other host/ports. Consider for example that it would allow a user to perform a port scan from inside your network to see what internal services are running. -- Gregory Stark EnterpriseDB http://www.enterprisedb.com
* Gregory Stark (stark@enterprisedb.com) wrote:
> "Joe Conway" <mail@joeconway.com> writes:
> > If there are no objections I'll commit this later today.
>
> My objection is that I think we should still revoke access for non-superuser
> by default. The patch makes granting execute reasonable for most users but
> nonetheless it shouldn't be the default.
>
> Being able to connect to a postgres server shouldn't mean being able to open
> tcp connections *from* that server to arbitrary other host/ports. Consider for
> example that it would allow a user to perform a port scan from inside your
> network to see what internal services are running.
I'm in agreement with Greg. It's a poor idea, overall, to allow users
to initiate TCP connections from the backend. That should be a
superuser-only ability and should require security definer functions
with appropriate safe-guards (which would be site-specific) to be
created by the end admins.
Thanks,
Stephen
Attachment
"Stephen Frost" <sfrost@snowman.net> writes: >> Being able to connect to a postgres server shouldn't mean being able to open >> tcp connections *from* that server to arbitrary other host/ports. Consider for >> example that it would allow a user to perform a port scan from inside your >> network to see what internal services are running. > > I'm in agreement with Greg. It's a poor idea, overall, to allow users > to initiate TCP connections from the backend. That should be a > superuser-only ability and should require security definer functions > with appropriate safe-guards (which would be site-specific) to be > created by the end admins. I think we rejected that idea as making it super-user-only would cause problems for existing installs. It would also take away the possibility for users to use the compromise policy the patch implements which is the most useful policy for many installs where the users have shell accounts anyways. I was only suggesting that we add the patch *and* revoke execute bits for public in the dblink install script. Existing installs would just get the benefit of the patch and continue to function. Actually from a security point of view revoking public execute is pretty much the same as making a function super-user-only. The only difference is how much of a hassle it is for the super-user to grant access. Perhaps we should reconsider whether any of the other super-user-only functions should be simply not executable by default but work normally if granted. -- Gregory Stark EnterpriseDB http://www.enterprisedb.com
* Gregory Stark (stark@enterprisedb.com) wrote:
> Actually from a security point of view revoking public execute is pretty much
> the same as making a function super-user-only. The only difference is how much
> of a hassle it is for the super-user to grant access. Perhaps we should
> reconsider whether any of the other super-user-only functions should be simply
> not executable by default but work normally if granted.
Revoking public execute on it by default would definitely make me
happier. I could be swayed either way on the explicit super-user check
in the function itself. In the general case, imv we should at least
attempt to consider the risk involved in improper handling of the
permissions around super-user-only functions. Higher risk implies an
extra check in the code to force use of SECURITY DEFINER functions to
work around it, in an attempt to impart the severity of the risk.
Thinking about it a bit more, I'd honestly like to see the check there
for dblink(). That's not entirely fair of me though I suppose, I really
don't feel comfortable with dblink() to begin with and don't expect I'll
ever use it. :)
Thanks,
Stephen
Attachment
Gregory Stark <stark@enterprisedb.com> writes:
> My objection is that I think we should still revoke access for non-superuser
> by default. The patch makes granting execute reasonable for most users but
> nonetheless it shouldn't be the default.
> Being able to connect to a postgres server shouldn't mean being able to open
> tcp connections *from* that server to arbitrary other host/ports.
You forget that dblink isn't even installed by default. I could see
having some more verbiage in the documentation explaining these possible
security risks, but making it unusable is an overreaction.
regards, tom lane
Tom Lane wrote: > Gregory Stark <stark@enterprisedb.com> writes: >> My objection is that I think we should still revoke access for non-superuser >> by default. The patch makes granting execute reasonable for most users but >> nonetheless it shouldn't be the default. > >> Being able to connect to a postgres server shouldn't mean being able to open >> tcp connections *from* that server to arbitrary other host/ports. > > You forget that dblink isn't even installed by default. I could see > having some more verbiage in the documentation explaining these possible > security risks, but making it unusable is an overreaction. > Agreed. If you are going to argue that we should revoke access for non-superusers by default for dblink, then you are also arguing that we should do the same for every function created with any untrusted language. E.g. as I pointed out to Robert last week, just because an unsafe function is created in plperlu, it doesn't mean that a non-superuser can't run it immediately after it is created. There is no difference. It is incumbent upon the DBA/superuser to be careful _whenever_ they create any function using an untrusted language. Joe
"Tom Lane" <tgl@sss.pgh.pa.us> writes: > Gregory Stark <stark@enterprisedb.com> writes: >> My objection is that I think we should still revoke access for non-superuser >> by default. The patch makes granting execute reasonable for most users but >> nonetheless it shouldn't be the default. > >> Being able to connect to a postgres server shouldn't mean being able to open >> tcp connections *from* that server to arbitrary other host/ports. > > You forget that dblink isn't even installed by default. I could see > having some more verbiage in the documentation explaining these possible > security risks, but making it unusable is an overreaction. It's not like granting execute privilege is a particularly complex or obscure command. It's a better policy that packages be designed so that merely installing them doesn't have direct security implications. That way sysadmins can install a wide range of packages to satisfy user demand and count on security infrastructure to control security. Consider a scenario like "package <x> uses dblink". Sysadmin follows instructions for package <x> and installs dblink. Now package <x>'s documentation isn't going to explain the second-order effects and discuss restricting who has access to dblink. The sysadmin has no particular interest in using dblink himself and probably will never read any dblink docs. On the other hand if dblink can't be executed by random users then when package x tells you to install dblink it will also tell you to grant access to the user that package runs as. The sysadmin can consider which users that should be. -- Gregory Stark EnterpriseDB http://www.enterprisedb.com
Gregory Stark wrote: > Consider a scenario like "package <x> uses dblink". Sysadmin follows > instructions for package <x> and installs dblink. Now package <x>'s > documentation isn't going to explain the second-order effects and discuss > restricting who has access to dblink. The sysadmin has no particular interest > in using dblink himself and probably will never read any dblink docs. > > On the other hand if dblink can't be executed by random users then when > package x tells you to install dblink it will also tell you to grant access to > the user that package runs as. The sysadmin can consider which users that > should be. > See my last email... Consider a scenario like "package <x> uses <arbitrary function y in an untrusted language z>". Exact same concerns arise. Joe
* Joe Conway (mail@joeconway.com) wrote:
> If you are going to argue that we should revoke access for non-superusers
> by default for dblink, then you are also arguing that we should do the same
> for every function created with any untrusted language.
Uh, no, one doesn't imply the other. It doesn't follow that because a
specific, known insecure, function shouldn't be available to all users
immediately that quite probably safe/secure functions (even though
they're written in an untrusted language- what has that got to do with
anything?) also shouldn't be.
> E.g. as I pointed out to Robert last week, just because an unsafe function
> is created in plperlu, it doesn't mean that a non-superuser can't run it
> immediately after it is created. There is no difference. It is incumbent
> upon the DBA/superuser to be careful _whenever_ they create any function
> using an untrusted language.
This isn't a case of the DBA/superuser writing the function. It's being
provided by a package. It's also *inherently* insecure and isn't just a
matter of "being careful". You can create functions in an untrusted
language carefully enough to allow it to be called by other users. It
is simply prudent for the package provider to disable insecure functions
by default.
Thanks,
Stephen
Attachment
* Joe Conway (mail@joeconway.com) wrote:
> Consider a scenario like "package <x> uses <arbitrary function y in an
> untrusted language z>". Exact same concerns arise.
No, it doesn't... Said arbitrary function in y, in untrusted language
z, could be perfectly safe for users to call. Being written in an
untrusted language has got next to nothing to do with the security
implications of a particular function. It depends entirely on what the
function is *doing*, not what language it's written in.
Thanks,
Stephen
Attachment
"Joe Conway" <mail@joeconway.com> writes: > Agreed. > > If you are going to argue that we should revoke access for non-superusers by > default for dblink, then you are also arguing that we should do the same for > every function created with any untrusted language. Only if the function created uses some facility of the untrusted language that we wouldn't want any arbitrary user to have access to without explicitly granting it. Privilege escalations like this are a serious problem. I am pretty confident that if this is left like this it will come up again in the future by someone else reporting it as a security hole again. > E.g. as I pointed out to Robert last week, just because an unsafe function is > created in plperlu, it doesn't mean that a non-superuser can't run it > immediately after it is created. There is no difference. It is incumbent upon > the DBA/superuser to be careful _whenever_ they create any function using an > untrusted language. And the author of the script here is not being careful in this respect. The sysadmin isn't the one writing the create function statement. -- Gregory Stark EnterpriseDB http://www.enterprisedb.com
Stephen Frost wrote:
> * Joe Conway (mail@joeconway.com) wrote:
>> Consider a scenario like "package <x> uses <arbitrary function y in an
>> untrusted language z>". Exact same concerns arise.
>
> No, it doesn't... Said arbitrary function in y, in untrusted language
> z, could be perfectly safe for users to call.
^^^^^
*Could* be. But we just said that the admin was not interested in
reading the documentation, and has no idea if it *is* safe. And, it very
well might not be safe. We have no way to know in advance because the
language is untrusted.
> Being written in an untrusted language has got next to nothing to do with the security
> implications of a particular function. It depends entirely on what the
> function is *doing*, not what language it's written in.
Sure it matters. A function written in a trusted language is known to be
safe, a priori. A function written in an untrusted language has no such
guarantees, and therefore has to be assumed unsafe unless carefully
proved otherwise.
Joe
* Joe Conway (mail@joeconway.com) wrote:
> Stephen Frost wrote:
>> No, it doesn't... Said arbitrary function in y, in untrusted language
>> z, could be perfectly safe for users to call.
> ^^^^^
> *Could* be. But we just said that the admin was not interested in reading
> the documentation, and has no idea if it *is* safe. And, it very well might
> not be safe. We have no way to know in advance because the language is
> untrusted.
If it's not safe then it shouldn't be enabled by default. That's pretty
much the point. If something is known to be unsafe for users to have
access to then it should be disabled by default.
>> Being written in an untrusted language has got next to nothing to do with
>> the security
>> implications of a particular function. It depends entirely on what the
>> function is *doing*, not what language it's written in.
>
> Sure it matters. A function written in a trusted language is known to be
> safe, a priori. A function written in an untrusted language has no such
> guarantees, and therefore has to be assumed unsafe unless carefully proved
> otherwise.
I see.. So all the functions in untrusted languages that come with PG
initially should be checked over by every sysadmin when installing PG
every time... And the same for PostGIS, and all of the PL's that use
untrusted languages?
On my pretty modest install that's 2,206 functions. For some reason I
see something of a difference between 'generate_series' and 'dblink' in
terms of security and which one I'm comfortable having enabled by
default and which one I'm not.
Thanks,
Stephen
Attachment
"Joe Conway" <mail@joeconway.com> writes: > See my last email... > > Consider a scenario like "package <x> uses <arbitrary function y in an > untrusted language z>". Exact same concerns arise. Well arbitrary function may or may not actually do anything that needs to be restricted. If it does then yes the same concerns arise and the same conclusion reached. That users should be granted permission to execute it based on local policies. Certainly granting execute permission to public by default is a bad start in that regard. -- Gregory Stark EnterpriseDB http://www.enterprisedb.com
* Gregory Stark (stark@enterprisedb.com) wrote:
> "Joe Conway" <mail@joeconway.com> writes:
> > Consider a scenario like "package <x> uses <arbitrary function y in an
> > untrusted language z>". Exact same concerns arise.
>
> Well arbitrary function may or may not actually do anything that needs to be
> restricted.
>
> If it does then yes the same concerns arise and the same conclusion reached.
> That users should be granted permission to execute it based on local policies.
> Certainly granting execute permission to public by default is a bad start in
> that regard.
Agreed, and regardless of the sysadmin doing x, y, or z, or what some
other package might be doing with untrusted languages, what matters here
is what we're doing and the functions we're providing. Best practice is
to disable functions by default which aren't safe & secure for users to
have access to.
If you know of any others in anything we're distributing, please point
them out. If there are some in related projects, point those out and
ask those projects to be careful with them and encourage them to disable
them by default.
Thanks,
Stephen
Attachment
Stephen Frost wrote: > * Joe Conway (mail@joeconway.com) wrote: >> Sure it matters. A function written in a trusted language is known to be >> safe, a priori. A function written in an untrusted language has no such >> guarantees, and therefore has to be assumed unsafe unless carefully proved >> otherwise. > > I see.. So all the functions in untrusted languages that come with PG > initially should be checked over by every sysadmin when installing PG > every time... And the same for PostGIS, and all of the PL's that use > untrusted languages? There are none installed by default -- that's the point. > On my pretty modest install that's 2,206 functions. For some reason I > see something of a difference between 'generate_series' and 'dblink' in > terms of security and which one I'm comfortable having enabled by > default and which one I'm not. generate_series is a built in function. We aren't discussing those. Joe
* Joe Conway (mail@joeconway.com) wrote:
> Stephen Frost wrote:
>> I see.. So all the functions in untrusted languages that come with PG
>> initially should be checked over by every sysadmin when installing PG
>> every time... And the same for PostGIS, and all of the PL's that use
>> untrusted languages?
>
> There are none installed by default -- that's the point.
Uhh... None what? Functions in untrusted languages? That's certainly
not the case, there's a whole slew of them, from boolin to
generate_series and beyond. They're available to regular users, even!
Or do you mean that there are no known-insecure functions which are
installed and enabled for users to use by default? I'd have to agree
with you there in general, would kind of like to keep it that way too.
Perhaps you're referring to PLs, but then, I thought trusted PLs were
safe, but they're written using untrusted languages! Are they safe, or
not? Safe to use, but not safe to install?
>> On my pretty modest install that's 2,206 functions. For some reason I
>> see something of a difference between 'generate_series' and 'dblink' in
>> terms of security and which one I'm comfortable having enabled by
>> default and which one I'm not.
>
> generate_series is a built in function. We aren't discussing those.
Uh, it's written in an untrusted language, isn't it? Us poor sysadmins
are supposed to review all of them before letting users have access to
them, aren't we? Now I'm just completely confused as to the distinction
you're making here. Are functions in untrusted languages are problem,
or not?
Thanks,
Stephen
Attachment
"Joe Conway" <mail@joeconway.com> writes: > Stephen Frost wrote: > >> I see.. So all the functions in untrusted languages that come with PG >> initially should be checked over by every sysadmin when installing PG >> every time... And the same for PostGIS, and all of the PL's that use >> untrusted languages? > > There are none installed by default -- that's the point. That's not a scalable approach. If you treat the mere installation of a package as potentially making significant changes to the security model then it makes a joke of our whole security infrastructure. We could just have said you shouldn't create functions that you don't want public to have access to. He has a point too. If a sysadmin has to audit the security implications of every package when installing it that makes installing PostGIS a pretty daunting task. If on the other hand packages make the promise that they don't change the security model by merely being installed then programmers or dependent modules can request packages and dbas can be confident that installing them won't introduce security holes. Isn't that a property software should have even if it's just an add-on module? -- Gregory Stark EnterpriseDB http://www.enterprisedb.com
Stephen Frost wrote: > * Joe Conway (mail@joeconway.com) wrote: >> There are none installed by default -- that's the point. > > Uhh... None what? Functions in untrusted languages? That's certainly > not the case, there's a whole slew of them, from boolin to > generate_series and beyond. They're available to regular users, even! Get serious. Internal functions are specifically designed and maintained to be safe within the confines of the database security model. We are discussing extensions to the core, all of which must be installed by choice, by a superuser. Joe
* Joe Conway (mail@joeconway.com) wrote:
> Get serious. Internal functions are specifically designed and maintained to
> be safe within the confines of the database security model. We are
> discussing extensions to the core, all of which must be installed by
> choice, by a superuser.
Extensions should also be designed and maintained to be safe within the
confines of the database security model. Having to be installed by a
superuser doesn't change that. I would consider it a serious risk which
would need to be fixed if, for example, a function in PostGIS was found
to allow priviledge escalation by a user. Claiming it was installed "by
choice, by a superuser" doesn't change that.
It's about as good as saying "Well, an admin had to install PostgreSQL
on the system, by choice, and therefore we don't need to worry about PG
allowing someone remote shell access to the system".
Thanks,
Stephen
Attachment
"Joe Conway" <mail@joeconway.com> writes: > Stephen Frost wrote: >> * Joe Conway (mail@joeconway.com) wrote: >>> There are none installed by default -- that's the point. >> >> Uhh... None what? Functions in untrusted languages? That's certainly >> not the case, there's a whole slew of them, from boolin to >> generate_series and beyond. They're available to regular users, even! > > Get serious. Internal functions are specifically designed and maintained to be > safe within the confines of the database security model. We are discussing > extensions to the core, all of which must be installed by choice, by a superuser. That doesn't mean they shouldn't be concerned with security. Consider dblink as an entirely separate product which depends on Postgres the way Postgres depends on the OS. We discussing how the dblink software should behave when installed with *its* default configuration. When *Postgres is installed on Unix* it modifies the Unix security model allowing internet users to connect and execute SQL queries. But it is configured to be "secure by default" by requiring explicit authorization for users who should be allowed to connect. Merely installing *Postgres on Unix* doesn't allow arbitrary internet users to use your machine to store data. Likewise when *dblink is installed on Postgres* it modifies the Postgres security model to allow exterior users to create tcp connections originating from your host. This is something Postgres and indeed Unix in general forbid. It should be configured so that when *dblink* is installed it is configured to be "secure by default" by requiring explicit authorization for users who should be allowed to form connections. Merely installing *dblink on Postgres* shouldn't allow arbitrary Postgres users to use your machine to launch attacks. -- Gregory Stark EnterpriseDB http://www.enterprisedb.com
Stephen Frost wrote: > It's about as good as saying "Well, an admin had to install PostgreSQL > on the system, by choice, and therefore we don't need to worry about PG > allowing someone remote shell access to the system". That's a ridiculous assertion -- I said nothing of the sort. Joe
Gregory Stark wrote: > "Joe Conway" <mail@joeconway.com> writes: > >> Stephen Frost wrote: >>> * Joe Conway (mail@joeconway.com) wrote: >>>> There are none installed by default -- that's the point. >>> Uhh... None what? Functions in untrusted languages? That's certainly >>> not the case, there's a whole slew of them, from boolin to >>> generate_series and beyond. They're available to regular users, even! >> Get serious. Internal functions are specifically designed and maintained to be >> safe within the confines of the database security model. We are discussing >> extensions to the core, all of which must be installed by choice, by a superuser. > > That doesn't mean they shouldn't be concerned with security. Of course they should be concerned with security. Its the job of the DBA/superuser to be concerned with security, and therefore they ought to know what the implications are for what they install, before they install it. > Consider dblink as an entirely separate product which depends on Postgres the > way Postgres depends on the OS. We discussing how the dblink software should > behave when installed with *its* default configuration. And the the security escalation scenario, which is a consequence of the DBA's inadequate understanding of the security setup of the system in the first place, has now been closed for them. Granted, this was an easy enough mistake to make, so I agree that what we've done to close it is a good thing. But if you know of a security risk related to using libpq with a password authenticated connection, let's hear it. Joe
Joe Conway <mail@joeconway.com> writes:
> But if you know of a security risk related to using libpq
> with a password authenticated connection, let's hear it.
As near as I can tell, the argument is that dblink might be used to send
connection-request packets to random addresses. Now this is only a
security issue if the attacker could not have reached such an address
directly; otherwise he might as well send the packet himself (and have a
lot more control over its content). So I guess the scenario is that
you're running your database on your firewall machine, where it is
accessible from outside your net but also can reach addresses inside.
And you're letting untrustworthy outside people log into the database.
And you put dblink on it for them to use. And even then, the amount of
damage they could do seems pretty limited due to lack of control over
the packet contents.
To me this scenario is too far-fetched to justify sacrificing
convenience and backwards compatibility. It should be sufficient to add
some paragraphs about security considerations to the dblink docs.
regards, tom lane
"Tom Lane" <tgl@sss.pgh.pa.us> writes: > So I guess the scenario is that you're running your database on your > firewall machine, where it is accessible from outside your net but also can > reach addresses inside. Well typically the database is behind the firewall but an application is outside the firewall and has permission to contact the database. That's a typical arrangement for a database-backed web application, for example. > And you're letting untrustworthy outside people log into the database. Or they've found a security hole in the application which allows sql-injection. That's why I referred to it as a privilege escalation. Being able to inject sql means your data is compromised but it shouldn't allow them to then mount attacks on further systems. > And you put dblink on it for them to use. Well consider a hosting provider which installs it at the request of a client or has a standard set of extensions they want to include in a hosted Postgres setup. > And even then, the amount of damage they could do seems pretty limited due > to lack of control over the packet contents. Mounting a port scan on an internal network is fairly nasty. They could discover that you're running some insecure service which can then be exploited. > To me this scenario is too far-fetched to justify sacrificing > convenience and backwards compatibility. It should be sufficient to add > some paragraphs about security considerations to the dblink docs. I'm not suggesting making dblink super-user only. Only revoking public execute bits in the default install script. That doesn't affect users upgrading so I don't see a backwards-compatibility issue. The doc changes are going to say to be very careful who you grant access to dblink to which means not granting public execute access if you have multiple users. All I'm suggesting is that the default install script should just do that rather than do something that the docs will then recommend you undo. -- Gregory Stark EnterpriseDB http://www.enterprisedb.com
* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Joe Conway <mail@joeconway.com> writes:
> > But if you know of a security risk related to using libpq
> > with a password authenticated connection, let's hear it.
>
> As near as I can tell, the argument is that dblink might be used to send
> connection-request packets to random addresses. Now this is only a
Yes.
> security issue if the attacker could not have reached such an address
> directly; otherwise he might as well send the packet himself (and have a
No. Being able to come from a different address is valuable even if you
can get to that address directly yourself.
> lot more control over its content). So I guess the scenario is that
> you're running your database on your firewall machine, where it is
> accessible from outside your net but also can reach addresses inside.
It wouldn't need to be "on your firewall", just behind it, which is
extremely common.
> And you're letting untrustworthy outside people log into the database.
It's not nearly so convoluted. SQL injections happen.
> And you put dblink on it for them to use. And even then, the amount of
> damage they could do seems pretty limited due to lack of control over
> the packet contents.
dblink could have been installed for a variety of reasons. Making it
openly available on install makes it much less likely any additional
restrictions were placed on it.
> To me this scenario is too far-fetched to justify sacrificing
> convenience and backwards compatibility. It should be sufficient to add
> some paragraphs about security considerations to the dblink docs.
I feel that requiring a sysadmin to issue a 'grant' if they want
that convenience is justified and reasonable. We could include the
statement itself in the documentation we're expecting them to read
anyway so they can just copy & paste it. Adding paragraphs to the
documentation is good but doesn't justify a insecure-by-default
approach.
Regardless of what core ends up doing, I'm hopeful it'll be disabled by
default under Debian. It'd certainly be easier if it was done upstream.
Thanks,
Stephen
Attachment
On Mon, Jul 09, 2007 at 06:13:54PM +0100, Gregory Stark wrote:
> I'm not suggesting making dblink super-user only. Only revoking public execute
> bits in the default install script. That doesn't affect users upgrading so I
> don't see a backwards-compatibility issue.
>
> The doc changes are going to say to be very careful who you grant access to
> dblink to which means not granting public execute access if you have multiple
> users. All I'm suggesting is that the default install script should just do
> that rather than do something that the docs will then recommend you undo.
+1
-dg
--
David Gould daveg@sonic.net
If simplicity worked, the world would be overrun with simpletons.
- Doug Earp