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