Thread: For review: Server instrumentation patch
[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.
Attachment
[ 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, Pennsylvania19073
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, Pennsylvania19073
> -----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, Pennsylvania19073
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, Pennsylvania19073
> >> 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/restartedvia 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,
Pennsylvania19073
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.