For review: Server instrumentation patch

[ pick up new version.]

Your patch has been added to the PostgreSQL unapplied patches list at:

http://momjian.postgresql.org/cgi-bin/pgpatches

It will be applied as soon as one of the PostgreSQL committers reviews
and approves it.

---------------------------------------------------------------------------

Dave Page wrote:

[Resent as the list seems to have rejected yesterdays attempt]

As per Bruce's request, here's a copy of Andreas' server
instrumentation patch for review. I've separated out the
dbsize stuff and pg_terminate_backend is also not included.

This version was generated against CVS today.

As far as I can tell from review of comments made back to
pre-8.0, all security and other concerns raised have been addressed.

Regards, Dave.

Content-Description: instrumentation.tar.gz

[ Attachment, skipping... ]

---------------------------(end of broadcast)---------------------------
TIP 2: you can get off all lists at once with the unregister command
(send "unregister YourEmailAddressHere" to majordomo@postgresql.org)

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

This patch looks good. The only question I have is why you didn't want
the pgport rename/unlink calls? We usually use them unless there is
some reason not to.

---------------------------------------------------------------------------

Dave Page wrote:

[Resent as the list seems to have rejected yesterdays attempt]

As per Bruce's request, here's a copy of Andreas' server
instrumentation patch for review. I've separated out the
dbsize stuff and pg_terminate_backend is also not included.

This version was generated against CVS today.

As far as I can tell from review of comments made back to
pre-8.0, all security and other concerns raised have been addressed.

Regards, Dave.

Content-Description: instrumentation.tar.gz

[ Attachment, skipping... ]

---------------------------(end of broadcast)---------------------------
TIP 2: you can get off all lists at once with the unregister command
(send "unregister YourEmailAddressHere" to majordomo@postgresql.org)

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

-----Original Message-----
From: Bruce Momjian [mailto:pgman@candle.pha.pa.us]
Sent: 23 July 2005 20:01
To: Dave Page
Cc: PostgreSQL-development
Subject: Re: [HACKERS] For review: Server instrumentation patch

This patch looks good. The only question I have is why you
didn't want
the pgport rename/unlink calls? We usually use them unless there is
some reason not to.

<thinks...> Probably because this was written originally for 7.4 (as a
pgAdmin contrib module) and I'm guessing the pgport rename/unlink
weren't there at that time. I can't think of any reason not to use them
- do you want an updated patch or are you OK to tweak it when applying?

Regards, Dave.

Dave Page wrote:

-----Original Message-----
From: Bruce Momjian [mailto:pgman@candle.pha.pa.us]
Sent: 23 July 2005 20:01
To: Dave Page
Cc: PostgreSQL-development
Subject: Re: [HACKERS] For review: Server instrumentation patch

This patch looks good. The only question I have is why you
didn't want
the pgport rename/unlink calls? We usually use them unless there is
some reason not to.

<thinks...> Probably because this was written originally for 7.4 (as a
pgAdmin contrib module) and I'm guessing the pgport rename/unlink
weren't there at that time. I can't think of any reason not to use them
- do you want an updated patch or are you OK to tweak it when applying?

No, I modified my version. Thanks.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Bruce Momjian wrote:

Dave Page wrote:

-----Original Message-----
From: Bruce Momjian [mailto:pgman@candle.pha.pa.us]
Sent: 23 July 2005 20:01
To: Dave Page
Cc: PostgreSQL-development
Subject: Re: [HACKERS] For review: Server instrumentation patch

This patch looks good. The only question I have is why you
didn't want
the pgport rename/unlink calls?

Because I wanted the standard platform behaviour of both. For backend
storage subsystem purposes, it's certainly necessary to emulate *ix
behaviour of deleting a file in use, but for generic file access IMHO
the generic behaviour should be exposed.
Please note that there's some rollback logic in pg_file_rename that
might break when using the pg_xxx calls.

Regards,
Andreas

Andreas Pflug <pgadmin@pse-consulting.de> writes:

This patch looks good. The only question I have is why you
didn't want the pgport rename/unlink calls?

Because I wanted the standard platform behaviour of both. For backend
storage subsystem purposes, it's certainly necessary to emulate *ix
behaviour of deleting a file in use, but for generic file access IMHO
the generic behaviour should be exposed.

I'm going to repeat my firm opposition to this patch. Under the
innocuous-sounding banner of "server instrumentation", you are once
again trying to put in generic file access capabilities that will allow
remote Postgres superusers full access to the server filesystem.

The potential security risks of this are obvious to anyone. The only
justification that has been offered is "this will make remote
administration easier". Well, yeah, but it will make remote breakins
easier too. Valuing ease of use over security is the philosophy that
got Microsoft into the mess they're in now --- do we want to follow
that precedent?

The use-case for this seems to be to substitute for having local login
privileges on the server machine. Well, if the sysadmins of that
machine refuse to give you manual login privileges, they probably have
good reasons for it, and will not be very happy to see an end-run around
their restriction in the form of the database giving you remote
filesystem access.

I would have no problem with this patch as an optional add-on (eg, a
contrib module), so that individual installations could decide whether
they want to take the incremental security risk. In that form it's
basically equivalent to deciding you want to install an untrusted
PL language. We don't install those by default and we shouldn't install
generic filesystem access by default either.

If we accept this patch I foresee security-conscious admins starting to
question whether they should allow Postgres to be installed at all on
their systems. We don't need that kind of impediment to acceptance.

regards, tom lane

Tom Lane wrote:

Andreas Pflug <pgadmin@pse-consulting.de> writes:

This patch looks good. The only question I have is why you
didn't want the pgport rename/unlink calls?

Because I wanted the standard platform behaviour of both. For backend
storage subsystem purposes, it's certainly necessary to emulate *ix
behaviour of deleting a file in use, but for generic file access IMHO
the generic behaviour should be exposed.

I'm going to repeat my firm opposition to this patch. Under the
innocuous-sounding banner of "server instrumentation", you are once
again trying to put in generic file access capabilities that will allow
remote Postgres superusers full access to the server filesystem.

[well stated security objections snipped]

(Since we're visiting this again)

It also just strikes me as just the wrong way to go about solving the
apparent problem. If we want to make remote configuration or other
operations possible, then instead of granting access to server resident
files we should invent and implement an API that provides superusers the
appropriate operations. For one thing, this would mean that if we ever
decided to replace the current flat file system we use with something
else we need not break clients that use the API. Just granting file
access even if restricted to the data dir strikes me as a kludge.

cheers

andrew

Because I wanted the standard platform behaviour of both.

For backend

storage subsystem purposes, it's certainly necessary to emulate *ix
behaviour of deleting a file in use, but for generic file

access IMHO

the generic behaviour should be exposed.

I'm going to repeat my firm opposition to this patch. Under
the innocuous-sounding banner of "server instrumentation",
you are once again trying to put in generic file access
capabilities that will allow remote Postgres superusers full
access to the server filesystem.

The potential security risks of this are obvious to anyone.
The only justification that has been offered is "this will
make remote administration easier". Well, yeah, but it will
make remote breakins easier too. Valuing ease of use over
security is the philosophy that got Microsoft into the mess
they're in now --- do we want to follow that precedent?

How is this different from the fact that the superuser can already use
COPY to accomplish the same thing? Sure, you have to go through a
temporary table but if you're superuser that is not exactly a problem.
You can read/write any file the service account has permissions on.

//Magnus

"Magnus Hagander" <mha@sollentuna.net> writes:

How is this different from the fact that the superuser can already use
COPY to accomplish the same thing?

COPY can accomplish a few of the same things, much less conveniently
(for instance, it's darn hard to write an arbitrary binary file through
COPY).

If COPY provided all the same functionality, then Andreas would just use
that and not be so worried about having this patch. QED.

regards, tom lane

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

I'm going to repeat my firm opposition to this patch. Under the
innocuous-sounding banner of "server instrumentation", you are once
again trying to put in generic file access capabilities that will allow
remote Postgres superusers full access to the server filesystem.

The potential security risks of this are obvious to anyone. The only
justification that has been offered is "this will make remote
administration easier". Well, yeah, but it will make remote breakins
easier too. Valuing ease of use over security is the philosophy that
got Microsoft into the mess they're in now --- do we want to follow
that precedent?

I'm not exactly convinced this *actually* provides superusers any more
permissions than they already have. As someone else has pointed out,
there's COPY, and I'd think the whole can-dynamically-load-.so's would
trump any claims that you can give someone remote postgres superuser
'safely' (ie: and think it prevents them from being able to do things as
the postgres account on the system).

Indeed, I'd tend to think that's just about *exactly* what giving
someone postgres superuser access means- full access to the system as
that unix account.

The use-case for this seems to be to substitute for having local login
privileges on the server machine. Well, if the sysadmins of that
machine refuse to give you manual login privileges, they probably have
good reasons for it, and will not be very happy to see an end-run around
their restriction in the form of the database giving you remote
filesystem access.

Those admins should probably be educated that giving someone postgres
superuser is pretty much giving them local access as the user postgres
runs as. I don't believe it'd be appropriate to try and claim anything
else...

I would have no problem with this patch as an optional add-on (eg, a
contrib module), so that individual installations could decide whether
they want to take the incremental security risk. In that form it's
basically equivalent to deciding you want to install an untrusted
PL language. We don't install those by default and we shouldn't install
generic filesystem access by default either.

Given the somewhat lacking use-case (Working through postgres isn't
exactly the way *I'd* tend to mess with the filesystem directly, so this
seems like a pretty poor use-case to me) I'd agree that it'd be more
appropriate as a contrib module, but let's not confuse that with
security concerns.

If we accept this patch I foresee security-conscious admins starting to
question whether they should allow Postgres to be installed at all on
their systems. We don't need that kind of impediment to acceptance.

I doubt this as I expect security-conscious admins will understand what
being able to dynamically load a .so means, and will be aware of such
things as COPY. As such, I'd tend to think security-conscious admins
would deny remote superuser access anyway...

Stephen

How is this different from the fact that the superuser can

already use

COPY to accomplish the same thing?

COPY can accomplish a few of the same things, much less
conveniently (for instance, it's darn hard to write an
arbitrary binary file through COPY).

Right. But the *security* problem is more or less equal. If somebody
hacks your superuser account, they can make at least almost the same
amount of damage. It may take a little more work, but if you just want
to kill the system by overwriting files, or overwriting say the password
file, it's just as easy. And if what you want to do is stick some kind
of executable o nthe system, you can just wrap it in a shellscript that
will unpack it.

If COPY provided all the same functionality, then Andreas
would just use that and not be so worried about having this
patch. QED.

Oh, Andreas could edit postgresql.conf and whatever using COPY, no
doubt. And he could read the logfiles that way. But it would be very
hackish. From what I see this is just providing a different interface to
similar functionality.
But the point I'm trying to make is that the *security implications* are
more or less the same, just with a thin layer of
security-through-obscurity over one of them.

Bottom line: If somebody hacks your superuser, you've lost your
database. If your database service user has write access to sensitive
areas, or if you later log in as root (or whatever) and execute any
files that the database service user has write access to, you've lost
your box. This holds true with or without the patch.

//Magnus

Magnus Hagander wrote:

How is this different from the fact that the superuser can

already use

COPY to accomplish the same thing?

COPY can accomplish a few of the same things, much less
conveniently (for instance, it's darn hard to write an
arbitrary binary file through COPY).

Right. But the *security* problem is more or less equal. If somebody
hacks your superuser account, they can make at least almost the same
amount of damage. It may take a little more work, but if you just want
to kill the system by overwriting files, or overwriting say the password
file, it's just as easy. And if what you want to do is stick some kind
of executable o nthe system, you can just wrap it in a shellscript that
will unpack it.

It could be argued that there should be provision for a limitation on
the locations in which COPY can write (and maybe read) files.

If COPY is a security hole then we should close it, not use that as
precedent to open another hole.

cheers

andrew

Andrew Dunstan <andrew@dunslane.net> writes:

It could be argued that there should be provision for a limitation on
the locations in which COPY can write (and maybe read) files.
If COPY is a security hole then we should close it, not use that as
precedent to open another hole.

Yeah. It's worth pointing out in this connection that server-side
COPY is already pretty well crippled if you are running under SELinux,
because the security policy constrains what parts of the filesystem
the daemon can reach at all. I've already been thinking seriously
of proposing that the regression tests be converted to use only
\copy and not COPY, because it's difficult to run them against an
installed server on Fedora 4, and it may be impossible in the near
future.

regards, tom lane

"Magnus Hagander" <mha@sollentuna.net> writes:

Bottom line: If somebody hacks your superuser, you've lost your
database. If your database service user has write access to sensitive
areas, or if you later log in as root (or whatever) and execute any
files that the database service user has write access to, you've lost
your box. This holds true with or without the patch.

Nonetheless, the patch makes it vastly easier for an attacker to do bad
things, and vastly harder for an admin to try to lock down the database
adequately. For instance, the question of .so security can be attacked
by not installing any .so's that you don't want used; likewise a contrib
file-access module can be left off the system if it's considered a
hazard. But if the functionality is part of the core database then it's
exceedingly difficult for someone who doesn't want it to get rid of it.
(I believe that you'd actually have to recompile the server with the
dangerous functions removed; just deleting their pg_proc entries doesn't
stop someone from recreating those entries.)

Saying "we don't need to lock this down because there are other possible
attacks" is about like leaving your front door open because you know
that a determined burglar could get in by breaking a window. You may
or may not want to install steel bars over the windows, but that's no
argument for leaving the door open.

regards, tom lane

Tom Lane wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

It could be argued that there should be provision for a limitation on
the locations in which COPY can write (and maybe read) files.
If COPY is a security hole then we should close it, not use that as
precedent to open another hole.

Yeah. It's worth pointing out in this connection that server-side
COPY is already pretty well crippled if you are running under SELinux,
because the security policy constrains what parts of the filesystem
the daemon can reach at all. I've already been thinking seriously
of proposing that the regression tests be converted to use only
\copy and not COPY, because it's difficult to run them against an
installed server on Fedora 4, and it may be impossible in the near
future.

That also occurred to me. I have taken to turning off SELinux altogether
but some day I'm going to have to stop that.

How about if we do something like this?:

. initdb creates a tmpdir inside the datadir
. a new GUC var called allowed_copy_locations which is a PATH type
string specifying what directories we can copy to/from. This would by
default be "$tmpdir"
. in addition to an absolute path, a copy path could begin with $tmpdir
. explicitly setting the GUC to "*" would allow any absolute location as
now (having this not the default means admins would have to turn it on
deliberately, which would be a Good Thing (tm)).

possible extra:
. another GUC var to specify an alternative location for $tmpdir.

cheers

andrew

Andrew Dunstan <andrew@dunslane.net> writes:

How about if we do something like this?:

. initdb creates a tmpdir inside the datadir
. a new GUC var called allowed_copy_locations which is a PATH type
string specifying what directories we can copy to/from. This would by
default be "$tmpdir"

Given that COPY to/from a file is already allowed only to superusers,
I'm not sure how effective a GUC variable will be in constraining what
they do with it. We'd have to at least restrict it to SIGHUP, which'd
mean you couldn't change it without the ability to write the config
file.

Also I'm not sure how useful it is to read and write inside the data
directory, which is an area one hopes is not accessible to most people,
even if they have superuser privs.

If we went down this path at all, I'd be inclined to just deprecate
and eventually remove server-side COPY altogether. Not sure about
the performance costs of that, though.

regards, tom lane

Tom,

On 7/24/05 4:47 PM, "Tom Lane" <tgl@sss.pgh.pa.us> wrote:

If we went down this path at all, I'd be inclined to just deprecate
and eventually remove server-side COPY altogether. Not sure about
the performance costs of that, though.

Interesting problem, in practice you've got to transfer the file to the
server over the network anyway, so if the libpq copy gets fast enough it
would seem more efficient overall to use libpq copy.

On the other side, Oracle retains a true server side copy in addition to an
"external table" which maps to a file. This implies an audience for server
side copy functionality.

- Luke

Tom Lane wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

How about if we do something like this?:

. initdb creates a tmpdir inside the datadir
. a new GUC var called allowed_copy_locations which is a PATH type
string specifying what directories we can copy to/from. This would by
default be "$tmpdir"

Given that COPY to/from a file is already allowed only to superusers,
I'm not sure how effective a GUC variable will be in constraining what
they do with it. We'd have to at least restrict it to SIGHUP, which'd
mean you couldn't change it without the ability to write the config
file.

If we actually had an API for remote config changes, rather than just
allowing file system level access, one might have a category of settings
that could not be set remotely - this would be a prime candidate ;-)

cheers

andrew

It could be argued that there should be provision for a

limitation on

the locations in which COPY can write (and maybe read) files.
If COPY is a security hole then we should close it, not use that as
precedent to open another hole.

Yeah. It's worth pointing out in this connection that
server-side COPY is already pretty well crippled if you are
running under SELinux, because the security policy constrains
what parts of the filesystem the daemon can reach at all.

I don't know a lot about SELinux, but wouldn't this give the exact same
level of security for the new admin functions in this case?

Nonetheless, the patch makes it vastly easier for an attacker to do

bad things, and vastly harder for an >

admin to try to lock down the database adequately. For instance, the

question of .so security can be

attacked by not installing any .so's that you don't want used;

likewise a contrib file-access module can

be left off the system if it's considered a hazard. But if the

functionality is part of the core database

then it's exceedingly difficult for someone who doesn't want it to get

rid of it.

(I believe that you'd actually have to recompile the server with the

dangerous functions removed; just

deleting their pg_proc entries doesn't stop someone from recreating

those entries.)

Let me suggest another nice way for a superuser to do whatever he wants.
How about "CREATE UNTRUSTED PROCEDURAL LANGUAGE"? If you have say
pl/perl or pl/tcl on the system, you just create the untrusted version
and away you go - because they use the same .so. This lets you not only
modify files on the system, but execute arbitrary code on the system.
If you want to protect the system from a hacked superuser, you will have
to remove the concept of untrusted procedural languages as well.
Oh, and probably LOAD; since the superuser can LOAD any .so on the
system IIRC - including stuf that a local user may stuff in /tmp/ or
wherever.

Saying "we don't need to lock this down because there are other

possible attacks" is about like leaving

your front door open because you know that a determined burglar could

get in by breaking a window. You

may or may not want to install steel bars over the windows, but that's

no argument for leaving the door open.

I would rather equal it with "we don't lock the door, because the door
1m to the left of it is open anyway". You don't need to break anything
to get in either way.

Instead of trying to pick on one feature, how about trying something
constructive instead? Let's say we add a GUC like "restrict_superuser",
that disables COPY to local files, untrusted procedural languages (both
creation and using the ones that already exist), the new access
functions, the LOAD command etc. Then the admin can chose what to do
about superuser access levels - the requirement may dependon SELinux for
example.

next mail:

Given that COPY to/from a file is already allowed only to superusers,

I'm not sure how effective a GUC

variable will be in constraining what they do with it. We'd have to

at least restrict it to SIGHUP,

which'd mean you couldn't change it without the ability to write the

config file.

You are kidding, right? Your original argument here was that "restricted
to superuser is not security for the new functions", how can restricted
to superusers be security for COPY?!

//Magnus

Magnus Hagander wrote:

Instead of trying to pick on one feature, how about trying something
constructive instead? Let's say we add a GUC like "restrict_superuser",
that disables COPY to local files, untrusted procedural languages (both
creation and using the ones that already exist), the new access
functions, the LOAD command etc. Then the admin can chose what to do
about superuser access levels - the requirement may dependon SELinux for
example.

I could go for this.

Creating a setting that disallowed creation/calling of plperlu
functions would be fairly trivial.

I still think, security considerations aside, that an API for config
settings would be a much better piece of design than providing file
system access functions.

cheers

andrew

"Magnus Hagander" <mha@sollentuna.net> writes:

Yeah. It's worth pointing out in this connection that
server-side COPY is already pretty well crippled if you are
running under SELinux, because the security policy constrains
what parts of the filesystem the daemon can reach at all.

I don't know a lot about SELinux, but wouldn't this give the exact same
level of security for the new admin functions in this case?

It would prevent them from reaching unwanted parts of the filesystem,
yes, but that has little to do with the privilege-escalation problem.

I see I had better spell out the reasoning here. Assume that a bad guy
has gotten hold of your database superuser password and is now trying
to parlay that into shell-level access to the underlying system (which
is to say, the ability to execute shell commands of his choice; whether
he ever acquires a login password is secondary). If you've installed an
utrusted PL then the game is over immediately, so let's assume you
didn't. One way that the attacker might proceed is to try to make a .so
file that he can LOAD into the backend containing the equivalent of a
system() function. I believe this is not feasible using COPY in its
current form, mainly because you can't write arbitrary binary files with
it (no embedded zeroes for instance). With a function to write
arbitrary file contents, one very large stumbling block goes away.
There's still a problem of getting the file to have the right executable
permission bits, but that might be surmounted in various ways, for
instance by finding an existing program or .so file that the backend has
permission to overwrite. (Somebody argued yesterday that the attacker
could always build such a file by executing a shell script, but that
misses the point: we are considering how the attacker can get to the
point of being able to issue shell commands, not what he can do once
he's got that.)

So it seems clear to me that adding a file-write function adds a very
substantial amount of risk from a privilege-escalation point of view.
Yes, it's only one link in a chain, but it's a big link.

Let me suggest another nice way for a superuser to do whatever he wants.
How about "CREATE UNTRUSTED PROCEDURAL LANGUAGE"? If you have say
pl/perl or pl/tcl on the system, you just create the untrusted version
and away you go - because they use the same .so.

Yeah, I was thinking earlier about proposing that the trusted and
untrusted versions need to be distinct .so's, so that the admin can
physically remove the untrusted ones to prevent this scenario.
But, again, the existence of security hole A is not justification for
introducing security hole B.

Instead of trying to pick on one feature, how about trying something
constructive instead?

That'd be fine with me --- but we have to introduce that *before* we add
obvious new security risks, not after.

regards, tom lane

<snip good explanation. Thanks.>

Let me suggest another nice way for a superuser to do

whatever he wants.

How about "CREATE UNTRUSTED PROCEDURAL LANGUAGE"? If you have say
pl/perl or pl/tcl on the system, you just create the

untrusted version

and away you go - because they use the same .so.

Yeah, I was thinking earlier about proposing that the trusted
and untrusted versions need to be distinct .so's, so that the
admin can physically remove the untrusted ones to prevent
this scenario.
But, again, the existence of security hole A is not
justification for introducing security hole B.

Instead of trying to pick on one feature, how about trying

something

constructive instead?

That'd be fine with me --- but we have to introduce that
*before* we add obvious new security risks, not after.

So what do you think of the proposed GUC?

Or what about a parameter to restrict both COPY and the utility
functions to certain subdirs only? (BTW, I was under the impression that
the admin functions were restricted to the pgdata directory already, but
I could be wrong - I don't have the latest version of the patch around)

//Magnus

"Magnus Hagander" <mha@sollentuna.net> writes:

That'd be fine with me --- but we have to introduce that
*before* we add obvious new security risks, not after.

So what do you think of the proposed GUC?

Well, it has more or less the same problem as the GUC in the
COPY-only-to-given-places proposal, which is that GUCs were never
intended to prevent superusers from changing their values. Right now
a remote superuser can't change a postmaster-start-time-only GUC, but
if we ever introduce a real remote admin facility I'd expect it to
support that, so it seems like a GUC intended not to be changeable by
superusers would have to be its own special category. I'd be inclined
not to expose it as a GUC at all, but make it some other mechanism
(maybe a postmaster command-line switch only?)

Or what about a parameter to restrict both COPY and the utility
functions to certain subdirs only? (BTW, I was under the impression that
the admin functions were restricted to the pgdata directory already, but
I could be wrong - I don't have the latest version of the patch around)

We've gone back and forth on that with respect to the proposed admin
functions, and I forget which way the current patch is. But it doesn't
do much to stop the privilege escalation risk: if you can write into any
of the same directories you can LOAD from, the risk exists. (And
detecting whether two paths overlap is very hard in general, considering
directory symlinks, AFS mounts, etc, so we probably couldn't hope to
forbid LOADing from any writable directory.)

regards, tom lane

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

didn't. One way that the attacker might proceed is to try to make a .so
file that he can LOAD into the backend containing the equivalent of a
system() function. I believe this is not feasible using COPY in its
current form, mainly because you can't write arbitrary binary files with
it (no embedded zeroes for instance). With a function to write

Now, I'm not the best hacker in the world, so I didn't actually get this
all the way to working (wish I had more time to play with it but I don't
really), but:

test=# create function unlink (text) RETURNS integer LANGUAGE 'C' AS
'/lib/libc-2.3.2.so', 'unlink';
CREATE FUNCTION
test=# select unlink('/tmp/test');

unlink
--------
-1

I had created /tmp/test, but it appears the 'oldstyle' function calls
pass in the arguments with some garbage on the front (about 4 bytes it
looked like from gdb). Figure out how to skip those 4 bytes per
argument and you hardly need any other .so, you've got libc. I suspect
it can be done. The newstyle API looks like it'd probably make it a bit
more difficult but still, being able to load any function from any .so
you've got access to seems *extremely* powerful to me, just as much as
any untrusted language.

If you want to secure your system against a superuser()-level intrusion
then you need to secure the unix account, or disable creation of
C-language and other untrusted languages (at least).

Stephen

Andrew Dunstan wrote:

It also just strikes me as just the wrong way to go about solving the
apparent problem. If we want to make remote configuration or other
operations possible, then instead of granting access to server resident
files we should invent and implement an API that provides superusers the
appropriate operations. For one thing, this would mean that if we ever
decided to replace the current flat file system we use with something
else we need not break clients that use the API. Just granting file
access even if restricted to the data dir strikes me as a kludge.

I thought an API for postgresql.conf is what we agreed to, but I don't
see it on the TODO list. Is that correct?

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

That'd be fine with me --- but we have to introduce that
*before* we add obvious new security risks, not after.

So what do you think of the proposed GUC?

Well, it has more or less the same problem as the GUC in the
COPY-only-to-given-places proposal, which is that GUCs were
never intended to prevent superusers from changing their
values. Right now a remote superuser can't change a
postmaster-start-time-only GUC, but if we ever introduce a
real remote admin facility I'd expect it to support that, so
it seems like a GUC intended not to be changeable by
superusers would have to be its own special category. I'd be
inclined not to expose it as a GUC at all, but make it some
other mechanism (maybe a postmaster command-line switch only?)

If you make it a postmaster-start-only, and it restricts the
remote-admin-functionality, then you will not be able to change it
remotely.

Making it a GUC makes it a whole lot easier to deal with for the admin -
especially in cases like win32 when it's not as easy to edit the startup
parameters for service based processes.

Or what about a parameter to restrict both COPY and the utility
functions to certain subdirs only? (BTW, I was under the impression
that the admin functions were restricted to the pgdata directory
already, but I could be wrong - I don't have the latest

version of the

patch around)

We've gone back and forth on that with respect to the
proposed admin functions, and I forget which way the current
patch is. But it doesn't do much to stop the privilege
escalation risk: if you can write into any of the same
directories you can LOAD from, the risk exists. (And
detecting whether two paths overlap is very hard in general,
considering directory symlinks, AFS mounts, etc, so we
probably couldn't hope to forbid LOADing from any writable directory.)

Right. That's gotta be at least as bad as dealing with URLs, and we've
seen how many bugs there have been in those in pretty much all
webservers...

//Magnus

Andrew Dunstan <andrew@dunslane.net> writes:

I still think, security considerations aside, that an API for config
settings would be a much better piece of design than providing file
system access functions.

I agree with that. Given what we currently have, though, remote config
and remote log examination do require filesystem access. But IMHO
there's no very good reason why admin actions requiring filesystem
access shouldn't be programmed in an untrusted PL, rather than through
separate file-access functions. Andreas argued that he didn't want to
make pgAdmin functionality dependent on the availability of an untrusted
PL, but I think that argument is bogus. If the admin doesn't want to
install an untrusted PL for pgAdmin to use, why in the world would he
be happy with equivalent functionality being installed in such a way
that he can't get rid of it?

regards, tom lane

I still think, security considerations aside, that an API

for config

settings would be a much better piece of design than providing file
system access functions.

I agree with that.

For the record, me too. But I don't see that happening for 8.1,
considering the feature freeze and timescale...

Given what we currently have, though,
remote config and remote log examination do require
filesystem access. But IMHO there's no very good reason why
admin actions requiring filesystem access shouldn't be
programmed in an untrusted PL, rather than through separate
file-access functions. Andreas argued that he didn't want to
make pgAdmin functionality dependent on the availability of
an untrusted PL, but I think that argument is bogus. If the
admin doesn't want to install an untrusted PL for pgAdmin to
use, why in the world would he be happy with equivalent
functionality being installed in such a way that he can't get
rid of it?

Not trying to speak for Andreas here, I see the problem as an added
dependency *outside* postgresql. If he were to use pl/perl, he couldn o
longer admin a postgresql server without perl on it (and perl installed
as a shared lib). Same for python and tcl - which I beleive rounds up
all the PLs.

Plus the admin will have to have included it in ./configure and run
createlang with it.

//Magnus

Stephen Frost <sfrost@snowman.net> writes:

If you want to secure your system against a superuser()-level intrusion
then you need to secure the unix account, or disable creation of
C-language and other untrusted languages (at least).

Very likely --- which is why Magnus' idea of an explicit switch to
prevent superuser filesystem access seems attractive to me. It'd
have to turn off LOAD and creation of new C functions as well as COPY
and the other stuff we discussed.

However, once again, the availability of security hole A does not
justify creating security hole B. For example, even with creation
of new C functions disabled, a superuser attacker might be able to use a
file-write function to overwrite an existing .so and thereby subvert an
existing C-function definition to do something bad.

regards, tom lane

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Andrew Dunstan wrote:

It also just strikes me as just the wrong way to go about solving the
apparent problem.

I thought an API for postgresql.conf is what we agreed to, but I don't
see it on the TODO list. Is that correct?

Like you, I seem to recall some prior discussion along this line, but
I think it didn't get solid enough to produce a TODO item. Right now
the best we could do is

* Better support for remote database administration, eg, APIs
to change configuration and restart the postmaster

which is mighty vague.

regards, tom lane

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Andrew Dunstan wrote:

It also just strikes me as just the wrong way to go about solving the
apparent problem.

I thought an API for postgresql.conf is what we agreed to, but I don't
see it on the TODO list. Is that correct?

Like you, I seem to recall some prior discussion along this line, but
I think it didn't get solid enough to produce a TODO item. Right now
the best we could do is

* Better support for remote database administration, eg, APIs
to change configuration and restart the postmaster

which is mighty vague.

Added to TODO:

o Allow postgresql.conf file values to be changed via an SQL API
o Allow the server to be stopped/restarted via an SQL API

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Andrew Dunstan wrote:

It could be argued that there should be provision for a limitation on
the locations in which COPY can write (and maybe read) files.

Please note that the genfile functions are already restricted.

Regards,
Andreas

Andreas Pflug wrote:

Andrew Dunstan wrote:

It could be argued that there should be provision for a limitation on
the locations in which COPY can write (and maybe read) files.

Please note that the genfile functions are already restricted.

Yes, that's what I thought. The argument is about how safe that is,
especially if you can't turn it off, isn't it?

cheers

andrew

-----Original Message-----
From: Tom Lane [mailto:tgl@sss.pgh.pa.us]
Sent: 25 July 2005 15:18
To: Magnus Hagander
Cc: Andrew Dunstan; Andreas Pflug; Bruce Momjian; Dave Page;
PostgreSQL-development
Subject: Re: [HACKERS] For review: Server instrumentation patch

Or what about a parameter to restrict both COPY and the utility
functions to certain subdirs only? (BTW, I was under the

impression that

the admin functions were restricted to the pgdata directory

already, but

I could be wrong - I don't have the latest version of the

patch around)

It does. Prior to feature freeze, that was the *only* concern raised
with the patch, despite significant discssion.

We've gone back and forth on that with respect to the proposed admin
functions, and I forget which way the current patch is. But
it doesn't
do much to stop the privilege escalation risk: if you can
write into any
of the same directories you can LOAD from, the risk exists. (And
detecting whether two paths overlap is very hard in general,
considering
directory symlinks, AFS mounts, etc, so we probably couldn't hope to
forbid LOADing from any writable directory.)

I'm not going to repeat all the other arguments here because they've
been put forward perfectly well by others, but I feel I must point out
my dismay at what seems like a complete disregard for non-core
applications that has been expressed to me off-list by a number of
people. This patch has been discussed on and off since before 8.0 was
released, and some time prior to feature freeze I took over the task of
trying to get it accepted from Andreas so he could continue with other
work. The patch was discussed in great depth again (prior to feature
freeze) and none of these concerns were raised. Had they been, we might
have worked to find an alternative solution to the problem to allow
PostgreSQL to boast simple features offered by every other modern DBMS
I've used. Instead, because we are so far past feature freeze, there is
no chance that an altenative solution will be accepted.

The common belief in the messages I've received seems to be that users
that prefer to use a GUI are simply not welcome. True or not (and I do
hope that is not the case), it's a great shame that it seems that
PostgreSQL will remain configurable only from the command line - as one
well respected member of the community wrote to me:

"I hope no other open source DBMS guys are following this thead. They
must be ROFL seeing how Core is trying to prevent remote
administrability."

Regards, Dave.

If you want to secure your system against a superuser()-level
intrusion then you need to secure the unix account, or disable
creation of C-language and other untrusted languages (at least).

Very likely --- which is why Magnus' idea of an explicit
switch to prevent superuser filesystem access seems
attractive to me. It'd have to turn off LOAD and creation of
new C functions as well as COPY and the other stuff we discussed.

So would a patch to do this be accepted for 8.1 even though we are past
feature freeze?

And if yes, would you like me to give I a shot or would you rather do it
yourself? (As I'm sure it'd need tweaking)

And finally, with something like that in place, would you be fine with
the file editing functions as they stand (limiting them to the pg
directories, as I believe it does)?

//Magnus

"Magnus Hagander" <mha@sollentuna.net> writes:

If you want to secure your system against a superuser()-level
intrusion then you need to secure the unix account, or disable
creation of C-language and other untrusted languages (at least).

Very likely --- which is why Magnus' idea of an explicit
switch to prevent superuser filesystem access seems
attractive to me. It'd have to turn off LOAD and creation of
new C functions as well as COPY and the other stuff we discussed.

So would a patch to do this be accepted for 8.1 even though we are past
feature freeze?

Given that we don't even have a design for it, I think it's a bit late
for 8.1 :-(.

Both Bruce and I have way more on our plates than we could wish, and the
other committers aren't getting a lot done, so the originally hoped-for
beta date of 1 Aug is looking completely out of reach. So adding yet
more stuff to the queue isn't going to get looked upon with great favor.

And finally, with something like that in place, would you be fine with
the file editing functions as they stand (limiting them to the pg
directories, as I believe it does)?

I'm OK with them even without the directory limitation as long as
there's a way to disable them. However, I fear the whole thing has to
wait for 8.2 at this point.

regards, tom lane

If you want to secure your system against a superuser()-level
intrusion then you need to secure the unix account, or disable
creation of C-language and other untrusted languages (at least).

Very likely --- which is why Magnus' idea of an explicit switch to
prevent superuser filesystem access seems attractive to me. It'd
have to turn off LOAD and creation of new C functions as

well as COPY

and the other stuff we discussed.

So would a patch to do this be accepted for 8.1 even though we are
past feature freeze?

Given that we don't even have a design for it, I think it's a
bit late for 8.1 :-(.

Both Bruce and I have way more on our plates than we could
wish, and the other committers aren't getting a lot done, so
the originally hoped-for beta date of 1 Aug is looking
completely out of reach. So adding yet more stuff to the
queue isn't going to get looked upon with great favor.

That's what I was afraid of. But I certainly understand, you guys
certainly have a lot of work pending.

And finally, with something like that in place, would you

be fine with

the file editing functions as they stand (limiting them to the pg
directories, as I believe it does)?

I'm OK with them even without the directory limitation as
long as there's a way to disable them. However, I fear the
whole thing has to wait for 8.2 at this point.

That would be very bad - considering it just missed 8.0 as well.

How about bolting similar functionality on top of just the new functions
for now, as an extension to that patch, and then externd it to cover the
rest of the functions by 8.2? Considering it'd only tough new code, it
couldn't really affect other parts of the system?
(Yes, I realise it's of course not the number of patches that count, but
the amount of code to review. But it'd be much more localised this way)

//Magnus

"Magnus Hagander" <mha@sollentuna.net> writes:

I'm OK with them even without the directory limitation as
long as there's a way to disable them. However, I fear the
whole thing has to wait for 8.2 at this point.

That would be very bad - considering it just missed 8.0 as well.

[ shrug... ] The same objections were raised during the 8.0 development
cycle, and nothing was done in response, except to submit essentially
the same patch at an equally late stage of the 8.1 cycle. The people
who are interested in this need to put a higher priority on developing
an acceptable patch, or it'll miss the next round as well.

Sorry to be hard-nosed, but I've already expended way more time on this
thread than I can afford just now.

regards, tom lane

-----Original Message-----
From: Tom Lane [mailto:tgl@sss.pgh.pa.us]
Sent: 26 July 2005 22:01
To: Magnus Hagander
Cc: Stephen Frost; Andrew Dunstan; Andreas Pflug; Bruce
Momjian; Dave Page; PostgreSQL-development
Subject: Re: [HACKERS] For review: Server instrumentation patch

"Magnus Hagander" <mha@sollentuna.net> writes:

I'm OK with them even without the directory limitation as
long as there's a way to disable them. However, I fear the
whole thing has to wait for 8.2 at this point.

That would be very bad - considering it just missed 8.0 as well.

[ shrug... ] The same objections were raised during the 8.0
development
cycle, and nothing was done in response, except to submit essentially
the same patch at an equally late stage of the 8.1 cycle.

But that's exactly the point - these objections *were not* raised as far
as anyone has been able to find in the archives, and the last time we
discussed it
(http://archives.postgresql.org/pgsql-hackers/2005-06/msg01147.php), I
pointed this out, with references to the original threads *and*
specifically asked what the outstanding issues were that had not been
addressed.

The only answer I got was that the file access functions should not
operate outside $PGDATA, which they do not.

The people
who are interested in this need to put a higher priority on developing
an acceptable patch, or it'll miss the next round as well.

We have no problem with that, and are happy to do so. What we object to
is not being told there is a show-stopping objection until it is too
late, despite specifically asking the question some weeks before feature
freeze, during an active discussion on the subject.

I'm assuming that you are specifically objecting to pg_file_write,
pg_file_rename, and pg_file_unlink? Would you accept the rest of the
patch without those for 8.1, and a new patch for 8.2 to add those three
functions in a manner you are happy with? That at least would allow
viewing of logfiles etc. in this release.

Regards, Dave.

Dave Page wrote:

[Resent as the list seems to have rejected yesterdays attempt]

As per Bruce's request, here's a copy of Andreas' server
instrumentation patch for review. I've separated out the
dbsize stuff and pg_terminate_backend is also not included.

This version was generated against CVS today.

As far as I can tell from review of comments made back to
pre-8.0, all security and other concerns raised have been addressed.

Here is a modified version of your patch that adds functions to do
configuration file reload, and log file rotation.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073