The attached patch is a draft for the discussion.
It cleans up the existing PG privileges checks related to databases
and schemas, and consolidates points where it applies privileges
checks as a groundwork for the upcoming security framework.
We have tried a few approaches to implement SE-PgSQL for this year,
however, it has a bit high hurdle to join development, because it
tried to separate features unless it loses something useful.
It naturally holds two parts within a patch. The one is modification
to the core routines. The other is selinux specific code.
The selinux-specific part was hurdle for pgsql-folks, and the core
pgsql part was hurdle for selinux-folks.
Under the CF#3, we had a fruitful discussion, especially BWPUG
meeting. Again, Stephen Frost suggested to start the development
from a common security framework for both of security models.
http://wiki.postgresql.org/wiki/SEPostgreSQL_Review_at_the_BWPUG#PostgreSQL_security_check_cleanup
It allows us to focus on the pure pgsql part, without any selinux
specific part at the moment.
In the CF#2, I tried to rework anything with a single patch, but this
approach was wrong, too large. So, I'll try to separate the changeset
smaller, per object class basis.
This patch is a groundwork before the security framework.
The existing PG checks requires multiple permission checks in separate
places for a single operation, but it makes harder to replace these
inlined permission checks by security hooks.
It tries to consolidate multiple separate permission checks into same
place for database and schema, as a discussion draft.
* LookupCreationNamespace
It checks CREATE permission on the reuiqred schema, when ALTER with
SCHEMA TO option. It will be consolidated to check_*_alter_schema()
hooks, so I removed this check and moved to the caller.
* createdb movedb
It repeats name resolve and permission checks if necessary.
So, I consolidate permission checks in a same place.
$ diffstat pgsql-01-ground-work-8.5devel-r2486.patch
catalog/namespace.c | 11 --!!!
commands/dbcommands.c | 89 ++++++++++++++++++++++++--------------------!!!
commands/functioncmds.c | 11 ++++!
commands/tablecmds.c | 11 ++++!
commands/typecmds.c | 11 ++++!
5 files changed, 72 insertions(+), 43 deletions(-), 18 modifications(!)
Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kai...@ak.jp.nec.com>
diff -Nrpc base/src/backend/catalog/namespace.c pgsec/src/backend/catalog/namespace.c
*** base/src/backend/catalog/namespace.c Mon Nov 9 18:42:41 2009
--- pgsec/src/backend/catalog/namespace.c Sat Dec 19 04:26:37 2009
*************** LookupExplicitNamespace(const char *nspn
*** 2366,2375 ****
/*
* LookupCreationNamespace
! * Look up the schema and verify we have CREATE rights on it.
*
! * This is just like LookupExplicitNamespace except for the different
! * permission check, and that we are willing to create pg_temp if needed.
*
* Note: calling this may result in a CommandCounterIncrement operation,
* if we have to create or clean out the temp namespace.
--- 2366,2375 ----
/*
* LookupCreationNamespace
! * Look up the schema on which caller tries to create an object
*
! * This is just like LookupExplicitNamespace except for we are willing
! * to create pg_temp if needed.
*
* Note: calling this may result in a CommandCounterIncrement operation,
* if we have to create or clean out the temp namespace.
*************** LookupCreationNamespace(const char *nspn
*** 2397,2407 ****
(errcode(ERRCODE_UNDEFINED_SCHEMA),
errmsg("schema \"%s\" does not exist", nspname)));
- aclresult = pg_namespace_aclcheck(namespaceId, GetUserId(), ACL_CREATE);
- if (aclresult != ACLCHECK_OK)
- aclcheck_error(aclresult, ACL_KIND_NAMESPACE,
- nspname);
-
return namespaceId;
}
--- 2397,2402 ----
diff -Nrpc base/src/backend/commands/dbcommands.c pgsec/src/backend/commands/dbcommands.c
*** base/src/backend/commands/dbcommands.c Thu Nov 12 23:28:17 2009
--- pgsec/src/backend/commands/dbcommands.c Sat Dec 19 04:10:11 2009
*************** createdb(const CreatedbStmt *stmt)
*** 103,109 ****
Oid src_lastsysoid;
TransactionId src_frozenxid;
Oid src_deftablespace;
! volatile Oid dst_deftablespace;
Relation pg_database_rel;
HeapTuple tuple;
Datum new_record[Natts_pg_database];
--- 103,109 ----
Oid src_lastsysoid;
TransactionId src_frozenxid;
Oid src_deftablespace;
! volatile Oid dst_deftablespace = InvalidOid;
Relation pg_database_rel;
HeapTuple tuple;
Datum new_record[Natts_pg_database];
*************** createdb(const CreatedbStmt *stmt)
*** 204,209 ****
--- 204,221 ----
defel->defname);
}
+ if (dtablespacename && dtablespacename->arg)
+ {
+ char *tablespacename;
+
+ tablespacename = strVal(dtablespacename->arg);
+ dst_deftablespace = get_tablespace_oid(tablespacename);
+ if (!OidIsValid(dst_deftablespace))
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_OBJECT),
+ errmsg("tablespace \"%s\" does not exist",
+ tablespacename)));
+ }
if (downer && downer->arg)
dbowner = strVal(downer->arg);
if (dtemplate && dtemplate->arg)
*************** createdb(const CreatedbStmt *stmt)
*** 258,277 ****
datdba = GetUserId();
/*
- * To create a database, must have createdb privilege and must be able to
- * become the target role (this does not imply that the target role itself
- * must have createdb privilege). The latter provision guards against
- * "giveaway" attacks. Note that a superuser will always have both of
- * these privileges a fortiori.
- */
- if (!have_createdb_privilege())
- ereport(ERROR,
- (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("permission denied to create database")));
-
- check_is_member_of_role(GetUserId(), datdba);
-
- /*
* Lookup database (template) to be cloned, and obtain share lock on it.
* ShareLock allows two CREATE DATABASEs to work from the same template
* concurrently, while ensuring no one is busy dropping it in parallel
--- 270,275 ----
*************** createdb(const CreatedbStmt *stmt)
*** 294,299 ****
--- 292,311 ----
dbtemplate)));
/*
+ * To create a database, must have createdb privilege and must be able to
+ * become the target role (this does not imply that the target role itself
+ * must have createdb privilege). The latter provision guards against
+ * "giveaway" attacks. Note that a superuser will always have both of
+ * these privileges a fortiori.
+ */
+ if (!have_createdb_privilege())
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("permission denied to create database")));
+
+ check_is_member_of_role(GetUserId(), datdba);
+
+ /*
* Permission check: to copy a DB that's not marked datistemplate, you
* must be superuser or the owner thereof.
*/
*************** createdb(const CreatedbStmt *stmt)
*** 306,311 ****
--- 318,338 ----
dbtemplate)));
}
+ /*
+ * Permission check: to create object under explicitly given
+ * tablespace, instead of the copied one from the source database
+ */
+ if (OidIsValid(dst_deftablespace))
+ {
+ AclResult aclresult;
+
+ aclresult = pg_tablespace_aclcheck(dst_deftablespace, GetUserId(),
+ ACL_CREATE);
+ if (aclresult != ACLCHECK_OK)
+ aclcheck_error(aclresult, ACL_KIND_TABLESPACE,
+ get_tablespace_name(dst_deftablespace));
+ }
+
/* If encoding or locales are defaulted, use source's setting */
if (encoding < 0)
encoding = src_encoding;
*************** createdb(const CreatedbStmt *stmt)
*** 421,445 ****
}
/* Resolve default tablespace for new database */
! if (dtablespacename && dtablespacename->arg)
{
- char *tablespacename;
- AclResult aclresult;
-
- tablespacename = strVal(dtablespacename->arg);
- dst_deftablespace = get_tablespace_oid(tablespacename);
- if (!OidIsValid(dst_deftablespace))
- ereport(ERROR,
- (errcode(ERRCODE_UNDEFINED_OBJECT),
- errmsg("tablespace \"%s\" does not exist",
- tablespacename)));
- /* check permissions */
- aclresult = pg_tablespace_aclcheck(dst_deftablespace, GetUserId(),
- ACL_CREATE);
- if (aclresult != ACLCHECK_OK)
- aclcheck_error(aclresult, ACL_KIND_TABLESPACE,
- tablespacename);
-
/* pg_global must never be the default tablespace */
if (dst_deftablespace == GLOBALTABLESPACE_OID)
ereport(ERROR,
--- 448,455 ----
}
/* Resolve default tablespace for new database */
! if (OidIsValid(dst_deftablespace))
{
/* pg_global must never be the default tablespace */
if (dst_deftablespace == GLOBALTABLESPACE_OID)
ereport(ERROR,
*************** createdb(const CreatedbStmt *stmt)
*** 471,477 ****
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot assign new default tablespace \"%s\"",
! tablespacename),
errdetail("There is a conflict because database \"%s\" already has some tables in this tablespace.",
dbtemplate)));
pfree(srcpath);
--- 481,487 ----
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot assign new default tablespace \"%s\"",
! get_tablespace_name(dst_deftablespace)),
errdetail("There is a conflict because database \"%s\" already has some tables in this tablespace.",
dbtemplate)));
pfree(srcpath);
*************** movedb(const char *dbname, const char *t
*** 1015,1027 ****
AccessExclusiveLock);
/*
- * Permission checks
- */
- if (!pg_database_ownercheck(db_id, GetUserId()))
- aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_DATABASE,
- dbname);
-
- /*
* Obviously can't move the tables of my own database
*/
if (db_id == MyDatabaseId)
--- 1025,1030 ----
*************** movedb(const char *dbname, const char *t
*** 1041,1046 ****
--- 1044,1053 ----
/*
* Permission checks
*/
+ if (!pg_database_ownercheck(db_id, GetUserId()))
+ aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_DATABASE,
+ dbname);
+
aclresult = pg_tablespace_aclcheck(dst_tblspcoid, GetUserId(),
ACL_CREATE);
if (aclresult != ACLCHECK_OK)
diff -Nrpc base/src/backend/commands/functioncmds.c pgsec/src/backend/commands/functioncmds.c
*** base/src/backend/commands/functioncmds.c Mon Nov 9 18:42:41 2009
--- pgsec/src/backend/commands/functioncmds.c Sat Dec 19 04:42:09 2009
*************** AlterFunctionNamespace(List *name, List
*** 1874,1879 ****
--- 1874,1880 ----
HeapTuple tup;
Relation procRel;
Form_pg_proc proc;
+ AclResult aclresult;
procRel = heap_open(ProcedureRelationId, RowExclusiveLock);
*************** AlterFunctionNamespace(List *name, List
*** 1897,1903 ****
oldNspOid = proc->pronamespace;
! /* get schema OID and check its permissions */
nspOid = LookupCreationNamespace(newschema);
if (oldNspOid == nspOid)
--- 1898,1904 ----
oldNspOid = proc->pronamespace;
! /* get schema OID */
nspOid = LookupCreationNamespace(newschema);
if (oldNspOid == nspOid)
*************** AlterFunctionNamespace(List *name, List
*** 1907,1912 ****
--- 1908,1921 ----
NameListToString(name),
newschema)));
+ /*
+ * check permissions on schema
+ * XXX - to be consolidated with pg_proc_ownercheck() later
+ */
+ aclresult = pg_namespace_aclcheck(nspOid, GetUserId(), ACL_CREATE);
+ if (aclresult != ACLCHECK_OK)
+ aclcheck_error(aclresult, ACL_KIND_NAMESPACE, newschema);
+
/* disallow renaming into or out of temp schemas */
if (isAnyTempNamespace(nspOid) || isAnyTempNamespace(oldNspOid))
ereport(ERROR,
diff -Nrpc base/src/backend/commands/tablecmds.c pgsec/src/backend/commands/tablecmds.c
*** base/src/backend/commands/tablecmds.c Fri Dec 11 14:58:31 2009
--- pgsec/src/backend/commands/tablecmds.c Sat Dec 19 04:42:26 2009
*************** AlterTableNamespace(RangeVar *relation,
*** 7780,7785 ****
--- 7780,7786 ----
Oid oldNspOid;
Oid nspOid;
Relation classRel;
+ AclResult aclresult;
rel = relation_openrv(relation, AccessExclusiveLock);
*************** AlterTableNamespace(RangeVar *relation,
*** 7856,7862 ****
RelationGetRelationName(rel))));
}
! /* get schema OID and check its permissions */
nspOid = LookupCreationNamespace(newschema);
if (oldNspOid == nspOid)
--- 7857,7863 ----
RelationGetRelationName(rel))));
}
! /* get schema OID */
nspOid = LookupCreationNamespace(newschema);
if (oldNspOid == nspOid)
*************** AlterTableNamespace(RangeVar *relation,
*** 7866,7871 ****
--- 7867,7880 ----
RelationGetRelationName(rel),
newschema)));
+ /*
+ * permission check on schema
+ * XXX - to be consolidated with pg_class_ownercheck() later
+ */
+ aclresult = pg_namespace_aclcheck(nspOid, GetUserId(), ACL_CREATE);
+ if (aclresult != ACLCHECK_OK)
+ aclcheck_error(aclresult, ACL_KIND_NAMESPACE, newschema);
+
/* disallow renaming into or out of temp schemas */
if (isAnyTempNamespace(nspOid) || isAnyTempNamespace(oldNspOid))
ereport(ERROR,
diff -Nrpc base/src/backend/commands/typecmds.c pgsec/src/backend/commands/typecmds.c
*** base/src/backend/commands/typecmds.c Fri Dec 11 14:58:31 2009
--- pgsec/src/backend/commands/typecmds.c Sat Dec 19 04:57:11 2009
*************** AlterTypeNamespace(List *names, const ch
*** 2690,2695 ****
--- 2690,2696 ----
Oid typeOid;
Oid nspOid;
Oid elemOid;
+ AclResult aclresult;
/* Make a TypeName so we can use standard type lookup machinery */
typename = makeTypeNameFromNameList(names);
*************** AlterTypeNamespace(List *names, const ch
*** 2700,2708 ****
aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_TYPE,
format_type_be(typeOid));
! /* get schema OID and check its permissions */
nspOid = LookupCreationNamespace(newschema);
/* don't allow direct alteration of array types */
elemOid = get_element_type(typeOid);
if (OidIsValid(elemOid) && get_array_type(elemOid) == typeOid)
--- 2701,2717 ----
aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_TYPE,
format_type_be(typeOid));
! /* get schema OID */
nspOid = LookupCreationNamespace(newschema);
+ /*
+ * permission checks on schema
+ * XXX - to be consolidated with pg_type_ownercheck() later
+ */
+ aclresult = pg_namespace_aclcheck(nspOid, GetUserId(), ACL_CREATE);
+ if (aclresult != ACLCHECK_OK)
+ aclcheck_error(aclresult, ACL_KIND_NAMESPACE, newschema);
+
/* don't allow direct alteration of array types */
elemOid = get_element_type(typeOid);
if (OidIsValid(elemOid) && get_array_type(elemOid) == typeOid)
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
