[userspace PATCH v2 0/3] Add support for sessionid user filters, sessionid_set

[userspace PATCH v2 0/3] Add support for sessionid user filters, sessionid_set

[Richard Guy Briggs]

Add support for sessionid, sessionid_set (first two patches) and
feature bitmap detection of the kernel feature (third patch) in user
filters.  This is to implement issue "ghak4":
	https://github.com/linux-audit/audit-kernel/issues/4
	https://github.com/linux-audit/audit-kernel/wiki/RFE-Session-ID-User-Filter

This patchset should be added after loginuid_set and exclude filter
extension to avoid merge conflicts.

Richard Guy Briggs (3):
  Add userspace support for session ID user filter.
  Add sessionid_set option from kernel uapi macro AUDIT_SESSIONID_SET
  Check sessionID* fields available in kernel

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

[userspace PATCH v2 1/3] Add userspace support for session ID user filter.

[Richard Guy Briggs]

Add support for the session ID user filter by adding the field name
"sessionid" using the kernel defined macro value AUDIT_SESSIONID.

https://github.com/linux-audit/audit-kernel/issues/4
RFE: add a session ID filter to the kernel's user filter

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

diff --git a/trunk/lib/errormsg.h b/trunk/lib/errormsg.h
index 4a897be..2c6b9fa 100644
--- a/trunk/lib/errormsg.h
+++ b/trunk/lib/errormsg.h
@@ -68,5 +68,6 @@ static const struct msg_tab err_msgtab[] = {
     { -30,    2,    "Field option not supported by kernel:" },
     { -31,    1,    "can only be used with exit, user and exclude filter lists" },
     { -32,    2,    "-F value should be boolean 0 or 1 for" },
+    { -33,    2,    "-F value should be positive number for" },
 };
 #endif
diff --git a/trunk/lib/fieldtab.h b/trunk/lib/fieldtab.h
index 107157d..84acc08 100644
--- a/trunk/lib/fieldtab.h
+++ b/trunk/lib/fieldtab.h
@@ -33,6 +33,7 @@ _S(AUDIT_LOGINUID,     "auid"         )
 _S(AUDIT_LOGINUID,     "loginuid"     )
 _S(AUDIT_LOGINUID_SET, "auid_set"     )
 _S(AUDIT_LOGINUID_SET, "loginuid_set" )
+_S(AUDIT_SESSIONID,    "sessionid"    )
 _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 5ffc38c..38776f4 100644
--- a/trunk/lib/libaudit.c
+++ b/trunk/lib/libaudit.c
@@ -1663,6 +1663,17 @@ int audit_rule_fieldpair_data(struct audit_rule_data **rulep, const char *pair,
 			else
 				return -32;
 			break;
+		case AUDIT_SESSIONID:
+			if (flags != AUDIT_FILTER_EXCLUDE &&
+			    flags != AUDIT_FILTER_USER &&
+			    flags != AUDIT_FILTER_EXIT)
+				return -31;
+			if (isdigit((char)*(v)))
+				rule->values[rule->field_count] =
+					strtol(v, NULL, 0);
+			else
+				return -33;
+			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 f77691f..95b7a78 100644
--- a/trunk/lib/libaudit.h
+++ b/trunk/lib/libaudit.h
@@ -377,6 +377,10 @@ extern "C" {
 #define AUDIT_LOGINUID_SET		24
 #endif
 
+#ifndef AUDIT_SESSIONID
+#define AUDIT_SESSIONID			25
+#endif
+
 /* Architectures */
 #ifndef EM_ARM
 #define EM_ARM  40
-- 
1.7.1

[userspace PATCH v2 2/3] Add sessionid_set option from kernel uapi macro AUDIT_SESSIONID_SET

[Richard Guy Briggs]

Add sessionid_set field option from kernel uapi macro SESSIONID_SET to
enable specifying that sessionID is set or not in user filters.

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

diff --git a/trunk/lib/fieldtab.h b/trunk/lib/fieldtab.h
index 84acc08..eeb951e 100644
--- a/trunk/lib/fieldtab.h
+++ b/trunk/lib/fieldtab.h
@@ -34,6 +34,7 @@ _S(AUDIT_LOGINUID,     "loginuid"     )
 _S(AUDIT_LOGINUID_SET, "auid_set"     )
 _S(AUDIT_LOGINUID_SET, "loginuid_set" )
 _S(AUDIT_SESSIONID,    "sessionid"    )
+_S(AUDIT_SESSIONID_SET,"sessionid_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 38776f4..5ffb720 100644
--- a/trunk/lib/libaudit.c
+++ b/trunk/lib/libaudit.c
@@ -1650,6 +1650,8 @@ int audit_rule_fieldpair_data(struct audit_rule_data **rulep, const char *pair,
 		case AUDIT_LOGINUID_SET:
 			if(!features)
 				return -30;
+			/* fallthrough */
+		case AUDIT_SESSIONID_SET:
 			if (flags != AUDIT_FILTER_EXCLUDE &&
 			    flags != AUDIT_FILTER_USER &&
 			    flags != AUDIT_FILTER_EXIT)
diff --git a/trunk/lib/libaudit.h b/trunk/lib/libaudit.h
index 95b7a78..f8007c1 100644
--- a/trunk/lib/libaudit.h
+++ b/trunk/lib/libaudit.h
@@ -381,6 +381,10 @@ extern "C" {
 #define AUDIT_SESSIONID			25
 #endif
 
+#ifndef AUDIT_SESSIONID_SET
+#define AUDIT_SESSIONID_SET		26
+#endif
+
 /* Architectures */
 #ifndef EM_ARM
 #define EM_ARM  40
-- 
1.7.1

Re: [userspace PATCH v2 2/3] Add sessionid_set option from kernel uapi macro AUDIT_SESSIONID_SET

[Steve Grubb]

On Thursday, August 18, 2016 2:47:33 PM EDT Richard Guy Briggs wrote:
> Add sessionid_set field option from kernel uapi macro SESSIONID_SET to
> enable specifying that sessionID is set or not in user filters.

Is there any compelling reason to support two differents fields that essentially 
decide how to audit sessions? I think its a bit clunky to expect that people 
write rules

-a always,exit -S open -F path=/path/file  -F sessionid>0 

but if you want to record daemons, then its not as simple as using -1 which is 
what is in the logs and the intuitive answer. Instead you have to use a new 
field.

-a always,exit -S open -F path=/path/file  -F sessionid_set=0

But then you can also do the first rule as:

-a always,exit -S open -F path=/path/file  -F sessionid_set=1

So, we have 2 ways of doing almost the same thing. I don't really like this.

-Steve


> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
>  trunk/lib/fieldtab.h |    1 +
>  trunk/lib/libaudit.c |    2 ++
>  trunk/lib/libaudit.h |    4 ++++
>  3 files changed, 7 insertions(+), 0 deletions(-)
> 
> diff --git a/trunk/lib/fieldtab.h b/trunk/lib/fieldtab.h
> index 84acc08..eeb951e 100644
> --- a/trunk/lib/fieldtab.h
> +++ b/trunk/lib/fieldtab.h
> @@ -34,6 +34,7 @@ _S(AUDIT_LOGINUID,     "loginuid"     )
>  _S(AUDIT_LOGINUID_SET, "auid_set"     )
>  _S(AUDIT_LOGINUID_SET, "loginuid_set" )
>  _S(AUDIT_SESSIONID,    "sessionid"    )
> +_S(AUDIT_SESSIONID_SET,"sessionid_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 38776f4..5ffb720 100644
> --- a/trunk/lib/libaudit.c
> +++ b/trunk/lib/libaudit.c
> @@ -1650,6 +1650,8 @@ int audit_rule_fieldpair_data(struct audit_rule_data
> **rulep, const char *pair, case AUDIT_LOGINUID_SET:
>  			if(!features)
>  				return -30;
> +			/* fallthrough */
> +		case AUDIT_SESSIONID_SET:
>  			if (flags != AUDIT_FILTER_EXCLUDE &&
>  			    flags != AUDIT_FILTER_USER &&
>  			    flags != AUDIT_FILTER_EXIT)
> diff --git a/trunk/lib/libaudit.h b/trunk/lib/libaudit.h
> index 95b7a78..f8007c1 100644
> --- a/trunk/lib/libaudit.h
> +++ b/trunk/lib/libaudit.h
> @@ -381,6 +381,10 @@ extern "C" {
>  #define AUDIT_SESSIONID			25
>  #endif
> 
> +#ifndef AUDIT_SESSIONID_SET
> +#define AUDIT_SESSIONID_SET		26
> +#endif
> +
>  /* Architectures */
>  #ifndef EM_ARM
>  #define EM_ARM  40

Re: [userspace PATCH v2 2/3] Add sessionid_set option from kernel uapi macro AUDIT_SESSIONID_SET

[Paul Moore]

On Wed, Oct 12, 2016 at 2:02 PM, Steve Grubb <sgrubb@redhat.com> wrote:
> On Thursday, August 18, 2016 2:47:33 PM EDT Richard Guy Briggs wrote:
>> Add sessionid_set field option from kernel uapi macro SESSIONID_SET to
>> enable specifying that sessionID is set or not in user filters.
>
> Is there any compelling reason to support two differents fields that essentially
> decide how to audit sessions? I think its a bit clunky to expect that people
> write rules
>
> -a always,exit -S open -F path=/path/file  -F sessionid>0
>
> but if you want to record daemons, then its not as simple as using -1 which is
> what is in the logs and the intuitive answer. Instead you have to use a new
> field.
>
> -a always,exit -S open -F path=/path/file  -F sessionid_set=0
>
> But then you can also do the first rule as:
>
> -a always,exit -S open -F path=/path/file  -F sessionid_set=1
>
> So, we have 2 ways of doing almost the same thing. I don't really like this.

I originally thought the loginuid_set filter functionality was added
to satisfy a request made by you for the audit userspace; I suggested
to Richard that we do the same for the session ID since there were
some definite similarities.  However, having looked back at the
original motivation for adding the loginuid_set functionality, I don't
we will face the same problem with the session ID.  If Richard can't
think of any compelling reason to keep a dedicated sessionid_set
filter, I think we can drop it.

Further, while I understand why Eric B. needed to make the internal
audit kernel changes to the loginuid code (and other audit UID/GID
bits for that matter), I'm less convinced on the need to change the
kernel/userspace filter API to add the loginuid_set capability.
However, that was before my time, and it's done now, we can't yank it
out at this point.

-- 
paul moore
www.paul-moore.com

Re: [userspace PATCH v2 2/3] Add sessionid_set option from kernel uapi macro AUDIT_SESSIONID_SET

[Richard Guy Briggs]

On 2016-10-13 08:32, Paul Moore wrote:
> On Wed, Oct 12, 2016 at 2:02 PM, Steve Grubb <sgrubb@redhat.com> wrote:
> > On Thursday, August 18, 2016 2:47:33 PM EDT Richard Guy Briggs wrote:
> >> Add sessionid_set field option from kernel uapi macro SESSIONID_SET to
> >> enable specifying that sessionID is set or not in user filters.
> >
> > Is there any compelling reason to support two differents fields that essentially
> > decide how to audit sessions? I think its a bit clunky to expect that people
> > write rules
> >
> > -a always,exit -S open -F path=/path/file  -F sessionid>0
> >
> > but if you want to record daemons, then its not as simple as using -1 which is
> > what is in the logs and the intuitive answer. Instead you have to use a new
> > field.
> >
> > -a always,exit -S open -F path=/path/file  -F sessionid_set=0
> >
> > But then you can also do the first rule as:
> >
> > -a always,exit -S open -F path=/path/file  -F sessionid_set=1
> >
> > So, we have 2 ways of doing almost the same thing. I don't really like this.
> 
> I originally thought the loginuid_set filter functionality was added
> to satisfy a request made by you for the audit userspace; I suggested
> to Richard that we do the same for the session ID since there were
> some definite similarities.  However, having looked back at the
> original motivation for adding the loginuid_set functionality, I don't
> we will face the same problem with the session ID.  If Richard can't
> think of any compelling reason to keep a dedicated sessionid_set
> filter, I think we can drop it.
> 
> Further, while I understand why Eric B. needed to make the internal
> audit kernel changes to the loginuid code (and other audit UID/GID
> bits for that matter), I'm less convinced on the need to change the
> kernel/userspace filter API to add the loginuid_set capability.
> However, that was before my time, and it's done now, we can't yank it
> out at this point.

Well, Steve argues that -1 is more intuitive than "unset", which I
disagree unless you have always done that or you are a programmer.  I'd
argue most of the users aren't programmers.  And certainly that huge
number of 4 billion and something isn't something I'm going to remember
off the top of my head (and I'm a programmer) and the users certainly
aren't going to understand the significance of when making rules.  Eric
isn't the first to reasonably propose not to use special values in-band
due to hoops that need to be jumped to work around complications that
can create (including needing to check for rolling the counter).

As far as Eric's patch set is concerned, he needed to make that change
only as a check that we weren't carelessly throwing around UIDs and GIDs
in the kernel without proper translation to the right user namespace,
but he didn't need to break the existing API to do that.  That was a
bug.  Now that cat was out of the bag, it was harder to stuff it back
in.  I agreed with the fix at the time to add AUDIT_LOGINUID_SET due to
my aversion to in-band signalling, but the better solution would have
been to unbreak userspace and re-allow -1 and not complicate things
further with the internal representation of another field to represent
unset.

Given sessionID is a new filter field, I'd prefer the dedicated field to
indicate it is unset from the get-go to avoid the more complicated mess
we are now in with loginuid_set in deprecating loginuid==-1, but that
isn't the one obvious solution.

I suspect there are no users of AUDIT_LOGINUID_SET, so we could likely
rip it out and nobody would notice, and in that case, it would be
obvious to make sessionID behave the same way, in-banding -1 to indicate
unset.

All of which to say, I guess deprecating AUDIT_LOGINUID_SET is the best
way to go and not add loginuid_set in userspace tools and not implement
sessionid_set at all.
> 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

[userspace PATCH v2 3/3] Check sessionID* fields available in kernel

[Richard Guy Briggs]

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

diff --git a/trunk/lib/libaudit.c b/trunk/lib/libaudit.c
index 5ffb720..a254a01 100644
--- a/trunk/lib/libaudit.c
+++ b/trunk/lib/libaudit.c
@@ -1647,11 +1647,13 @@ int audit_rule_fieldpair_data(struct audit_rule_data **rulep, const char *pair,
 			else 
 				return -21;
 			break;
+		case AUDIT_SESSIONID_SET:
+			if ((features & AUDIT_FEATURE_BITMAP_SESSIONID_FILTER) == 0)
+				return -30;
+			/* fallthrough */
 		case AUDIT_LOGINUID_SET:
 			if(!features)
 				return -30;
-			/* fallthrough */
-		case AUDIT_SESSIONID_SET:
 			if (flags != AUDIT_FILTER_EXCLUDE &&
 			    flags != AUDIT_FILTER_USER &&
 			    flags != AUDIT_FILTER_EXIT)
@@ -1666,6 +1668,8 @@ int audit_rule_fieldpair_data(struct audit_rule_data **rulep, const char *pair,
 				return -32;
 			break;
 		case AUDIT_SESSIONID:
+			if ((features & AUDIT_FEATURE_BITMAP_SESSIONID_FILTER) == 0)
+				return -30;
 			if (flags != AUDIT_FILTER_EXCLUDE &&
 			    flags != AUDIT_FILTER_USER &&
 			    flags != AUDIT_FILTER_EXIT)
diff --git a/trunk/lib/libaudit.h b/trunk/lib/libaudit.h
index f8007c1..14bbf2d 100644
--- a/trunk/lib/libaudit.h
+++ b/trunk/lib/libaudit.h
@@ -281,6 +281,9 @@ extern "C" {
 #ifndef AUDIT_FEATURE_BITMAP_EXCLUDE_EXTEND
 #define AUDIT_FEATURE_BITMAP_EXCLUDE_EXTEND	0x00000008
 #endif
+#ifndef AUDIT_FEATURE_BITMAP_SESSIONID_FILTER
+#define AUDIT_FEATURE_BITMAP_SESSIONID_FILTER   0x00000010
+#endif
 
 /* Defines for interfield comparison update */
 #ifndef AUDIT_OBJ_UID
-- 
1.7.1

Re: [userspace PATCH v2 0/3] Add support for sessionid user filters, sessionid_set

[Steve Grubb]

On Thursday, August 18, 2016 2:47:31 PM EDT Richard Guy Briggs wrote:
> Add support for sessionid, sessionid_set (first two patches) and
> feature bitmap detection of the kernel feature (third patch) in user
> filters.  This is to implement issue "ghak4":
>         https://github.com/linux-audit/audit-kernel/issues/4
>        
> https://github.com/linux-audit/audit-kernel/wiki/RFE-Session-ID-User-Filter
> 
> This patchset should be added after loginuid_set and exclude filter
> extension to avoid merge conflicts.

Applied after removing sessionid_set code

-Steve