danger of stats_temp_directory = /dev/shm

Jeff Janes escribió:

With the stats file split patch 187492b6c2e8cafc5 introduced in 9.3dev, now
after a crash the postmaster will try to delete all files in the directory
stats_temp_directory. When that is just a subdirectory of PGDATA, this is
fine. But it seems rather hostile when it is set to a shared directory,
like the popular /dev/shm.

Does this need to be fixed, or at least documented?

I think we need it fixed so that it only deletes the files matching a
well-known pattern. In fact, there's an XXX comment about this in the
code:

/* XXX should we try to ignore files other than the ones we write? */

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Alvaro Herrera <alvherre@2ndquadrant.com> writes:

Jeff Janes escribi�:

With the stats file split patch 187492b6c2e8cafc5 introduced in 9.3dev, now
after a crash the postmaster will try to delete all files in the directory
stats_temp_directory. When that is just a subdirectory of PGDATA, this is
fine. But it seems rather hostile when it is set to a shared directory,
like the popular /dev/shm.

Does this need to be fixed, or at least documented?

I think we need it fixed so that it only deletes the files matching a
well-known pattern.

I think we need it fixed to reject any stats_temp_directory that is not
postgres-owned with restrictive permissions. The problem here is not
with what it deletes, it's with the insanely insecure configuration.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 4/25/13 12:09 AM, Tom Lane wrote:

I think we need it fixed to reject any stats_temp_directory that is not
postgres-owned with restrictive permissions. The problem here is not
with what it deletes, it's with the insanely insecure configuration.

Yeah, the requirements should be similar to what initdb requires for
PGDATA and pg_xlog.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 04/25/2013 11:24 AM, Peter Eisentraut wrote:

On 4/25/13 12:09 AM, Tom Lane wrote:

I think we need it fixed to reject any stats_temp_directory that is not
postgres-owned with restrictive permissions. The problem here is not
with what it deletes, it's with the insanely insecure configuration.

Yeah, the requirements should be similar to what initdb requires for
PGDATA and pg_xlog.

Right.

I do think that best practice suggests using a dedicated ram drive
rather than /dev/shm. Here's an fstab entry I have used at one client's
site:

tmpfs /var/lib/pgsql/stats_tmp tmpfs
size=5G,uid=postgres,gid=postgres 0 0

I guess if we put in the sort of restrictions being suggested above I'd
add a mode argument to the mount options.

(This drive might seem large, but total RAM on this machine is 512Gb.)

cheers

andrew

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Thu, Apr 25, 2013 at 12:09 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Alvaro Herrera <alvherre@2ndquadrant.com> writes:

Jeff Janes escribió:

With the stats file split patch 187492b6c2e8cafc5 introduced in 9.3dev, now
after a crash the postmaster will try to delete all files in the directory
stats_temp_directory. When that is just a subdirectory of PGDATA, this is
fine. But it seems rather hostile when it is set to a shared directory,
like the popular /dev/shm.

Does this need to be fixed, or at least documented?

I think we need it fixed so that it only deletes the files matching a
well-known pattern.

I think we need it fixed to reject any stats_temp_directory that is not
postgres-owned with restrictive permissions. The problem here is not
with what it deletes, it's with the insanely insecure configuration.

Only deleting files matching the relevant pattern might not be a bad
idea either, though.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Thu, Apr 25, 2013 at 8:24 AM, Peter Eisentraut <peter_e@gmx.net> wrote:

On 4/25/13 12:09 AM, Tom Lane wrote:

I think we need it fixed to reject any stats_temp_directory that is not
postgres-owned with restrictive permissions. The problem here is not
with what it deletes, it's with the insanely insecure configuration.

Yeah, the requirements should be similar to what initdb requires for
PGDATA and pg_xlog.

Is this a blocker for 9.3?

If it is a concern of not what is deleted but rather that someone can
inject a poisoned stats file into the directory, does it need to be
back-patched all the way, as that could be done before the split
patch?

Cheers,

Jeff

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 08/13/2013 09:57 AM, Jeff Janes wrote:

Is this a blocker for 9.3?

Why would it be? This issue doesn't originate with 9.3.

If it is a concern of not what is deleted but rather that someone can
inject a poisoned stats file into the directory, does it need to be
back-patched all the way, as that could be done before the split
patch?

I'd say it's a backpatch. We'll need to warn the heck out of users, though.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Tuesday, August 13, 2013, Josh Berkus wrote:

On 08/13/2013 09:57 AM, Jeff Janes wrote:

Is this a blocker for 9.3?

Why would it be? This issue doesn't originate with 9.3.

Before 9.3, it would delete one specific file from a potentially shared
directory. In 9.3 it deletes the entire contents of a potentially shared
directory. That is a massive expansion in the surface area for
unintentional deletion. If we will disallow using shared directories
before the time 9.3 is released, that would fix it one way, but I don't
know if that is the plan or not.

Cheers,

Jeff

Jeff,

Before 9.3, it would delete one specific file from a potentially shared
directory. In 9.3 it deletes the entire contents of a potentially shared
directory. That is a massive expansion in the surface area for
unintentional deletion. If we will disallow using shared directories
before the time 9.3 is released, that would fix it one way, but I don't
know if that is the plan or not.

I can't see doing that. I can see adding the requirement for 9.3, and
then documenting it though.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Josh Berkus <josh@agliodbs.com> writes:

Before 9.3, it would delete one specific file from a potentially shared
directory. In 9.3 it deletes the entire contents of a potentially shared
directory. That is a massive expansion in the surface area for
unintentional deletion. If we will disallow using shared directories
before the time 9.3 is released, that would fix it one way, but I don't
know if that is the plan or not.

I can't see doing that. I can see adding the requirement for 9.3, and
then documenting it though.

I think we should change 9.3 to be restrictive about ownership/permissions
on the stats_temp_directory (ie, require owner = postgres user,
permissions = 0700, same as for the $PGDATA directory). I agree that
back-patching such a change to the older branches is probably not a good
plan. I can't quite parse what you say above, so I'm not sure if you're
fully agreeing with that position or not.

In addition to that, it might be a good idea to do what the comment in the
code suggests, namely do more than zero checking on each file name to try
to make sure it looks like a stats temp file name that we'd generate
before we delete it. The ownership/permissions test wouldn't be enough
to prevent you from pointing at, say, ~postgres and thereby losing some
files you'd rather not.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Tom Lane wrote:

I think we should change 9.3 to be restrictive about ownership/permissions
on the stats_temp_directory (ie, require owner = postgres user,
permissions = 0700, same as for the $PGDATA directory). I agree that
back-patching such a change to the older branches is probably not a good
plan. I can't quite parse what you say above, so I'm not sure if you're
fully agreeing with that position or not.

In addition to that, it might be a good idea to do what the comment in the
code suggests, namely do more than zero checking on each file name to try
to make sure it looks like a stats temp file name that we'd generate
before we delete it. The ownership/permissions test wouldn't be enough
to prevent you from pointing at, say, ~postgres and thereby losing some
files you'd rather not.

I will look into this.

--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Aug 15, 2013 3:44 AM, "Tom Lane" <tgl@sss.pgh.pa.us> wrote:

Josh Berkus <josh@agliodbs.com> writes:

Before 9.3, it would delete one specific file from a potentially shared
directory. In 9.3 it deletes the entire contents of a potentially

shared

directory. That is a massive expansion in the surface area for
unintentional deletion. If we will disallow using shared directories
before the time 9.3 is released, that would fix it one way, but I don't
know if that is the plan or not.

I can't see doing that. I can see adding the requirement for 9.3, and
then documenting it though.

I think we should change 9.3 to be restrictive about ownership/permissions
on the stats_temp_directory (ie, require owner = postgres user,
permissions = 0700, same as for the $PGDATA directory). I agree that
back-patching such a change to the older branches is probably not a good

+1

In addition to that, it might be a good idea to do what the comment in the
code suggests, namely do more than zero checking on each file name to try
to make sure it looks like a stats temp file name that we'd generate
before we delete it. The ownership/permissions test wouldn't be enough
to prevent you from pointing at, say, ~postgres and thereby losing some
files you'd rather not.

+1 on that as well. It shouldn't be that hard to do.

/Magnus

Tom Lane wrote:

I think we should change 9.3 to be restrictive about ownership/permissions
on the stats_temp_directory (ie, require owner = postgres user,
permissions = 0700, same as for the $PGDATA directory).

Not an easy thing to do, this. It should be done as a GUC check hook,
ISTM, but this doesn't work because the first time those are run we
haven't yet changed to the data directory, and so any relative path
(which the default value is) will cause the check to fail (I *assume*
setting an absolute path would work, but I haven't tried). We could
skip the check on the first run, and verify the directory separately in
PostmasterMain() after changing CWD, but I don't see any way to detect
that we're in the initial run of GUC processing. Any thoughts? Maybe
the idea of using a GUC check hook is flawed, but I don't think so
because we also need to verify a directory when the setting changes on
SIGHUP.

The implementation I chose for the actual check was to separate the
permission checks from checkDataDir() into src/port/pgcheckdir.c that
returns different error codes for each case; see first attachment.
This part seems pretty reasonable, except that the code should be in
src/common rather than src/port, but I believe the entire pgcheckdir.c
file should be moved.

In addition to that, it might be a good idea to do what the comment in the
code suggests, namely do more than zero checking on each file name to try
to make sure it looks like a stats temp file name that we'd generate
before we delete it. The ownership/permissions test wouldn't be enough
to prevent you from pointing at, say, ~postgres and thereby losing some
files you'd rather not.

This seems pretty simple to do; see second attachment. (It would delete
files named, "db_1234.tmpfoobar", that is, valid names with suffixes,
but I can't see that being a problem). (I haven't really tested this
part at all.)

--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Alvaro Herrera wrote:

In addition to that, it might be a good idea to do what the comment in the
code suggests, namely do more than zero checking on each file name to try
to make sure it looks like a stats temp file name that we'd generate
before we delete it. The ownership/permissions test wouldn't be enough
to prevent you from pointing at, say, ~postgres and thereby losing some
files you'd rather not.

This seems pretty simple to do; see second attachment. (It would delete
files named, "db_1234.tmpfoobar", that is, valid names with suffixes,
but I can't see that being a problem). (I haven't really tested this
part at all.)

Here's the second attachment.

--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Alvaro Herrera <alvherre@2ndquadrant.com> writes:

Alvaro Herrera wrote:

In addition to that, it might be a good idea to do what the comment in the
code suggests, namely do more than zero checking on each file name to try
to make sure it looks like a stats temp file name that we'd generate
before we delete it. The ownership/permissions test wouldn't be enough
to prevent you from pointing at, say, ~postgres and thereby losing some
files you'd rather not.

This seems pretty simple to do; see second attachment. (It would delete
files named, "db_1234.tmpfoobar", that is, valid names with suffixes,
but I can't see that being a problem). (I haven't really tested this
part at all.)

Here's the second attachment.

This looks good except that it can't tell "db_123.statfoo" isn't a match.
The scan limit/buffer size needs to be greater than the longest string
you care about, not only equal to. I think strcmp not strncmp would be
better coding, too. Please fix that and commit -- I think this part
is pretty noncontroversial.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Alvaro Herrera <alvherre@2ndquadrant.com> writes:

The implementation I chose for the actual check was to separate the
permission checks from checkDataDir() into src/port/pgcheckdir.c that
returns different error codes for each case; see first attachment.
This part seems pretty reasonable, except that the code should be in
src/common rather than src/port, but I believe the entire pgcheckdir.c
file should be moved.

s/CKDIR_TOOACCESIBLE/CKDIR_TOOACCESSIBLE/, and maybe use underscores
to separate the words in those names? Otherwise no objection. But
there's not much point in this unless we can figure out where to call
it from for the stat_directory case.

One possibility is to do the initial check somewhere shortly after
ChangeToDataDir(), and have the GUC check hook only attempt to make a
check in SIGHUP context. Unfortunately we aren't passing the context to
check hooks, only GucSource which isn't adequate for this. Not sure if we
want to go so far as to change the check-hook API at this point. We could
probably think of some other, klugy way to tell if it's initial startup.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2013-08-19 14:28:28 -0400, Tom Lane wrote:

One possibility is to do the initial check somewhere shortly after
ChangeToDataDir(), and have the GUC check hook only attempt to make a
check in SIGHUP context. Unfortunately we aren't passing the context to
check hooks, only GucSource which isn't adequate for this. Not sure if we
want to go so far as to change the check-hook API at this point. We could
probably think of some other, klugy way to tell if it's initial startup.

Is it even actually safe to have stats_temp_directory PGC_SIGHUP after
the per DB splitup? I haven't audited the code for it, but it seems
somewhat likely that we would end up with some files in the old and some
in the new directory?

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Andres Freund <andres@2ndquadrant.com> writes:

On 2013-08-19 14:28:28 -0400, Tom Lane wrote:

One possibility is to do the initial check somewhere shortly after
ChangeToDataDir(), and have the GUC check hook only attempt to make a
check in SIGHUP context. Unfortunately we aren't passing the context to
check hooks, only GucSource which isn't adequate for this. Not sure if we
want to go so far as to change the check-hook API at this point. We could
probably think of some other, klugy way to tell if it's initial startup.

Is it even actually safe to have stats_temp_directory PGC_SIGHUP after
the per DB splitup? I haven't audited the code for it, but it seems
somewhat likely that we would end up with some files in the old and some
in the new directory?

That's a good point. For the moment we could just change it to
PGC_POSTMASTER and eliminate this problem. Reducing it back to SIGHUP
would be a future feature, which is evidently less than trivial.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2013-08-19 13:50:38 -0400, Alvaro Herrera wrote:

Tom Lane wrote:

I think we should change 9.3 to be restrictive about ownership/permissions
on the stats_temp_directory (ie, require owner = postgres user,
permissions = 0700, same as for the $PGDATA directory).

Not an easy thing to do, this. It should be done as a GUC check hook,
ISTM, but this doesn't work because the first time those are run we
haven't yet changed to the data directory, and so any relative path
(which the default value is) will cause the check to fail (I *assume*
setting an absolute path would work, but I haven't tried). We could
skip the check on the first run, and verify the directory separately in
PostmasterMain() after changing CWD, but I don't see any way to detect
that we're in the initial run of GUC processing. Any thoughts? Maybe
the idea of using a GUC check hook is flawed, but I don't think so
because we also need to verify a directory when the setting changes on
SIGHUP.

Hm. Is a check like that actually sufficient? The idea of setting
stats_temp_directory to /dev/shm/postgres or similar in all of several
clusters on one machine doesn't seem to be that far fetched.

The only idea I have to prevent that is writing some minimal pg_control
like file into the temp stats directory iff it's empty. Then, when
reusing a stats temp directory, refuse to work unless it has the same
ids.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers