Re: add a MAC check for TRUNCATE - Mailing list pgsql-hackers

On Fri, Sep 6, 2019 at 11:26 AM Yuli Khodorkovskiy
<yuli.khodorkovskiy@crunchydata.com> wrote:
>
> On Fri, Sep 6, 2019 at 10:40 AM Stephen Frost <sfrost@snowman.net> wrote:
> >
> > Greetings,
>
> Hello Stephen,
>
> >
> > * Yuli Khodorkovskiy (yuli.khodorkovskiy@crunchydata.com) wrote:
> > > On Tue, Sep 3, 2019 at 3:25 PM Stephen Frost <sfrost@snowman.net> wrote:
> > > > * Kohei KaiGai (kaigai@heterodb.com) wrote:
> > > > > 2019年7月25日(木) 3:52 Yuli Khodorkovskiy <yuli.khodorkovskiy@crunchydata.com>:
> > > > > > Since all DAC checks should have corresponding MAC, this patch adds a
> > > > > > hook to allow extensions to implement a MAC check on TRUNCATE. I have
> > > > > > also implemented this access check in the sepgsql extension.
> > > > > >
> > > > > > One important thing to note is that refpolicy [1] and Redhat based
> > > > > > distributions do not have the SELinux permission for db_table {truncate}
> > > > > > implemented.
> > > > > >
> > > > > How db_table:{delete} permission is different from truncate?
> > > > > >From the standpoint of data access, TRUNCATE is equivalent to DELETE
> > > > > without WHERE, isn't it?
> > > > > Of course, there are some differences between them. TRUNCATE takes
> > > > > exclusive locks and eliminates underlying data blocks, on the other hands,
> > > > > DELETE removes rows under MVCC manner. However, both of them
> > > > > eventually removes data from the target table.
> > > > >
> > > > > I like to recommend to reuse "db_table:{delete}" permission for TRUNCATE.
> > > > > How about your opinions?
> > > >
> > > > There's been much discussion and justifcation for adding an independent
> > > > TRUNCATE privilege to GRANT (which actually took many years to be
> > > > allowed).  I don't see why we wouldn't represent that as a different
> > > > privilege to external MAC systems.  If the external MAC system wishes to
> > > > use db_table:{delete} to decide if the privilege is allowed or not, they
> > > > can, but I don't think core should force that when we have them as
> > > > independent permissions.
> > > >
> > > > So, perhaps we can argue about what the sepgsql extension should do, but
> > > > it's clear that we should have an independent hook for this in core.
> > > >
> > > > Isn't there a way to allow an admin to control if db_table:{truncate} is
> > > > allowed for users with db_table:{delete}, or not?  Ideally, this could
> > > > be managed at the SELinux level instead of having to have something
> > > > different in sepgsql or core, but if it needs to be configurable there
> > > > too then hopefully we can come up with a good solution.
> > >
> > > If I understand you correctly, you are asking if an SELinux domain can
> > > have the db_table:{truncate} permission but not db_table:{delete}
> > > using SELinux policy? This would only work if the userspace object
> > > manager, sepgsql in this case, reaches out to the policy server to
> > > check if db_table:{truncate} is allowed for a subject accessing an
> > > object.
> >
> > I was saying that, I believe, it would be pretty straight-forward for an
> > SELinux admin to add db_table:{truncate} to whatever set of individuals
> > are allowed to use db_table:{delete}.
>
> Okay that makes sense. Yes that can definitely be done, and the
> original sepgsql patch accomplished what you are describing. I did not
> add tests or SELinux policy granting `db_table: { truncate }` in the
> regressions of the original patch. If the community decides a new
> SELinux permission in sepgsql for TRUNCATE is the correct path, I will
> gladly update the original patch.

Ah, now I remember why I didn't add regressions to the original patch.
As stated at the top of the thread, the "db_table: { truncate }"
permission does not currently exist in refpolicy. A workaround would
be to add the policy with CIL, but that adds unneeded complexity to
the regressions. I think the correct path forward is:

1) Get the sepgsql changes in without policy/regressions
2) Send a patch to refpolicy for the new permission
3) Once Redhat updates the selinux-policy-targeted RPM to include the
new permissions, I will send an update to the sepgsql regressions and
policy.

Thank you.

>
> >
> > > I think it should be okay to use db_table:{delete} as the permission
> > > to check for TRUNCATE in the object manager. I have attached a second
> > > version of the hook and sepgsql changes to demonstrate this.
> >
> > There are actual reasons why the 'DELETE' privilege is *not* the same as
> > 'TRUNCATE' in PostgreSQL and I'm really not convinced that we should
> > just be tossing that distinction out the window for users of SELinux.  A
> > pretty obvious one is that DELETE triggers don't get fired for a
> > TRUNCATE command, but TRUNCATE also doesn't follow the same MVCC rules
> > that the rest of the system does.
>
> I do agree with you there should be a distinction between TRUNCATE and
> DELETE in the SELinux perms. I'll wait a few days for more discussion
> and send an updated patch.
>
> Thank you.
>
> >
> > If TRUNCATE and DELETE were the same, we'd only have DELETE and we would
> > just make it super-fast by implementing it the way we implement
> > TRUNCATE.
> >
> > Thanks,
> >
> > Stephen