KaiGai Kohei wrote:
As I promised last week, SE-PostgreSQL patches are revised here:
I think I now understand what sepgsqlCheckProcedureInstall is trying to
achieve. It's trying to stop attacks where you trick another user to run
your malicious code. We had a serious vulnerability of that kind a while
ago (http://archives.postgresql.org//pgsql-hackers/2008-01/msg00268.php)
when ANALYZE and VACUUM FULL ran expression and partial index predicates
with (typically) superuser privileges.
It seems that sepgsqlCheckProcedureInstall is trying to provide a more
thorough solution to the trojan horse problem than what we did back
then. It stops you from installing an untrusted function as a type
input/output function, operator implementing function etc. Now that
could be useful on its own, quite apart from the rest of the
SE-PostgreSQL patch, in which case I'd like to see that implemented as a
separate patch, so that you can use the facility even if you're not
using SE-PostgreSQL.
Some details of that:
+ void
+ sepgsqlCheckProcedureInstall(Relation rel, HeapTuple newtup, HeapTuple oldtup)
+ {
+ /*
+ * db_procedure:{install} check prevent a malicious functions
+ * to be installed, as a part of system catalogs.
+ * It is necessary to prevent other person implicitly to invoke
+ * malicious functions.
+ */
+ switch (RelationGetRelid(rel))
+ {
+ case AggregateRelationId:
+ /*
+ * db_procedure:{execute} is checked on invocations of:
+ * pg_aggregate.aggfnoid
+ * pg_aggregate.aggtransfn
+ * pg_aggregate.aggfinalfn
+ */
+ break;
+
+ case AccessMethodRelationId:
+ CHECK_PROC_INSTALL_PERM(pg_am, aminsert, newtup, oldtup);
+ CHECK_PROC_INSTALL_PERM(pg_am, ambeginscan, newtup, oldtup);
+ CHECK_PROC_INSTALL_PERM(pg_am, amgettuple, newtup, oldtup);
+ CHECK_PROC_INSTALL_PERM(pg_am, amgetbitmap, newtup, oldtup);
+ CHECK_PROC_INSTALL_PERM(pg_am, amrescan, newtup, oldtup);
+ CHECK_PROC_INSTALL_PERM(pg_am, amendscan, newtup, oldtup);
+ CHECK_PROC_INSTALL_PERM(pg_am, ammarkpos, newtup, oldtup);
+ CHECK_PROC_INSTALL_PERM(pg_am, amrestrpos, newtup, oldtup);
+ CHECK_PROC_INSTALL_PERM(pg_am, ambuild, newtup, oldtup);
+ CHECK_PROC_INSTALL_PERM(pg_am, ambulkdelete, newtup, oldtup);
+ CHECK_PROC_INSTALL_PERM(pg_am, amvacuumcleanup, newtup, oldtup);
+ CHECK_PROC_INSTALL_PERM(pg_am, amcostestimate, newtup, oldtup);
+ CHECK_PROC_INSTALL_PERM(pg_am, amoptions, newtup, oldtup);
+ break;
ISTM that you should just forbid any changes to pg_am in the default
policy. That's very low level stuff. If you can modify that, you can
wreck a lot of havoc, quite possibly turning it into a vulnerability
even if you can't directly install a malicious function there.
+ case AccessMethodProcedureRelationId:
+ CHECK_PROC_INSTALL_PERM(pg_amproc, amproc, newtup, oldtup);
+ break;
+
+ case CastRelationId:
+ CHECK_PROC_INSTALL_PERM(pg_cast, castfunc, newtup, oldtup);
+ break;
We check execute permission on the cast function at runtime.
+ case ConversionRelationId:
+ CHECK_PROC_INSTALL_PERM(pg_conversion, conproc, newtup, oldtup);
+ break;
This ought to be unnecessary now. Only C-functions can be installed as
conversion procs, and a C function can do anything, so there's little
point in checking this anymore.
+ case ForeignDataWrapperRelationId:
+ CHECK_PROC_INSTALL_PERM(pg_foreign_data_wrapper, fdwvalidator,
newtup, oldtup);
+ break;
Hmm, calls to fdwvalidator are not at all performance critical, so maybe
we should just check execute permission when it's called.
+ case LanguageRelationId:
+ CHECK_PROC_INSTALL_PERM(pg_language, lanplcallfoid, newtup,
oldtup);
+ CHECK_PROC_INSTALL_PERM(pg_language, lanvalidator, newtup,
oldtup);
+ break;
I think these need to be C-functions.
+ case OperatorRelationId:
+ CHECK_PROC_INSTALL_PERM(pg_operator, oprcode, newtup, oldtup);
+ CHECK_PROC_INSTALL_PERM(pg_operator, oprrest, newtup, oldtup);
+ CHECK_PROC_INSTALL_PERM(pg_operator, oprjoin, newtup, oldtup);
+ break;
oprcode is checked for execute permission when the operator is used. For
oprrest and oprjoin, we could add checks into the planner where they're
called. (pg_operator.oprcom and pg_operator.oprnegate are missing?)
+ case TSParserRelationId:
+ CHECK_PROC_INSTALL_PERM(pg_ts_parser, prsstart, newtup, oldtup);
+ CHECK_PROC_INSTALL_PERM(pg_ts_parser, prstoken, newtup, oldtup);
+ CHECK_PROC_INSTALL_PERM(pg_ts_parser, prsend, newtup, oldtup);
+ CHECK_PROC_INSTALL_PERM(pg_ts_parser, prsheadline, newtup,
oldtup);
+ CHECK_PROC_INSTALL_PERM(pg_ts_parser, prslextype, newtup,
oldtup);
+ break;
+
+ case TSTemplateRelationId:
+ CHECK_PROC_INSTALL_PERM(pg_ts_template, tmplinit, newtup,
oldtup);
+ CHECK_PROC_INSTALL_PERM(pg_ts_template, tmpllexize, newtup,
oldtup);
+ break;
Not sure about these. Maybe we should add checks to where these are called.
+ case TypeRelationId:
+ CHECK_PROC_INSTALL_PERM(pg_type, typinput, newtup, oldtup);
+ CHECK_PROC_INSTALL_PERM(pg_type, typoutput, newtup, oldtup);
+ CHECK_PROC_INSTALL_PERM(pg_type, typreceive, newtup, oldtup);
+ CHECK_PROC_INSTALL_PERM(pg_type, typsend, newtup, oldtup);
+ CHECK_PROC_INSTALL_PERM(pg_type, typmodin, newtup, oldtup);
+ CHECK_PROC_INSTALL_PERM(pg_type, typmodout, newtup, oldtup);
+ CHECK_PROC_INSTALL_PERM(pg_type, typanalyze, newtup, oldtup);
+ break;
Hmm, input/output functions have to be in C, so I'm not concerned about
those. send/receive and typmodin/typmodout are a bit problematic.
ANALYZE calls typanalyze as the table owner, so I think that's safe.
All of these require the victim to willingly (although indirectly) call
a non-security definer function created by the attacker, with varying
degrees of difficultness in tricking someone to do that. Can't you just
create a policy that forbids creating non-security definer functions in
the first place? It's much more coarse-grained, but would probably be
enough in practice.
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
