Re: security hook on authorization

Sorry for this late responding. I got a cold later half of the last week.

(2010/10/20 12:10), Robert Haas wrote:
> On Wed, Oct 13, 2010 at 2:13 AM, Robert Haas<robertmhaas(at)gmail(dot)com> wrote:
>> 2010/8/24 KaiGai Kohei<kaigai(at)ak(dot)jp(dot)nec(dot)com>:
>>> I tried to revise the patch. It allows plugins to get control next to
>>> client authentication, but before returning the status to users.
>>>
>>> This change enables plugins which should be invoked on authentication
>>> failed to utilize this hook, not only assignment of session security
>>> label.
>>> At the same time, it disables to hook on SET SESSION AUTHORIZATION.
>>> But it is a bit unclear whether we should hook here, or not.
>>
>> Stephen -
>>
>> You've been listed as a reviewer for this in the CF app since 9/17 -
>> are you planning to review it?
>
> I guess not.
>
> I took a brief look at this tonight, and it seems to me that it still
> fails the test I proposed nearly two months ago:
>
> http://archives.postgresql.org/pgsql-hackers/2010-08/msg01458.php
>
> KaiGai responded with:
>
>> If and when a connection came from a host but we don't accept the
>> delivered security label, or labeled networking is misconfigured,
>> getpeercon(3) returns NULL. In this case, server cannot identify
>> what label should be applied on the client, then, we should
>> disconnect this connection due to the error on database login,
>> not any access control decision.
>>
>> In similar case, psm_selinux.so disconnect the connection when
>> it cannot identify what security label shall be assigned on the
>> session, due to some reasons such as misconfigurations.
>>
>> Without any hooks at authorization stage (but it might be different
>> place from this patch, of course), we need to delay the error
>> handling by the time when SE-PostgreSQL module is invoked at first.
>> But it is already connection established and user sends a query.
>> It seems to me quite strange behavior.
>
> I don't find this very convincing. We are still several patches from
> having a working SE-PostgreSQL module, and I think we should be
> worried about getting off the ground before we worry about this sort
> of fine-tuning. I don't see reporting an SE-PostgreSQL error slightly
> sooner is worth a separate hook, especially given that we're still
> several patches from having even a toy SE-PostgreSQL implementation.
> For example, we may find that some other hook that is more certainly
> necessary can also serve the purpose intended for this one.
>
At least, we need a feature to raise an error when the SE-PgSQL module
cannot retrieve security context of the peer process, because it is
similar to a connection string without username/password.

However, the post-authentication hook is not the only option for us.
What I want here is the core PG gives the SE-PgSQL module a chance to
call getpeercon(3) before accepting user's queries.

One possible candidate is CheckMyDatabase() that checks ACL_CONNECT
permission for the required database prior to execution of all the
queries.
Currently, we don't have any security hook around here.
But, if we have "InvokeSecurityHook()" here, it will be able to
kill two birds with one stone. The 1st bird is getpeercon(3), and
the 2nd bird is permission check on the selected database.

> And later with:
>
>> Yes, I also think this kind of authorization hook should benefit other
>> applications, not only label based mac features.
>>
>> For example, something like 'last' command in operations system which
>> records username and login-time. Stephen mentioned pam_tally that locks
>> down certain accounts who failed authentication too much.
>> Perhaps, PAM modules in operating system give us some hints about other
>> possible applications.
>
> This is closer to the mark, but mostly speculative, and not detailed
> enough to determine whether the proposed hook is properly located. It
> seems rather early to me: this is before we've sent the authentication
> packet to the client, so we couldn't, for example, log the success or
> failure of the authentication; we don't know whether it will succeed
> or fail.
>
Hmm. But the auth_failed() raises a fatal error, so we need to put
a hook before the invocation to log a case of authentication failed.

| + if (ClientAuthentication_hook)
| + (*ClientAuthentication_hook)(port, status);
| +
| if (status == STATUS_OK)
| sendAuthRequest(port, AUTH_REQ_OK);
| else
| auth_failed(port, status);

Or, perhaps, we should modify this if-block to ensure the hook being
called after sendAuthRequest() but before auth_failed().

> I am going to mark this Returned with Feedback. I would like to
> request that any future submissions to add a hook in this area be
> accompanied by a working sample contrib module that is not SE-Linux
> specific.
>
For example, if a contrib module provides a feature to sleep a few
seconds when authentication failed, it prevents brute-force attack.
Do you think it is a good example as an evidence of this module?

Anyway, I have any preference on these two ideas right now.
It seems to me the contrib module will be small enough, but well
works as proof of concept. On the other hand, eventually we will
put a hook on CheckMyDatabase(), if so, it is not a bad idea to
kill two birds in a stone (hook) now.

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>