Re: SE-PgSQL patch review

Itagaki Takahiro wrote:
> I'm reviewing SE-PgSQL patch and have some comments.
> https://commitfest.postgresql.org/action/patch_view?id=212
>
> Forgive me, but I'm a novice of SELinux. Some of the comments
> and question might be nonsense.

Itagaki-san, thanks for your volunteering so much!

> ==== Patch overview ====
> The patch itself is large (>13K), but more than half of it are
> well-written regression test, comments and README.
>
> It adds object-level and column-level security checks after normal
> permission checks. Row-level checks are not included in the patch.

Yes, I separated most of features to reduce code size.
The developer documentation and source code comments were added instead.

> ==== Syntax and identifier names ====
> * Can we use "security context" only for securty checking but also
> for general "context" handling? If so, we can call it just "context".
> It might be useful if the context is designed for more general purpose.
> For example, we could develop context-based logging or contex-aware
> setting parameters. I think "Application Name" patch is one of the
> implementations of context-based logging.
> https://commitfest.postgresql.org/action/patch_view?id=195

The "security context" is a proper-noun in SELinux.
I don't think it is a good idea to call it just a "context".

If we have a set of properties of the client called as a context,
security context is one of the property in a context, such as user
identifier, application name and so on.

If Log_line_prefix supports the security context of the client,
it seems me a good idea. (However, we can implement it in the future.)

BTW, SELinux also provides indications about what should be logged
on access violation events. It is also an duty of userspace object
manager. See the sepgsql_audit_log() in selinux.c.

> * CREATE TABLE tbl (...) SECURITY_CONTEXT = '...'
> IMHO, '_' and '=' don't suit for SQL.
> If there are no conflicts in gram.y, how about removing them?
> CREATE TABLE tbl (...) SECURITY CONTEXT '...'

I tried it, and it seems to me fine.

> * CREATE TABLE tbl (col integer AS SECURITY_CONTEXT = '...')
> Is the syntax "<AS> SECURITY_CONTEXT" natural in English?

We need to put a reserved token, such as "AS", prior to the SECURITY_CONTEXT
to avoid syntax conflicts to "DEFAULT b_expr" option.

> * Since SE-PgSQL will be integrated into the core, we can use pg_*
> names for the feature. For example, we can rename sepgsql_getcon() to
> pg_get_security_context(). Also, we should not use 'con' as an
> abbreviated form of 'context' because we often use it for 'connection'.
> The same can be said for GUC parameter names.

What is your opinion for '*_secon()', instead of '*_security_context'?
If OK, these functions will be renamed as follows:

- sepgsql_template1_getcon -> pg_get_template1_secon
- sepgsql_default_getcon -> pg_get_default_secon
- sepgsql_getcon -> pg_get_client_secon
- sepgsql_database_getcon -> pg_get_database_secon
- sepgsql_schema_getcon -> pg_get_schema_secon
- sepgsql_relation_getcon -> pg_get_relation_secon
- sepgsql_attribute_getcon -> pg_get_attribute_secon

> (ex: "sepostgresql" to be "security_policy")

I prefer "selinux_support" more than "security_policy", because
it is a noun to mean a set of access control rules in the kernel.

> ==== Error messages and error codes ====
> * It uses dedicated 'SExxx' error codes, but I think they should belong to
> the same family of ERRCODE_INSUFFICIENT_PRIVILEGE (42501).
> /* Class SE - SE-PostgreSQL Error (SE-PgSQL-specific error class) */
> #define ERRCODE_SELINUX_INTERNAL_ERROR MAKE_SQLSTATE('S','E', '0','0','1')
> #define ERRCODE_INVALID_SECURITY_CONTEXT MAKE_SQLSTATE('S','E', '0','0','2')
> #define ERRCODE_SELINUX_AUDIT_LOG MAKE_SQLSTATE('S','E', '0','0','3')

I already uses predefined error code, if exist.
For example, sepgsql_compute_perms() is the routine to make access control
decision in SELinux. It uses ERRCODE_INSUFFICIENT_PRIVILEGE, when it needs
to raise an error.

extern bool
sepgsql_compute_perms(char *scontext, char *tcontext,
uint16 tclass, uint32 required,
const char *audit_name, bool abort)
{
:
/*
* It asks SELinux whether the given access should be allowed,
* or not. It set the given avd structure correctly, then returns.
*/
compute_perms_internal(scontext, tcontext, tclass, &avd);
:
/*
* If here is no policy violations, or SE-PgSQL performs in permissive
* mode, or the client process peforms in permissive domain, it returns
* normally with 'true'.
*/
if (!denied ||
!sepgsql_get_enforce() ||
(avd.flags & SELINUX_AVD_FLAGS_PERMISSIVE) != 0)
return true;
:
/*
* Otherwise, it raises an error or returns 'false', depending on the
* caller's indication by 'abort'.
*/
if (abort)
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("SELinux: security policy violation")));

return false;
}

These error codes has its own meanings.
* ERRCODE_SELINUX_INTERNAL_ERROR
It is an error due to the communication between PostgreSQL and SELinux.
It seems similar to ERRCODE_INTERNAL_ERROR, but it is not "can't-happen"
conditions, so I defined it individually.

* ERRCODE_INVALID_SECURITY_CONTEXT
It is used when client gives an invalid security context on CREATE or
ALTER statement. I could not find any similar predefined error code.

* ERRCODE_SELINUX_AUDIT_LOG
It is used when SE-PgSQL generates access violation logs, not an error.
I also could not find any similar predefined error code.

> * Can we use error messages that looks like existing privilege errors?
> ERRCODE_INSUFFICIENT_PRIVILEGE:
> => permission denied to rename database

Here is a point that we should provide users a hint which enables
to make clear the reason of access violations. So, I think we should
contains a mark to show it come from SE-PgSQL.

Currently, it raises an error on access violation in sepgsql_compute_perms().
It always prints out "SELinux: security policy violation".

> Error messages from SE-PgSQL
> => SE-PgSQL prevents to modify "pg_class" by hand
> looks very different. I'd suggest something like
> => security context denied to rename database

If we follow the existing manner, it may be:
"SELinux: permission denied: \"%s\" is a system catalog"

> I'll suggest we avoid using the term "SE-PgSQL" in the user visible
> message and documentation because because the feature will be integrated
> into the core deeply. Of course, we can use "SE-PgSQL" in *promotion*.

If we use another name for the feature, "SELinux support" may be preferable?

> ==== Internal structures ====
> * Are the security labels enough stable?
> What will we need when SELinux configuration is modified?
> We store security labels as text for each object and column.

If the security labels get invalid due to the modification of SELinux
configuration or other reasons, it considers the database objects are
unlabeled.

Whenever we fetch a text representation of security label from system
catalog, it invokes security_check_context_raw() to check whether the
security label is valid on the current configuration, or not.

If invalid, it obtains an "unlabeled" security context from the system.

For example,

bool
sepgsql_database_common(Oid datOid, uint32 required, bool abort)
{
:
/*
* (1) It fetches a security context from pg_database.datsecon
* as a text data.
*/
tuple = SearchSysCache(DATABASEOID,
ObjectIdGetDatum(datOid),
0, 0, 0);
if (!HeapTupleIsValid(tuple))
elog(ERROR, "cache lookup failed for database %u", datOid);

datsecon = SysCacheGetAttr(DATABASEOID, tuple,
Anum_pg_database_datsecon, &isnull);
if (!isnull)
context = TextDatumGetCString(datsecon);
/*
* (2) If the fetched security context is not a valid one,
* we obtain system's "unlabeled" security context as a fallback.
* - security_check_context_raw() applies validation checks
* - sepgsql_get_unlabeled_context() returns "unlabeled" context.
* Typically, it is "system_u:object_r:unlabeled_t:s0"
*/
if (!context || security_check_context_raw(context) < 0)
context = sepgsql_get_unlabeled_context();
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
/*
* (3) We call permission check routine on a couple of security
* contexts (client and the target database).
* It may be a valid security context, or "unlabeld" security
* context. In either case, we can provide a valid security context.
*/
audit_name = NameStr(((Form_pg_database) GETSTRUCT(tuple))->datname);
rc = sepgsql_compute_perms(sepgsql_get_client_context(),
context,
SEPG_CLASS_DB_DATABASE,
required,
audit_name, abort);
ReleaseSysCache(tuple);

return rc;
}

> * Each security checks are called after pg_*_aclcheck(). Is it possible to
> merge SE-PgSQL checks into ACL functions in aclchk.c ?

It is impossible due to some reasons.

For example, some of entrypoints are enclosed by "if (superuser())".
It implicitly allows any clients with superuser attribute to bypass MAC
checks, but it is quite violates SE-PgSQL's design, if we put the hooks
in the PG default model routines.

For example, SE-PgSQL model distinguish "setattr" permission from "drop".
But pg_class_ownercheck() is used for both ALTER and DROP statement.
So, we cannot know which permission should be applied inside from the
pg_class_ownercheck().

For example, ...

We already tried to provide a common set of entrypoints for both of DAC
and MAC at the last commit fest ("Reworks for Access Controls" patch).
This abstraction layer had to provide enough information to make access
control decisions for both of models, and be deployed on the strategic
points for both models. But it needed massive amount of reworks in the
core routines, so we decided this approach is not a right direction.

> * What is difference between sepgsql_database_getcon(oid) and
> pg_database.datsecon? Why do we need those getcon functions?

SELinux allows to translate the last field of security context between
a human readable form and a raw form.

For example, if a table is labeled as follows:
system_u:object_r:sepgsql_table_t:s0:c1
^^^^^
We call this field as "range" or "mcs label". In the raw-format, it is
a complex of symbols, but we can assign human meaningfull alias on them.
If "s0:c1" has its alias ("1st_sales_division"), above security context
shall be printed as:
system_u:object_r:sepgsql_table_t:1st_sales_division
^^^^^^^^^^^^^^^^^^
This feature is called "mcstrans".
It is necessary to port a definition of security context to other hosts
which may not have same mapping between a human-readable and raw format.

Thus, I used sepgsql_xxxx_getcon() function in the pg_dump to follows
the manner in SELinux when we export/import security context of objects
from/to the object manager (PostgreSQL).

I'll revise the following things at first:
- Replace SE-PgSQL from user visible error messages and documentations.
- SECURITY_CONTEXT [=] '<label>' to be SECURITY CONTEXT '<label>'

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>