Thread: Updated instrumentation patch
Per recent discussion, here is yet another updated version of the instrumentation patch. Changes: * Added guc option "disable_remote_admin", that disables any write operations (write, unlink, rename) even for the superuser. Set as PGC_POSTMASTER so it cannot be changed remotely. I put this under "file locations", because that's where all the other config file information is. Though that doesn't feel completely right, I couldn't find a better place without creating a whole new category (it's not *connection* security, after all), and if that's to be done I think it's better if one of the committers pick name etc for it :-) * Make sure pg_file_stat() can only be used by superuser. It lacked this check previously. * Updated so it applies to current cvs. This means all oids have changed, since they were all used for other things now. Also added a required header that had moved with the datetime stuff. Actual code changes against the previous patch are very small. //Magnus
Attachment
I just realised the entry for pg_file_rename is duplicated in pg_proc.h. Unless someone can say it's a good thing (it was in the original patch..), please remove one of those entries before applying. It breaks the opr_sanity test. //Magnus > -----Original Message----- > From: pgsql-patches-owner@postgresql.org > [mailto:pgsql-patches-owner@postgresql.org] On Behalf Of > Magnus Hagander > Sent: Saturday, July 30, 2005 4:39 PM > To: PostgreSQL-patches > Subject: [PATCHES] Updated instrumentation patch > > Per recent discussion, here is yet another updated version of > the instrumentation patch. Changes: > > * Added guc option "disable_remote_admin", that disables any > write operations (write, unlink, rename) even for the > superuser. Set as PGC_POSTMASTER so it cannot be changed remotely. > I put this under "file locations", because that's where all > the other config file information is. Though that doesn't > feel completely right, I couldn't find a better place without > creating a whole new category (it's not *connection* > security, after all), and if that's to be done I think it's > better if one of the committers pick name etc for it :-) > > * Make sure pg_file_stat() can only be used by superuser. It > lacked this check previously. > > * Updated so it applies to current cvs. This means all oids > have changed, since they were all used for other things now. > Also added a required header that had moved with the datetime stuff. > > > Actual code changes against the previous patch are very small. > > //Magnus >
"Magnus Hagander" <mha@sollentuna.net> writes:
> Per recent discussion, here is yet another updated version of the
> instrumentation patch. Changes:
> * Added guc option "disable_remote_admin", that disables any write
> operations (write, unlink, rename) even for the superuser. Set as
> PGC_POSTMASTER so it cannot be changed remotely.
I was envisioning it as disabling all filesystem access --- read as well
as write. Essentially the abstract concept I want is that with this on,
even a superuser cannot use Postgres to get at the underlying operating
system. A name like "enable_filesystem_access" would probably be more
appropriate.
Also, as I already said, marking it as PGC_POSTMASTER is simply not
adequate security. Once we have some sort of remote admin feature,
I would expect it to support adjustment of even postmaster-level options
(this would mean forcing a database restart of course) --- you can
hardly say that you have a complete remote admin solution if you can't
change shared_buffers or max_connections.
regards, tom lane
Tom Lane wrote: > "Magnus Hagander" <mha@sollentuna.net> writes: > > Per recent discussion, here is yet another updated version of the > > instrumentation patch. Changes: > > > * Added guc option "disable_remote_admin", that disables any write > > operations (write, unlink, rename) even for the superuser. Set as > > PGC_POSTMASTER so it cannot be changed remotely. > > I was envisioning it as disabling all filesystem access --- read as well > as write. Essentially the abstract concept I want is that with this on, > even a superuser cannot use Postgres to get at the underlying operating > system. A name like "enable_filesystem_access" would probably be more > appropriate. > > Also, as I already said, marking it as PGC_POSTMASTER is simply not > adequate security. Once we have some sort of remote admin feature, > I would expect it to support adjustment of even postmaster-level options > (this would mean forcing a database restart of course) --- you can > hardly say that you have a complete remote admin solution if you can't > change shared_buffers or max_connections. How does this affect COPY? Is it not important because COPY can not write a null byte? -- 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
> > Per recent discussion, here is yet another updated version of the > > instrumentation patch. Changes: > > > * Added guc option "disable_remote_admin", that disables any write > > operations (write, unlink, rename) even for the superuser. Set as > > PGC_POSTMASTER so it cannot be changed remotely. > > I was envisioning it as disabling all filesystem access --- > read as well as write. Essentially the abstract concept I > want is that with this on, even a superuser cannot use > Postgres to get at the underlying operating system. A name > like "enable_filesystem_access" would probably be more appropriate. Um. I thought the entire argument was about *writing* files. But it should be easy enough to stick requireRemoteAdmin() to all the functions. For the long term I was thinking something like "restrict_superuser", which would disable both read and write, and COPY, and untrusted PL creation, etc, etc. But that's not for 8.1. > Also, as I already said, marking it as PGC_POSTMASTER is > simply not adequate security. Once we have some sort of > remote admin feature, I would expect it to support adjustment > of even postmaster-level options (this would mean forcing a > database restart of course) --- you can hardly say that you > have a complete remote admin solution if you can't change > shared_buffers or max_connections. The point is you cannot *enable* it once it is *disabled*. Thus you cannot *elevate* your privileges. Thus not a security issue. Yes, if it is enabled, you can remotely disbale it and lock yourself out. But you can *not* do it the other way around. Once it's off, you need shell access on the box to edit the file and re-enable it. Once we have a "real remote admin API", it becomes an argument, and it will have to be adjusted. But we don't have that today, and I see no need to create a new guc category just for this. After all, some of these functions will probably go away completely once we have such an API. //Magnus
Magnus Hagander wrote: > > > Per recent discussion, here is yet another updated version of the > > > instrumentation patch. Changes: > > > > > * Added guc option "disable_remote_admin", that disables any write > > > operations (write, unlink, rename) even for the superuser. Set as > > > PGC_POSTMASTER so it cannot be changed remotely. > > > > I was envisioning it as disabling all filesystem access --- > > read as well as write. Essentially the abstract concept I > > want is that with this on, even a superuser cannot use > > Postgres to get at the underlying operating system. A name > > like "enable_filesystem_access" would probably be more appropriate. > > Um. I thought the entire argument was about *writing* files. But it > should be easy enough to stick requireRemoteAdmin() to all the > functions. > > For the long term I was thinking something like "restrict_superuser", > which would disable both read and write, and COPY, and untrusted PL > creation, etc, etc. But that's not for 8.1. > > > Also, as I already said, marking it as PGC_POSTMASTER is > > simply not adequate security. Once we have some sort of > > remote admin feature, I would expect it to support adjustment > > of even postmaster-level options (this would mean forcing a > > database restart of course) --- you can hardly say that you > > have a complete remote admin solution if you can't change > > shared_buffers or max_connections. > > The point is you cannot *enable* it once it is *disabled*. Thus you > cannot *elevate* your privileges. Thus not a security issue. I think any secure solution is going to have to block all write access to postgresql.conf, and that includes all the COPY TO and all the untrusted languages. -- 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
> > > Also, as I already said, marking it as PGC_POSTMASTER is > simply not > > > adequate security. Once we have some sort of remote > admin feature, > > > I would expect it to support adjustment of even postmaster-level > > > options (this would mean forcing a database restart of > course) --- > > > you can hardly say that you have a complete remote admin > solution if > > > you can't change shared_buffers or max_connections. > > > > The point is you cannot *enable* it once it is *disabled*. Thus you > > cannot *elevate* your privileges. Thus not a security issue. > > I think any secure solution is going to have to block all > write access to postgresql.conf, and that includes all the > COPY TO and all the untrusted languages. Exactly. But we won't get that for 8.1. So for now, we block all write access through *new* functions, per the "let's at least not add more security holes" rule. //Magnus
Magnus Hagander wrote: > > > > > Also, as I already said, marking it as PGC_POSTMASTER is > > simply not > > > > adequate security. Once we have some sort of remote > > admin feature, > > > > I would expect it to support adjustment of even postmaster-level > > > > options (this would mean forcing a database restart of > > course) --- > > > > you can hardly say that you have a complete remote admin > > solution if > > > > you can't change shared_buffers or max_connections. > > > > > > The point is you cannot *enable* it once it is *disabled*. Thus you > > > cannot *elevate* your privileges. Thus not a security issue. > > > > I think any secure solution is going to have to block all > > write access to postgresql.conf, and that includes all the > > COPY TO and all the untrusted languages. > > Exactly. But we won't get that for 8.1. So for now, we block all write > access through *new* functions, per the "let's at least not add more > security holes" rule. As far as I know, the only new functionality the patch adds _over_ copy is the ability to write nulls, and rename/unlink. Should we just throw an error when writing null bytes? -- 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
> > > I think any secure solution is going to have to block all write > > > access to postgresql.conf, and that includes all the COPY > TO and all > > > the untrusted languages. > > > > Exactly. But we won't get that for 8.1. So for now, we > block all write > > access through *new* functions, per the "let's at least not > add more > > security holes" rule. > > As far as I know, the only new functionality the patch adds > _over_ copy is the ability to write nulls, and rename/unlink. > Should we just throw an error when writing null bytes? Um. Yes. This patch goes one step further and allows you to block the writing of *any* file using these functions. The question is wether that one step further is far enough.. //Magnus
Magnus Hagander wrote: > > > > I think any secure solution is going to have to block all write > > > > access to postgresql.conf, and that includes all the COPY > > TO and all > > > > the untrusted languages. > > > > > > Exactly. But we won't get that for 8.1. So for now, we > > block all write > > > access through *new* functions, per the "let's at least not > > add more > > > security holes" rule. > > > > As far as I know, the only new functionality the patch adds > > _over_ copy is the ability to write nulls, and rename/unlink. > > Should we just throw an error when writing null bytes? > > Um. Yes. This patch goes one step further and allows you to block the > writing of *any* file using these functions. The question is wether that > one step further is far enough.. I am thinking we can just block null byte writes and say it is the same as COPY, which we have always used. -- 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
"Magnus Hagander" <mha@sollentuna.net> writes:
> For the long term I was thinking something like "restrict_superuser",
> which would disable both read and write, and COPY, and untrusted PL
> creation, etc, etc. But that's not for 8.1.
That's exactly what I'm talking about.
>> Also, as I already said, marking it as PGC_POSTMASTER is
>> simply not adequate security. Once we have some sort of
>> remote admin feature, I would expect it to support adjustment
>> of even postmaster-level options (this would mean forcing a
>> database restart of course) --- you can hardly say that you
>> have a complete remote admin solution if you can't change
>> shared_buffers or max_connections.
> The point is you cannot *enable* it once it is *disabled*. Thus you
> cannot *elevate* your privileges. Thus not a security issue.
It will be as soon as we have remote admin.
> Once we have a "real remote admin API", it becomes an argument, and it
> will have to be adjusted. But we don't have that today, and I see no
> need to create a new guc category just for this. After all, some of
> these functions will probably go away completely once we have such an
> API.
None of these functions are getting into 8.1 anyway; we should be
designing the long-term solution not making up short-lived hacks.
regards, tom lane
> > Once we have a "real remote admin API", it becomes an > argument, and it > > will have to be adjusted. But we don't have that today, and > I see no > > need to create a new guc category just for this. After all, some of > > these functions will probably go away completely once we > have such an > > API. > > None of these functions are getting into 8.1 anyway; we > should be designing the long-term solution not making up > short-lived hacks. I'm sorry, but then why the **** did my question: > 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)? get the answer: > I'm OK with them even without the directory limitation as long as > there's a way to disable them. If you had just said from the start that these functions would not be accepted even if the specific concerns raised were fixed, a lot of time invested by a lot of people would not have been necessary. I guess I just join the rank of people giving up on this. Too bad for the people who want to be able to remotely admin their stuff, because I now think everybody who actually cared have given up. //Magnus
-----Original Message----- From: pgsql-patches-owner@postgresql.org on behalf of Tom Lane Sent: Sat 7/30/2005 4:58 PM To: Magnus Hagander Cc: PostgreSQL-patches Subject: Re: [PATCHES] Updated instrumentation patch > None of these functions are getting into 8.1 anyway; we should be > designing the long-term solution not making up short-lived hacks. So, going back to pre 8.0, we fixed them so they don't work outside of the data directory as requested, yet they were notincluded for unknown reasons. We revisited some weeks before prior to feature freeze, and I researched all issues raised and ask for clarification on whatyou weren't happy with as all I'd found in the archives was a sentence along the lines of "I really don't see any valuein these". I found no outstanding issues in the archives, nor did I receive any in response to my questions. Having received no further objections, the patch was added to the queue. As soon as Bruce starts to look at it, presumablyto apply it, you decide it's an unnacceptable security problem, and say you'd be perfectly happy if there was aGUC to disable the potentially dangerous functions. This info would have been nice before feature freeze, but, OK, I appreciateyou're busy. Magnus updates the patch because he's yet another one of us that thinks this is useful functionality and adds the GUC yousaid would make you happy with these functions. You then state, with no discussion at all, that they're not going into 8.1 anyway, despite us doing everything you have asked. I have two questions if I may: 1) Is there any point us working on any kind of enhanced API for remote admin in the future, or will the same treatment begiven to that? 2) Do you now have sole say over what does and doesn't go into the project? I don't mean to be disrespectful - your hard work and skills are hugely appreciated by the whole community, but I know fora fact that a number of them, who between them have contributed thousands of hours and lines of code to the project (andI'm talking about the core project, never mind pgAdmin et al) cannot understand your apparent insistence on us not providingremote admin capabilities. I think we simply need clarification on how the project works these days. Regards, Dave
Am Samstag, 30. Juli 2005 16:39 schrieb Magnus Hagander: > * Added guc option "disable_remote_admin", that disables any write > operations (write, unlink, rename) even for the superuser. I think it would be better to avoid "double negatives", so the option might be better named "enable_remote_admin" with the inverted logic. -- Peter Eisentraut http://developer.postgresql.org/~petere/