Re: Updates of SE-PostgreSQL 8.4devel patches (r1704)
От | Heikki Linnakangas |
---|---|
Тема | Re: Updates of SE-PostgreSQL 8.4devel patches (r1704) |
Дата | |
Msg-id | 49B4DD51.7030605@enterprisedb.com обсуждение исходный текст |
Ответ на | Updates of SE-PostgreSQL 8.4devel patches (r1704) (KaiGai Kohei <kaigai@ak.jp.nec.com>) |
Ответы |
Re: Updates of SE-PostgreSQL 8.4devel patches (r1704)
Re: Updates of SE-PostgreSQL 8.4devel patches (r1704) |
Список | pgsql-hackers |
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
В списке pgsql-hackers по дате отправления:
Предыдущее
От: Heikki LinnakangasДата:
Сообщение: Re: Updates of SE-PostgreSQL 8.4devel patches (r1704)