Thread: Identifying user-created objects
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. 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. Regards, -- Masahiko Sawada http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachment
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
Attachment
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
Attachment
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
Attachment
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
Attachment
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
Attachment
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
Attachment
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
Attachment
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