Identifying user-created objects

On Wed, Feb 5, 2020 at 8:27 PM Masahiko Sawada
<masahiko.sawada@2ndquadrant.com> wrote:

User can create database objects such as functions into pg_catalog.
But if I'm not missing something, currently there is no
straightforward way to identify if the object is a user created object
or a system object which is created during initdb. If we can do that
user will be able to check if malicious functions are not created in
the database, which is important from the security perspective.

I've attached PoC patch to introduce a SQL function
pg_is_user_object() that returns true if the given oid is user object
oid, that is greater than or equal to FirstNormalObjectId. Feedback is
very welcome.

+1.

About the implementation, how about defining a static inline function,
say is_user_object(), next to FirstNormalObjectId's definition and
make pg_is_user_object() call it? There are a few placed in the
backend code that perform the same computation as pg_is_user_object(),
which could be changed to use is_user_object() instead.

Thanks,
Amit

On Thu, Feb 06, 2020 at 04:25:47PM +0900, Amit Langote wrote:

About the implementation, how about defining a static inline function,
say is_user_object(), next to FirstNormalObjectId's definition and
make pg_is_user_object() call it? There are a few placed in the
backend code that perform the same computation as pg_is_user_object(),
which could be changed to use is_user_object() instead.

FWIW, if we bother adding SQL functions for that, my first impression
was to have three functions, each one of them returning:
- FirstNormalObjectId
- FirstGenbkiObjectId
- FirstNormalObjectId
Perhaps you should add a minimal set of regression tests in the
patch.
--
Michael

On Thu, Feb 6, 2020 at 4:31 PM Michael Paquier <michael@paquier.xyz> wrote:

On Thu, Feb 06, 2020 at 04:25:47PM +0900, Amit Langote wrote:

About the implementation, how about defining a static inline function,
say is_user_object(), next to FirstNormalObjectId's definition and
make pg_is_user_object() call it? There are a few placed in the
backend code that perform the same computation as pg_is_user_object(),
which could be changed to use is_user_object() instead.

FWIW, if we bother adding SQL functions for that, my first impression
was to have three functions, each one of them returning:
- FirstNormalObjectId
- FirstGenbkiObjectId
- FirstNormalObjectId

Did you miss FirstBootstrapObjectId by any chance?

I see the following ranges as defined in transam.h.

1-(FirstGenbkiObjectId - 1): manually assigned OIDs
FirstGenbkiObjectId-(FirstBootstrapObjectId - 1): genbki.pl assigned OIDs
FirstBootstrapObjectId-(FirstNormalObjectId - 1): initdb requested
FirstNormalObjectId or greater: user-defined objects

Sawada-san's proposal covers #4. Do we need an SQL function for the
first three? IOW, would the distinction between OIDs belonging to the
first three ranges be of interest to anyone except core PG hackers?

Thanks,
Amit

On Thu, Feb 6, 2020 at 8:53 AM Amit Langote <amitlangote09@gmail.com> wrote:

On Thu, Feb 6, 2020 at 4:31 PM Michael Paquier <michael@paquier.xyz> wrote:

On Thu, Feb 06, 2020 at 04:25:47PM +0900, Amit Langote wrote:

About the implementation, how about defining a static inline function,
say is_user_object(), next to FirstNormalObjectId's definition and
make pg_is_user_object() call it? There are a few placed in the
backend code that perform the same computation as pg_is_user_object(),
which could be changed to use is_user_object() instead.

FWIW, if we bother adding SQL functions for that, my first impression
was to have three functions, each one of them returning:
- FirstNormalObjectId
- FirstGenbkiObjectId
- FirstNormalObjectId

Did you miss FirstBootstrapObjectId by any chance?

I see the following ranges as defined in transam.h.

1-(FirstGenbkiObjectId - 1): manually assigned OIDs
FirstGenbkiObjectId-(FirstBootstrapObjectId - 1): genbki.pl assigned OIDs
FirstBootstrapObjectId-(FirstNormalObjectId - 1): initdb requested
FirstNormalObjectId or greater: user-defined objects

Sawada-san's proposal covers #4. Do we need an SQL function for the
first three? IOW, would the distinction between OIDs belonging to the
first three ranges be of interest to anyone except core PG hackers?

+1 for #4, but I'm not sure that the other 3 are really interesting to
have at SQL level.

On Thu, Feb 06, 2020 at 04:52:48PM +0900, Amit Langote wrote:

On Thu, Feb 6, 2020 at 4:31 PM Michael Paquier <michael@paquier.xyz> wrote:

FWIW, if we bother adding SQL functions for that, my first impression
was to have three functions, each one of them returning:
- FirstNormalObjectId
- FirstGenbkiObjectId
- FirstNormalObjectId

Did you miss FirstBootstrapObjectId by any chance?

Yep, incorrect copy-pasto.
--
Michael

On Thu, 6 Feb 2020 at 16:53, Amit Langote <amitlangote09@gmail.com> wrote:

On Thu, Feb 6, 2020 at 4:31 PM Michael Paquier <michael@paquier.xyz> wrote:

On Thu, Feb 06, 2020 at 04:25:47PM +0900, Amit Langote wrote:

About the implementation, how about defining a static inline function,
say is_user_object(), next to FirstNormalObjectId's definition and
make pg_is_user_object() call it? There are a few placed in the
backend code that perform the same computation as pg_is_user_object(),
which could be changed to use is_user_object() instead.

FWIW, if we bother adding SQL functions for that, my first impression
was to have three functions, each one of them returning:
- FirstNormalObjectId
- FirstGenbkiObjectId
- FirstNormalObjectId

Did you miss FirstBootstrapObjectId by any chance?

I see the following ranges as defined in transam.h.

1-(FirstGenbkiObjectId - 1): manually assigned OIDs
FirstGenbkiObjectId-(FirstBootstrapObjectId - 1): genbki.pl assigned OIDs
FirstBootstrapObjectId-(FirstNormalObjectId - 1): initdb requested
FirstNormalObjectId or greater: user-defined objects

Sawada-san's proposal covers #4. Do we need an SQL function for the
first three? IOW, would the distinction between OIDs belonging to the
first three ranges be of interest to anyone except core PG hackers?

Yeah I thought of these three values but I'm also not sure it's worth for users.

If we have these functions returning the values respectively, when we
want to check if an oid is assigned during initdb we will end up with
doing something like 'WHERE oid >= pg_first_bootstrap_oid() and oid <
pg_first_normal_oid()', which is not intuitive, I think. Users have to
remember the order of these values.

Regards,

--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Thu, 6 Feb 2020 at 17:18, Masahiko Sawada
<masahiko.sawada@2ndquadrant.com> wrote:

On Thu, 6 Feb 2020 at 16:53, Amit Langote <amitlangote09@gmail.com> wrote:

On Thu, Feb 6, 2020 at 4:31 PM Michael Paquier <michael@paquier.xyz> wrote:

On Thu, Feb 06, 2020 at 04:25:47PM +0900, Amit Langote wrote:

About the implementation, how about defining a static inline function,
say is_user_object(), next to FirstNormalObjectId's definition and
make pg_is_user_object() call it? There are a few placed in the
backend code that perform the same computation as pg_is_user_object(),
which could be changed to use is_user_object() instead.

FWIW, if we bother adding SQL functions for that, my first impression
was to have three functions, each one of them returning:
- FirstNormalObjectId
- FirstGenbkiObjectId
- FirstNormalObjectId

Did you miss FirstBootstrapObjectId by any chance?

I see the following ranges as defined in transam.h.

1-(FirstGenbkiObjectId - 1): manually assigned OIDs
FirstGenbkiObjectId-(FirstBootstrapObjectId - 1): genbki.pl assigned OIDs
FirstBootstrapObjectId-(FirstNormalObjectId - 1): initdb requested
FirstNormalObjectId or greater: user-defined objects

Sawada-san's proposal covers #4. Do we need an SQL function for the
first three? IOW, would the distinction between OIDs belonging to the
first three ranges be of interest to anyone except core PG hackers?

Yeah I thought of these three values but I'm also not sure it's worth for users.

If we have these functions returning the values respectively, when we
want to check if an oid is assigned during initdb we will end up with
doing something like 'WHERE oid >= pg_first_bootstrap_oid() and oid <
pg_first_normal_oid()', which is not intuitive, I think. Users have to
remember the order of these values.

Attached the updated version patch that includes regression tests. And
I have registered this to the next commit fest.

Regards,

--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Sawada-san,

On Mon, Feb 10, 2020 at 12:25 PM Masahiko Sawada
<masahiko.sawada@2ndquadrant.com> wrote:

On Thu, Feb 06, 2020 at 04:25:47PM +0900, Amit Langote wrote:

About the implementation, how about defining a static inline function,
say is_user_object(), next to FirstNormalObjectId's definition and
make pg_is_user_object() call it? There are a few placed in the
backend code that perform the same computation as pg_is_user_object(),
which could be changed to use is_user_object() instead.

Attached the updated version patch that includes regression tests. And
I have registered this to the next commit fest.

Thank you.

Any thoughts on the above suggestion?

Regards,
Amit

On Mon, 10 Feb 2020 at 12:54, Amit Langote <amitlangote09@gmail.com> wrote:

Sawada-san,

On Mon, Feb 10, 2020 at 12:25 PM Masahiko Sawada
<masahiko.sawada@2ndquadrant.com> wrote:

On Thu, Feb 06, 2020 at 04:25:47PM +0900, Amit Langote wrote:

About the implementation, how about defining a static inline function,
say is_user_object(), next to FirstNormalObjectId's definition and
make pg_is_user_object() call it? There are a few placed in the
backend code that perform the same computation as pg_is_user_object(),
which could be changed to use is_user_object() instead.

Attached the updated version patch that includes regression tests. And
I have registered this to the next commit fest.

Thank you.

Any thoughts on the above suggestion?

Oops, I had overlooked it. I agree with you.

How about having it as a macro like:

#define ObjectIdIsUserObject(oid) ((Oid)(oid) >= FirstNormalObjectId)

Regards,

--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Mon, Feb 10, 2020 at 1:06 PM Masahiko Sawada
<masahiko.sawada@2ndquadrant.com> wrote:

On Mon, 10 Feb 2020 at 12:54, Amit Langote <amitlangote09@gmail.com> wrote:

Sawada-san,

On Mon, Feb 10, 2020 at 12:25 PM Masahiko Sawada
<masahiko.sawada@2ndquadrant.com> wrote:

On Thu, Feb 06, 2020 at 04:25:47PM +0900, Amit Langote wrote:

About the implementation, how about defining a static inline function,
say is_user_object(), next to FirstNormalObjectId's definition and
make pg_is_user_object() call it? There are a few placed in the
backend code that perform the same computation as pg_is_user_object(),
which could be changed to use is_user_object() instead.

Attached the updated version patch that includes regression tests. And
I have registered this to the next commit fest.

Thank you.

Any thoughts on the above suggestion?

Oops, I had overlooked it. I agree with you.

How about having it as a macro like:

#define ObjectIdIsUserObject(oid) ((Oid)(oid) >= FirstNormalObjectId)

I'm fine with a macro.

Thanks,
Amit

On Mon, Feb 10, 2020 at 01:16:30PM +0900, Amit Langote wrote:

On Mon, Feb 10, 2020 at 1:06 PM Masahiko Sawada
<masahiko.sawada@2ndquadrant.com> wrote:

How about having it as a macro like:

#define ObjectIdIsUserObject(oid) ((Oid)(oid) >= FirstNormalObjectId)

I'm fine with a macro.

I am not sure that it is worth having one extra abstraction layer for
that.
--
Michael

On Mon, 10 Feb 2020 at 14:09, Michael Paquier <michael@paquier.xyz> wrote:

On Mon, Feb 10, 2020 at 01:16:30PM +0900, Amit Langote wrote:

On Mon, Feb 10, 2020 at 1:06 PM Masahiko Sawada
<masahiko.sawada@2ndquadrant.com> wrote:

How about having it as a macro like:

#define ObjectIdIsUserObject(oid) ((Oid)(oid) >= FirstNormalObjectId)

I'm fine with a macro.

I am not sure that it is worth having one extra abstraction layer for
that.

Hmm I'm not going to insist on that but I thought that it could
somewhat improve readability at places where they already compares an
oid to FirstNormalObjectId as Amit mentioned:

src/backend/catalog/pg_publication.c: relid >= FirstNormalObjectId;
src/backend/utils/adt/json.c: if (typoid >= FirstNormalObjectId)
src/backend/utils/adt/jsonb.c: if (typoid >= FirstNormalObjectId)

Regards,

--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Mon, Feb 10, 2020 at 2:23 PM Masahiko Sawada
<masahiko.sawada@2ndquadrant.com> wrote:

On Mon, 10 Feb 2020 at 14:09, Michael Paquier <michael@paquier.xyz> wrote:

On Mon, Feb 10, 2020 at 01:16:30PM +0900, Amit Langote wrote:

On Mon, Feb 10, 2020 at 1:06 PM Masahiko Sawada
<masahiko.sawada@2ndquadrant.com> wrote:

How about having it as a macro like:

#define ObjectIdIsUserObject(oid) ((Oid)(oid) >= FirstNormalObjectId)

I'm fine with a macro.

I am not sure that it is worth having one extra abstraction layer for
that.

Hmm I'm not going to insist on that but I thought that it could
somewhat improve readability at places where they already compares an
oid to FirstNormalObjectId as Amit mentioned:

src/backend/catalog/pg_publication.c: relid >= FirstNormalObjectId;
src/backend/utils/adt/json.c: if (typoid >= FirstNormalObjectId)
src/backend/utils/adt/jsonb.c: if (typoid >= FirstNormalObjectId)

Agree that ObjectIsUserObject(oid) is easier to read than oid >=
FirstNormalObject. I would have not bothered, for example, if it was
something like oid >= FirstUserObjectId to begin with.

Thanks,
Amit

At Mon, 10 Feb 2020 14:32:44 +0900, Amit Langote <amitlangote09@gmail.com> wrote in

On Mon, Feb 10, 2020 at 2:23 PM Masahiko Sawada
<masahiko.sawada@2ndquadrant.com> wrote:

On Mon, 10 Feb 2020 at 14:09, Michael Paquier <michael@paquier.xyz> wrote:

On Mon, Feb 10, 2020 at 01:16:30PM +0900, Amit Langote wrote:

On Mon, Feb 10, 2020 at 1:06 PM Masahiko Sawada
<masahiko.sawada@2ndquadrant.com> wrote:

How about having it as a macro like:

#define ObjectIdIsUserObject(oid) ((Oid)(oid) >= FirstNormalObjectId)

I'm fine with a macro.

I am not sure that it is worth having one extra abstraction layer for
that.

Hmm I'm not going to insist on that but I thought that it could
somewhat improve readability at places where they already compares an
oid to FirstNormalObjectId as Amit mentioned:

src/backend/catalog/pg_publication.c: relid >= FirstNormalObjectId;
src/backend/utils/adt/json.c: if (typoid >= FirstNormalObjectId)
src/backend/utils/adt/jsonb.c: if (typoid >= FirstNormalObjectId)

Agree that ObjectIsUserObject(oid) is easier to read than oid >=
FirstNormalObject. I would have not bothered, for example, if it was
something like oid >= FirstUserObjectId to begin with.

Aside from the naming, I'm not sure it's sensible to use
FirstNormalObjectId since I don't see a clear definition or required
characteristics for "user created objects" is. If we did CREATE
TABLE, FUNCTION or maybe any objects during single-user mode before
the first object is created during normal multiuser operation, the
"user-created(or not?)" object has an OID less than
FirstNormalObjectId. If such objects are the "user created object", we
need FirstUserObjectId defferent from FirstNormalObjectId.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Thu, Feb 13, 2020 at 10:30 AM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:

At Mon, 10 Feb 2020 14:32:44 +0900, Amit Langote <amitlangote09@gmail.com> wrote in

Agree that ObjectIsUserObject(oid) is easier to read than oid >=
FirstNormalObject. I would have not bothered, for example, if it was
something like oid >= FirstUserObjectId to begin with.

Aside from the naming, I'm not sure it's sensible to use
FirstNormalObjectId since I don't see a clear definition or required
characteristics for "user created objects" is. If we did CREATE
TABLE, FUNCTION or maybe any objects during single-user mode before
the first object is created during normal multiuser operation, the
"user-created(or not?)" object has an OID less than
FirstNormalObjectId. If such objects are the "user created object", we
need FirstUserObjectId defferent from FirstNormalObjectId.

Interesting observation. Connecting to database in --single mode,
whether done using initdb or directly, is always considered
"bootstrapping", so the OIDs from the bootstrapping range are
consumed.

$ postgres --single -D pgdata postgres

PostgreSQL stand-alone backend 13devel
backend> create table a (a int);
backend> select 'a'::regclass::oid;
1: oid (typeid = 26, len = 4, typmod = -1, byval = t)
----
1: oid = "14168" (typeid = 26, len = 4, typmod = -1, byval = t)

Here, FirstBootstrapObjectId < 14168 < FirstNormalObjectId

Maybe we could document that pg_is_user_object() and its internal
counterpart returns true only for objects that are created during
"normal" multi-user database operation.

Thanks,
Amit

On Thu, Feb 13, 2020 at 8:32 AM Amit Langote <amitlangote09@gmail.com> wrote:

On Thu, Feb 13, 2020 at 10:30 AM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:

At Mon, 10 Feb 2020 14:32:44 +0900, Amit Langote <amitlangote09@gmail.com> wrote in

Agree that ObjectIsUserObject(oid) is easier to read than oid >=
FirstNormalObject. I would have not bothered, for example, if it was
something like oid >= FirstUserObjectId to begin with.

Aside from the naming, I'm not sure it's sensible to use
FirstNormalObjectId since I don't see a clear definition or required
characteristics for "user created objects" is. If we did CREATE
TABLE, FUNCTION or maybe any objects during single-user mode before
the first object is created during normal multiuser operation, the
"user-created(or not?)" object has an OID less than
FirstNormalObjectId. If such objects are the "user created object", we
need FirstUserObjectId defferent from FirstNormalObjectId.

Interesting observation. Connecting to database in --single mode,
whether done using initdb or directly, is always considered
"bootstrapping", so the OIDs from the bootstrapping range are
consumed.

$ postgres --single -D pgdata postgres

PostgreSQL stand-alone backend 13devel
backend> create table a (a int);
backend> select 'a'::regclass::oid;
1: oid (typeid = 26, len = 4, typmod = -1, byval = t)
----
1: oid = "14168" (typeid = 26, len = 4, typmod = -1, byval = t)

Here, FirstBootstrapObjectId < 14168 < FirstNormalObjectId

FTR it's also possible to get the same result using binary mode and
binary_upgrade_set_next_XXX functions.

Maybe we could document that pg_is_user_object() and its internal
counterpart returns true only for objects that are created during
"normal" multi-user database operation.

+1

On Thu, 13 Feb 2020 at 17:13, Julien Rouhaud <rjuju123@gmail.com> wrote:

On Thu, Feb 13, 2020 at 8:32 AM Amit Langote <amitlangote09@gmail.com> wrote:

On Thu, Feb 13, 2020 at 10:30 AM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:

At Mon, 10 Feb 2020 14:32:44 +0900, Amit Langote <amitlangote09@gmail.com> wrote in

Agree that ObjectIsUserObject(oid) is easier to read than oid >=
FirstNormalObject. I would have not bothered, for example, if it was
something like oid >= FirstUserObjectId to begin with.

Aside from the naming, I'm not sure it's sensible to use
FirstNormalObjectId since I don't see a clear definition or required
characteristics for "user created objects" is. If we did CREATE
TABLE, FUNCTION or maybe any objects during single-user mode before
the first object is created during normal multiuser operation, the
"user-created(or not?)" object has an OID less than
FirstNormalObjectId. If such objects are the "user created object", we
need FirstUserObjectId defferent from FirstNormalObjectId.

Interesting observation. Connecting to database in --single mode,
whether done using initdb or directly, is always considered
"bootstrapping", so the OIDs from the bootstrapping range are
consumed.

$ postgres --single -D pgdata postgres

PostgreSQL stand-alone backend 13devel
backend> create table a (a int);
backend> select 'a'::regclass::oid;
1: oid (typeid = 26, len = 4, typmod = -1, byval = t)
----
1: oid = "14168" (typeid = 26, len = 4, typmod = -1, byval = t)

Here, FirstBootstrapObjectId < 14168 < FirstNormalObjectId

FTR it's also possible to get the same result using binary mode and
binary_upgrade_set_next_XXX functions.

Maybe we could document that pg_is_user_object() and its internal
counterpart returns true only for objects that are created during
"normal" multi-user database operation.

+1

Agreed.

Attached updated version patch.

Regards,

--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Wed, Feb 26, 2020 at 4:48 PM Masahiko Sawada
<masahiko.sawada@2ndquadrant.com> wrote:

On Thu, 13 Feb 2020 at 17:13, Julien Rouhaud <rjuju123@gmail.com> wrote:

On Thu, Feb 13, 2020 at 8:32 AM Amit Langote <amitlangote09@gmail.com> wrote:

Maybe we could document that pg_is_user_object() and its internal
counterpart returns true only for objects that are created during
"normal" multi-user database operation.

+1

Agreed.

Attached updated version patch.

Thanks for updating the patch. Some comments:

+      <row>
+       <entry><literal><function>pg_is_user_object(<parameter>oid</parameter>)</function></literal></entry>
+       <entry><type>bool</type></entry>
+       <entry>
+        true if <parameter>oid</parameter> is the object which is
created during
+        normal multi-user database operation.
+       </entry>
+      </row>

How about clarifying the description further as follows:

"true for objects created while database is operating in normal
multi-user mode, as opposed to single-user mode (see <xref
linkend="app-postgres"/>)."

Term "multi-user operation" is not mentioned elsewhere in the
documentation, so better to clarify what it means.

Also, maybe a minor nitpick, but how about adding the new function's
row at the end of the table (Table 9.72) instead of in the middle?

Other than that, patch looks to be in pretty good shape.

Thanks,
Amit

On Wed, Feb 26, 2020 at 1:18 PM Masahiko Sawada
<masahiko.sawada@2ndquadrant.com> wrote:

On Thu, 13 Feb 2020 at 17:13, Julien Rouhaud <rjuju123@gmail.com> wrote:

On Thu, Feb 13, 2020 at 8:32 AM Amit Langote <amitlangote09@gmail.com> wrote:

On Thu, Feb 13, 2020 at 10:30 AM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:

At Mon, 10 Feb 2020 14:32:44 +0900, Amit Langote <amitlangote09@gmail.com> wrote in

Agree that ObjectIsUserObject(oid) is easier to read than oid >=
FirstNormalObject. I would have not bothered, for example, if it was
something like oid >= FirstUserObjectId to begin with.

Aside from the naming, I'm not sure it's sensible to use
FirstNormalObjectId since I don't see a clear definition or required
characteristics for "user created objects" is. If we did CREATE
TABLE, FUNCTION or maybe any objects during single-user mode before
the first object is created during normal multiuser operation, the
"user-created(or not?)" object has an OID less than
FirstNormalObjectId. If such objects are the "user created object", we
need FirstUserObjectId defferent from FirstNormalObjectId.

Interesting observation. Connecting to database in --single mode,
whether done using initdb or directly, is always considered
"bootstrapping", so the OIDs from the bootstrapping range are
consumed.

$ postgres --single -D pgdata postgres

PostgreSQL stand-alone backend 13devel
backend> create table a (a int);
backend> select 'a'::regclass::oid;
1: oid (typeid = 26, len = 4, typmod = -1, byval = t)
----
1: oid = "14168" (typeid = 26, len = 4, typmod = -1, byval = t)

Here, FirstBootstrapObjectId < 14168 < FirstNormalObjectId

FTR it's also possible to get the same result using binary mode and
binary_upgrade_set_next_XXX functions.

Maybe we could document that pg_is_user_object() and its internal
counterpart returns true only for objects that are created during
"normal" multi-user database operation.

+1

Agreed.

Attached updated version patch.

Should we add some check if object exists or not here:
+Datum
+pg_is_user_object(PG_FUNCTION_ARGS)
+{
+    Oid oid = PG_GETARG_OID(0);
+
+    PG_RETURN_BOOL(ObjectIsUserObject(oid));
+}

I was trying some scenarios where we pass an object which does not exist:
postgres=# SELECT pg_is_user_object(0);
pg_is_user_object
-------------------
f
(1 row)
postgres=# SELECT pg_is_user_object(222222);
pg_is_user_object
-------------------
t
(1 row)
SELECT pg_is_user_object('pg_class1'::regclass);
ERROR: relation "pg_class1" does not exist
LINE 1: SELECT pg_is_user_object('pg_class1'::regclass);
^
I felt these behavior seems to be slightly inconsistent.
Thoughts?

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com

On Tue, 3 Mar 2020 at 23:33, vignesh C <vignesh21@gmail.com> wrote:

Should we add some check if object exists or not here:
+Datum
+pg_is_user_object(PG_FUNCTION_ARGS)
+{
+    Oid oid = PG_GETARG_OID(0);
+
+    PG_RETURN_BOOL(ObjectIsUserObject(oid));
+}

I was trying some scenarios where we pass an object which does not exist:
postgres=# SELECT pg_is_user_object(0);
pg_is_user_object
-------------------
f
(1 row)
postgres=# SELECT pg_is_user_object(222222);
pg_is_user_object
-------------------
t
(1 row)
SELECT pg_is_user_object('pg_class1'::regclass);
ERROR: relation "pg_class1" does not exist
LINE 1: SELECT pg_is_user_object('pg_class1'::regclass);
^
I felt these behavior seems to be slightly inconsistent.
Thoughts?

Hmm I'm not sure we should existing check in that function. Main use
case would be passing an oid of a tuple of a system catalog to that
function to check if the given object was created while multi-user
mode. So I think this function can assume that the given object id
exists. And if we want to do that check, we will end up with checking
if the object having that oid in all system catalogs, which is very
high cost I think.

I suspect perhaps the function name pg_is_user_object led that
confusion. That name looks like it checks if the given 'object' is
created while multi-user mode. So maybe we can improve it either by
renaming to pg_is_user_object_id (or pg_is_user_oid?) or leaving the
name but describing in the doc (based on Amit's suggestion in previous
mail):

"true for oids of objects assigned while database is operating in
normal multi-user mode, as opposed to single-user mode (see
<xreflinkend="app-postgres"/>)."

What do you think?

Regards,

--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Wed, Mar 4, 2020 at 9:02 AM Masahiko Sawada
<masahiko.sawada@2ndquadrant.com> wrote:

On Tue, 3 Mar 2020 at 23:33, vignesh C <vignesh21@gmail.com> wrote:

Should we add some check if object exists or not here:
+Datum
+pg_is_user_object(PG_FUNCTION_ARGS)
+{
+    Oid oid = PG_GETARG_OID(0);
+
+    PG_RETURN_BOOL(ObjectIsUserObject(oid));
+}

I was trying some scenarios where we pass an object which does not exist:
postgres=# SELECT pg_is_user_object(0);
pg_is_user_object
-------------------
f
(1 row)
postgres=# SELECT pg_is_user_object(222222);
pg_is_user_object
-------------------
t
(1 row)
SELECT pg_is_user_object('pg_class1'::regclass);
ERROR: relation "pg_class1" does not exist
LINE 1: SELECT pg_is_user_object('pg_class1'::regclass);
^
I felt these behavior seems to be slightly inconsistent.
Thoughts?

Hmm I'm not sure we should existing check in that function. Main use
case would be passing an oid of a tuple of a system catalog to that
function to check if the given object was created while multi-user
mode. So I think this function can assume that the given object id
exists. And if we want to do that check, we will end up with checking
if the object having that oid in all system catalogs, which is very
high cost I think.

I suspect perhaps the function name pg_is_user_object led that
confusion. That name looks like it checks if the given 'object' is
created while multi-user mode. So maybe we can improve it either by
renaming to pg_is_user_object_id (or pg_is_user_oid?) or leaving the
name but describing in the doc (based on Amit's suggestion in previous
mail):

I liked pg_is_user_oid over pg_is_user_object_id but this liking may
vary from person to person, so I'm still ok if you don't change the
name. I'm fine about adding the information in the document unless
someone else feels that this check is required in this function.

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com

On 2020/02/05 20:26, Masahiko Sawada wrote:

Hi,

User can create database objects such as functions into pg_catalog.
But if I'm not missing something, currently there is no
straightforward way to identify if the object is a user created object
or a system object which is created during initdb. If we can do that
user will be able to check if malicious functions are not created in
the database, which is important from the security perspective.

The function that you are proposing is really enough for this use case?
What if malicious users directly change the oid of function
to < FirstNormalObjectId? Or you're assuming that malicious users will
never log in as superuser and not be able to change the oid?

Regards,

--
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters

On Wed, 4 Mar 2020 at 16:43, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:

On 2020/02/05 20:26, Masahiko Sawada wrote:

Hi,

User can create database objects such as functions into pg_catalog.
But if I'm not missing something, currently there is no
straightforward way to identify if the object is a user created object
or a system object which is created during initdb. If we can do that
user will be able to check if malicious functions are not created in
the database, which is important from the security perspective.

The function that you are proposing is really enough for this use case?
What if malicious users directly change the oid of function
to < FirstNormalObjectId? Or you're assuming that malicious users will
never log in as superuser and not be able to change the oid?

That's a good point! I'm surprised that user is allowed to update an
oid of database object. In addition, surprisingly we can update it to
0, which in turn leads the assertion failure:

TRAP: BadArgument("OidIsValid(relid)", File: "autovacuum.c", Line: 2990)

As you pointed out, it's not enough as long as users can manually
update oid to < FirstNormalObjectId. But I wonder if we should rather
forbid that.

Regards,

--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2020/03/04 17:05, Masahiko Sawada wrote:

On Wed, 4 Mar 2020 at 16:43, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:

On 2020/02/05 20:26, Masahiko Sawada wrote:

Hi,

User can create database objects such as functions into pg_catalog.
But if I'm not missing something, currently there is no
straightforward way to identify if the object is a user created object
or a system object which is created during initdb. If we can do that
user will be able to check if malicious functions are not created in
the database, which is important from the security perspective.

The function that you are proposing is really enough for this use case?
What if malicious users directly change the oid of function
to < FirstNormalObjectId? Or you're assuming that malicious users will
never log in as superuser and not be able to change the oid?

That's a good point! I'm surprised that user is allowed to update an
oid of database object. In addition, surprisingly we can update it to
0, which in turn leads the assertion failure:

Since non-superusers are not allowed to do that by default,
that's not so bad? That is, to avoid such unexpected change of oid,
admin just should prevent malicious users from logging in as superusers
and not give the permission on system catalogs to such users.

Regards,

--
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters

On Wed, 4 Mar 2020 at 18:02, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:

On 2020/03/04 17:05, Masahiko Sawada wrote:

On Wed, 4 Mar 2020 at 16:43, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:

On 2020/02/05 20:26, Masahiko Sawada wrote:

Hi,

User can create database objects such as functions into pg_catalog.
But if I'm not missing something, currently there is no
straightforward way to identify if the object is a user created object
or a system object which is created during initdb. If we can do that
user will be able to check if malicious functions are not created in
the database, which is important from the security perspective.

The function that you are proposing is really enough for this use case?
What if malicious users directly change the oid of function
to < FirstNormalObjectId? Or you're assuming that malicious users will
never log in as superuser and not be able to change the oid?

That's a good point! I'm surprised that user is allowed to update an
oid of database object. In addition, surprisingly we can update it to
0, which in turn leads the assertion failure:

Since non-superusers are not allowed to do that by default,
that's not so bad? That is, to avoid such unexpected change of oid,
admin just should prevent malicious users from logging in as superusers
and not give the permission on system catalogs to such users.

I think there is still insider threats. As long as we depend on
superuser privilege to do some DBA work, a malicious DBA might be able
to log in as superuser and modify oid.

This behavior is introduced in PG12 where we made oid column
non-system column. A table having oid = 0 is shown in pg_class but we
cannot drop it.

Regards,

--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2020/03/04 18:36, Masahiko Sawada wrote:

On Wed, 4 Mar 2020 at 18:02, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:

On 2020/03/04 17:05, Masahiko Sawada wrote:

On Wed, 4 Mar 2020 at 16:43, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:

On 2020/02/05 20:26, Masahiko Sawada wrote:

Hi,

User can create database objects such as functions into pg_catalog.
But if I'm not missing something, currently there is no
straightforward way to identify if the object is a user created object
or a system object which is created during initdb. If we can do that
user will be able to check if malicious functions are not created in
the database, which is important from the security perspective.

The function that you are proposing is really enough for this use case?
What if malicious users directly change the oid of function
to < FirstNormalObjectId? Or you're assuming that malicious users will
never log in as superuser and not be able to change the oid?

That's a good point! I'm surprised that user is allowed to update an
oid of database object. In addition, surprisingly we can update it to
0, which in turn leads the assertion failure:

Since non-superusers are not allowed to do that by default,
that's not so bad? That is, to avoid such unexpected change of oid,
admin just should prevent malicious users from logging in as superusers
and not give the permission on system catalogs to such users.

I think there is still insider threats. As long as we depend on
superuser privilege to do some DBA work, a malicious DBA might be able
to log in as superuser and modify oid.

Yes. But I'm sure that DBA has already considered the measures
againt such threads. Otherwise malicious users can do anything
more malicious rather than changing oid.

Regards,

--
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters

On Wed, 4 Mar 2020 at 18:57, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:

On 2020/03/04 18:36, Masahiko Sawada wrote:

On Wed, 4 Mar 2020 at 18:02, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:

On 2020/03/04 17:05, Masahiko Sawada wrote:

On Wed, 4 Mar 2020 at 16:43, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:

On 2020/02/05 20:26, Masahiko Sawada wrote:

Hi,

User can create database objects such as functions into pg_catalog.
But if I'm not missing something, currently there is no
straightforward way to identify if the object is a user created object
or a system object which is created during initdb. If we can do that
user will be able to check if malicious functions are not created in
the database, which is important from the security perspective.

The function that you are proposing is really enough for this use case?
What if malicious users directly change the oid of function
to < FirstNormalObjectId? Or you're assuming that malicious users will
never log in as superuser and not be able to change the oid?

That's a good point! I'm surprised that user is allowed to update an
oid of database object. In addition, surprisingly we can update it to
0, which in turn leads the assertion failure:

Since non-superusers are not allowed to do that by default,
that's not so bad? That is, to avoid such unexpected change of oid,
admin just should prevent malicious users from logging in as superusers
and not give the permission on system catalogs to such users.

I think there is still insider threats. As long as we depend on
superuser privilege to do some DBA work, a malicious DBA might be able
to log in as superuser and modify oid.

Yes. But I'm sure that DBA has already considered the measures
againt such threads. Otherwise malicious users can do anything
more malicious rather than changing oid.

Agreed. So that's not a serious problem in practice but we cannot say
the checking by pg_is_user_object() is totally enough for checking
whether malicious object exists or not. Is that right?

Regards,

--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Wed, 4 Mar 2020 at 15:28, vignesh C <vignesh21@gmail.com> wrote:

On Wed, Mar 4, 2020 at 9:02 AM Masahiko Sawada
<masahiko.sawada@2ndquadrant.com> wrote:

On Tue, 3 Mar 2020 at 23:33, vignesh C <vignesh21@gmail.com> wrote:

Should we add some check if object exists or not here:
+Datum
+pg_is_user_object(PG_FUNCTION_ARGS)
+{
+    Oid oid = PG_GETARG_OID(0);
+
+    PG_RETURN_BOOL(ObjectIsUserObject(oid));
+}

I was trying some scenarios where we pass an object which does not exist:
postgres=# SELECT pg_is_user_object(0);
pg_is_user_object
-------------------
f
(1 row)
postgres=# SELECT pg_is_user_object(222222);
pg_is_user_object
-------------------
t
(1 row)
SELECT pg_is_user_object('pg_class1'::regclass);
ERROR: relation "pg_class1" does not exist
LINE 1: SELECT pg_is_user_object('pg_class1'::regclass);
^
I felt these behavior seems to be slightly inconsistent.
Thoughts?

Hmm I'm not sure we should existing check in that function. Main use
case would be passing an oid of a tuple of a system catalog to that
function to check if the given object was created while multi-user
mode. So I think this function can assume that the given object id
exists. And if we want to do that check, we will end up with checking
if the object having that oid in all system catalogs, which is very
high cost I think.

I suspect perhaps the function name pg_is_user_object led that
confusion. That name looks like it checks if the given 'object' is
created while multi-user mode. So maybe we can improve it either by
renaming to pg_is_user_object_id (or pg_is_user_oid?) or leaving the
name but describing in the doc (based on Amit's suggestion in previous
mail):

I liked pg_is_user_oid over pg_is_user_object_id but this liking may
vary from person to person, so I'm still ok if you don't change the
name. I'm fine about adding the information in the document unless
someone else feels that this check is required in this function.

Attached updated patch that incorporated comments from Amit and Vignesh.

Regards,

--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2020/03/04 19:14, Masahiko Sawada wrote:

On Wed, 4 Mar 2020 at 18:57, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:

On 2020/03/04 18:36, Masahiko Sawada wrote:

On Wed, 4 Mar 2020 at 18:02, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:

On 2020/03/04 17:05, Masahiko Sawada wrote:

On Wed, 4 Mar 2020 at 16:43, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:

On 2020/02/05 20:26, Masahiko Sawada wrote:

Hi,

User can create database objects such as functions into pg_catalog.
But if I'm not missing something, currently there is no
straightforward way to identify if the object is a user created object
or a system object which is created during initdb. If we can do that
user will be able to check if malicious functions are not created in
the database, which is important from the security perspective.

The function that you are proposing is really enough for this use case?
What if malicious users directly change the oid of function
to < FirstNormalObjectId? Or you're assuming that malicious users will
never log in as superuser and not be able to change the oid?

That's a good point! I'm surprised that user is allowed to update an
oid of database object. In addition, surprisingly we can update it to
0, which in turn leads the assertion failure:

Since non-superusers are not allowed to do that by default,
that's not so bad? That is, to avoid such unexpected change of oid,
admin just should prevent malicious users from logging in as superusers
and not give the permission on system catalogs to such users.

I think there is still insider threats. As long as we depend on
superuser privilege to do some DBA work, a malicious DBA might be able
to log in as superuser and modify oid.

Yes. But I'm sure that DBA has already considered the measures
againt such threads. Otherwise malicious users can do anything
more malicious rather than changing oid.

Agreed. So that's not a serious problem in practice but we cannot say
the checking by pg_is_user_object() is totally enough for checking
whether malicious object exists or not. Is that right?

Yes.

My opinion is that, if malious users are not allowed to log in
as superusers and the admin give no permission on the system
schema/catalog to them, checking whether the object is defined
under pg_catalog schema or not is enough for your purpose.
Because they are also not allowed to create the object under
pg_catalog. pg_is_user_object() seems not necessary.

OTOH, if you address the case where malicious users can create
the object under pg_catalog, of course, checking whether
the object is defined under pg_catalog schema or not is enough
for the purpose. But pg_is_user_object() is also not enough
because such users can change oid.

Regards,

--
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters

At Wed, 4 Mar 2020 21:07:05 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in

The function that you are proposing is really enough for this use
case?
What if malicious users directly change the oid of function
to < FirstNormalObjectId? Or you're assuming that malicious users will
never log in as superuser and not be able to change the oid?

That's a good point! I'm surprised that user is allowed to update an
oid of database object. In addition, surprisingly we can update it to
0, which in turn leads the assertion failure:

Since non-superusers are not allowed to do that by default,
that's not so bad? That is, to avoid such unexpected change of oid,
admin just should prevent malicious users from logging in as
superusers
and not give the permission on system catalogs to such users.

I think there is still insider threats. As long as we depend on
superuser privilege to do some DBA work, a malicious DBA might be able
to log in as superuser and modify oid.

Yes. But I'm sure that DBA has already considered the measures
againt such threads. Otherwise malicious users can do anything
more malicious rather than changing oid.

Agreed. So that's not a serious problem in practice but we cannot say
the checking by pg_is_user_object() is totally enough for checking
whether malicious object exists or not. Is that right?

Yes.

My opinion is that, if malious users are not allowed to log in
as superusers and the admin give no permission on the system
schema/catalog to them, checking whether the object is defined
under pg_catalog schema or not is enough for your purpose.
Because they are also not allowed to create the object under
pg_catalog. pg_is_user_object() seems not necessary.

OTOH, if you address the case where malicious users can create
the object under pg_catalog, of course, checking whether
the object is defined under pg_catalog schema or not is enough
for the purpose. But pg_is_user_object() is also not enough
because such users can change oid.

The discussion seems assuming the feature is related to some security
measure. But I think I haven't seen the objective or use case for the
feature. I don't see how we should treat them according the result
from the "user-defined objects detection" feature.

For example, we could decide a function whether to be pushed-out or
not to remote server on postgres_fdw. In this case, we need to ask "is
the behavior of this function known to us?", in short, "is this
function is predefined?". In this use case, we have no concern if DBA
have added some functions as "not user-defined", since it's their own
risk.

I don't come up with another use cases but, anyway, I think we need to
clarify the scope of the feature.

regads.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On 2020/03/05 12:32, Kyotaro Horiguchi wrote:

At Wed, 4 Mar 2020 21:07:05 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in

The function that you are proposing is really enough for this use
case?
What if malicious users directly change the oid of function
to < FirstNormalObjectId? Or you're assuming that malicious users will
never log in as superuser and not be able to change the oid?

That's a good point! I'm surprised that user is allowed to update an
oid of database object. In addition, surprisingly we can update it to
0, which in turn leads the assertion failure:

Since non-superusers are not allowed to do that by default,
that's not so bad? That is, to avoid such unexpected change of oid,
admin just should prevent malicious users from logging in as
superusers
and not give the permission on system catalogs to such users.

I think there is still insider threats. As long as we depend on
superuser privilege to do some DBA work, a malicious DBA might be able
to log in as superuser and modify oid.

Yes. But I'm sure that DBA has already considered the measures
againt such threads. Otherwise malicious users can do anything
more malicious rather than changing oid.

Agreed. So that's not a serious problem in practice but we cannot say
the checking by pg_is_user_object() is totally enough for checking
whether malicious object exists or not. Is that right?

Yes.

My opinion is that, if malious users are not allowed to log in
as superusers and the admin give no permission on the system
schema/catalog to them, checking whether the object is defined
under pg_catalog schema or not is enough for your purpose.
Because they are also not allowed to create the object under
pg_catalog. pg_is_user_object() seems not necessary.

OTOH, if you address the case where malicious users can create
the object under pg_catalog, of course, checking whether
the object is defined under pg_catalog schema or not is enough
for the purpose. But pg_is_user_object() is also not enough
because such users can change oid.

The discussion seems assuming the feature is related to some security
measure. But I think I haven't seen the objective or use case for the
feature. I don't see how we should treat them according the result
from the "user-defined objects detection" feature.

For example, we could decide a function whether to be pushed-out or
not to remote server on postgres_fdw. In this case, we need to ask "is
the behavior of this function known to us?", in short, "is this
function is predefined?". In this use case, we have no concern if DBA
have added some functions as "not user-defined", since it's their own
risk.

I don't come up with another use cases but, anyway, I think we need to
clarify the scope of the feature.

Agreed. Also we would need to consider that the existing approach
(e.g., checking whether the object is defined under pg_catalog or not,
or seeing pg_stat_user_functions, _indexes, and _tables) is enough
for the use cases. If enough, new function might not be necessary.
If not enough, we might also need to reconsider the definitions of
pg_stat_user_xxx after considering the function.

Regards,

--
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters

On Wed, Mar 04, 2020 at 06:57:00PM +0900, Fujii Masao wrote:

Yes. But I'm sure that DBA has already considered the measures
againt such threads. Otherwise malicious users can do anything
more malicious rather than changing oid.

A superuser is by definition able to do anything on the system using
the rights of the OS user running the Postgres backend. One thing for
example is to take a base backup of the full instance, but you can do
much more interesting things once you have such rights. So I don't
quite get the line of arguments used on this thread regarding the
relation with somebody being malicious with superuser rights, and the
arguments about a superuser able to manipulate freely the catalog's
contents.
--
Michael

On Thu, 5 Mar 2020 at 13:23, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:

On 2020/03/05 12:32, Kyotaro Horiguchi wrote:

At Wed, 4 Mar 2020 21:07:05 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in

The function that you are proposing is really enough for this use
case?
What if malicious users directly change the oid of function
to < FirstNormalObjectId? Or you're assuming that malicious users will
never log in as superuser and not be able to change the oid?

That's a good point! I'm surprised that user is allowed to update an
oid of database object. In addition, surprisingly we can update it to
0, which in turn leads the assertion failure:

Since non-superusers are not allowed to do that by default,
that's not so bad? That is, to avoid such unexpected change of oid,
admin just should prevent malicious users from logging in as
superusers
and not give the permission on system catalogs to such users.

I think there is still insider threats. As long as we depend on
superuser privilege to do some DBA work, a malicious DBA might be able
to log in as superuser and modify oid.

Yes. But I'm sure that DBA has already considered the measures
againt such threads. Otherwise malicious users can do anything
more malicious rather than changing oid.

Agreed. So that's not a serious problem in practice but we cannot say
the checking by pg_is_user_object() is totally enough for checking
whether malicious object exists or not. Is that right?

Yes.

My opinion is that, if malious users are not allowed to log in
as superusers and the admin give no permission on the system
schema/catalog to them, checking whether the object is defined
under pg_catalog schema or not is enough for your purpose.
Because they are also not allowed to create the object under
pg_catalog. pg_is_user_object() seems not necessary.

OTOH, if you address the case where malicious users can create
the object under pg_catalog, of course, checking whether
the object is defined under pg_catalog schema or not is enough
for the purpose. But pg_is_user_object() is also not enough
because such users can change oid.

The discussion seems assuming the feature is related to some security
measure. But I think I haven't seen the objective or use case for the
feature. I don't see how we should treat them according the result
from the "user-defined objects detection" feature.

For example, we could decide a function whether to be pushed-out or
not to remote server on postgres_fdw. In this case, we need to ask "is
the behavior of this function known to us?", in short, "is this
function is predefined?". In this use case, we have no concern if DBA
have added some functions as "not user-defined", since it's their own
risk.

I don't come up with another use cases but, anyway, I think we need to
clarify the scope of the feature.

Agreed. Also we would need to consider that the existing approach
(e.g., checking whether the object is defined under pg_catalog or not,
or seeing pg_stat_user_functions, _indexes, and _tables) is enough
for the use cases. If enough, new function might not be necessary.
If not enough, we might also need to reconsider the definitions of
pg_stat_user_xxx after considering the function.

Originally the motivation of this feature is that while studying PCI
DSS 2.2.5 I thought that a running PostgreSQL server is not able to
prove that there is no malicious function in database. PCI DSS 2.2.5
states "Remove all unnecessary functionality, such as scripts,
drivers, features, subsystems, file systems, and unnecessary web
servers." If we want to clarify unnecessary or malicious functions we
can check public schema and user schema but once a function is created
on pg_proc we cannot distinguish whether it's a built-in (i.g. safe)
function or not. I totally agree that if malicious someone logs in as
a superuser he/she can do anything more serious than changing catalog
contents but I wanted to have a way to prove soundness of running
database.

Regards,

--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

At Thu, 5 Mar 2020 15:21:49 +0900, Masahiko Sawada <masahiko.sawada@2ndquadrant.com> wrote in

I don't come up with another use cases but, anyway, I think we need to
clarify the scope of the feature.

Agreed. Also we would need to consider that the existing approach
(e.g., checking whether the object is defined under pg_catalog or not,
or seeing pg_stat_user_functions, _indexes, and _tables) is enough
for the use cases. If enough, new function might not be necessary.
If not enough, we might also need to reconsider the definitions of
pg_stat_user_xxx after considering the function.

Originally the motivation of this feature is that while studying PCI
DSS 2.2.5 I thought that a running PostgreSQL server is not able to
prove that there is no malicious function in database. PCI DSS 2.2.5
states "Remove all unnecessary functionality, such as scripts,
drivers, features, subsystems, file systems, and unnecessary web
servers." If we want to clarify unnecessary or malicious functions we
can check public schema and user schema but once a function is created
on pg_proc we cannot distinguish whether it's a built-in (i.g. safe)
function or not. I totally agree that if malicious someone logs in as
a superuser he/she can do anything more serious than changing catalog
contents but I wanted to have a way to prove soundness of running
database.

Thanks for the elaboration. It doesn't seem to me as the
resposibility of PostgreSQL program. The same can be said to OSes.

I think the section is not saying that "keep you system only with
defaultly installed components", but "remove all features unncecessary
to your system even if it is defaultly installed as far as you can".
And whether A system is needing a feature or not cannot be the matter
of PostgreSQL or OSes.

So you need to remove some system-admistrative functions if you know
it is not required by your system in order to comform the
requirement. But they would be "non-user-defined" objects.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Thu, 5 Mar 2020 at 16:36, Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:

At Thu, 5 Mar 2020 15:21:49 +0900, Masahiko Sawada <masahiko.sawada@2ndquadrant.com> wrote in

I don't come up with another use cases but, anyway, I think we need to
clarify the scope of the feature.

Agreed. Also we would need to consider that the existing approach
(e.g., checking whether the object is defined under pg_catalog or not,
or seeing pg_stat_user_functions, _indexes, and _tables) is enough
for the use cases. If enough, new function might not be necessary.
If not enough, we might also need to reconsider the definitions of
pg_stat_user_xxx after considering the function.

Originally the motivation of this feature is that while studying PCI
DSS 2.2.5 I thought that a running PostgreSQL server is not able to
prove that there is no malicious function in database. PCI DSS 2.2.5
states "Remove all unnecessary functionality, such as scripts,
drivers, features, subsystems, file systems, and unnecessary web
servers." If we want to clarify unnecessary or malicious functions we
can check public schema and user schema but once a function is created
on pg_proc we cannot distinguish whether it's a built-in (i.g. safe)
function or not. I totally agree that if malicious someone logs in as
a superuser he/she can do anything more serious than changing catalog
contents but I wanted to have a way to prove soundness of running
database.

Thanks for the elaboration. It doesn't seem to me as the
resposibility of PostgreSQL program. The same can be said to OSes.

I think the section is not saying that "keep you system only with
defaultly installed components", but "remove all features unncecessary
to your system even if it is defaultly installed as far as you can".

Agreed.

And whether A system is needing a feature or not cannot be the matter
of PostgreSQL or OSes.

So you need to remove some system-admistrative functions if you know
it is not required by your system in order to comform the
requirement. But they would be "non-user-defined" objects.

I think normally users don't want to remove built-in functions because
they think these functions are trusted and it's hard to restore them
when they want later. So I thought user want to search functions that
is unnecessary but not a built-in function.

Regards,

--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

At Thu, 5 Mar 2020 18:06:26 +0900, Masahiko Sawada <masahiko.sawada@2ndquadrant.com> wrote in

On Thu, 5 Mar 2020 at 16:36, Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:

At Thu, 5 Mar 2020 15:21:49 +0900, Masahiko Sawada <masahiko.sawada@2ndquadrant.com> wrote in

I don't come up with another use cases but, anyway, I think we need to
clarify the scope of the feature.

Agreed. Also we would need to consider that the existing approach
(e.g., checking whether the object is defined under pg_catalog or not,
or seeing pg_stat_user_functions, _indexes, and _tables) is enough
for the use cases. If enough, new function might not be necessary.
If not enough, we might also need to reconsider the definitions of
pg_stat_user_xxx after considering the function.

Originally the motivation of this feature is that while studying PCI
DSS 2.2.5 I thought that a running PostgreSQL server is not able to
prove that there is no malicious function in database. PCI DSS 2.2.5
states "Remove all unnecessary functionality, such as scripts,
drivers, features, subsystems, file systems, and unnecessary web
servers." If we want to clarify unnecessary or malicious functions we
can check public schema and user schema but once a function is created
on pg_proc we cannot distinguish whether it's a built-in (i.g. safe)
function or not. I totally agree that if malicious someone logs in as
a superuser he/she can do anything more serious than changing catalog
contents but I wanted to have a way to prove soundness of running
database.

Thanks for the elaboration. It doesn't seem to me as the
resposibility of PostgreSQL program. The same can be said to OSes.

I think the section is not saying that "keep you system only with
defaultly installed components", but "remove all features unncecessary
to your system even if it is defaultly installed as far as you can".

Agreed.

And whether A system is needing a feature or not cannot be the matter
of PostgreSQL or OSes.

So you need to remove some system-admistrative functions if you know
it is not required by your system in order to comform the
requirement. But they would be "non-user-defined" objects.

I think normally users don't want to remove built-in functions because
they think these functions are trusted and it's hard to restore them
when they want later. So I thought user want to search functions that
is unnecessary but not a built-in function.

I'm not sure those who wants to introduce PCI-DSS are under a normal
sitautation, though:p

That seems beside the point. pg_read_file is known to be usable for
drawing out database files. If you leave the function alone, the
security officer (designer?) have to consider the possibility that
someone draws out files in the database system using the function and
have to plan the action for the threat. In that context,
built-in-or-not distinction is useless.

In the first place, if you assume that someone may install malicious
functions in the server after beginning operation, distinction by OID
doesn't work at all because who can illegally install a malicious
function also be able to modify its OID with quite low odds.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Thu, 5 Mar 2020 at 18:39, Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:

At Thu, 5 Mar 2020 18:06:26 +0900, Masahiko Sawada <masahiko.sawada@2ndquadrant.com> wrote in

On Thu, 5 Mar 2020 at 16:36, Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:

At Thu, 5 Mar 2020 15:21:49 +0900, Masahiko Sawada <masahiko.sawada@2ndquadrant.com> wrote in

I don't come up with another use cases but, anyway, I think we need to
clarify the scope of the feature.

Agreed. Also we would need to consider that the existing approach
(e.g., checking whether the object is defined under pg_catalog or not,
or seeing pg_stat_user_functions, _indexes, and _tables) is enough
for the use cases. If enough, new function might not be necessary.
If not enough, we might also need to reconsider the definitions of
pg_stat_user_xxx after considering the function.

Originally the motivation of this feature is that while studying PCI
DSS 2.2.5 I thought that a running PostgreSQL server is not able to
prove that there is no malicious function in database. PCI DSS 2.2.5
states "Remove all unnecessary functionality, such as scripts,
drivers, features, subsystems, file systems, and unnecessary web
servers." If we want to clarify unnecessary or malicious functions we
can check public schema and user schema but once a function is created
on pg_proc we cannot distinguish whether it's a built-in (i.g. safe)
function or not. I totally agree that if malicious someone logs in as
a superuser he/she can do anything more serious than changing catalog
contents but I wanted to have a way to prove soundness of running
database.

Thanks for the elaboration. It doesn't seem to me as the
resposibility of PostgreSQL program. The same can be said to OSes.

I think the section is not saying that "keep you system only with
defaultly installed components", but "remove all features unncecessary
to your system even if it is defaultly installed as far as you can".

Agreed.

And whether A system is needing a feature or not cannot be the matter
of PostgreSQL or OSes.

So you need to remove some system-admistrative functions if you know
it is not required by your system in order to comform the
requirement. But they would be "non-user-defined" objects.

I think normally users don't want to remove built-in functions because
they think these functions are trusted and it's hard to restore them
when they want later. So I thought user want to search functions that
is unnecessary but not a built-in function.

I'm not sure those who wants to introduce PCI-DSS are under a normal
sitautation, though:p

That seems beside the point. pg_read_file is known to be usable for
drawing out database files. If you leave the function alone, the
security officer (designer?) have to consider the possibility that
someone draws out files in the database system using the function and
have to plan the action for the threat. In that context,
built-in-or-not distinction is useless.

So how do you check if unnecessary, malicious or unauthorized function
exists in database after that, for example when periodical security
check? Functions defined after initdb must be checked under user's
responsibility but since normally there are many built-in functions in
pg_proc the check in pg_proc could be cumbersome. So the idea of this
feature is to make that check easier by marking built-in functions.

In the first place, if you assume that someone may install malicious
functions in the server after beginning operation, distinction by OID
doesn't work at all because who can illegally install a malicious
function also be able to modify its OID with quite low odds.

Yes, that's what Fujii-san also pointed out. It's better to find a way
to distinct functions while not relying on OID.

Regards,

--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

At Sun, 8 Mar 2020 11:55:06 +0900, Masahiko Sawada <masahiko.sawada@2ndquadrant.com> wrote in

On Thu, 5 Mar 2020 at 18:39, Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:

At Thu, 5 Mar 2020 18:06:26 +0900, Masahiko Sawada <masahiko.sawada@2ndquadrant.com> wrote in

On Thu, 5 Mar 2020 at 16:36, Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:
I think normally users don't want to remove built-in functions because
they think these functions are trusted and it's hard to restore them
when they want later. So I thought user want to search functions that
is unnecessary but not a built-in function.

I'm not sure those who wants to introduce PCI-DSS are under a normal
sitautation, though:p

That seems beside the point. pg_read_file is known to be usable for
drawing out database files. If you leave the function alone, the
security officer (designer?) have to consider the possibility that
someone draws out files in the database system using the function and
have to plan the action for the threat. In that context,
built-in-or-not distinction is useless.

So how do you check if unnecessary, malicious or unauthorized function
exists in database after that, for example when periodical security
check? Functions defined after initdb must be checked under user's
responsibility but since normally there are many built-in functions in
pg_proc the check in pg_proc could be cumbersome. So the idea of this
feature is to make that check easier by marking built-in functions.

I think there's no easy way to accomplish it. If PostgreSQL
documentation says that "Yeah, the function tells if using the
function or feature complies the request of PCI-DSS 2.2.5!" and it
tells safe for all built-in functions, it is an outright lie even if
the server is just after initdb'ed.

Sparating from PCI-DSS and we document it just as "the function tells
if the function is built-in or not", it's true. (But I'm not sure
about its usage..)

I might be misunderstanding about the operation steps in your mind.

In the first place, if you assume that someone may install malicious
functions in the server after beginning operation, distinction by OID
doesn't work at all because who can illegally install a malicious
function also be able to modify its OID with quite low odds.

Yes, that's what Fujii-san also pointed out. It's better to find a way
to distinct functions while not relying on OID.

And it is out-of-scope of PCI-DSS 2.2.5. It mentions design or
system-building time.

Apart from PCI-DSS, if you are concerning operation-time threats. If
once someone malicious could install a function to the server, I think
that kind of feature with any criteria no longer work as a
countermeasure for further threats. Maybe something like tripwire
would work. That is, maybe a kind of checksum over system catalogs.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On 4 Mar 2020, at 12:06, Masahiko Sawada <masahiko.sawada@2ndquadrant.com> wrote:

Attached updated patch that incorporated comments from Amit and Vignesh.

This patch fails to compile due to an Oid collision in pg_proc.dat. Please
submit a new version with an Oid from the recommended range for new patches:
8000-9999. See the below documentation page for more information on this.

https://www.postgresql.org/docs/devel/system-catalog-initial-data.html

I'm marking the entry Waiting on Author in the meantime.

cheers ./daniel

On 1 Jul 2020, at 14:15, Daniel Gustafsson <daniel@yesql.se> wrote:

On 4 Mar 2020, at 12:06, Masahiko Sawada <masahiko.sawada@2ndquadrant.com> wrote:

Attached updated patch that incorporated comments from Amit and Vignesh.

This patch fails to compile due to an Oid collision in pg_proc.dat. Please
submit a new version with an Oid from the recommended range for new patches:
8000-9999. See the below documentation page for more information on this.

https://www.postgresql.org/docs/devel/system-catalog-initial-data.html

I'm marking the entry Waiting on Author in the meantime.

As no new patch has been presented, and the thread contains doubts over the
proposed functionality, I'm marking this returned with feedback.

cheers ./daniel