Re: add a MAC check for TRUNCATE
On 11/22/19 3:07 AM, Michael Paquier wrote:
> On Wed, Nov 20, 2019 at 02:30:12PM -0500, Joe Conway wrote:
>> I tested this successfully on Rhinoceros, both with and without
>> "db_table: { truncate }" loaded in the policy. Updated patches attached
>> here with some editorialization. If there are no objections I will
>> commit/push both in about a day or two.
>
> Thanks for the update, Joe. I have switched the patch as ready for
> committer, with your name as committer of the entry to reflect this
> status.
Pushed.
Thanks,
Joe
--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development
Re: add a MAC check for TRUNCATE
On Wed, Nov 20, 2019 at 02:30:12PM -0500, Joe Conway wrote:
> I tested this successfully on Rhinoceros, both with and without
> "db_table: { truncate }" loaded in the policy. Updated patches attached
> here with some editorialization. If there are no objections I will
> commit/push both in about a day or two.
Thanks for the update, Joe. I have switched the patch as ready for
committer, with your name as committer of the entry to reflect this
status.
--
Michael
signature.asc
Description: PGP signature
Re: add a MAC check for TRUNCATE
On 11/20/19 2:30 PM, Joe Conway wrote:
> On 11/8/19 9:16 AM, Joe Conway wrote:
>> On 11/8/19 9:02 AM, Yuli Khodorkovskiy wrote:
>>> On Thu, Nov 7, 2019 at 7:46 PM Michael Paquier wrote:
On Mon, Sep 30, 2019 at 11:38:05AM -0300, Alvaro Herrera wrote:
> On 2019-Sep-30, Joe Conway wrote:
>
> > I am not sure I will get to this today. I assume it is ok for me to
> > move
> > it forward e.g. next weekend, or is that not in line with commitfest
> > rules?
>
> You can commit whatever patch whenever you feel like it. I will
> probably move this patch to the next commitfest before that, but you can
> mark it committed there as soon as you commit it.
One month later, nothing has happened here. Joe, are you planning to
look at this patch?
The last patch I found does not apply properly, so please provide a
rebase. I am switching the patch as waiting on author.
>>>
>>> Michael,
>>>
>>> I was able to apply the latest patches in the thread (9/25/19) on top
>>> of master. I have attached them for convenience.
>>
>> Yes, I will look when I am able. Hopefully this weekend, almost
>> certainly before the end of this commitfest.
>
> I tested this successfully on Rhinoceros, both with and without
> "db_table: { truncate }" loaded in the policy. Updated patches attached
> here with some editorialization. If there are no objections I will
> commit/push both in about a day or two.
...and I managed to drop the new files from the sepgsql patch. Complete
version attached.
Joe
--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development
Add object TRUNCATE hook
All operations with acl permissions checks should have a corresponding hook
so that, for example, mandatory access control (MAC) may be enforced by an
extension. The command TRUNCATE is missing this hook, so add it. Patch by
Yuli Khodorkovskiy with some editorialization by me. Based on the discussion
not back-patched. A separate patch will exercise the hook in the sepgsql
extension.
Author: Yuli Khodorkovskiy
Reviewed-by: Joe Conway
Discussion: https://postgr.es/m/CAFL5wJcomybj1Xdw7qWmPJRpGuFukKgNrDb6uVBaCMgYS9dkaA%40mail.gmail.com
---
diff --git a/src/backend/catalog/objectaccess.c b/src/backend/catalog/objectaccess.c
index b1619b8..d51d01f 100644
*** a/src/backend/catalog/objectaccess.c
--- b/src/backend/catalog/objectaccess.c
***
*** 11,16
--- 11,17
#include "postgres.h"
#include "catalog/objectaccess.h"
+ #include "catalog/pg_class.h"
#include "catalog/pg_namespace.h"
#include "catalog/pg_proc.h"
*** RunObjectDropHook(Oid classId, Oid objec
*** 65,70
--- 66,87
}
/*
+ * RunObjectTruncateHook
+ *
+ * It is the entrypoint of OAT_TRUNCATE event
+ */
+ void
+ RunObjectTruncateHook(Oid objectId)
+ {
+ /* caller should check, but just in case... */
+ Assert(object_access_hook != NULL);
+
+ (*object_access_hook) (OAT_TRUNCATE,
+ RelationRelationId, objectId, 0,
+ NULL);
+ }
+
+ /*
* RunObjectPostAlterHook
*
* It is entrypoint of OAT_POST_ALTER event
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 45aae59..5440eb9 100644
*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
*** truncate_check_rel(Oid relid, Form_pg_cl
*** 1937,1942
--- 1937,1944
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("permission denied: \"%s\" is a system catalog",
relname)));
+
+ InvokeObjectTruncateHook(relid);
}
/*
diff --git a/src/include/catalog/objectaccess.h b/src/include/catalog/objectaccess.h
index 0170f1f..9824adf 100644
*** a/src/include/catalog/objectaccess.h
--- b/src/include/catalog/objectaccess.h
***
*** 37,42
--- 37,46
* creation or altering, because OAT_POST_CREATE or OAT_POST_ALTER are
* sufficient for extensions to track these kind of checks.
*
+ * OAT_TRUNCATE should be invoked just before truncation of objects. This
+ * event is equivalent to truncate permission on a relation under the
+ * default access control mechanism.
+ *
* Other types may be added in the future.
*/
typedef enum ObjectAccessType
*** typedef enum ObjectAccessType
*** 45,51
OAT_DROP,
OAT_POST_ALTER,
OAT_NAMESPACE_SEARCH,
! OAT_FUNCTION_EXECUTE
} ObjectAccessType;
/*
--- 49,56
OAT_DROP,
OAT_POST_ALTER,
OAT_NAMESPACE_SEARCH,
! OAT_FUNCTION_EXECUTE,
! OAT_TRUNCATE
} ObjectAccessType;
/*
*** extern void RunObjectPostCreateHook(Oid
*** 131,136
--- 136,142
bool is_internal);
extern void RunObjectDropHook(Oid classId, Oid objectId, int subId,
int dropflags);
+ extern void RunObjectTruncateHook(Oid objectId);
extern void RunObjectPostAlterHook(Oid classId, Oid objectId, int subId,
Re: add a MAC check for TRUNCATE
On 11/8/19 9:16 AM, Joe Conway wrote:
> On 11/8/19 9:02 AM, Yuli Khodorkovskiy wrote:
>> On Thu, Nov 7, 2019 at 7:46 PM Michael Paquier wrote:
>>>
>>> On Mon, Sep 30, 2019 at 11:38:05AM -0300, Alvaro Herrera wrote:
>>> > On 2019-Sep-30, Joe Conway wrote:
>>> >
>>> > > I am not sure I will get to this today. I assume it is ok for me to move
>>> > > it forward e.g. next weekend, or is that not in line with commitfest
>>> > > rules?
>>> >
>>> > You can commit whatever patch whenever you feel like it. I will
>>> > probably move this patch to the next commitfest before that, but you can
>>> > mark it committed there as soon as you commit it.
>>>
>>> One month later, nothing has happened here. Joe, are you planning to
>>> look at this patch?
>>>
>>> The last patch I found does not apply properly, so please provide a
>>> rebase. I am switching the patch as waiting on author.
>>
>> Michael,
>>
>> I was able to apply the latest patches in the thread (9/25/19) on top
>> of master. I have attached them for convenience.
>
> Yes, I will look when I am able. Hopefully this weekend, almost
> certainly before the end of this commitfest.
I tested this successfully on Rhinoceros, both with and without
"db_table: { truncate }" loaded in the policy. Updated patches attached
here with some editorialization. If there are no objections I will
commit/push both in about a day or two.
Joe
--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development
Add object TRUNCATE hook
All operations with acl permissions checks should have a corresponding hook
so that, for example, mandatory access control (MAC) may be enforced by an
extension. The command TRUNCATE is missing this hook, so add it. Patch by
Yuli Khodorkovskiy with some editorialization by me. Based on the discussion
not back-patched. A separate patch will exercise the hook in the sepgsql
extension.
Author: Yuli Khodorkovskiy
Reviewed-by: Joe Conway
Discussion: https://postgr.es/m/CAFL5wJcomybj1Xdw7qWmPJRpGuFukKgNrDb6uVBaCMgYS9dkaA%40mail.gmail.com
---
diff --git a/src/backend/catalog/objectaccess.c b/src/backend/catalog/objectaccess.c
index b1619b8..d51d01f 100644
*** a/src/backend/catalog/objectaccess.c
--- b/src/backend/catalog/objectaccess.c
***
*** 11,16
--- 11,17
#include "postgres.h"
#include "catalog/objectaccess.h"
+ #include "catalog/pg_class.h"
#include "catalog/pg_namespace.h"
#include "catalog/pg_proc.h"
*** RunObjectDropHook(Oid classId, Oid objec
*** 65,70
--- 66,87
}
/*
+ * RunObjectTruncateHook
+ *
+ * It is the entrypoint of OAT_TRUNCATE event
+ */
+ void
+ RunObjectTruncateHook(Oid objectId)
+ {
+ /* caller should check, but just in case... */
+ Assert(object_access_hook != NULL);
+
+ (*object_access_hook) (OAT_TRUNCATE,
+ RelationRelationId, objectId, 0,
+ NULL);
+ }
+
+ /*
* RunObjectPostAlterHook
*
* It is entrypoint of OAT_POST_ALTER event
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 45aae59..5440eb9 100644
*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
*** truncate_check_rel(Oid relid, Form_pg_cl
*** 1937,1942
--- 1937,1944
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("permission denied: \"%s\" is a system catalog",
relname)));
+
+ InvokeObjectTruncateHook(relid);
}
/*
diff --git a/src/include/catalog/objectaccess.h b/src/include/catalog/objectaccess.h
index 0170f1f..9824adf 100644
*** a/src/include/catalog/objectaccess.h
--- b/src/include/catalog/objectaccess.h
***
*** 37,42
--- 37,46
* creation or altering, because OAT_POST_CREATE or OAT_POST_ALTER are
* sufficient for extensions to track these kind of checks.
*
+ * OAT_TRUNCATE should be invoked just before truncation of objects. This
+ * event is equivalent to truncate permission on a relation under the
+ * default access control mechanism.
+ *
* Other types may be added in the future.
*/
typedef enum ObjectAccessType
*** typedef enum ObjectAccessType
*** 45,51
OAT_DROP,
OAT_POST_ALTER,
OAT_NAMESPACE_SEARCH,
! OAT_FUNCTION_EXECUTE
} ObjectAccessType;
/*
--- 49,56
OAT_DROP,
OAT_POST_ALTER,
OAT_NAMESPACE_SEARCH,
! OAT_FUNCTION_EXECUTE,
! OAT_TRUNCATE
} ObjectAccessType;
/*
*** extern void RunObjectPostCreateHook(Oid
*** 131,136
--- 136,142
bool is_internal);
extern void RunObjectDropHook(Oid classId, Oid objectId, int subId,
int dropflags);
+ extern void RunObjectTruncateHook(Oid objectId);
extern void RunObjectPostAlterHook(Oid classId, Oid objectId, int subId,
Oid auxiliaryId, bool is_internal);
extern bool RunNamespaceSearchHook(Oid objectId, bool ereport_on_violation);
*** extern void RunFunctionExecuteHook(Oid o
***
Re: add a MAC check for TRUNCATE
On 11/8/19 9:02 AM, Yuli Khodorkovskiy wrote: > On Thu, Nov 7, 2019 at 7:46 PM Michael Paquier wrote: >> >> On Mon, Sep 30, 2019 at 11:38:05AM -0300, Alvaro Herrera wrote: >> > On 2019-Sep-30, Joe Conway wrote: >> > >> > > I am not sure I will get to this today. I assume it is ok for me to move >> > > it forward e.g. next weekend, or is that not in line with commitfest >> > > rules? >> > >> > You can commit whatever patch whenever you feel like it. I will >> > probably move this patch to the next commitfest before that, but you can >> > mark it committed there as soon as you commit it. >> >> One month later, nothing has happened here. Joe, are you planning to >> look at this patch? >> >> The last patch I found does not apply properly, so please provide a >> rebase. I am switching the patch as waiting on author. > > Michael, > > I was able to apply the latest patches in the thread (9/25/19) on top > of master. I have attached them for convenience. Yes, I will look when I am able. Hopefully this weekend, almost certainly before the end of this commitfest. Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
Re: add a MAC check for TRUNCATE
On Thu, Nov 7, 2019 at 7:46 PM Michael Paquier wrote: > > On Mon, Sep 30, 2019 at 11:38:05AM -0300, Alvaro Herrera wrote: > > On 2019-Sep-30, Joe Conway wrote: > > > > > I am not sure I will get to this today. I assume it is ok for me to move > > > it forward e.g. next weekend, or is that not in line with commitfest > > > rules? > > > > You can commit whatever patch whenever you feel like it. I will > > probably move this patch to the next commitfest before that, but you can > > mark it committed there as soon as you commit it. > > One month later, nothing has happened here. Joe, are you planning to > look at this patch? > > The last patch I found does not apply properly, so please provide a > rebase. I am switching the patch as waiting on author. Michael, I was able to apply the latest patches in the thread (9/25/19) on top of master. I have attached them for convenience. ⇒ git rev-parse HEAD 879c1176157175e0a83742b810f137aebccef4a4 ⇒ md5sum Truncate-Hook.patch v3-Sepgsql-Truncate.patch 3b8c2b03e30f519f32ebb9fcbc943c70 Truncate-Hook.patch 728e90596b99cfb8eef74dc1effce46d v3-Sepgsql-Truncate.patch ⇒ git am Truncate-Hook.patch Applying: Add a hook to allow MAC check for TRUNCATE ⇒ git am v3-Sepgsql-Truncate.patch Applying: Update sepgsql to add MAC for TRUNCATE Thank you, Yuli v3-Sepgsql-Truncate.patch Description: Binary data Truncate-Hook.patch Description: Binary data
Re: add a MAC check for TRUNCATE
On Mon, Sep 30, 2019 at 11:38:05AM -0300, Alvaro Herrera wrote: > On 2019-Sep-30, Joe Conway wrote: > > > I am not sure I will get to this today. I assume it is ok for me to move > > it forward e.g. next weekend, or is that not in line with commitfest rules? > > You can commit whatever patch whenever you feel like it. I will > probably move this patch to the next commitfest before that, but you can > mark it committed there as soon as you commit it. One month later, nothing has happened here. Joe, are you planning to look at this patch? The last patch I found does not apply properly, so please provide a rebase. I am switching the patch as waiting on author. -- Michael signature.asc Description: PGP signature
Re: add a MAC check for TRUNCATE
On 2019-Sep-30, Joe Conway wrote: > I am not sure I will get to this today. I assume it is ok for me to move > it forward e.g. next weekend, or is that not in line with commitfest rules? You can commit whatever patch whenever you feel like it. I will probably move this patch to the next commitfest before that, but you can mark it committed there as soon as you commit it. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: add a MAC check for TRUNCATE
On 9/25/19 4:47 PM, Joe Conway wrote: > On 9/25/19 3:56 PM, Alvaro Herrera wrote: >> Hello >> >> On 2019-Sep-09, Yuli Khodorkovskiy wrote: >> >>> I have included an updated version of the sepgql patch. The >>> Truncate-Hook patch is unchanged from the last version. >> >> This patch no longer applies. Can you please rebase? >> >> Joe, do you plan on being committer for this patch? There seems to be >> substantial agreement on it. > > > I should be able to do that. I am not sure I will get to this today. I assume it is ok for me to move it forward e.g. next weekend, or is that not in line with commitfest rules? Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
Re: add a MAC check for TRUNCATE
On Wed, Sep 25, 2019 at 5:57 PM Tom Lane wrote: > I don't see how the addition of a new permissions check could sanely > be back-patched unless it were to default to "allow", which seems like > an odd choice. > > regards, tom lane That makes sense. Alternatively, we could back patch just the hook to at least allow the option for an integrator to implement MAC using an extension. Then the sepgsql changes could be back patched once the SELinux policy has been merged into Fedora. Thank you
Re: add a MAC check for TRUNCATE
Alvaro Herrera writes: > On 2019-Sep-25, Yuli Khodorkovskiy wrote: >> Since all existing DAC checks should have MAC, should these patches be >> considered a bug fix and therefore back patched? > I don't know the answer to that. My impression from earlier discussion > is that this was seen as a non-backpatchable change, but I defer to Joe > on that as committer. If it were up to me, the ultimate question would > be: would such a change adversely affect existing running systems? I don't see how the addition of a new permissions check could sanely be back-patched unless it were to default to "allow", which seems like an odd choice. regards, tom lane
Re: add a MAC check for TRUNCATE
On 2019-Sep-25, Yuli Khodorkovskiy wrote: > Hi Alvaro, > > I have attached the updated patches which should rebase. Great, thanks. > Since all existing DAC checks should have MAC, should these patches be > considered a bug fix and therefore back patched? I don't know the answer to that. My impression from earlier discussion is that this was seen as a non-backpatchable change, but I defer to Joe on that as committer. If it were up to me, the ultimate question would be: would such a change adversely affect existing running systems? Thanks, -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: add a MAC check for TRUNCATE
On Wed, Sep 25, 2019 at 3:57 PM Alvaro Herrera wrote: > > Hello > > On 2019-Sep-09, Yuli Khodorkovskiy wrote: > > > I have included an updated version of the sepgql patch. The > > Truncate-Hook patch is unchanged from the last version. > > This patch no longer applies. Can you please rebase? Hi Alvaro, I have attached the updated patches which should rebase. Since all existing DAC checks should have MAC, should these patches be considered a bug fix and therefore back patched? Thank you Truncate-Hook.patch Description: Binary data v3-Sepgsql-Truncate.patch Description: Binary data
Re: add a MAC check for TRUNCATE
On 9/25/19 3:56 PM, Alvaro Herrera wrote: > Hello > > On 2019-Sep-09, Yuli Khodorkovskiy wrote: > >> I have included an updated version of the sepgql patch. The >> Truncate-Hook patch is unchanged from the last version. > > This patch no longer applies. Can you please rebase? > > Joe, do you plan on being committer for this patch? There seems to be > substantial agreement on it. I should be able to do that. Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
Re: add a MAC check for TRUNCATE
Hello On 2019-Sep-09, Yuli Khodorkovskiy wrote: > I have included an updated version of the sepgql patch. The > Truncate-Hook patch is unchanged from the last version. This patch no longer applies. Can you please rebase? Joe, do you plan on being committer for this patch? There seems to be substantial agreement on it. Thanks -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: add a MAC check for TRUNCATE
Hello, I moved this patch from "Bug Fixes" to Security. Thanks, -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: add a MAC check for TRUNCATE
On Fri, Sep 6, 2019 at 9:09 PM Joe Conway wrote:
>
> On 9/6/19 8:07 PM, Tom Lane wrote:
> > Joe Conway writes:
> >> On 9/6/19 2:18 PM, Tom Lane wrote:
> >>> sepgsql hasn't worked on RHEL6 in a long time, if ever; it requires
> >>> a newer version of libselinux than what ships in RHEL6. So I'm not
> >>> concerned about that. We do need to worry about RHEL7, and whatever
> >>> is the oldest version of Fedora that is running the sepgsql tests
> >>> in the buildfarm.
> >
> >> I could be wrong, but as far as I know rhinoceros is the only buildfarm
> >> animal running sepgsql tests.
> >
> > It seems reasonable to define RHEL7 as the oldest SELinux version we
> > still care about. But it'd be a good idea for somebody to be running
> > a fairly bleeding-edge Fedora animal with sepgsql enabled, so we get
> > coverage of the other end of the scale.
>
>
> Yeah -- I was planning to eventually register a RHEL8 animal, but I
> should probably do one for Fedora as well. I'll bump the priority for
> that on my personal TODO.
>
> Joe
> --
> Crunchy Data - http://crunchydata.com
> PostgreSQL Support for Secure Enterprises
> Consulting, Training, & Open Source Development
Hello,
I have included an updated version of the sepgql patch. The
Truncate-Hook patch is unchanged from the last version.
The sepgsql changes now check if the db_table:{ truncate } permission
exists in the loaded SELinux policy before running the truncate
regression test. If the permission does not exist, then the new
regression test will not run.
Testing the TRUNCATE regression test can be done by manually adding
the permission with CIL:
```
sudo semodule -cE base
sudo sed -i -E 's/(class db_table.*?) \)/\1 truncate\)/' base.cil
sudo semodule -i base.cil
```
Thanks,
Yuli
Truncate-Hook.patch
Description: Binary data
v2-Sepgsql-Truncate.patch
Description: Binary data
Re: add a MAC check for TRUNCATE
On 9/6/19 8:07 PM, Tom Lane wrote: > Joe Conway writes: >> On 9/6/19 2:18 PM, Tom Lane wrote: >>> sepgsql hasn't worked on RHEL6 in a long time, if ever; it requires >>> a newer version of libselinux than what ships in RHEL6. So I'm not >>> concerned about that. We do need to worry about RHEL7, and whatever >>> is the oldest version of Fedora that is running the sepgsql tests >>> in the buildfarm. > >> I could be wrong, but as far as I know rhinoceros is the only buildfarm >> animal running sepgsql tests. > > It seems reasonable to define RHEL7 as the oldest SELinux version we > still care about. But it'd be a good idea for somebody to be running > a fairly bleeding-edge Fedora animal with sepgsql enabled, so we get > coverage of the other end of the scale. Yeah -- I was planning to eventually register a RHEL8 animal, but I should probably do one for Fedora as well. I'll bump the priority for that on my personal TODO. Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
Re: add a MAC check for TRUNCATE
Joe Conway writes: > On 9/6/19 2:18 PM, Tom Lane wrote: >> sepgsql hasn't worked on RHEL6 in a long time, if ever; it requires >> a newer version of libselinux than what ships in RHEL6. So I'm not >> concerned about that. We do need to worry about RHEL7, and whatever >> is the oldest version of Fedora that is running the sepgsql tests >> in the buildfarm. > I could be wrong, but as far as I know rhinoceros is the only buildfarm > animal running sepgsql tests. It seems reasonable to define RHEL7 as the oldest SELinux version we still care about. But it'd be a good idea for somebody to be running a fairly bleeding-edge Fedora animal with sepgsql enabled, so we get coverage of the other end of the scale. regards, tom lane
Re: add a MAC check for TRUNCATE
On Fri, Sep 6, 2019 at 4:31 PM Joe Conway wrote: > > On 9/6/19 2:13 PM, Yuli Khodorkovskiy wrote: > > As Joe Conway pointed out to me out of band, the build animal for RHEL > > 7 has handle_unknown set to `0`. Are there any other concerns with > > this approach? > > > You mean deny_unknown I believe. I do, thanks. Not sure where I pulled handle_unknown from. > > "Allow unknown object class / permissions. This will set the returned AV > with all 1's." > > As I understand it, this would make the sepgsql behavior unchanged from > before if the policy does not support the new permission. > > Joe > > > On Fri, Sep 6, 2019 at 1:00 PM Yuli Khodorkovskiy wrote: > >> The default SELinux policy on Fedora ships with deny_unknown set to 0. > >> Deny_unknown was added to the kernel in 2.6.24, so unless someone is > >> using RHEL 5.x, which is in ELS, they will have the ability to > >> override the default behavior on CentOS/RHEL. > > > > -- > Crunchy Data - http://crunchydata.com > PostgreSQL Support for Secure Enterprises > Consulting, Training, & Open Source Development
Re: add a MAC check for TRUNCATE
On 9/6/19 2:13 PM, Yuli Khodorkovskiy wrote: > As Joe Conway pointed out to me out of band, the build animal for RHEL > 7 has handle_unknown set to `0`. Are there any other concerns with > this approach? You mean deny_unknown I believe. "Allow unknown object class / permissions. This will set the returned AV with all 1's." As I understand it, this would make the sepgsql behavior unchanged from before if the policy does not support the new permission. Joe > On Fri, Sep 6, 2019 at 1:00 PM Yuli Khodorkovskiy wrote: >> The default SELinux policy on Fedora ships with deny_unknown set to 0. >> Deny_unknown was added to the kernel in 2.6.24, so unless someone is >> using RHEL 5.x, which is in ELS, they will have the ability to >> override the default behavior on CentOS/RHEL. -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
Re: add a MAC check for TRUNCATE
On 9/6/19 2:18 PM, Tom Lane wrote: > Yuli Khodorkovskiy writes: >> On Fri, Sep 6, 2019 at 11:57 AM Tom Lane wrote: >>> Well, the larger question, independent of the regression tests, is >>> will the new policy work at all on older SELinux? If not, that >>> doesn't seem very acceptable. > >> The default SELinux policy on Fedora ships with deny_unknown set to 0. >> Deny_unknown was added to the kernel in 2.6.24, so unless someone is >> using RHEL 5.x, which is in ELS, they will have the ability to >> override the default behavior on CentOS/RHEL. > > OK, that sounds like it will work. > >> On RHEL 6, which goes into ELS in 2020, it's a bit more complicated >> and requires rebuilding the base SELinux module from source. > > sepgsql hasn't worked on RHEL6 in a long time, if ever; it requires > a newer version of libselinux than what ships in RHEL6. So I'm not > concerned about that. We do need to worry about RHEL7, and whatever > is the oldest version of Fedora that is running the sepgsql tests > in the buildfarm. I could be wrong, but as far as I know rhinoceros is the only buildfarm animal running sepgsql tests. Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
Re: add a MAC check for TRUNCATE
Yuli Khodorkovskiy writes: > On Fri, Sep 6, 2019 at 11:57 AM Tom Lane wrote: >> Well, the larger question, independent of the regression tests, is >> will the new policy work at all on older SELinux? If not, that >> doesn't seem very acceptable. > The default SELinux policy on Fedora ships with deny_unknown set to 0. > Deny_unknown was added to the kernel in 2.6.24, so unless someone is > using RHEL 5.x, which is in ELS, they will have the ability to > override the default behavior on CentOS/RHEL. OK, that sounds like it will work. > On RHEL 6, which goes into ELS in 2020, it's a bit more complicated > and requires rebuilding the base SELinux module from source. sepgsql hasn't worked on RHEL6 in a long time, if ever; it requires a newer version of libselinux than what ships in RHEL6. So I'm not concerned about that. We do need to worry about RHEL7, and whatever is the oldest version of Fedora that is running the sepgsql tests in the buildfarm. regards, tom lane
Re: add a MAC check for TRUNCATE
As Joe Conway pointed out to me out of band, the build animal for RHEL 7 has handle_unknown set to `0`. Are there any other concerns with this approach? On Fri, Sep 6, 2019 at 1:00 PM Yuli Khodorkovskiy wrote: > > On Fri, Sep 6, 2019 at 11:57 AM Tom Lane wrote: > > > > Stephen Frost writes: > > > * Tom Lane (t...@sss.pgh.pa.us) wrote: > > >> Yuli Khodorkovskiy writes: > > >>> 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. > > > > >> That's going to be a problem. I do not think it will be acceptable > > >> to commit tests that fail on less-than-bleeding-edge SELinux. > > > > > This is why I was suggesting up-thread that it'd be neat if we made this > > > somehow optional, though I don't quite see a way to do that sensibly. > > > We could though, of course, make running the regression test optional > > > and then have a buildfarm member that's got the bleeding-edge SELinux > > > (or is just configured with the additional control) and then have it > > > enabled there. > > > > Well, the larger question, independent of the regression tests, is > > will the new policy work at all on older SELinux? If not, that > > doesn't seem very acceptable. Worse, it implies we're going to > > have another flag day anytime we want to add any new element > > to sepgsql's view of the universe. I think we need some hard > > thought about upgrade paths here --- at least, if we want to > > believe that sepgsql is anything but a toy for demonstration > > purposes. > > > > regards, tom lane > > The default SELinux policy on Fedora ships with deny_unknown set to 0. > Deny_unknown was added to the kernel in 2.6.24, so unless someone is > using RHEL 5.x, which is in ELS, they will have the ability to > override the default behavior on CentOS/RHEL. > > CIL was added to RHEL starting with RHEL 7. As stated before, an > integrator can export the base module and override the deny_unknown > behavior. > > On RHEL 6, which goes into ELS in 2020, it's a bit more complicated > and requires rebuilding the base SELinux module from source. > > Hope this helps, > > Yuli
Re: add a MAC check for TRUNCATE
On Fri, Sep 6, 2019 at 11:57 AM Tom Lane wrote: > > Stephen Frost writes: > > * Tom Lane (t...@sss.pgh.pa.us) wrote: > >> Yuli Khodorkovskiy writes: > >>> 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. > > >> That's going to be a problem. I do not think it will be acceptable > >> to commit tests that fail on less-than-bleeding-edge SELinux. > > > This is why I was suggesting up-thread that it'd be neat if we made this > > somehow optional, though I don't quite see a way to do that sensibly. > > We could though, of course, make running the regression test optional > > and then have a buildfarm member that's got the bleeding-edge SELinux > > (or is just configured with the additional control) and then have it > > enabled there. > > Well, the larger question, independent of the regression tests, is > will the new policy work at all on older SELinux? If not, that > doesn't seem very acceptable. Worse, it implies we're going to > have another flag day anytime we want to add any new element > to sepgsql's view of the universe. I think we need some hard > thought about upgrade paths here --- at least, if we want to > believe that sepgsql is anything but a toy for demonstration > purposes. > > regards, tom lane The default SELinux policy on Fedora ships with deny_unknown set to 0. Deny_unknown was added to the kernel in 2.6.24, so unless someone is using RHEL 5.x, which is in ELS, they will have the ability to override the default behavior on CentOS/RHEL. CIL was added to RHEL starting with RHEL 7. As stated before, an integrator can export the base module and override the deny_unknown behavior. On RHEL 6, which goes into ELS in 2020, it's a bit more complicated and requires rebuilding the base SELinux module from source. Hope this helps, Yuli
Re: add a MAC check for TRUNCATE
Stephen Frost writes: > * Tom Lane (t...@sss.pgh.pa.us) wrote: >> Yuli Khodorkovskiy writes: >>> 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. >> That's going to be a problem. I do not think it will be acceptable >> to commit tests that fail on less-than-bleeding-edge SELinux. > This is why I was suggesting up-thread that it'd be neat if we made this > somehow optional, though I don't quite see a way to do that sensibly. > We could though, of course, make running the regression test optional > and then have a buildfarm member that's got the bleeding-edge SELinux > (or is just configured with the additional control) and then have it > enabled there. Well, the larger question, independent of the regression tests, is will the new policy work at all on older SELinux? If not, that doesn't seem very acceptable. Worse, it implies we're going to have another flag day anytime we want to add any new element to sepgsql's view of the universe. I think we need some hard thought about upgrade paths here --- at least, if we want to believe that sepgsql is anything but a toy for demonstration purposes. regards, tom lane
Re: add a MAC check for TRUNCATE
On Fri, Sep 6, 2019 at 11:47 AM Tom Lane wrote:
>
> Yuli Khodorkovskiy writes:
> > 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.
>
> That's going to be a problem. I do not think it will be acceptable
> to commit tests that fail on less-than-bleeding-edge SELinux.
>
> regards, tom lane
The tests pass as long as deny_unknown is set to 0, which is the
default on fedora 30.
Re: add a MAC check for TRUNCATE
Greetings,
* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Yuli Khodorkovskiy writes:
> > 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.
>
> That's going to be a problem. I do not think it will be acceptable
> to commit tests that fail on less-than-bleeding-edge SELinux.
This is why I was suggesting up-thread that it'd be neat if we made this
somehow optional, though I don't quite see a way to do that sensibly.
We could though, of course, make running the regression test optional
and then have a buildfarm member that's got the bleeding-edge SELinux
(or is just configured with the additional control) and then have it
enabled there.
Thanks,
Stephen
signature.asc
Description: PGP signature
Re: add a MAC check for TRUNCATE
Yuli Khodorkovskiy writes:
> 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.
That's going to be a problem. I do not think it will be acceptable
to commit tests that fail on less-than-bleeding-edge SELinux.
regards, tom lane
Re: add a MAC check for TRUNCATE
On Fri, Sep 6, 2019 at 11:26 AM Yuli Khodorkovskiy
wrote:
>
> On Fri, Sep 6, 2019 at 10:40 AM Stephen Frost wrote:
> >
> > Greetings,
>
> Hello Stephen,
>
> >
> > * Yuli Khodorkovskiy (yuli.khodorkovs...@crunchydata.com) wrote:
> > > On Tue, Sep 3, 2019 at 3:25 PM Stephen Frost wrote:
> > > > * Kohei KaiGai (kai...@heterodb.com) wrote:
> > > > > 2019年7月25日(木) 3:52 Yuli Khodorkovskiy
> > > > > :
> > > > > > 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
Re: add a MAC check for TRUNCATE
On 9/6/19 11:26 AM, Yuli Khodorkovskiy wrote: > On Fri, Sep 6, 2019 at 10:40 AM Stephen Frost wrote: >> 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. +1 - I don't think there is any question about it. Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development signature.asc Description: OpenPGP digital signature
Re: add a MAC check for TRUNCATE
On Fri, Sep 6, 2019 at 10:40 AM Stephen Frost wrote:
>
> Greetings,
Hello Stephen,
>
> * Yuli Khodorkovskiy (yuli.khodorkovs...@crunchydata.com) wrote:
> > On Tue, Sep 3, 2019 at 3:25 PM Stephen Frost wrote:
> > > * Kohei KaiGai (kai...@heterodb.com) wrote:
> > > > 2019年7月25日(木) 3:52 Yuli Khodorkovskiy
> > > > :
> > > > > 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.
>
> > 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
Re: add a MAC check for TRUNCATE
On Fri, Sep 6, 2019 at 10:40 AM Stephen Frost wrote: > 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. +1. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: add a MAC check for TRUNCATE
Greetings,
* Yuli Khodorkovskiy (yuli.khodorkovs...@crunchydata.com) wrote:
> On Tue, Sep 3, 2019 at 3:25 PM Stephen Frost wrote:
> > * Kohei KaiGai (kai...@heterodb.com) wrote:
> > > 2019年7月25日(木) 3:52 Yuli Khodorkovskiy
> > > :
> > > > 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}.
> 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.
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
signature.asc
Description: PGP signature
Re: add a MAC check for TRUNCATE
On Tue, Sep 3, 2019 at 3:25 PM Stephen Frost wrote:
>
> Greetings,
>
> * Kohei KaiGai (kai...@heterodb.com) wrote:
> > 2019年7月25日(木) 3:52 Yuli Khodorkovskiy :
> > > 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 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.
Thank you.
>
> Thanks,
>
> Stephen
Truncate-Hook.patch
Description: Binary data
Sepgsql-Truncate.patch
Description: Binary data
Re: add a MAC check for TRUNCATE
On Mon, Sep 2, 2019 at 10:58 AM Kohei KaiGai wrote:
>
> Hello Yuli,
Hello KaiGai,
>
> 2019年7月25日(木) 3:52 Yuli Khodorkovskiy :
> > 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?
To echo Stephen's reply, since TRUNCATE has a dedicated privilege in
the GRANT system, there should be a MAC based permission as well.
Increased granularity for an integrator to add least privileged policy
is a good idea in my view.
> 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?
Now that I think about it, using "db_table { delete }" would be fine,
and that would remove the CIL requirement that I stated earlier. Thank
you for the suggestion. I'll send a v2 patch using the delete
permission.
Thank you,
Yuli
>
> Best regards,
> --
> HeteroDB, Inc / The PG-Strom Project
> KaiGai Kohei
Re: add a MAC check for TRUNCATE
Greetings,
* Kohei KaiGai (kai...@heterodb.com) wrote:
> 2019年7月25日(木) 3:52 Yuli Khodorkovskiy :
> > 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.
Thanks,
Stephen
signature.asc
Description: PGP signature
Re: add a MAC check for TRUNCATE
Hello Yuli,
2019年7月25日(木) 3:52 Yuli Khodorkovskiy :
> 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?
Best regards,
--
HeteroDB, Inc / The PG-Strom Project
KaiGai Kohei
add a MAC check for TRUNCATE
Hackers,
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. This patch is the first step to add this permission to the
upstream SELinux policy. If this permission does not exist in the
policy, sepgsql is being used, and `deny_unknown` is set to 1, the
TRUNCATE will be denied.
As a workaround for this behavior, the SELinux aware system would need
to have `/sys/fs/selinux/deny_unknown` set to 0 until the permission has
been added to refpolicy/Redhat SELinux policy.
The deny_unknown behavior can be set using CIL [2] by extracting the
base SELinux module, and setting how the kernel handles unknown
permissions. The dependencies for overriding handle_unknown are
policycoreutils, selinux-policy-targeted, and a libsemanage version that
supports CIL (CentOS 7+).
$ sudo semodule -cE base
$ sed -Ei 's/(handleunknown )deny/\1allow/g' base.cil
$ sudo semodule -i base.cil
Thanks,
Yuli
[1]
https://github.com/SELinuxProject/refpolicy/blob/master/policy/flask/access_vectors#L794
[2]
https://github.com/SELinuxProject/selinux/blob/master/secilc/docs/cil_policy_config_statements.md#handleunknown
0001-Use-MAC-in-addition-to-DAC-for-TRUNCATE.patch
0001-Use-MAC-in-addition-to-DAC-for-TRUNCATE.patch
Description: Binary data
