Re: Updates of SE-PostgreSQL 8.4devel patches (r1704)

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 the given
loadable library file has appropriate security context, or not.
In the default security policy, only files labeled as "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(at)ak(dot)jp(dot)nec(dot)com>