[userspace PATCH v2 0/2] Add support for loginuid_set

[userspace PATCH v2 0/2] Add support for loginuid_set

[Richard Guy Briggs]

loginuid_set support should have been added to userspace when it was
added to the kernel around v3.10.  Add it before we do similar for
sessionID and sessionID_set.

There will be a number of users of features_bitmap within the same
function (exclude filter extension, sessionID filter), so refactor
audit_rule_fieldpair_data() to put audit_get_features earlier in the
function.

Richard Guy Briggs (2):
  get feature list only once
  Add user filter option loginuid_set from uapi macro
    AUDIT_LOGINUID_SET

 trunk/lib/errormsg.h |    2 ++
 trunk/lib/fieldtab.h |    2 ++
 trunk/lib/libaudit.c |   17 ++++++++++++++++-
 trunk/lib/libaudit.h |    6 ++++++
 4 files changed, 26 insertions(+), 1 deletions(-)

[userspace PATCH v2 1/2] get feature list only once

[Richard Guy Briggs]

Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
 trunk/lib/libaudit.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/trunk/lib/libaudit.c b/trunk/lib/libaudit.c
index 70b8ea8..566b89e 100644
--- a/trunk/lib/libaudit.c
+++ b/trunk/lib/libaudit.c
@@ -1345,6 +1345,7 @@ int audit_rule_fieldpair_data(struct audit_rule_data **rulep, const char *pair,
 	int        vlen;
 	int        offset;
 	struct audit_rule_data *rule = *rulep;
+	uint32_t features = audit_get_features();
 
 	if (f == NULL)
 		return -1;
@@ -1508,7 +1509,6 @@ int audit_rule_fieldpair_data(struct audit_rule_data **rulep, const char *pair,
 		case AUDIT_FILTERKEY:
 		case AUDIT_EXE:
 			if (field == AUDIT_EXE) {
-				uint32_t features = audit_get_features();
 				if ((features & AUDIT_FEATURE_BITMAP_EXECUTABLE_PATH) == 0)
 					return -30;
 				if (op != AUDIT_EQUAL)
-- 
1.7.1

[userspace PATCH v2 2/2] Add user filter option loginuid_set from uapi macro AUDIT_LOGINUID_SET

[Richard Guy Briggs]

Add macro if not in headers, check for version or feature bitmap.
Check for user or exit list use, check for boolean.

See upstream kernel commits:
	780a7654cee8d61819512385e778e4827db4bfbc
	041d7b98ffe59c59fdd639931dea7d74f9aa9a59

Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
 trunk/lib/errormsg.h |    2 ++
 trunk/lib/fieldtab.h |    2 ++
 trunk/lib/libaudit.c |   15 +++++++++++++++
 trunk/lib/libaudit.h |    6 ++++++
 4 files changed, 25 insertions(+), 0 deletions(-)

diff --git a/trunk/lib/errormsg.h b/trunk/lib/errormsg.h
index a4602d5..c678315 100644
--- a/trunk/lib/errormsg.h
+++ b/trunk/lib/errormsg.h
@@ -66,5 +66,7 @@ static const struct msg_tab err_msgtab[] = {
     { -28,    2,    "Too many fields in rule:" },
     { -29,    1,    "only takes = operator" },
     { -30,    2,    "Field option not supported by kernel:" },
+    { -31,    1,    "can only be used with exit and user filter lists" },
+    { -32,    2,    "-F value should be boolean 0 or 1 for" },
 };
 #endif
diff --git a/trunk/lib/fieldtab.h b/trunk/lib/fieldtab.h
index bf48c95..107157d 100644
--- a/trunk/lib/fieldtab.h
+++ b/trunk/lib/fieldtab.h
@@ -31,6 +31,8 @@ _S(AUDIT_SGID,         "sgid"         )
 _S(AUDIT_FSGID,        "fsgid"        )
 _S(AUDIT_LOGINUID,     "auid"         )
 _S(AUDIT_LOGINUID,     "loginuid"     )
+_S(AUDIT_LOGINUID_SET, "auid_set"     )
+_S(AUDIT_LOGINUID_SET, "loginuid_set" )
 _S(AUDIT_PERS,         "pers"         )
 _S(AUDIT_ARCH,         "arch"         )
 _S(AUDIT_MSGTYPE,      "msgtype"      )
diff --git a/trunk/lib/libaudit.c b/trunk/lib/libaudit.c
index 566b89e..236f8bc 100644
--- a/trunk/lib/libaudit.c
+++ b/trunk/lib/libaudit.c
@@ -1627,6 +1627,21 @@ int audit_rule_fieldpair_data(struct audit_rule_data **rulep, const char *pair,
 			else 
 				return -21;
 			break;
+		case AUDIT_LOGINUID_SET:
+			if(!features)
+				return -30;
+			if (flags != AUDIT_FILTER_USER &&
+			    flags != AUDIT_FILTER_EXIT)
+				return -31;
+			if (isdigit((char)*(v))) {
+				rule->values[rule->field_count] =
+					strtol(v, NULL, 0);
+				if (rule->values[rule->field_count] > 1)
+					return -32;
+			}
+			else
+				return -32;
+			break;
 		case AUDIT_DEVMAJOR...AUDIT_INODE:
 		case AUDIT_SUCCESS:
 			if (flags != AUDIT_FILTER_EXIT)
diff --git a/trunk/lib/libaudit.h b/trunk/lib/libaudit.h
index 9640f17..0852bcc 100644
--- a/trunk/lib/libaudit.h
+++ b/trunk/lib/libaudit.h
@@ -369,6 +369,12 @@ extern "C" {
 #define AUDIT_COMPARE_SGID_TO_FSGID    25
 #endif
 
+/* Rule fields */
+#ifndef AUDIT_LOGINUID_SET
+#define AUDIT_LOGINUID_SET		24
+#endif
+
+/* Architectures */
 #ifndef EM_ARM
 #define EM_ARM  40
 #endif
-- 
1.7.1

Re: [userspace PATCH v2 0/2] Add support for loginuid_set

[Steve Grubb]

On Thursday, August 18, 2016 2:18:55 PM EDT Richard Guy Briggs wrote:
> loginuid_set support should have been added to userspace when it was
> added to the kernel around v3.10.  Add it before we do similar for
> sessionID and sessionID_set.

If this were accepted, how would this change writing rules? IOW, can you give 
an example rule so we can see what this looks like?

Thanks,
-Steve


> There will be a number of users of features_bitmap within the same
> function (exclude filter extension, sessionID filter), so refactor
> audit_rule_fieldpair_data() to put audit_get_features earlier in the
> function.
> 
> Richard Guy Briggs (2):
>   get feature list only once
>   Add user filter option loginuid_set from uapi macro
>     AUDIT_LOGINUID_SET
> 
>  trunk/lib/errormsg.h |    2 ++
>  trunk/lib/fieldtab.h |    2 ++
>  trunk/lib/libaudit.c |   17 ++++++++++++++++-
>  trunk/lib/libaudit.h |    6 ++++++
>  4 files changed, 26 insertions(+), 1 deletions(-)

Re: [userspace PATCH v2 0/2] Add support for loginuid_set

[Paul Moore]

On Mon, Oct 10, 2016 at 1:24 PM, Steve Grubb <sgrubb@redhat.com> wrote:
> On Thursday, August 18, 2016 2:18:55 PM EDT Richard Guy Briggs wrote:
>> loginuid_set support should have been added to userspace when it was
>> added to the kernel around v3.10.  Add it before we do similar for
>> sessionID and sessionID_set.
>
> If this were accepted, how would this change writing rules? IOW, can you give
> an example rule so we can see what this looks like?

We have a RFE feature page which documents some rule examples:

* https://github.com/linux-audit/audit-kernel/wiki/RFE-Session-ID-User-Filter

-- 
paul moore
security @ redhat

Re: [userspace PATCH v2 0/2] Add support for loginuid_set

[Steve Grubb]

On Monday, October 10, 2016 5:10:39 PM EDT Paul Moore wrote:
> On Mon, Oct 10, 2016 at 1:24 PM, Steve Grubb <sgrubb@redhat.com> wrote:
> > On Thursday, August 18, 2016 2:18:55 PM EDT Richard Guy Briggs wrote:
> >> loginuid_set support should have been added to userspace when it was
> >> added to the kernel around v3.10.  Add it before we do similar for
> >> sessionID and sessionID_set.
> > 
> > If this were accepted, how would this change writing rules? IOW, can you
> > give an example rule so we can see what this looks like?
> 
> We have a RFE feature page which documents some rule examples:
> 
> *
> https://github.com/linux-audit/audit-kernel/wiki/RFE-Session-ID-User-Filter

OK, thanks. This is helpful. So, what is the difference between these rules?

-a always,exit -F path=/tmp/sessionid_test -F loginuid=-1

-a always,exit -F path=/tmp/sessionid_set_test -F loginuid_set=0

-Steve

Re: [userspace PATCH v2 0/2] Add support for loginuid_set

[Richard Guy Briggs]

On 2016-10-11 12:40, Steve Grubb wrote:
> On Monday, October 10, 2016 5:10:39 PM EDT Paul Moore wrote:
> > On Mon, Oct 10, 2016 at 1:24 PM, Steve Grubb <sgrubb@redhat.com> wrote:
> > > On Thursday, August 18, 2016 2:18:55 PM EDT Richard Guy Briggs wrote:
> > >> loginuid_set support should have been added to userspace when it was
> > >> added to the kernel around v3.10.  Add it before we do similar for
> > >> sessionID and sessionID_set.
> > > 
> > > If this were accepted, how would this change writing rules? IOW, can you
> > > give an example rule so we can see what this looks like?
> > 
> > We have a RFE feature page which documents some rule examples:
> > 
> > *
> > https://github.com/linux-audit/audit-kernel/wiki/RFE-Session-ID-User-Filter
> 
> OK, thanks. This is helpful. So, what is the difference between these rules?
> 
> -a always,exit -F path=/tmp/sessionid_test -F loginuid=-1
> 
> -a always,exit -F path=/tmp/sessionid_set_test -F loginuid_set=0

The only difference is one flag in the kernel to indicate how it was
invoked to be able to report when queried exactly the same way it was
invoked, but there is no difference in the actual behaviour of the
filter.  This was added because of your report that "f24=0" was reported
instead of loginuid_set=0 for backwards compatibility.

Going forward, the implementation of the sessionid_set field (which
works similarly) will not allow an unset value of sessionid since these
are a new addition that didn't need to accomodate backward
compatibility.

> -Steve

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Kernel Security Engineering, Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635

Re: [userspace PATCH v2 0/2] Add support for loginuid_set

[Steve Grubb]

On Tuesday, October 11, 2016 2:27:54 PM EDT Richard Guy Briggs wrote:
> On 2016-10-11 12:40, Steve Grubb wrote:
> > On Monday, October 10, 2016 5:10:39 PM EDT Paul Moore wrote:
> > > On Mon, Oct 10, 2016 at 1:24 PM, Steve Grubb <sgrubb@redhat.com> wrote:
> > > > On Thursday, August 18, 2016 2:18:55 PM EDT Richard Guy Briggs wrote:
> > > >> loginuid_set support should have been added to userspace when it was
> > > >> added to the kernel around v3.10.  Add it before we do similar for
> > > >> sessionID and sessionID_set.
> > > > 
> > > > If this were accepted, how would this change writing rules? IOW, can
> > > > you
> > > > give an example rule so we can see what this looks like?
> > > 
> > > We have a RFE feature page which documents some rule examples:
> > > 
> > > *
> > > https://github.com/linux-audit/audit-kernel/wiki/RFE-Session-ID-User-Fil
> > > ter
> > 
> > OK, thanks. This is helpful. So, what is the difference between these
> > rules?
> > 
> > -a always,exit -F path=/tmp/sessionid_test -F loginuid=-1
> > 
> > -a always,exit -F path=/tmp/sessionid_set_test -F loginuid_set=0
> 
> The only difference is one flag in the kernel to indicate how it was
> invoked to be able to report when queried exactly the same way it was
> invoked, but there is no difference in the actual behaviour of the
> filter.  This was added because of your report that "f24=0" was reported
> instead of loginuid_set=0 for backwards compatibility.

OK. Generally its bad to have 2 ways to do the same thing. People use SCAP 
content to check system configurations. If there's two ways to do the same 
thing, then someone can accidentally choose the wrong way and fail their scan. 
We run into this in the past where we allowed -a exit,always and -a 
always,exit. All the rules had to be reworked to be consistent. Therefore, I 
would recommend not using the loginuid_set option. We still get questions 
about -w /path/file -p wa  vs -a always,exit -F path=/path/file -F perm=wa. But 
that one is so deeply embedded that it should not be fixed.

> Going forward, the implementation of the sessionid_set field (which
> works similarly) will not allow an unset value of sessionid since these
> are a new addition that didn't need to accomodate backward
> compatibility.

As long as we can trigger on sessionid=-1, then we are fine.

-Steve

Re: [userspace PATCH v2 0/2] Add support for loginuid_set

[Paul Moore]

On Tue, Oct 11, 2016 at 3:22 PM, Steve Grubb <sgrubb@redhat.com> wrote:
> On Tuesday, October 11, 2016 2:27:54 PM EDT Richard Guy Briggs wrote:
>> On 2016-10-11 12:40, Steve Grubb wrote:
>> > On Monday, October 10, 2016 5:10:39 PM EDT Paul Moore wrote:
>> > > On Mon, Oct 10, 2016 at 1:24 PM, Steve Grubb <sgrubb@redhat.com> wrote:
>> > > > On Thursday, August 18, 2016 2:18:55 PM EDT Richard Guy Briggs wrote:
>> > > >> loginuid_set support should have been added to userspace when it was
>> > > >> added to the kernel around v3.10.  Add it before we do similar for
>> > > >> sessionID and sessionID_set.
>> > > >
>> > > > If this were accepted, how would this change writing rules? IOW, can
>> > > > you
>> > > > give an example rule so we can see what this looks like?
>> > >
>> > > We have a RFE feature page which documents some rule examples:
>> > >
>> > > *
>> > > https://github.com/linux-audit/audit-kernel/wiki/RFE-Session-ID-User-Fil
>> > > ter
>> >
>> > OK, thanks. This is helpful. So, what is the difference between these
>> > rules?
>> >
>> > -a always,exit -F path=/tmp/sessionid_test -F loginuid=-1
>> >
>> > -a always,exit -F path=/tmp/sessionid_set_test -F loginuid_set=0
>>
>> The only difference is one flag in the kernel to indicate how it was
>> invoked to be able to report when queried exactly the same way it was
>> invoked, but there is no difference in the actual behaviour of the
>> filter.  This was added because of your report that "f24=0" was reported
>> instead of loginuid_set=0 for backwards compatibility.
>
> OK. Generally its bad to have 2 ways to do the same thing. People use SCAP
> content to check system configurations. If there's two ways to do the same
> thing, then someone can accidentally choose the wrong way and fail their scan.
> We run into this in the past where we allowed -a exit,always and -a
> always,exit. All the rules had to be reworked to be consistent. Therefore, I
> would recommend not using the loginuid_set option. We still get questions
> about -w /path/file -p wa  vs -a always,exit -F path=/path/file -F perm=wa. But
> that one is so deeply embedded that it should not be fixed.
>
>> Going forward, the implementation of the sessionid_set field (which
>> works similarly) will not allow an unset value of sessionid since these
>> are a new addition that didn't need to accomodate backward
>> compatibility.
>
> As long as we can trigger on sessionid=-1, then we are fine.

Wait a minute ... what happened to the loginuid_set patches?  Didn't
those get merged to userspace?

-- 
paul moore
security @ redhat

Re: [userspace PATCH v2 0/2] Add support for loginuid_set

[Steve Grubb]

On Tuesday, October 11, 2016 4:42:58 PM EDT Paul Moore wrote:
> On Tue, Oct 11, 2016 at 3:22 PM, Steve Grubb <sgrubb@redhat.com> wrote:
> > On Tuesday, October 11, 2016 2:27:54 PM EDT Richard Guy Briggs wrote:
> >> On 2016-10-11 12:40, Steve Grubb wrote:
> >> > On Monday, October 10, 2016 5:10:39 PM EDT Paul Moore wrote:
> >> > > On Mon, Oct 10, 2016 at 1:24 PM, Steve Grubb <sgrubb@redhat.com> 
wrote:
> >> > > > On Thursday, August 18, 2016 2:18:55 PM EDT Richard Guy Briggs 
wrote:
> >> > > >> loginuid_set support should have been added to userspace when it
> >> > > >> was
> >> > > >> added to the kernel around v3.10.  Add it before we do similar for
> >> > > >> sessionID and sessionID_set.
> >> > > > 
> >> > > > If this were accepted, how would this change writing rules? IOW,
> >> > > > can
> >> > > > you
> >> > > > give an example rule so we can see what this looks like?
> >> > > 
> >> > > We have a RFE feature page which documents some rule examples:
> >> > > 
> >> > > *
> >> > > https://github.com/linux-audit/audit-kernel/wiki/RFE-Session-ID-User-> >> > > Fil
> >> > > ter
> >> > 
> >> > OK, thanks. This is helpful. So, what is the difference between these
> >> > rules?
> >> > 
> >> > -a always,exit -F path=/tmp/sessionid_test -F loginuid=-1
> >> > 
> >> > -a always,exit -F path=/tmp/sessionid_set_test -F loginuid_set=0
> >> 
> >> The only difference is one flag in the kernel to indicate how it was
> >> invoked to be able to report when queried exactly the same way it was
> >> invoked, but there is no difference in the actual behaviour of the
> >> filter.  This was added because of your report that "f24=0" was reported
> >> instead of loginuid_set=0 for backwards compatibility.
> > 
> > OK. Generally its bad to have 2 ways to do the same thing. People use SCAP
> > content to check system configurations. If there's two ways to do the same
> > thing, then someone can accidentally choose the wrong way and fail their
> > scan. We run into this in the past where we allowed -a exit,always and -a
> > always,exit. All the rules had to be reworked to be consistent.
> > Therefore, I would recommend not using the loginuid_set option. We still
> > get questions about -w /path/file -p wa  vs -a always,exit -F
> > path=/path/file -F perm=wa. But that one is so deeply embedded that it
> > should not be fixed.
> > 
> >> Going forward, the implementation of the sessionid_set field (which
> >> works similarly) will not allow an unset value of sessionid since these
> >> are a new addition that didn't need to accomodate backward
> >> compatibility.
> > 
> > As long as we can trigger on sessionid=-1, then we are fine.
> 
> Wait a minute ... what happened to the loginuid_set patches?  Didn't
> those get merged to userspace?

I'm reviewing this patch set for merging now that we are past all the 2.6 bug 
fixing.

-Steve

Re: [userspace PATCH v2 0/2] Add support for loginuid_set

[Paul Moore]

On Tue, Oct 11, 2016 at 4:50 PM, Steve Grubb <sgrubb@redhat.com> wrote:
> On Tuesday, October 11, 2016 4:42:58 PM EDT Paul Moore wrote:
>> On Tue, Oct 11, 2016 at 3:22 PM, Steve Grubb <sgrubb@redhat.com> wrote:
>> > On Tuesday, October 11, 2016 2:27:54 PM EDT Richard Guy Briggs wrote:
>> >> On 2016-10-11 12:40, Steve Grubb wrote:
>> >> > On Monday, October 10, 2016 5:10:39 PM EDT Paul Moore wrote:
>> >> > > On Mon, Oct 10, 2016 at 1:24 PM, Steve Grubb <sgrubb@redhat.com>
> wrote:
>> >> > > > On Thursday, August 18, 2016 2:18:55 PM EDT Richard Guy Briggs
> wrote:
>> >> > > >> loginuid_set support should have been added to userspace when it
>> >> > > >> was
>> >> > > >> added to the kernel around v3.10.  Add it before we do similar for
>> >> > > >> sessionID and sessionID_set.
>> >> > > >
>> >> > > > If this were accepted, how would this change writing rules? IOW,
>> >> > > > can
>> >> > > > you
>> >> > > > give an example rule so we can see what this looks like?
>> >> > >
>> >> > > We have a RFE feature page which documents some rule examples:
>> >> > >
>> >> > > *
>> >> > > https://github.com/linux-audit/audit-kernel/wiki/RFE-Session-ID-User-> >> > > Fil
>> >> > > ter
>> >> >
>> >> > OK, thanks. This is helpful. So, what is the difference between these
>> >> > rules?
>> >> >
>> >> > -a always,exit -F path=/tmp/sessionid_test -F loginuid=-1
>> >> >
>> >> > -a always,exit -F path=/tmp/sessionid_set_test -F loginuid_set=0
>> >>
>> >> The only difference is one flag in the kernel to indicate how it was
>> >> invoked to be able to report when queried exactly the same way it was
>> >> invoked, but there is no difference in the actual behaviour of the
>> >> filter.  This was added because of your report that "f24=0" was reported
>> >> instead of loginuid_set=0 for backwards compatibility.
>> >
>> > OK. Generally its bad to have 2 ways to do the same thing. People use SCAP
>> > content to check system configurations. If there's two ways to do the same
>> > thing, then someone can accidentally choose the wrong way and fail their
>> > scan. We run into this in the past where we allowed -a exit,always and -a
>> > always,exit. All the rules had to be reworked to be consistent.
>> > Therefore, I would recommend not using the loginuid_set option. We still
>> > get questions about -w /path/file -p wa  vs -a always,exit -F
>> > path=/path/file -F perm=wa. But that one is so deeply embedded that it
>> > should not be fixed.
>> >
>> >> Going forward, the implementation of the sessionid_set field (which
>> >> works similarly) will not allow an unset value of sessionid since these
>> >> are a new addition that didn't need to accomodate backward
>> >> compatibility.
>> >
>> > As long as we can trigger on sessionid=-1, then we are fine.
>>
>> Wait a minute ... what happened to the loginuid_set patches?  Didn't
>> those get merged to userspace?
>
> I'm reviewing this patch set for merging now that we are past all the 2.6 bug
> fixing.

Ah, nevermind ... I confused loginuid and sessionid, sorry about the confusion.

Anyway, I thought the desire for having a dedicated "is the loginuid
value set?" filter came from userspace?  If not, where did this
requirement come from?

-- 
paul moore
security @ redhat

Re: [userspace PATCH v2 0/2] Add support for loginuid_set

[Steve Grubb]

On Tuesday, October 11, 2016 4:54:26 PM EDT Paul Moore wrote:
> On Tue, Oct 11, 2016 at 4:50 PM, Steve Grubb <sgrubb@redhat.com> wrote:
> > On Tuesday, October 11, 2016 4:42:58 PM EDT Paul Moore wrote:
> >> On Tue, Oct 11, 2016 at 3:22 PM, Steve Grubb <sgrubb@redhat.com> wrote:
> >> > On Tuesday, October 11, 2016 2:27:54 PM EDT Richard Guy Briggs wrote:
> >> >> On 2016-10-11 12:40, Steve Grubb wrote:
> >> >> > On Monday, October 10, 2016 5:10:39 PM EDT Paul Moore wrote:
> >> >> > > On Mon, Oct 10, 2016 at 1:24 PM, Steve Grubb <sgrubb@redhat.com>
> > 
> > wrote:
> >> >> > > > On Thursday, August 18, 2016 2:18:55 PM EDT Richard Guy Briggs
> > 
> > wrote:
> >> >> > > >> loginuid_set support should have been added to userspace when
> >> >> > > >> it
> >> >> > > >> was
> >> >> > > >> added to the kernel around v3.10.  Add it before we do similar
> >> >> > > >> for
> >> >> > > >> sessionID and sessionID_set.
> >> >> > > > 
> >> >> > > > If this were accepted, how would this change writing rules? IOW,
> >> >> > > > can
> >> >> > > > you
> >> >> > > > give an example rule so we can see what this looks like?
> >> >> > > 
> >> >> > > We have a RFE feature page which documents some rule examples:
> >> >> > > 
> >> >> > > *
> >> >> > > https://github.com/linux-audit/audit-kernel/wiki/RFE-Session-ID-Us
> >> >> > > er-> >> > > Fil ter
> >> >> > 
> >> >> > OK, thanks. This is helpful. So, what is the difference between
> >> >> > these
> >> >> > rules?
> >> >> > 
> >> >> > -a always,exit -F path=/tmp/sessionid_test -F loginuid=-1
> >> >> > 
> >> >> > -a always,exit -F path=/tmp/sessionid_set_test -F loginuid_set=0
> >> >> 
> >> >> The only difference is one flag in the kernel to indicate how it was
> >> >> invoked to be able to report when queried exactly the same way it was
> >> >> invoked, but there is no difference in the actual behaviour of the
> >> >> filter.  This was added because of your report that "f24=0" was
> >> >> reported
> >> >> instead of loginuid_set=0 for backwards compatibility.
> >> > 
> >> > OK. Generally its bad to have 2 ways to do the same thing. People use
> >> > SCAP
> >> > content to check system configurations. If there's two ways to do the
> >> > same
> >> > thing, then someone can accidentally choose the wrong way and fail
> >> > their
> >> > scan. We run into this in the past where we allowed -a exit,always and
> >> > -a
> >> > always,exit. All the rules had to be reworked to be consistent.
> >> > Therefore, I would recommend not using the loginuid_set option. We
> >> > still
> >> > get questions about -w /path/file -p wa  vs -a always,exit -F
> >> > path=/path/file -F perm=wa. But that one is so deeply embedded that it
> >> > should not be fixed.
> >> > 
> >> >> Going forward, the implementation of the sessionid_set field (which
> >> >> works similarly) will not allow an unset value of sessionid since
> >> >> these
> >> >> are a new addition that didn't need to accomodate backward
> >> >> compatibility.
> >> > 
> >> > As long as we can trigger on sessionid=-1, then we are fine.
> >> 
> >> Wait a minute ... what happened to the loginuid_set patches?  Didn't
> >> those get merged to userspace?
> > 
> > I'm reviewing this patch set for merging now that we are past all the 2.6
> > bug fixing.
> 
> Ah, nevermind ... I confused loginuid and sessionid, sorry about the
> confusion.
> 
> Anyway, I thought the desire for having a dedicated "is the loginuid
> value set?" filter came from userspace?  If not, where did this
> requirement come from?

I don't know where it came from. We have always used -1 for unset loginuid and 
session id.

-Steve

Re: [userspace PATCH v2 0/2] Add support for loginuid_set

[Paul Moore]

On Tue, Oct 11, 2016 at 5:31 PM, Steve Grubb <sgrubb@redhat.com> wrote:
> On Tuesday, October 11, 2016 4:54:26 PM EDT Paul Moore wrote:
>> On Tue, Oct 11, 2016 at 4:50 PM, Steve Grubb <sgrubb@redhat.com> wrote:
>> > On Tuesday, October 11, 2016 4:42:58 PM EDT Paul Moore wrote:
>> >> On Tue, Oct 11, 2016 at 3:22 PM, Steve Grubb <sgrubb@redhat.com> wrote:
>> >> > On Tuesday, October 11, 2016 2:27:54 PM EDT Richard Guy Briggs wrote:
>> >> >> On 2016-10-11 12:40, Steve Grubb wrote:
>> >> >> > On Monday, October 10, 2016 5:10:39 PM EDT Paul Moore wrote:
>> >> >> > > On Mon, Oct 10, 2016 at 1:24 PM, Steve Grubb <sgrubb@redhat.com>
>> >
>> > wrote:
>> >> >> > > > On Thursday, August 18, 2016 2:18:55 PM EDT Richard Guy Briggs
>> >
>> > wrote:
>> >> >> > > >> loginuid_set support should have been added to userspace when
>> >> >> > > >> it
>> >> >> > > >> was
>> >> >> > > >> added to the kernel around v3.10.  Add it before we do similar
>> >> >> > > >> for
>> >> >> > > >> sessionID and sessionID_set.
>> >> >> > > >
>> >> >> > > > If this were accepted, how would this change writing rules? IOW,
>> >> >> > > > can
>> >> >> > > > you
>> >> >> > > > give an example rule so we can see what this looks like?
>> >> >> > >
>> >> >> > > We have a RFE feature page which documents some rule examples:
>> >> >> > >
>> >> >> > > *
>> >> >> > > https://github.com/linux-audit/audit-kernel/wiki/RFE-Session-ID-Us
>> >> >> > > er-> >> > > Fil ter
>> >> >> >
>> >> >> > OK, thanks. This is helpful. So, what is the difference between
>> >> >> > these
>> >> >> > rules?
>> >> >> >
>> >> >> > -a always,exit -F path=/tmp/sessionid_test -F loginuid=-1
>> >> >> >
>> >> >> > -a always,exit -F path=/tmp/sessionid_set_test -F loginuid_set=0
>> >> >>
>> >> >> The only difference is one flag in the kernel to indicate how it was
>> >> >> invoked to be able to report when queried exactly the same way it was
>> >> >> invoked, but there is no difference in the actual behaviour of the
>> >> >> filter.  This was added because of your report that "f24=0" was
>> >> >> reported
>> >> >> instead of loginuid_set=0 for backwards compatibility.
>> >> >
>> >> > OK. Generally its bad to have 2 ways to do the same thing. People use
>> >> > SCAP
>> >> > content to check system configurations. If there's two ways to do the
>> >> > same
>> >> > thing, then someone can accidentally choose the wrong way and fail
>> >> > their
>> >> > scan. We run into this in the past where we allowed -a exit,always and
>> >> > -a
>> >> > always,exit. All the rules had to be reworked to be consistent.
>> >> > Therefore, I would recommend not using the loginuid_set option. We
>> >> > still
>> >> > get questions about -w /path/file -p wa  vs -a always,exit -F
>> >> > path=/path/file -F perm=wa. But that one is so deeply embedded that it
>> >> > should not be fixed.
>> >> >
>> >> >> Going forward, the implementation of the sessionid_set field (which
>> >> >> works similarly) will not allow an unset value of sessionid since
>> >> >> these
>> >> >> are a new addition that didn't need to accomodate backward
>> >> >> compatibility.
>> >> >
>> >> > As long as we can trigger on sessionid=-1, then we are fine.
>> >>
>> >> Wait a minute ... what happened to the loginuid_set patches?  Didn't
>> >> those get merged to userspace?
>> >
>> > I'm reviewing this patch set for merging now that we are past all the 2.6
>> > bug fixing.
>>
>> Ah, nevermind ... I confused loginuid and sessionid, sorry about the
>> confusion.
>>
>> Anyway, I thought the desire for having a dedicated "is the loginuid
>> value set?" filter came from userspace?  If not, where did this
>> requirement come from?
>
> I don't know where it came from. We have always used -1 for unset loginuid and
> session id.

Looking back through the git logs, it looks like it originally came
out of the user namespace work by Eric Biederman.

-- 
paul moore
security @ redhat

Re: [userspace PATCH v2 0/2] Add support for loginuid_set

[Richard Guy Briggs]

On 2016-10-11 18:15, Paul Moore wrote:
> On Tue, Oct 11, 2016 at 5:31 PM, Steve Grubb <sgrubb@redhat.com> wrote:
> > On Tuesday, October 11, 2016 4:54:26 PM EDT Paul Moore wrote:
> >> On Tue, Oct 11, 2016 at 4:50 PM, Steve Grubb <sgrubb@redhat.com> wrote:
> >> > On Tuesday, October 11, 2016 4:42:58 PM EDT Paul Moore wrote:
> >> >> On Tue, Oct 11, 2016 at 3:22 PM, Steve Grubb <sgrubb@redhat.com> wrote:
> >> >> > On Tuesday, October 11, 2016 2:27:54 PM EDT Richard Guy Briggs wrote:
> >> >> >> On 2016-10-11 12:40, Steve Grubb wrote:
> >> >> >> > On Monday, October 10, 2016 5:10:39 PM EDT Paul Moore wrote:
> >> >> >> > > On Mon, Oct 10, 2016 at 1:24 PM, Steve Grubb <sgrubb@redhat.com>
> >> >
> >> > wrote:
> >> >> >> > > > On Thursday, August 18, 2016 2:18:55 PM EDT Richard Guy Briggs
> >> >
> >> > wrote:
> >> >> >> > > >> loginuid_set support should have been added to userspace when
> >> >> >> > > >> it
> >> >> >> > > >> was
> >> >> >> > > >> added to the kernel around v3.10.  Add it before we do similar
> >> >> >> > > >> for
> >> >> >> > > >> sessionID and sessionID_set.
> >> >> >> > > >
> >> >> >> > > > If this were accepted, how would this change writing rules? IOW,
> >> >> >> > > > can
> >> >> >> > > > you
> >> >> >> > > > give an example rule so we can see what this looks like?
> >> >> >> > >
> >> >> >> > > We have a RFE feature page which documents some rule examples:
> >> >> >> > >
> >> >> >> > > *
> >> >> >> > > https://github.com/linux-audit/audit-kernel/wiki/RFE-Session-ID-Us
> >> >> >> > > er-> >> > > Fil ter
> >> >> >> >
> >> >> >> > OK, thanks. This is helpful. So, what is the difference between
> >> >> >> > these
> >> >> >> > rules?
> >> >> >> >
> >> >> >> > -a always,exit -F path=/tmp/sessionid_test -F loginuid=-1
> >> >> >> >
> >> >> >> > -a always,exit -F path=/tmp/sessionid_set_test -F loginuid_set=0
> >> >> >>
> >> >> >> The only difference is one flag in the kernel to indicate how it was
> >> >> >> invoked to be able to report when queried exactly the same way it was
> >> >> >> invoked, but there is no difference in the actual behaviour of the
> >> >> >> filter.  This was added because of your report that "f24=0" was
> >> >> >> reported
> >> >> >> instead of loginuid_set=0 for backwards compatibility.
> >> >> >
> >> >> > OK. Generally its bad to have 2 ways to do the same thing. People use
> >> >> > SCAP
> >> >> > content to check system configurations. If there's two ways to do the
> >> >> > same
> >> >> > thing, then someone can accidentally choose the wrong way and fail
> >> >> > their
> >> >> > scan. We run into this in the past where we allowed -a exit,always and
> >> >> > -a
> >> >> > always,exit. All the rules had to be reworked to be consistent.
> >> >> > Therefore, I would recommend not using the loginuid_set option. We
> >> >> > still
> >> >> > get questions about -w /path/file -p wa  vs -a always,exit -F
> >> >> > path=/path/file -F perm=wa. But that one is so deeply embedded that it
> >> >> > should not be fixed.
> >> >> >
> >> >> >> Going forward, the implementation of the sessionid_set field (which
> >> >> >> works similarly) will not allow an unset value of sessionid since
> >> >> >> these
> >> >> >> are a new addition that didn't need to accomodate backward
> >> >> >> compatibility.
> >> >> >
> >> >> > As long as we can trigger on sessionid=-1, then we are fine.
> >> >>
> >> >> Wait a minute ... what happened to the loginuid_set patches?  Didn't
> >> >> those get merged to userspace?
> >> >
> >> > I'm reviewing this patch set for merging now that we are past all the 2.6
> >> > bug fixing.
> >>
> >> Ah, nevermind ... I confused loginuid and sessionid, sorry about the
> >> confusion.
> >>
> >> Anyway, I thought the desire for having a dedicated "is the loginuid
> >> value set?" filter came from userspace?  If not, where did this
> >> requirement come from?
> >
> > I don't know where it came from. We have always used -1 for unset loginuid and
> > session id.
> 
> Looking back through the git logs, it looks like it originally came
> out of the user namespace work by Eric Biederman.

That's exactly where it came from.  Eric submitted the patch 780a7654 to
fix the regression caused by e1760bd (userns: Convert the audit loginuid
to be a kuid) and its set of 9 patches that were part of a 41-patch set.
I notice Paul was Cc:-ed on that set...  I had to work around the work
around when Steve reported the "f24=..." values.

I can accept that Steve doesn't want to add more ways of doing the same
thing, so I don't have an easy answer in terms of AUDIT_LOGINUID_SET
being exposed in the UAPI.

Since sessionid is a new field for filter specification (but not
reporting and searching), I blocked sessionid==-1 in the api for setting
filters.  This unfortunately makes it a different way to specify it than
loginuid when it is not set.


> paul moore

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Kernel Security Engineering, Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635

Re: [userspace PATCH v2 0/2] Add support for loginuid_set

[Steve Grubb]

On Monday, October 17, 2016 11:40:17 AM EDT Richard Guy Briggs wrote:
> Since sessionid is a new field for filter specification (but not
> reporting and searching), I blocked sessionid==-1 in the api for setting
> filters.  This unfortunately makes it a different way to specify it than
> loginuid when it is not set.

Can we unblock that?

-Steve

Re: [userspace PATCH v2 0/2] Add support for loginuid_set

[Richard Guy Briggs]

On 2016-10-17 12:04, Steve Grubb wrote:
> On Monday, October 17, 2016 11:40:17 AM EDT Richard Guy Briggs wrote:
> > Since sessionid is a new field for filter specification (but not
> > reporting and searching), I blocked sessionid==-1 in the api for setting
> > filters.  This unfortunately makes it a different way to specify it than
> > loginuid when it is not set.
> 
> Can we unblock that?

Sure, then we would have two ways to express the same thing and ends up
using in-band signalling which is what Eric was trying to avoid in the
first place.

> -Steve

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Kernel Security Engineering, Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635

Re: [userspace PATCH v2 0/2] Add support for loginuid_set

[Steve Grubb]

On Monday, October 17, 2016 12:51:53 PM EDT Richard Guy Briggs wrote:
> On 2016-10-17 12:04, Steve Grubb wrote:
> > On Monday, October 17, 2016 11:40:17 AM EDT Richard Guy Briggs wrote:
> > > Since sessionid is a new field for filter specification (but not
> > > reporting and searching), I blocked sessionid==-1 in the api for setting
> > > filters.  This unfortunately makes it a different way to specify it than
> > > loginuid when it is not set.
> > 
> > Can we unblock that?
> 
> Sure, then we would have two ways to express the same thing and ends up
> using in-band signalling which is what Eric was trying to avoid in the
> first place.

The plan would be to not use sessionid_set at all. Then we have one way to do 
things. I doubt anyone is using either of the set functions. Just mark them 
deprecated in the comments.

-Steve

Re: [userspace PATCH v2 0/2] Add support for loginuid_set

[Paul Moore]

On Mon, Oct 17, 2016 at 11:40 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2016-10-11 18:15, Paul Moore wrote:
>> Looking back through the git logs, it looks like it originally came
>> out of the user namespace work by Eric Biederman.
>
> That's exactly where it came from.  Eric submitted the patch 780a7654 to
> fix the regression caused by e1760bd (userns: Convert the audit loginuid
> to be a kuid) and its set of 9 patches that were part of a 41-patch set.
> I notice Paul was Cc:-ed on that set...

I don't have the time to dig through my mail to see what all was
included in that patchset, but based on the git log that patch was
from April 2013 and I didn't become responsible for the audit code
until October 2014.  I also don't see my Acked-by/Reviewed-by tag on
that commit so it is safe to say I was busy with other things at the
time.  There are plenty of things you can blame me for, this ain't one
of 'em.

> I had to work around the work
> around when Steve reported the "f24=..." values.
>
> I can accept that Steve doesn't want to add more ways of doing the same
> thing, so I don't have an easy answer in terms of AUDIT_LOGINUID_SET
> being exposed in the UAPI.
>
> Since sessionid is a new field for filter specification (but not
> reporting and searching), I blocked sessionid==-1 in the api for setting
> filters.  This unfortunately makes it a different way to specify it than
> loginuid when it is not set.

We are not going to change the loginuid related mechanisms at this
point; they aren't causing any breakage, and I don't want to break the
existing kernel/user API without a good reason.

We haven't merged any of the session ID code into the kernel so
changes are still possible.  The logic for supporting loginuid_set
(UID namespace issues) don't really apply to session IDs so I think we
can drop the sessionid_set part of the API and just use the -1
sentinel.  If you are all still looking to blame somebody, you can all
blame me for suggesting session ID to Richard.

Richard, if we use -1 as a magic number for the session ID, we should
make sure we roll the session ID value assigned to new sessions before
we hit -1 in audit_set_loginuid(...).

-- 
paul moore
security @ redhat

Re: [userspace PATCH v2 0/2] Add support for loginuid_set

[Steve Grubb]

On Monday, October 17, 2016 5:19:59 PM EDT Paul Moore wrote:
> We haven't merged any of the session ID code into the kernel so
> changes are still possible.  The logic for supporting loginuid_set
> (UID namespace issues) don't really apply to session IDs so I think we
> can drop the sessionid_set part of the API and just use the -1
> sentinel.

OK, that's good to hear. I'll fix up and merge the sessionid patch - no need to 
re-send the user space piece.

-Steve

Re: [userspace PATCH v2 0/2] Add support for loginuid_set

[Richard Guy Briggs]

On 2016-10-17 18:21, Steve Grubb wrote:
> On Monday, October 17, 2016 5:19:59 PM EDT Paul Moore wrote:
> > We haven't merged any of the session ID code into the kernel so
> > changes are still possible.  The logic for supporting loginuid_set
> > (UID namespace issues) don't really apply to session IDs so I think we
> > can drop the sessionid_set part of the API and just use the -1
> > sentinel.
> 
> OK, that's good to hear. I'll fix up and merge the sessionid patch - no need to 
> re-send the user space piece.

userspace patch 2 gets dropped, paches 1 and 3 need rework to not block
-1 and to remove sessionid_set respectively.  kernel patch 2 gets
dropped and patch 1 I think needs rework to allow -1.

The test patches also need rework as does the RFE page.

> -Steve

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Kernel Security Engineering, Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635

Re: [userspace PATCH v2 0/2] Add support for loginuid_set

[Richard Guy Briggs]

On 2016-10-18 00:35, Richard Guy Briggs wrote:
> On 2016-10-17 18:21, Steve Grubb wrote:
> > On Monday, October 17, 2016 5:19:59 PM EDT Paul Moore wrote:
> > > We haven't merged any of the session ID code into the kernel so
> > > changes are still possible.  The logic for supporting loginuid_set
> > > (UID namespace issues) don't really apply to session IDs so I think we
> > > can drop the sessionid_set part of the API and just use the -1
> > > sentinel.
> > 
> > OK, that's good to hear. I'll fix up and merge the sessionid patch - no need to 
> > re-send the user space piece.
> 
> userspace patch 2 gets dropped, paches 1 and 3 need rework to not block
> -1 and to remove sessionid_set respectively.  kernel patch 2 gets
> dropped and patch 1 I think needs rework to allow -1.

Kernel patch 1 does not need rework because I properly put the positive
integer check for sessionID in the sessionID_set patch that adds that.

> The test patches also need rework as does the RFE page.
> 
> > -Steve
> 
> - RGB

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Kernel Security Engineering, Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635