Re: Updates of SE-PostgreSQL 8.4devel patches (r1704) - Mailing list pgsql-hackers
From | KaiGai Kohei |
---|---|
Subject | Re: Updates of SE-PostgreSQL 8.4devel patches (r1704) |
Date | |
Msg-id | 49B5F87D.7020506@ak.jp.nec.com Whole thread Raw |
In response to | Re: Updates of SE-PostgreSQL 8.4devel patches (r1704) (Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>) |
List | pgsql-hackers |
Heikki Linnakangas wrote: >> + void >> + sepgsqlCheckProcedureInstall(Relation rel, HeapTuple newtup, >> HeapTuple oldtup) >> + { [snip] >> + + 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. Heikki, My opinion is still we should check "db_procedure:{install}" privilege for functions expected to be implemented by C-language. In the basis of security, it requires security facilities should improve confidentiality, integrity and availability in the assumption and environment required by the facility. For example, existing database ACL improves confidentiality and integrity with applying DAC policy, and improves availability to prevent to load C-library by users except for superuser. (Here, the assumption is that database superuser is trusted.) If we write a oid of SQL-function onto "pg_am.aminsert", it will not work correctly independent from existence of maliciou code, but it also enables to crash the backend immediately. It can be a damage to the availability of the backend, so I still think we need to check this permission for functions expected to be implemented by C-language. NOTE: when we create a new C-function or replace its definition, sepgsqlCheckDatabaseInstallModule() checks whether thegiven loadable library file has appropriate security context, or not. In the default security policy, only files labeledas "lib_t" are allowed to load. >> + 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. We have a corner case. The ri_HashCompareOp() in ri_triggers.c can invokes castfunc without runtime checks, if I can understand the implementation correctly. Indeed, most cases invokes pg_proc_aclcheck() and SE-PostgreSQL also checks "db_procedure:{execute}" permission in runtime. This design requires either of "install" or "execute" should be checked at least, so double checks are not matter. [snip] >> + 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. If pg_proc_aclcheck() is added on the invocation of fdwvalidator, I'll remove "install" checks on it from here. [snip] >> + 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?) For example, ExecInitAgg() set up opcode function as follows: fmgr_info(get_opcode(eq_opr), &(peraggstate->equalfn)); and it can be invoked later without checks. I think it is a case to be checked. Indeed, pg_operator.oprcom and pg_operator.oprnegate were missed. Thanks for your pointed out. >> + 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. DefineTSParser() and DefineTSTemplate() checks the invoker has superuser privileges, so these operations are considered as trusted. However, I'm not clear whether adding checks affects compatibility, or not. Thanks, >> + 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. > -- OSS Platform Development Division, NEC KaiGai Kohei <kaigai@ak.jp.nec.com>
pgsql-hackers by date: