Re: Updates of SE-PostgreSQL 8.4devel patches (r1704) - Mailing list pgsql-hackers

Heikki Linnakangas wrote:
> 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.

Yes, the purpose of sepgsqlCheckProcedureInstall() is to prevent users
to invoke functions installed by other malicious/untrusted one, typically
known as trojan-horse.

Basically, I can agree the vanilla PostgreSQL also provide similar stuff
to prevent to install "untrusted" functions as a part of system internal
codes. If we have such a facility as a basis, we can implement
sepgsqlCheckProcedureInstall() hook more simple and easier to maintenance.

[snip]
>> +     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.

We should not assume only C-functions can be installed on pg_conversion
(and other internal stuff), because a superuser can update system catalog
by hand.
  Example)  postgres=# CREATE OR REPLACE FUNCTION testfn()  postgres-#     RETURNS int LANGUAGE sql AS 'SELECT 1';
CREATEFUNCTION  postgres=# UPDATE pg_conversion SET conproc = 'testfn'::regproc where oid=11276;  UPDATE 1  postgres=#
setclient_encoding = 'sjis';  server closed the connection unexpectedly          This probably means the server
terminatedabnormally          before or while processing the request.  The connection to the server was lost.
Attemptingreset: WARNING:  terminating connection because of crash of another server process  DETAIL:  The postmaster
hascommanded this server process to roll back the current transaction and exit, because another server process exited
abnormallyand possibly corrupted shared memory.  HINT:  In a moment you should be able to reconnect to the database and
repeatyour command.  Failed.  !>
 

SE-PostgreSQL intends to acquire them and apply access control policy
in this case also.

Aside note: sepgsqlCheckDatabaseInstallModule() check permissions on
a newly installed C-library file, to prevent to load a file deployed
by untrusted client.

>> +     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() on its invocation, it is not necessary to check
on the installation time.

[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?)

If runtime checks are added, it is more desirable.

>> +     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.

I'll check the behavior of them tomorrow.

>> +     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.

I think it is possible, and I welcome the vanilla PostgreSQL also have
such a facility. It also make easier to maintain SE-PostgreSQL code. :-)
The issue it what policy should be applied on the vanilla side.

The following rules may be able to be a draft. - Any installed functions should not security definer. - Any installed
functionsshould be owned by superuser.   (to prevent replacement by normal users.)
 

What is your opinion? I'll try to consider it more...

Thanks,
-- 
KaiGai Kohei <kaigai@kaigai.gr.jp>