Обсуждение: sepgsql logging

On 4/1/21 8:32 AM, Dave Page wrote:
> Hi
>
> I've been trying to figure out selinux with sepgsql (which is proving
> quite difficult as there is an almost total lack of
> documentation/blogs etc. on the topic) and ran into an issue. Whilst
> my system had selinux in enforcing mode, I mistakenly had sepgsql in
> permissive mode. I created a table and restricted access to one column
> to regular users using the label
> system_u:object_r:sepgsql_secret_table_t:s0. Because sepgsql was in
> permissive mode, my test user could still access the restricted column.
>
> Postgres logged this:
>
> 2021-03-31 17:12:29.713 BST [3917] LOG:  SELinux: allowed { select }
> scontext=user_u:user_r:user_t:s0
> tcontext=system_u:object_r:sepgsql_secret_table_t:s0 tclass=db_column
> name="column private of table t1"
>
> That's very confusing, because the norm in selinux is to log denials
> as if the system were in enforcing mode, but then allow the action to
> proceed anyway, when in permissive mode. For example, log entries such
> as this are created when my restricted user tries to run an executable
> from /tmp after running "setsebool -P user_exec_content off":
>
> type=AVC msg=audit(1617278924.917:484): avc:  denied  { execute } for
>  pid=53036 comm="bash" name="ls" dev="dm-0" ino=319727
> scontext=user_u:user_r:user_t:s0
> tcontext=user_u:object_r:user_tmp_t:s0 tclass=file permissive=1
>
> The point being to let the admin know what would fail if the system
> were switched to enforcing mode. Whilst that wasn't the point of what
> I was trying to do, such a message would have indicated to me that I
> was in permissive mode without realising.
>
> It seems to me that sepgsql should also log the denial, but flag that
> permissive mode is on.
>
> Any reason not to do that?


+1 for doing what selinux does if possible.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Andrew Dunstan <andrew@dunslane.net> writes:
> On 4/1/21 8:32 AM, Dave Page wrote:
>> It seems to me that sepgsql should also log the denial, but flag that
>> permissive mode is on.

> +1 for doing what selinux does if possible.

+1.  If selinux itself is doing that, it's hard to see a reason why
we should not; and I concur that the info is useful.

            regards, tom lane

On Wed, Apr 14, 2021 at 8:42 AM Dave Page <dpage@pgadmin.org> wrote:
> Attached is a patch to clean this up. It will log denials as such regardless of whether or not either selinux or
sepgsqlis in permissive mode. When either is in permissive mode, it'll add " permissive=1" to the end of the log
messages.e.g.
 

Looks superficially reasonable on first glance, but I think we should
try to get an opinion from someone who knows more about SELinux.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

On Wed, Apr 14, 2021 at 8:42 AM Dave Page <dpage@pgadmin.org> wrote:
> Attached is a patch to clean this up. It will log denials as such
> regardless of whether or not either selinux or sepgsql is in
> permissive mode. When either is in permissive mode, it'll add "
> permissive=1" to the end of the log messages. e.g.

Dave,

Just to clarify -- it looks like this patch *only* adds the
"permissive=1" part, right? I don't see any changes around denied-vs-
allowed.

I read the previous posts to mean that you were seeing "allowed" when
you should have been seeing "denied". I don't see that behavior --
without this patch, I see the correct "denied" entries even when
running in permissive mode. (It's been a while since the patch was
posted, so I checked to make sure there hadn't been any relevant
changes in the meantime, and none jumped out at me.)

That said, the patch looks good as-is and seems to be working for me on
a Rocky 8 VM. (You weren't kidding about the setup difficulty.) Having
permissive mode show up in the logs seems very useful.

As an aside, I don't see the "allowed" verbiage that sepgsql uses in
any of the SELinux documentation. I do see third-party references to
"granted", though, as in e.g.

    avc: granted { execute } for ...

That's not something that I think this patch should touch, but it
seemed tangentially relevant for future convergence work.

On Wed, 2021-04-14 at 09:49 -0400, Robert Haas wrote:
> Looks superficially reasonable on first glance, but I think we should
> try to get an opinion from someone who knows more about SELinux.

I am not that someone, but this looks straightforward, it's been
stalled for a while, and I think it should probably go in.

--Jacob

On 1/11/22 10:40, Dave Page wrote:
>
>
>     On Wed, 2021-04-14 at 09:49 -0400, Robert Haas wrote:
>     > Looks superficially reasonable on first glance, but I think we
>     should
>     > try to get an opinion from someone who knows more about SELinux.
>
>     I am not that someone, but this looks straightforward, it's been
>     stalled for a while, and I think it should probably go in.
>
>
> I'd like to see that. Thanks for the review. 
>

I am not that person either. I agree this looks reasonable, but I also
would like the opinion of an expert, if we have one.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Andrew Dunstan <andrew@dunslane.net> writes:
> I am not that person either. I agree this looks reasonable, but I also
> would like the opinion of an expert, if we have one.

I'm not sure we do anymore.  Anyway, I tried this on Fedora 35 and
confirmed that it compiles and the (very tedious) test process
described in the sepgsql docs still passes.  Looking in the system's
logs, it appears that Dave didn't precisely emulate how SELinux
logs this setting, because I see messages like

Jan  4 12:25:46 nuc1 audit[1754]: AVC avc:  denied  { setgid } for  pid=1754 comm="sss_cache" capability=6
scontext=unconfined_u:unconfined_r:useradd_t:s0-s0:c0.c1023tcontext=unconfined_u:unconfined_r:useradd_t:s0-s0:c0.c1023
tclass=capabilitypermissive=0 

So it looks like their plan is to unconditionally write "permissive=0"
or "permissive=1", while Dave's patch just prints nothing in enforcing
mode.  While I can see some virtue in brevity, I think that doing
exactly what SELinux does is probably a better choice.  For one thing,
it'd remove doubt about whether one is looking at a log from a sepgsql
version that logs this or one that doesn't.

Other than that nitpick, I think we should just push this.

            regards, tom lane

Dave Page <dpage@pgadmin.org> writes:
> On Tue, Jan 11, 2022 at 5:55 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> So it looks like their plan is to unconditionally write "permissive=0"
>> or "permissive=1", while Dave's patch just prints nothing in enforcing
>> mode.  While I can see some virtue in brevity, I think that doing
>> exactly what SELinux does is probably a better choice.  For one thing,
>> it'd remove doubt about whether one is looking at a log from a sepgsql
>> version that logs this or one that doesn't.

> Here's an update that adds the "permissive=0" case.

You forgot to update the expected-results files :-(.
Done and pushed.

            regards, tom lane