Re: pgsql: Keep pg_stat_statements' query texts in a file, not in shared me

Keep pg_stat_statements' query texts in a file, not in shared memory.

This change allows us to eliminate the previous limit on stored query
length, and it makes the shared-memory hash table very much smaller,
allowing more statements to be tracked. (The default value of
pg_stat_statements.max is therefore increased from 1000 to 5000.)
In typical scenarios, the hash table can be large enough to hold all the
statements commonly issued by an application, so that there is little
"churn" in the set of tracked statements, and thus little need to do I/O
to the file.

To further reduce the need for I/O to the query-texts file, add a way
to retrieve all the columns of the pg_stat_statements view except for
the query text column. This is probably not of much interest for human
use but it could be exploited by programs, which will prefer using the
queryid anyway.

Ordinarily, we'd need to bump the extension version number for the latter
change. But since we already advanced pg_stat_statements' version number
from 1.1 to 1.2 in the 9.4 development cycle, it seems all right to just
redefine what 1.2 means.

Peter Geoghegan, reviewed by Pavel Stehule

Branch
------
master

Details
-------
http://git.postgresql.org/pg/commitdiff/f0d6f20278b7c5c412ce40a9b86c6b31dc2fbfdd

Modified Files
--------------
.../pg_stat_statements--1.1--1.2.sql | 8 +-
.../pg_stat_statements/pg_stat_statements--1.2.sql | 8 +-
contrib/pg_stat_statements/pg_stat_statements.c | 1024 +++++++++++++++++---
doc/src/sgml/pgstatstatements.sgml | 80 +-
4 files changed, 929 insertions(+), 191 deletions(-)

Hi,

(2014/01/28 5:38), Tom Lane wrote:
> Keep pg_stat_statements' query texts in a file, not in shared memory.
This patch has security problem that root can easily see the statement file in
database cluster.
Is it OK or disscussed? If root user and operation user don't have access
privilege in their security policy,
they must not look statement-log at all. And they are not always database superuser.
Regards,
--
Mitsumasa KONDO
NTT Open Source Software Center

KONDO Mitsumasa <kondo(dot)mitsumasa(at)lab(dot)ntt(dot)co(dot)jp> writes:
> (2014/01/28 5:38), Tom Lane wrote:
>> Keep pg_stat_statements' query texts in a file, not in shared memory.

> This patch has security problem that root can easily see the statement file in
> database cluster.

What's your point? It's idle to imagine that root can't see anything and
everything that an unprivileged server is doing.

regards, tom lane

On Mon, Jan 27, 2014 at 5:12 PM, KONDO Mitsumasa
<kondo(dot)mitsumasa(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> This patch has security problem that root can easily see the statement file
> in database cluster.

By default, we always serialize statements along with their query
texts to disk on shutdown. Until May of 2012, pg_stat_statements
didn't bother unlinking on startup, and so the file with query texts
was always on the PGDATA filesystem. What's the difference?

--
Peter Geoghegan

Peter Geoghegan <pg(at)heroku(dot)com> writes:
> On Mon, Jan 27, 2014 at 5:12 PM, KONDO Mitsumasa
> <kondo(dot)mitsumasa(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>> This patch has security problem that root can easily see the statement file
>> in database cluster.

> By default, we always serialize statements along with their query
> texts to disk on shutdown. Until May of 2012, pg_stat_statements
> didn't bother unlinking on startup, and so the file with query texts
> was always on the PGDATA filesystem. What's the difference?

Root can certainly also look at query texts in shared memory, or for that
matter in the local memory of any process. So can anybody else running as
the postgres userid.

Also, current query texts are probably less interesting to an intruder
than the contents of the database itself, which is stored in the same
directory tree with the same permissions (0600) as the query-text file.

So I'm failing to detect any incremental increase in risk here. Anybody
who can read that file can already do pretty much whatever he wants with
either the server processes or the database contents.

regards, tom lane

On 01/27/2014 08:23 PM, Tom Lane wrote:
> Peter Geoghegan <pg(at)heroku(dot)com> writes:
>> On Mon, Jan 27, 2014 at 5:12 PM, KONDO Mitsumasa
>> <kondo(dot)mitsumasa(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>>> This patch has security problem that root can easily see the statement file
>>> in database cluster.
>> By default, we always serialize statements along with their query
>> texts to disk on shutdown. Until May of 2012, pg_stat_statements
>> didn't bother unlinking on startup, and so the file with query texts
>> was always on the PGDATA filesystem. What's the difference?
> Root can certainly also look at query texts in shared memory, or for that
> matter in the local memory of any process. So can anybody else running as
> the postgres userid.
>
> Also, current query texts are probably less interesting to an intruder
> than the contents of the database itself, which is stored in the same
> directory tree with the same permissions (0600) as the query-text file.
>
> So I'm failing to detect any incremental increase in risk here. Anybody
> who can read that file can already do pretty much whatever he wants with
> either the server processes or the database contents.
>
>

The query texts are particularly uninteresting since I assume the data
values in the query have already been mostly dissolved away by
pg_stat_statements.

cheers

andrew

On Mon, Jan 27, 2014 at 5:23 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Root can certainly also look at query texts in shared memory, or for that
> matter in the local memory of any process. So can anybody else running as
> the postgres userid.

I think that the concern may have had something to do with a
MAC-centric viewpoint (e.g. SELinux users), where bizarrely it doesn't
necessarily follow that root would be able to do any of those things.
But in that world, it is surely the security officer's responsibility
to make a special effort to meet those strange requirements. It's
totally orthogonal to our security model.

--
Peter Geoghegan

On Mon, Jan 27, 2014 at 5:34 PM, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
> The query texts are particularly uninteresting since I assume the data
> values in the query have already been mostly dissolved away by
> pg_stat_statements.

Actually, it is possible for the query string to still have constants
if things are timed just right. I do see it in the wild, albeit very
infrequently.

--
Peter Geoghegan

(2014/01/28 10:15), Peter Geoghegan wrote:
> On Mon, Jan 27, 2014 at 5:12 PM, KONDO Mitsumasa
> <kondo(dot)mitsumasa(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>> This patch has security problem that root can easily see the statement file
>> in database cluster.
>
> By default, we always serialize statements along with their query
> texts to disk on shutdown. Until May of 2012, pg_stat_statements
> didn't bother unlinking on startup, and so the file with query texts
> was always on the PGDATA filesystem. What's the difference?
It is written in documents; "For security reasons, non-superusers are not allowed
to see the text of queries executed by other users." Is root user superuser? And
initdb user might change to non-superuser after creating database cluster. In
japan, database operation user isn't always database admin. Because database
admin's salary is expensive than system operator's.

I test pg_stat_statement in PG9.1.0 that is released at 08/09/2011. But I cannot
see pg_stat_statement query at external text.. Can you tell me where is it?
I think it is in database file and is protected by postgres authority.

Regards,
--
Mitsumasa KONDO
NTT Open Source Software Center

On Mon, Jan 27, 2014 at 6:04 PM, KONDO Mitsumasa
<kondo(dot)mitsumasa(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> It is written in documents; "For security reasons, non-superusers are not
> allowed to see the text of queries executed by other users." Is root user
> superuser? And initdb user might change to non-superuser after creating
> database cluster. In japan, database operation user isn't always database
> admin. Because database admin's salary is expensive than system operator's.

initdb will not run as a superuser to begin with. It flatly refuses.

Why is your concern with pg_stat_statements after this patch in particular?

You'll need to serialize the file at least once before seeing it, but
then it's there for good (on old versions, before Magnus got annoyed
that that affected basebackups).
--
Peter Geoghegan

Peter Geoghegan <pg(at)heroku(dot)com> writes:
> On Mon, Jan 27, 2014 at 5:34 PM, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
>> The query texts are particularly uninteresting since I assume the data
>> values in the query have already been mostly dissolved away by
>> pg_stat_statements.

> Actually, it is possible for the query string to still have constants
> if things are timed just right. I do see it in the wild, albeit very
> infrequently.

"Timed just right"? I could see it possibly happening due to queryid
collisions, but I'm not seeing how it would happen absent such a hash
collision.

regards, tom lane

(2014/01/28 10:23), Tom Lane wrote:
> Peter Geoghegan <pg(at)heroku(dot)com> writes:
>> On Mon, Jan 27, 2014 at 5:12 PM, KONDO Mitsumasa
>> <kondo(dot)mitsumasa(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>>> This patch has security problem that root can easily see the statement file
>>> in database cluster.
>
>> By default, we always serialize statements along with their query
>> texts to disk on shutdown. Until May of 2012, pg_stat_statements
>> didn't bother unlinking on startup, and so the file with query texts
>> was always on the PGDATA filesystem. What's the difference?
>
> Root can certainly also look at query texts in shared memory, or for that
> matter in the local memory of any process. So can anybody else running as
> the postgres userid.
This assumption is too hacker...

> Also, current query texts are probably less interesting to an intruder
> than the contents of the database itself, which is stored in the same
> directory tree with the same permissions (0600) as the query-text file.
Yes, that's right. However, table name or function name might be include sequrity
information. When we consult my client which needs high sequrity, they replace
function name or table name to other by using regular expression.

I still think this feature may cause sequrity problem, and we need to discuss
about it, or add document in detail.

Regards,
--
Mitsumasa KONDO
NTT Open Source Software Center

KONDO Mitsumasa <kondo(dot)mitsumasa(at)lab(dot)ntt(dot)co(dot)jp> writes:
> (2014/01/28 10:23), Tom Lane wrote:
>> Also, current query texts are probably less interesting to an intruder
>> than the contents of the database itself, which is stored in the same
>> directory tree with the same permissions (0600) as the query-text file.

> Yes, that's right. However, table name or function name might be include sequrity
> information. When we consult my client which needs high sequrity, they replace
> function name or table name to other by using regular expression.

So? Those names also appear in cleartext in the files corresponding to
system catalogs. If you're concerned about unauthorized access to files
in the database directory tree, you are concerned about something that
is outside Postgres' ability to defend against. You might consider
keeping the database files in an encrypted filesystem or some such
(not that that's likely to save you against an attacker who has root).

> I still think this feature may cause sequrity problem, and we need to discuss
> about it, or add document in detail.

You've offered not one credible argument in support of that position.

regards, tom lane

(2014/01/28 11:07), Peter Geoghegan wrote:
> On Mon, Jan 27, 2014 at 6:04 PM, KONDO Mitsumasa
> <kondo(dot)mitsumasa(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>> It is written in documents; "For security reasons, non-superusers are not
>> allowed to see the text of queries executed by other users." Is root user
>> superuser? And initdb user might change to non-superuser after creating
>> database cluster. In japan, database operation user isn't always database
>> admin. Because database admin's salary is expensive than system operator's.
>
> initdb will not run as a superuser to begin with. It flatly refuses.
No. I don't say root user is superuser. Executing initdb user will be postgres
superuser. But it can change non-superuser after creating database.

> Why is your concern with pg_stat_statements after this patch in particular?
>
> You'll need to serialize the file at least once before seeing it, but
> then it's there for good (on old versions, before Magnus got annoyed
> that that affected basebackups).
I feel the sense of incongruity that is stored database data in text file.
I'd like to hear from other people...

Regards,
--
Mitsumasa KONDO
NTT Open Source Software Center

On Mon, Jan 27, 2014 at 6:08 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> "Timed just right"? I could see it possibly happening due to queryid
> collisions, but I'm not seeing how it would happen absent such a hash
> collision.

Consider what happens when there is a pg_stat_statements_reset() call
query after another query's parse analysis, but before its execution
finishes. That's one obvious way. But you don't even need a reset - a
badly timed entry_dealloc() could do it too.

I don't see what hash collisions have to do with it, though.

--
Peter Geoghegan

On Mon, Jan 27, 2014 at 9:31 PM, KONDO Mitsumasa
<kondo(dot)mitsumasa(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>> Why is your concern with pg_stat_statements after this patch in
>> particular?
>>
>> You'll need to serialize the file at least once before seeing it, but
>> then it's there for good (on old versions, before Magnus got annoyed
>> that that affected basebackups).
>
> I feel the sense of incongruity that is stored database data in text file.
> I'd like to hear from other people...

OK, I'll bite: I have no idea why you think that's a problem. The
entire database is stored in a bunch of files in the same directory
tree as that text file. If somebody can read the text file, they can
probably also read the entire contents of the database. Sure, the
actual relations are binary data rather than *text* files, but if you
can read them, you can *most definitely* extract the whole database
contents, which is several orders of magnitude worse than anything you
can do with pg_stat_statements.

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

On Mon, Jan 27, 2014 at 6:31 PM, KONDO Mitsumasa
<kondo(dot)mitsumasa(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> No. I don't say root user is superuser. Executing initdb user will be
> postgres superuser. But it can change non-superuser after creating database.

Okay. I still don't understand what your point is, or how this patch
makes any worse what you'd consider to be a general problem. It
doesn't even differ from a security standpoint to the original
pg_stat_statements from 2009.

> I feel the sense of incongruity that is stored database data in text file.
> I'd like to hear from other people...

I think it's incongruous that you chose to make your opinion known at
this time and in this way. You knew about this patch several months
ago; are your surprised that it does what it was prominently
advertised to do?

--
Peter Geoghegan

Then I will post my response.

On Mon, Jan 27, 2014 at 8:54 PM, Peter Geoghegan <pg(at)heroku(dot)com> wrote:
> On Mon, Jan 27, 2014 at 8:20 PM, KONDO Mitsumasa
> <kondo(dot)mitsumasa(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>> At least, only postgres superuser can see pg_stat_statemnet view in old
>> version.
>> And you should change document at this sentences.
>
> No, it was precisely the same situation in every way that matters; the
> texts and other details were serialized to disk, on the same
> filesystem, with the same permissions. The only difference was that
> the texts might not be completely current back then, because time can
> pass between shutdown/serialization. From a security perspective,
> that's obviously 100% equivalent. Now, if you're saying that the
> option to not store the texts existed back then and that made a big
> difference, I'm completely unconvinced. That is not the security model
> that we follow. How could it be? You're asking us to believe that
> there is an environment in which statement execution costs and
> normalized query strings are more confidential than *all* of the
> actual data stored in the database. You're proposing that for some
> users the former is top secret, but the latter is somewhat less
> confidential, so it matters that root can also read query texts, a
> user already naturally entitled to read *all* data in the database.
> You have forced me to say what I would have preferred not to: that's
> nonsense, pure and simple.
>
>> IMHO, I have thought your approach is very rough and have some problems.
>> Therefore, I thought
>> it will be return with feed back by Tom.
>> I'm not sure about your patch in detail.
>
> How did you know it was very rough without reading it?
>
>> However, I think your patch have
>> another
>> porblem that is happened in lessor write-back cache enviroment systems
>> which are
>> like Windows system. It may cause extreme less performance in these systems.
>> Did
>> you test it? When we use shared_buffers, it does not let you do physical-
>> disk-write untill we want to write it. But your patch cannot control it, it
>> may
>> cause more lessor performance than linux systems. It will be less
>> performance
>> than removing LWLock. And your patch might works well only at linux system
>> or
>> having good write-back cache enviroment systems.
>
> This is completely fatuous. Tom spent a number of days scrutinizing it
> in detail. Perhaps you should actually read the patch, or read the
> discussion, and then you'll know why he concluded that my approach was
> fundamentally sound. If you have a filesystem that can see huge spikes
> in latency due to a write() of a relatively tiny query text, you have
> many problems, but blocking other queries that need to record
> statement execution costs in the shared hashtable actually isn't one
> of them.

--
Peter Geoghegan

Sorry, I forgot to add pgsql-commiters email adress. So I re-post our e-mail
discussion.

> (2014/01/28 11:50), Peter Geoghegan wrote:
>> On Mon, Jan 27, 2014 at 6:31 PM, KONDO Mitsumasa
>> <kondo(dot)mitsumasa(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>>> No. I don't say root user is superuser. Executing initdb user will be
>>> postgres superuser. But it can change non-superuser after creating database.
>>
>> Okay. I still don't understand what your point is, or how this patch
>> makes any worse what you'd consider to be a general problem. It
>> doesn't even differ from a security standpoint to the original
>> pg_stat_statements from 2009.
> At least, only postgres superuser can see pg_stat_statemnet view in old version.
> And you should change document at this sentences.
>
> By the way, when we set postgres log to only syslog, I think that it is because
> we don't show statement log to outside when we get backup or other operation. But
> your patch doesn't have option that is on or off. So user must show statement
> log to outside without what to do. I think that Robert and Tom saying is right,
> when we can see database file we can see the whole data. However, even so, it
> feels sense of incongruity to make most of all statement logs are always open
> with a text format every time.
>
>> I think it's incongruous that you chose to make your opinion known at
>> this time and in this way. You knew about this patch several months
>> ago; are your surprised that it does what it was prominently
>> advertised to do?
> IMHO, I have thought your approach is very rough and have some problems. Therefore, I thought
> it will be return with feed back by Tom.
>
> I'm not sure about your patch in detail. However, I think your patch have another
> porblem that is happened in lessor write-back cache enviroment systems which are
> like Windows system. It may cause extreme less performance in these systems. Did
> you test it? When we use shared_buffers, it does not let you do physical-
> disk-write untill we want to write it. But your patch cannot control it, it may
> cause more lessor performance than linux systems. It will be less performance
> than removing LWLock. And your patch might works well only at linux system or
> having good write-back cache enviroment systems.
>
> I'm very sorry for my late comment. If community say that is no problem, I
> will sincerely accept this patch.
>
> Regards,
> --
> Mitsumasa KONDO
> NTT Open Source Software Center
--
Mitsumasa KONDO
NTT Open Source Software Center

And it was from Peter.

> On Mon, Jan 27, 2014 at 8:20 PM, KONDO Mitsumasa
> <kondo(dot)mitsumasa(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>> At least, only postgres superuser can see pg_stat_statemnet view in old
>> version.
>> And you should change document at this sentences.
>
> No, it was precisely the same situation in every way that matters; the
> texts and other details were serialized to disk, on the same
> filesystem, with the same permissions. The only difference was that
> the texts might not be completely current back then, because time can
> pass between shutdown/serialization. From a security perspective,
> that's obviously 100% equivalent. Now, if you're saying that the
> option to not store the texts existed back then and that made a big
> difference, I'm completely unconvinced. That is not the security model
> that we follow. How could it be? You're asking us to believe that
> there is an environment in which statement execution costs and
> normalized query strings are more confidential than *all* of the
> actual data stored in the database. You're proposing that for some
> users the former is top secret, but the latter is somewhat less
> confidential, so it matters that root can also read query texts, a
> user already naturally entitled to read *all* data in the database.
> You have forced me to say what I would have preferred not to: that's
> nonsense, pure and simple.
>
>> IMHO, I have thought your approach is very rough and have some problems.
>> Therefore, I thought
>> it will be return with feed back by Tom.
>> I'm not sure about your patch in detail.
>
> How did you know it was very rough without reading it?
>
>> However, I think your patch have
>> another
>> porblem that is happened in lessor write-back cache enviroment systems
>> which are
>> like Windows system. It may cause extreme less performance in these systems.
>> Did
>> you test it? When we use shared_buffers, it does not let you do physical-
>> disk-write untill we want to write it. But your patch cannot control it, it
>> may
>> cause more lessor performance than linux systems. It will be less
>> performance
>> than removing LWLock. And your patch might works well only at linux system
>> or
>> having good write-back cache enviroment systems.
>
> This is completely fatuous. Tom spent a number of days scrutinizing it
> in detail. Perhaps you should actually read the patch, or read the
> discussion, and then you'll know why he concluded that my approach was
> fundamentally sound. If you have a filesystem that can see huge spikes
> in latency due to a write() of a relatively tiny query text, you have
> many problems, but blocking other queries that need to record
> statement execution costs in the shared hashtable actually isn't one
> of them.
>
> -- Peter Geoghegan

--
Mitsumasa KONDO
NTT Open Source Software Center