| Message ID | 6452955c1e038227a5cd169f689f3fd3db27513f.1577736799.git.rgb@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | audit: implement container identifier | expand |
On Tue, Dec 31, 2019 at 2:51 PM Richard Guy Briggs <rgb@redhat.com> wrote: > > Track the parent container of a container to be able to filter and > report nesting. > > Now that we have a way to track and check the parent container of a > container, modify the contid field format to be able to report that > nesting using a carrat ("^") separator to indicate nesting. The > original field format was "contid=<contid>" for task-associated records > and "contid=<contid>[,<contid>[...]]" for network-namespace-associated > records. The new field format is > "contid=<contid>[^<contid>[...]][,<contid>[...]]". Let's make sure we always use a comma as a separator, even when recording the parent information, for example: "contid=<contid>[,^<contid>[...]][,<contid>[...]]" > Signed-off-by: Richard Guy Briggs <rgb@redhat.com> > --- > include/linux/audit.h | 1 + > kernel/audit.c | 53 +++++++++++++++++++++++++++++++++++++++++++-------- > kernel/audit.h | 1 + > kernel/auditfilter.c | 17 ++++++++++++++++- > kernel/auditsc.c | 2 +- > 5 files changed, 64 insertions(+), 10 deletions(-) ... > diff --git a/kernel/audit.c b/kernel/audit.c > index ef8e07524c46..68be59d1a89b 100644 > --- a/kernel/audit.c > +++ b/kernel/audit.c > @@ -492,6 +493,7 @@ void audit_switch_task_namespaces(struct nsproxy *ns, struct task_struct *p) > audit_netns_contid_add(new->net_ns, contid); > } > > +void audit_log_contid(struct audit_buffer *ab, u64 contid); If we need a forward declaration, might as well just move it up near the top of the file with the rest of the declarations. > +void audit_log_contid(struct audit_buffer *ab, u64 contid) > +{ > + struct audit_contobj *cont = NULL, *prcont = NULL; > + int h; It seems safer to pass the audit container ID object and not the u64. > + if (!audit_contid_valid(contid)) { > + audit_log_format(ab, "%llu", contid); Do we really want to print (u64)-1 here? Since this is a known invalid number, would "?" be a better choice? > + return; > + } > + h = audit_hash_contid(contid); > + rcu_read_lock(); > + list_for_each_entry_rcu(cont, &audit_contid_hash[h], list) > + if (cont->id == contid) { > + prcont = cont; Why not just pull the code below into the body of this if statement? It all needs to be done under the RCU read lock anyway and the code would read much better this way. > + break; > + } > + if (!prcont) { > + audit_log_format(ab, "%llu", contid); > + goto out; > + } > + while (prcont) { > + audit_log_format(ab, "%llu", prcont->id); > + prcont = prcont->parent; > + if (prcont) > + audit_log_format(ab, "^"); In the interest of limiting the number of calls to audit_log_format(), how about something like the following: audit_log_format("%llu", cont); iter = cont->parent; while (iter) { if (iter->parent) audit_log_format("^%llu,", iter); else audit_log_format("^%llu", iter); iter = iter->parent; } > + } > +out: > + rcu_read_unlock(); > +} > + > /* > * audit_log_container_id - report container info > * @context: task or local context for record ... > @@ -2705,9 +2741,10 @@ int audit_set_contid(struct task_struct *task, u64 contid) > if (!ab) > return rc; > > - audit_log_format(ab, > - "op=set opid=%d contid=%llu old-contid=%llu", > - task_tgid_nr(task), contid, oldcontid); > + audit_log_format(ab, "op=set opid=%d contid=", task_tgid_nr(task)); > + audit_log_contid(ab, contid); > + audit_log_format(ab, " old-contid="); > + audit_log_contid(ab, oldcontid); This is an interesting case where contid and old-contid are going to be largely the same, only the first (current) ID is going to be different; do we want to duplicate all of those IDs? > audit_log_end(ab); > return rc; > } > @@ -2723,9 +2760,9 @@ void audit_log_container_drop(void) -- paul moore www.paul-moore.com
On 2020-01-22 16:29, Paul Moore wrote: > On Tue, Dec 31, 2019 at 2:51 PM Richard Guy Briggs <rgb@redhat.com> wrote: > > > > Track the parent container of a container to be able to filter and > > report nesting. > > > > Now that we have a way to track and check the parent container of a > > container, modify the contid field format to be able to report that > > nesting using a carrat ("^") separator to indicate nesting. The > > original field format was "contid=<contid>" for task-associated records > > and "contid=<contid>[,<contid>[...]]" for network-namespace-associated > > records. The new field format is > > "contid=<contid>[^<contid>[...]][,<contid>[...]]". > > Let's make sure we always use a comma as a separator, even when > recording the parent information, for example: > "contid=<contid>[,^<contid>[...]][,<contid>[...]]" The intent here is to clearly indicate and separate nesting from parallel use of several containers by one netns. If we do away with that distinction, then we lose that inheritance accountability and should really run the list through a "uniq" function to remove the produced redundancies. This clear inheritance is something Steve was looking for since tracking down individual events/records to show that inheritance was not aways feasible due to rolled logs or search effort. > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com> > > --- > > include/linux/audit.h | 1 + > > kernel/audit.c | 53 +++++++++++++++++++++++++++++++++++++++++++-------- > > kernel/audit.h | 1 + > > kernel/auditfilter.c | 17 ++++++++++++++++- > > kernel/auditsc.c | 2 +- > > 5 files changed, 64 insertions(+), 10 deletions(-) > > ... > > > diff --git a/kernel/audit.c b/kernel/audit.c > > index ef8e07524c46..68be59d1a89b 100644 > > --- a/kernel/audit.c > > +++ b/kernel/audit.c > > > @@ -492,6 +493,7 @@ void audit_switch_task_namespaces(struct nsproxy *ns, struct task_struct *p) > > audit_netns_contid_add(new->net_ns, contid); > > } > > > > +void audit_log_contid(struct audit_buffer *ab, u64 contid); > > If we need a forward declaration, might as well just move it up near > the top of the file with the rest of the declarations. Ok. > > +void audit_log_contid(struct audit_buffer *ab, u64 contid) > > +{ > > + struct audit_contobj *cont = NULL, *prcont = NULL; > > + int h; > > It seems safer to pass the audit container ID object and not the u64. It would also be faster, but in some places it isn't available such as for ptrace and signal targets. This also links back to the drop record refcounts to hold onto the contobj until process exit, or signal delivery. What we could do is to supply two potential parameters, a contobj and/or a contid, and have it use the contobj if it is valid, otherwise, use the contid, as is done for names and paths supplied to audit_log_name(). > > + if (!audit_contid_valid(contid)) { > > + audit_log_format(ab, "%llu", contid); > > Do we really want to print (u64)-1 here? Since this is a known > invalid number, would "?" be a better choice? I'll defer to Steve here. "?" would be one character vs 20 for (u64)-1. I don't expect there to be that many records containing (u64)-1, but it would also make them visually easier to pick out if that is a factor. > > + return; > > + } > > + h = audit_hash_contid(contid); > > + rcu_read_lock(); > > + list_for_each_entry_rcu(cont, &audit_contid_hash[h], list) > > + if (cont->id == contid) { > > + prcont = cont; > > Why not just pull the code below into the body of this if statement? > It all needs to be done under the RCU read lock anyway and the code > would read much better this way. Ok. > > + break; > > + } > > + if (!prcont) { > > + audit_log_format(ab, "%llu", contid); > > + goto out; > > + } > > + while (prcont) { > > + audit_log_format(ab, "%llu", prcont->id); > > + prcont = prcont->parent; > > + if (prcont) > > + audit_log_format(ab, "^"); > > In the interest of limiting the number of calls to audit_log_format(), > how about something like the following: > > audit_log_format("%llu", cont); > iter = cont->parent; > while (iter) { > if (iter->parent) > audit_log_format("^%llu,", iter); > else > audit_log_format("^%llu", iter); > iter = iter->parent; > } Ok. > > + } > > +out: > > + rcu_read_unlock(); > > +} > > + > > /* > > * audit_log_container_id - report container info > > * @context: task or local context for record > > ... > > > @@ -2705,9 +2741,10 @@ int audit_set_contid(struct task_struct *task, u64 contid) > > if (!ab) > > return rc; > > > > - audit_log_format(ab, > > - "op=set opid=%d contid=%llu old-contid=%llu", > > - task_tgid_nr(task), contid, oldcontid); > > + audit_log_format(ab, "op=set opid=%d contid=", task_tgid_nr(task)); > > + audit_log_contid(ab, contid); > > + audit_log_format(ab, " old-contid="); > > + audit_log_contid(ab, oldcontid); > > This is an interesting case where contid and old-contid are going to > be largely the same, only the first (current) ID is going to be > different; do we want to duplicate all of those IDs? At first when I read your comment, I thought we could just take contid and drop oldcontid, but if it fails, we still want all the information, so given the way I've set up the search code in userspace, listing only the newest contid in the contid field and all the rest in oldcontid could be a good compromise. > > audit_log_end(ab); > > return rc; > > } > > @@ -2723,9 +2760,9 @@ void audit_log_container_drop(void) > > paul moore - RGB -- Richard Guy Briggs <rgb@redhat.com> Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada IRC: rgb, SunRaycer Voice: +1.647.777.2635, Internal: (81) 32635
On Wednesday, January 22, 2020 4:29:12 PM EST Paul Moore wrote: > On Tue, Dec 31, 2019 at 2:51 PM Richard Guy Briggs <rgb@redhat.com> wrote: > > Track the parent container of a container to be able to filter and > > report nesting. > > > > Now that we have a way to track and check the parent container of a > > container, modify the contid field format to be able to report that > > nesting using a carrat ("^") separator to indicate nesting. The > > original field format was "contid=<contid>" for task-associated records > > and "contid=<contid>[,<contid>[...]]" for network-namespace-associated > > records. The new field format is > > "contid=<contid>[^<contid>[...]][,<contid>[...]]". > > Let's make sure we always use a comma as a separator, even when > recording the parent information, for example: > "contid=<contid>[,^<contid>[...]][,<contid>[...]]" > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com> > > --- > > > > include/linux/audit.h | 1 + > > kernel/audit.c | 53 > > +++++++++++++++++++++++++++++++++++++++++++-------- kernel/audit.h > > | 1 + > > kernel/auditfilter.c | 17 ++++++++++++++++- > > kernel/auditsc.c | 2 +- > > 5 files changed, 64 insertions(+), 10 deletions(-) > > ... > > > diff --git a/kernel/audit.c b/kernel/audit.c > > index ef8e07524c46..68be59d1a89b 100644 > > --- a/kernel/audit.c > > +++ b/kernel/audit.c > > > > @@ -492,6 +493,7 @@ void audit_switch_task_namespaces(struct nsproxy *ns, > > struct task_struct *p)> > > audit_netns_contid_add(new->net_ns, contid); > > > > } > > > > +void audit_log_contid(struct audit_buffer *ab, u64 contid); > > If we need a forward declaration, might as well just move it up near > the top of the file with the rest of the declarations. > > > +void audit_log_contid(struct audit_buffer *ab, u64 contid) > > +{ > > + struct audit_contobj *cont = NULL, *prcont = NULL; > > + int h; > > It seems safer to pass the audit container ID object and not the u64. > > > + if (!audit_contid_valid(contid)) { > > + audit_log_format(ab, "%llu", contid); > > Do we really want to print (u64)-1 here? Since this is a known > invalid number, would "?" be a better choice? The established pattern is that we print -1 when its unset and "?" when its totalling missing. So, how could this be invalid? It should be set or not. That is unless its totally missing just like when we do not run with selinux enabled and a context just doesn't exist. -Steve > > + return; > > + } > > + h = audit_hash_contid(contid); > > + rcu_read_lock(); > > + list_for_each_entry_rcu(cont, &audit_contid_hash[h], list) > > + if (cont->id == contid) { > > + prcont = cont; > > Why not just pull the code below into the body of this if statement? > It all needs to be done under the RCU read lock anyway and the code > would read much better this way. > > > + break; > > + } > > + if (!prcont) { > > + audit_log_format(ab, "%llu", contid); > > + goto out; > > + } > > + while (prcont) { > > + audit_log_format(ab, "%llu", prcont->id); > > + prcont = prcont->parent; > > + if (prcont) > > + audit_log_format(ab, "^"); > > In the interest of limiting the number of calls to audit_log_format(), > how about something like the following: > > audit_log_format("%llu", cont); > iter = cont->parent; > while (iter) { > if (iter->parent) > audit_log_format("^%llu,", iter); > else > audit_log_format("^%llu", iter); > iter = iter->parent; > } > > > + } > > +out: > > + rcu_read_unlock(); > > +} > > + > > > > /* > > > > * audit_log_container_id - report container info > > * @context: task or local context for record > > ... > > > @@ -2705,9 +2741,10 @@ int audit_set_contid(struct task_struct *task, u64 > > contid)> > > if (!ab) > > > > return rc; > > > > - audit_log_format(ab, > > - "op=set opid=%d contid=%llu old-contid=%llu", > > - task_tgid_nr(task), contid, oldcontid); > > + audit_log_format(ab, "op=set opid=%d contid=", > > task_tgid_nr(task)); + audit_log_contid(ab, contid); > > + audit_log_format(ab, " old-contid="); > > + audit_log_contid(ab, oldcontid); > > This is an interesting case where contid and old-contid are going to > be largely the same, only the first (current) ID is going to be > different; do we want to duplicate all of those IDs? > > > audit_log_end(ab); > > return rc; > > > > } > > > > @@ -2723,9 +2760,9 @@ void audit_log_container_drop(void) > > -- > paul moore > www.paul-moore.com
On 2020-01-31 09:50, Steve Grubb wrote: > On Wednesday, January 22, 2020 4:29:12 PM EST Paul Moore wrote: > > On Tue, Dec 31, 2019 at 2:51 PM Richard Guy Briggs <rgb@redhat.com> wrote: > > > Track the parent container of a container to be able to filter and > > > report nesting. > > > > > > Now that we have a way to track and check the parent container of a > > > container, modify the contid field format to be able to report that > > > nesting using a carrat ("^") separator to indicate nesting. The > > > original field format was "contid=<contid>" for task-associated records > > > and "contid=<contid>[,<contid>[...]]" for network-namespace-associated > > > records. The new field format is > > > "contid=<contid>[^<contid>[...]][,<contid>[...]]". > > > > Let's make sure we always use a comma as a separator, even when > > recording the parent information, for example: > > "contid=<contid>[,^<contid>[...]][,<contid>[...]]" > > > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com> > > > --- > > > > > > include/linux/audit.h | 1 + > > > kernel/audit.c | 53 > > > +++++++++++++++++++++++++++++++++++++++++++-------- kernel/audit.h > > > | 1 + > > > kernel/auditfilter.c | 17 ++++++++++++++++- > > > kernel/auditsc.c | 2 +- > > > 5 files changed, 64 insertions(+), 10 deletions(-) > > > > ... > > > > > diff --git a/kernel/audit.c b/kernel/audit.c > > > index ef8e07524c46..68be59d1a89b 100644 > > > --- a/kernel/audit.c > > > +++ b/kernel/audit.c > > > > > > @@ -492,6 +493,7 @@ void audit_switch_task_namespaces(struct nsproxy *ns, > > > struct task_struct *p)> > > > audit_netns_contid_add(new->net_ns, contid); > > > > > > } > > > > > > +void audit_log_contid(struct audit_buffer *ab, u64 contid); > > > > If we need a forward declaration, might as well just move it up near > > the top of the file with the rest of the declarations. > > > > > +void audit_log_contid(struct audit_buffer *ab, u64 contid) > > > +{ > > > + struct audit_contobj *cont = NULL, *prcont = NULL; > > > + int h; > > > > It seems safer to pass the audit container ID object and not the u64. > > > > > + if (!audit_contid_valid(contid)) { > > > + audit_log_format(ab, "%llu", contid); > > > > Do we really want to print (u64)-1 here? Since this is a known > > invalid number, would "?" be a better choice? > > The established pattern is that we print -1 when its unset and "?" when its > totalling missing. So, how could this be invalid? It should be set or not. > That is unless its totally missing just like when we do not run with selinux > enabled and a context just doesn't exist. Ok, so in this case it is clearly unset, so should be -1, which will be a 20-digit number when represented as an unsigned long long int. Thank you for that clarification Steve. > -Steve > > > > + return; > > > + } > > > + h = audit_hash_contid(contid); > > > + rcu_read_lock(); > > > + list_for_each_entry_rcu(cont, &audit_contid_hash[h], list) > > > + if (cont->id == contid) { > > > + prcont = cont; > > > > Why not just pull the code below into the body of this if statement? > > It all needs to be done under the RCU read lock anyway and the code > > would read much better this way. > > > > > + break; > > > + } > > > + if (!prcont) { > > > + audit_log_format(ab, "%llu", contid); > > > + goto out; > > > + } > > > + while (prcont) { > > > + audit_log_format(ab, "%llu", prcont->id); > > > + prcont = prcont->parent; > > > + if (prcont) > > > + audit_log_format(ab, "^"); > > > > In the interest of limiting the number of calls to audit_log_format(), > > how about something like the following: > > > > audit_log_format("%llu", cont); > > iter = cont->parent; > > while (iter) { > > if (iter->parent) > > audit_log_format("^%llu,", iter); > > else > > audit_log_format("^%llu", iter); > > iter = iter->parent; > > } > > > > > + } > > > +out: > > > + rcu_read_unlock(); > > > +} > > > + > > > > > > /* > > > > > > * audit_log_container_id - report container info > > > * @context: task or local context for record > > > > ... > > > > > @@ -2705,9 +2741,10 @@ int audit_set_contid(struct task_struct *task, u64 > > > contid)> > > > if (!ab) > > > > > > return rc; > > > > > > - audit_log_format(ab, > > > - "op=set opid=%d contid=%llu old-contid=%llu", > > > - task_tgid_nr(task), contid, oldcontid); > > > + audit_log_format(ab, "op=set opid=%d contid=", > > > task_tgid_nr(task)); + audit_log_contid(ab, contid); > > > + audit_log_format(ab, " old-contid="); > > > + audit_log_contid(ab, oldcontid); > > > > This is an interesting case where contid and old-contid are going to > > be largely the same, only the first (current) ID is going to be > > different; do we want to duplicate all of those IDs? > > > > > audit_log_end(ab); > > > return rc; > > > > > > } > > > > > > @@ -2723,9 +2760,9 @@ void audit_log_container_drop(void) > > > > paul moore - RGB -- Richard Guy Briggs <rgb@redhat.com> Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada IRC: rgb, SunRaycer Voice: +1.647.777.2635, Internal: (81) 32635
On Tuesday, February 4, 2020 8:19:44 AM EST Richard Guy Briggs wrote: > > The established pattern is that we print -1 when its unset and "?" when > > its totalling missing. So, how could this be invalid? It should be set > > or not. That is unless its totally missing just like when we do not run > > with selinux enabled and a context just doesn't exist. > > Ok, so in this case it is clearly unset, so should be -1, which will be a > 20-digit number when represented as an unsigned long long int. > > Thank you for that clarification Steve. It is literally a -1. ( 2 characters) -Steve
On Tue, Feb 4, 2020 at 10:47 AM Steve Grubb <sgrubb@redhat.com> wrote: > On Tuesday, February 4, 2020 8:19:44 AM EST Richard Guy Briggs wrote: > > > The established pattern is that we print -1 when its unset and "?" when > > > its totalling missing. So, how could this be invalid? It should be set > > > or not. That is unless its totally missing just like when we do not run > > > with selinux enabled and a context just doesn't exist. > > > > Ok, so in this case it is clearly unset, so should be -1, which will be a > > 20-digit number when represented as an unsigned long long int. > > > > Thank you for that clarification Steve. > > It is literally a -1. ( 2 characters) Well, not as Richard has currently written the code, it is a "%llu". This was why I asked the question I did; if we want the "-1" here we probably want to special case that as I don't think we want to display audit container IDs as signed numbers in general.
On Tuesday, February 4, 2020 10:52:36 AM EST Paul Moore wrote: > On Tue, Feb 4, 2020 at 10:47 AM Steve Grubb <sgrubb@redhat.com> wrote: > > On Tuesday, February 4, 2020 8:19:44 AM EST Richard Guy Briggs wrote: > > > > The established pattern is that we print -1 when its unset and "?" > > > > when > > > > its totalling missing. So, how could this be invalid? It should be > > > > set > > > > or not. That is unless its totally missing just like when we do not > > > > run > > > > with selinux enabled and a context just doesn't exist. > > > > > > Ok, so in this case it is clearly unset, so should be -1, which will be > > > a > > > 20-digit number when represented as an unsigned long long int. > > > > > > Thank you for that clarification Steve. > > > > It is literally a -1. ( 2 characters) > > Well, not as Richard has currently written the code, it is a "%llu". > This was why I asked the question I did; if we want the "-1" here we > probably want to special case that as I don't think we want to display > audit container IDs as signed numbers in general. OK, then go with the long number, we'll fix it in the interpretation. I guess we do the same thing for auid. -Steve
On Tue, Feb 4, 2020 at 1:12 PM Steve Grubb <sgrubb@redhat.com> wrote: > On Tuesday, February 4, 2020 10:52:36 AM EST Paul Moore wrote: > > On Tue, Feb 4, 2020 at 10:47 AM Steve Grubb <sgrubb@redhat.com> wrote: > > > On Tuesday, February 4, 2020 8:19:44 AM EST Richard Guy Briggs wrote: > > > > > The established pattern is that we print -1 when its unset and "?" > > > > > when > > > > > its totalling missing. So, how could this be invalid? It should be > > > > > set > > > > > or not. That is unless its totally missing just like when we do not > > > > > run > > > > > with selinux enabled and a context just doesn't exist. > > > > > > > > Ok, so in this case it is clearly unset, so should be -1, which will be > > > > a > > > > 20-digit number when represented as an unsigned long long int. > > > > > > > > Thank you for that clarification Steve. > > > > > > It is literally a -1. ( 2 characters) > > > > Well, not as Richard has currently written the code, it is a "%llu". > > This was why I asked the question I did; if we want the "-1" here we > > probably want to special case that as I don't think we want to display > > audit container IDs as signed numbers in general. > > OK, then go with the long number, we'll fix it in the interpretation. I guess > we do the same thing for auid. As I said above, I'm okay with a special case handling for unset/"-1" in this case.
On Thu, Jan 30, 2020 at 2:28 PM Richard Guy Briggs <rgb@redhat.com> wrote: > On 2020-01-22 16:29, Paul Moore wrote: > > On Tue, Dec 31, 2019 at 2:51 PM Richard Guy Briggs <rgb@redhat.com> wrote: > > > > > > Track the parent container of a container to be able to filter and > > > report nesting. > > > > > > Now that we have a way to track and check the parent container of a > > > container, modify the contid field format to be able to report that > > > nesting using a carrat ("^") separator to indicate nesting. The > > > original field format was "contid=<contid>" for task-associated records > > > and "contid=<contid>[,<contid>[...]]" for network-namespace-associated > > > records. The new field format is > > > "contid=<contid>[^<contid>[...]][,<contid>[...]]". > > > > Let's make sure we always use a comma as a separator, even when > > recording the parent information, for example: > > "contid=<contid>[,^<contid>[...]][,<contid>[...]]" > > The intent here is to clearly indicate and separate nesting from > parallel use of several containers by one netns. If we do away with > that distinction, then we lose that inheritance accountability and > should really run the list through a "uniq" function to remove the > produced redundancies. This clear inheritance is something Steve was > looking for since tracking down individual events/records to show that > inheritance was not aways feasible due to rolled logs or search effort. Perhaps my example wasn't clear. I'm not opposed to the little carat/hat character indicating a container's parent, I just think it would be good to also include a comma *in*addition* to the carat/hat. > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com> > > > --- > > > include/linux/audit.h | 1 + > > > kernel/audit.c | 53 +++++++++++++++++++++++++++++++++++++++++++-------- > > > kernel/audit.h | 1 + > > > kernel/auditfilter.c | 17 ++++++++++++++++- > > > kernel/auditsc.c | 2 +- > > > 5 files changed, 64 insertions(+), 10 deletions(-) > > > > ... > > > > > diff --git a/kernel/audit.c b/kernel/audit.c > > > index ef8e07524c46..68be59d1a89b 100644 > > > --- a/kernel/audit.c > > > +++ b/kernel/audit.c > > > > > @@ -492,6 +493,7 @@ void audit_switch_task_namespaces(struct nsproxy *ns, struct task_struct *p) > > > audit_netns_contid_add(new->net_ns, contid); > > > } > > > > > > +void audit_log_contid(struct audit_buffer *ab, u64 contid); > > > > If we need a forward declaration, might as well just move it up near > > the top of the file with the rest of the declarations. > > Ok. > > > > +void audit_log_contid(struct audit_buffer *ab, u64 contid) > > > +{ > > > + struct audit_contobj *cont = NULL, *prcont = NULL; > > > + int h; > > > > It seems safer to pass the audit container ID object and not the u64. > > It would also be faster, but in some places it isn't available such as > for ptrace and signal targets. This also links back to the drop record > refcounts to hold onto the contobj until process exit, or signal > delivery. > > What we could do is to supply two potential parameters, a contobj and/or > a contid, and have it use the contobj if it is valid, otherwise, use the > contid, as is done for names and paths supplied to audit_log_name(). Let's not do multiple parameters, that begs for misuse, let's take the wrapper function route: func a(int id) { // important stuff } func ao(struct obj) { a(obj.id); } ... and we can add a comment that you *really* should be using the variant that passes an object. > > > @@ -2705,9 +2741,10 @@ int audit_set_contid(struct task_struct *task, u64 contid) > > > if (!ab) > > > return rc; > > > > > > - audit_log_format(ab, > > > - "op=set opid=%d contid=%llu old-contid=%llu", > > > - task_tgid_nr(task), contid, oldcontid); > > > + audit_log_format(ab, "op=set opid=%d contid=", task_tgid_nr(task)); > > > + audit_log_contid(ab, contid); > > > + audit_log_format(ab, " old-contid="); > > > + audit_log_contid(ab, oldcontid); > > > > This is an interesting case where contid and old-contid are going to > > be largely the same, only the first (current) ID is going to be > > different; do we want to duplicate all of those IDs? > > At first when I read your comment, I thought we could just take contid > and drop oldcontid, but if it fails, we still want all the information, > so given the way I've set up the search code in userspace, listing only > the newest contid in the contid field and all the rest in oldcontid > could be a good compromise. This is along the lines of what I was thinking.
On 2020-02-05 18:05, Paul Moore wrote: > On Thu, Jan 30, 2020 at 2:28 PM Richard Guy Briggs <rgb@redhat.com> wrote: > > On 2020-01-22 16:29, Paul Moore wrote: > > > On Tue, Dec 31, 2019 at 2:51 PM Richard Guy Briggs <rgb@redhat.com> wrote: > > > > > > > > Track the parent container of a container to be able to filter and > > > > report nesting. > > > > > > > > Now that we have a way to track and check the parent container of a > > > > container, modify the contid field format to be able to report that > > > > nesting using a carrat ("^") separator to indicate nesting. The > > > > original field format was "contid=<contid>" for task-associated records > > > > and "contid=<contid>[,<contid>[...]]" for network-namespace-associated > > > > records. The new field format is > > > > "contid=<contid>[^<contid>[...]][,<contid>[...]]". > > > > > > Let's make sure we always use a comma as a separator, even when > > > recording the parent information, for example: > > > "contid=<contid>[,^<contid>[...]][,<contid>[...]]" > > > > The intent here is to clearly indicate and separate nesting from > > parallel use of several containers by one netns. If we do away with > > that distinction, then we lose that inheritance accountability and > > should really run the list through a "uniq" function to remove the > > produced redundancies. This clear inheritance is something Steve was > > looking for since tracking down individual events/records to show that > > inheritance was not aways feasible due to rolled logs or search effort. > > Perhaps my example wasn't clear. I'm not opposed to the little > carat/hat character indicating a container's parent, I just think it > would be good to also include a comma *in*addition* to the carat/hat. Ah, ok. Well, I'd offer that it would be slightly shorter, slightly less cluttered and having already written the parser in userspace, I think the parser would be slightly simpler. I must admit, I was a bit puzzled by your snippet of code that was used as a prefix to the next item rather than as a postfix to the given item. Can you say why you prefer the comma in addition? > > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com> > > > > --- > > > > include/linux/audit.h | 1 + > > > > kernel/audit.c | 53 +++++++++++++++++++++++++++++++++++++++++++-------- > > > > kernel/audit.h | 1 + > > > > kernel/auditfilter.c | 17 ++++++++++++++++- > > > > kernel/auditsc.c | 2 +- > > > > 5 files changed, 64 insertions(+), 10 deletions(-) > > > > > > ... > > > > > > > diff --git a/kernel/audit.c b/kernel/audit.c > > > > index ef8e07524c46..68be59d1a89b 100644 > > > > --- a/kernel/audit.c > > > > +++ b/kernel/audit.c > > > > > > > @@ -492,6 +493,7 @@ void audit_switch_task_namespaces(struct nsproxy *ns, struct task_struct *p) > > > > audit_netns_contid_add(new->net_ns, contid); > > > > } > > > > > > > > +void audit_log_contid(struct audit_buffer *ab, u64 contid); > > > > > > If we need a forward declaration, might as well just move it up near > > > the top of the file with the rest of the declarations. > > > > Ok. > > > > > > +void audit_log_contid(struct audit_buffer *ab, u64 contid) > > > > +{ > > > > + struct audit_contobj *cont = NULL, *prcont = NULL; > > > > + int h; > > > > > > It seems safer to pass the audit container ID object and not the u64. > > > > It would also be faster, but in some places it isn't available such as > > for ptrace and signal targets. This also links back to the drop record > > refcounts to hold onto the contobj until process exit, or signal > > delivery. > > > > What we could do is to supply two potential parameters, a contobj and/or > > a contid, and have it use the contobj if it is valid, otherwise, use the > > contid, as is done for names and paths supplied to audit_log_name(). > > Let's not do multiple parameters, that begs for misuse, let's take the > wrapper function route: > > func a(int id) { > // important stuff > } > > func ao(struct obj) { > a(obj.id); > } > > ... and we can add a comment that you *really* should be using the > variant that passes an object. I was already doing that where it available, and dereferencing the id for the call. But I see an advantage to having both parameters supplied to the function, since it saves us the trouble of dereferencing it, searching for the id in the hash list and re-locating the object if the object is already available. > > > > @@ -2705,9 +2741,10 @@ int audit_set_contid(struct task_struct *task, u64 contid) > > > > if (!ab) > > > > return rc; > > > > > > > > - audit_log_format(ab, > > > > - "op=set opid=%d contid=%llu old-contid=%llu", > > > > - task_tgid_nr(task), contid, oldcontid); > > > > + audit_log_format(ab, "op=set opid=%d contid=", task_tgid_nr(task)); > > > > + audit_log_contid(ab, contid); > > > > + audit_log_format(ab, " old-contid="); > > > > + audit_log_contid(ab, oldcontid); > > > > > > This is an interesting case where contid and old-contid are going to > > > be largely the same, only the first (current) ID is going to be > > > different; do we want to duplicate all of those IDs? > > > > At first when I read your comment, I thought we could just take contid > > and drop oldcontid, but if it fails, we still want all the information, > > so given the way I've set up the search code in userspace, listing only > > the newest contid in the contid field and all the rest in oldcontid > > could be a good compromise. > > This is along the lines of what I was thinking. Good. > paul moore - RGB -- Richard Guy Briggs <rgb@redhat.com> Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada IRC: rgb, SunRaycer Voice: +1.647.777.2635, Internal: (81) 32635
On Wed, Feb 5, 2020 at 6:51 PM Richard Guy Briggs <rgb@redhat.com> wrote: > On 2020-02-05 18:05, Paul Moore wrote: > > On Thu, Jan 30, 2020 at 2:28 PM Richard Guy Briggs <rgb@redhat.com> wrote: > > > On 2020-01-22 16:29, Paul Moore wrote: > > > > On Tue, Dec 31, 2019 at 2:51 PM Richard Guy Briggs <rgb@redhat.com> wrote: > > > > > > > > > > Track the parent container of a container to be able to filter and > > > > > report nesting. > > > > > > > > > > Now that we have a way to track and check the parent container of a > > > > > container, modify the contid field format to be able to report that > > > > > nesting using a carrat ("^") separator to indicate nesting. The > > > > > original field format was "contid=<contid>" for task-associated records > > > > > and "contid=<contid>[,<contid>[...]]" for network-namespace-associated > > > > > records. The new field format is > > > > > "contid=<contid>[^<contid>[...]][,<contid>[...]]". > > > > > > > > Let's make sure we always use a comma as a separator, even when > > > > recording the parent information, for example: > > > > "contid=<contid>[,^<contid>[...]][,<contid>[...]]" > > > > > > The intent here is to clearly indicate and separate nesting from > > > parallel use of several containers by one netns. If we do away with > > > that distinction, then we lose that inheritance accountability and > > > should really run the list through a "uniq" function to remove the > > > produced redundancies. This clear inheritance is something Steve was > > > looking for since tracking down individual events/records to show that > > > inheritance was not aways feasible due to rolled logs or search effort. > > > > Perhaps my example wasn't clear. I'm not opposed to the little > > carat/hat character indicating a container's parent, I just think it > > would be good to also include a comma *in*addition* to the carat/hat. > > Ah, ok. Well, I'd offer that it would be slightly shorter, slightly > less cluttered and having already written the parser in userspace, I > think the parser would be slightly simpler. > > I must admit, I was a bit puzzled by your snippet of code that was used > as a prefix to the next item rather than as a postfix to the given item. > > Can you say why you prefer the comma in addition? Generally speaking, I believe that a single delimiter is both easier for the eyes to parse, and easier/safer for machines to parse as well. In this particular case I think of the comma as a delimiter and the carat as a modifier, reusing the carat as a delimiter seems like a bad idea to me. > > > > > diff --git a/kernel/audit.c b/kernel/audit.c > > > > > index ef8e07524c46..68be59d1a89b 100644 > > > > > --- a/kernel/audit.c > > > > > +++ b/kernel/audit.c > > > > > > > > > @@ -492,6 +493,7 @@ void audit_switch_task_namespaces(struct nsproxy *ns, struct task_struct *p) > > > > > audit_netns_contid_add(new->net_ns, contid); > > > > > } > > > > > > > > > > +void audit_log_contid(struct audit_buffer *ab, u64 contid); > > > > > > > > If we need a forward declaration, might as well just move it up near > > > > the top of the file with the rest of the declarations. > > > > > > Ok. > > > > > > > > +void audit_log_contid(struct audit_buffer *ab, u64 contid) > > > > > +{ > > > > > + struct audit_contobj *cont = NULL, *prcont = NULL; > > > > > + int h; > > > > > > > > It seems safer to pass the audit container ID object and not the u64. > > > > > > It would also be faster, but in some places it isn't available such as > > > for ptrace and signal targets. This also links back to the drop record > > > refcounts to hold onto the contobj until process exit, or signal > > > delivery. > > > > > > What we could do is to supply two potential parameters, a contobj and/or > > > a contid, and have it use the contobj if it is valid, otherwise, use the > > > contid, as is done for names and paths supplied to audit_log_name(). > > > > Let's not do multiple parameters, that begs for misuse, let's take the > > wrapper function route: > > > > func a(int id) { > > // important stuff > > } > > > > func ao(struct obj) { > > a(obj.id); > > } > > > > ... and we can add a comment that you *really* should be using the > > variant that passes an object. > > I was already doing that where it available, and dereferencing the id > for the call. But I see an advantage to having both parameters supplied > to the function, since it saves us the trouble of dereferencing it, > searching for the id in the hash list and re-locating the object if the > object is already available. I strongly prefer we not do multiple parameters for the same "thing"; I would much rather do the wrapper approach as described above. I would also like to see us use the audit container ID object as much as possible, using a bare integer should be a last resort.
On 2020-02-13 16:49, Paul Moore wrote: > On Wed, Feb 5, 2020 at 6:51 PM Richard Guy Briggs <rgb@redhat.com> wrote: > > On 2020-02-05 18:05, Paul Moore wrote: > > > On Thu, Jan 30, 2020 at 2:28 PM Richard Guy Briggs <rgb@redhat.com> wrote: > > > > On 2020-01-22 16:29, Paul Moore wrote: > > > > > On Tue, Dec 31, 2019 at 2:51 PM Richard Guy Briggs <rgb@redhat.com> wrote: > > > > > > > > > > > > Track the parent container of a container to be able to filter and > > > > > > report nesting. > > > > > > > > > > > > Now that we have a way to track and check the parent container of a > > > > > > container, modify the contid field format to be able to report that > > > > > > nesting using a carrat ("^") separator to indicate nesting. The > > > > > > original field format was "contid=<contid>" for task-associated records > > > > > > and "contid=<contid>[,<contid>[...]]" for network-namespace-associated > > > > > > records. The new field format is > > > > > > "contid=<contid>[^<contid>[...]][,<contid>[...]]". > > > > > > > > > > Let's make sure we always use a comma as a separator, even when > > > > > recording the parent information, for example: > > > > > "contid=<contid>[,^<contid>[...]][,<contid>[...]]" > > > > > > > > The intent here is to clearly indicate and separate nesting from > > > > parallel use of several containers by one netns. If we do away with > > > > that distinction, then we lose that inheritance accountability and > > > > should really run the list through a "uniq" function to remove the > > > > produced redundancies. This clear inheritance is something Steve was > > > > looking for since tracking down individual events/records to show that > > > > inheritance was not aways feasible due to rolled logs or search effort. > > > > > > Perhaps my example wasn't clear. I'm not opposed to the little > > > carat/hat character indicating a container's parent, I just think it > > > would be good to also include a comma *in*addition* to the carat/hat. > > > > Ah, ok. Well, I'd offer that it would be slightly shorter, slightly > > less cluttered and having already written the parser in userspace, I > > think the parser would be slightly simpler. > > > > I must admit, I was a bit puzzled by your snippet of code that was used > > as a prefix to the next item rather than as a postfix to the given item. > > > > Can you say why you prefer the comma in addition? > > Generally speaking, I believe that a single delimiter is both easier > for the eyes to parse, and easier/safer for machines to parse as well. > In this particular case I think of the comma as a delimiter and the > carat as a modifier, reusing the carat as a delimiter seems like a bad > idea to me. I'm not crazy about this idea, but I'll have a look at how much work it is to recode the userspace search tools. It also adds extra characters and noise into the string format that seems counterproductive. > > > > > > diff --git a/kernel/audit.c b/kernel/audit.c > > > > > > index ef8e07524c46..68be59d1a89b 100644 > > > > > > --- a/kernel/audit.c > > > > > > +++ b/kernel/audit.c > > > > > > > > > > > @@ -492,6 +493,7 @@ void audit_switch_task_namespaces(struct nsproxy *ns, struct task_struct *p) > > > > > > audit_netns_contid_add(new->net_ns, contid); > > > > > > } > > > > > > > > > > > > +void audit_log_contid(struct audit_buffer *ab, u64 contid); > > > > > > > > > > If we need a forward declaration, might as well just move it up near > > > > > the top of the file with the rest of the declarations. > > > > > > > > Ok. > > > > > > > > > > +void audit_log_contid(struct audit_buffer *ab, u64 contid) > > > > > > +{ > > > > > > + struct audit_contobj *cont = NULL, *prcont = NULL; > > > > > > + int h; > > > > > > > > > > It seems safer to pass the audit container ID object and not the u64. > > > > > > > > It would also be faster, but in some places it isn't available such as > > > > for ptrace and signal targets. This also links back to the drop record > > > > refcounts to hold onto the contobj until process exit, or signal > > > > delivery. > > > > > > > > What we could do is to supply two potential parameters, a contobj and/or > > > > a contid, and have it use the contobj if it is valid, otherwise, use the > > > > contid, as is done for names and paths supplied to audit_log_name(). > > > > > > Let's not do multiple parameters, that begs for misuse, let's take the > > > wrapper function route: > > > > > > func a(int id) { > > > // important stuff > > > } > > > > > > func ao(struct obj) { > > > a(obj.id); > > > } > > > > > > ... and we can add a comment that you *really* should be using the > > > variant that passes an object. > > > > I was already doing that where it available, and dereferencing the id > > for the call. But I see an advantage to having both parameters supplied > > to the function, since it saves us the trouble of dereferencing it, > > searching for the id in the hash list and re-locating the object if the > > object is already available. > > I strongly prefer we not do multiple parameters for the same "thing"; So do I, ideally. However... > I would much rather do the wrapper approach as described above. I > would also like to see us use the audit container ID object as much as > possible, using a bare integer should be a last resort. It is not clear to me that you understood what I wrote above. I can't use the object pointer where preferable because there are a few cases where only the ID is available. If only the ID is available, I would have to make a best effort to look up the object pointer and am not guaranteed to find it (invalid, stale, signal info...). If I am forced to use only one, it becomes the ID that is used, and I no longer have the benefit of already having the object pointer for certainty and saving work. For all cases where I have the object pointer, which is most cases, and most frequently used cases, I will have to dereference the object pointer to an ID, then go through the work again to re-locate the object pointer. This is less certain, and more work. Reluctantly, the only practical solution I see here is to supply both, favouring the object pointer if it is valid, then falling back on the ID from the next parameter. > paul moore - RGB -- Richard Guy Briggs <rgb@redhat.com> Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada IRC: rgb, SunRaycer Voice: +1.647.777.2635, Internal: (81) 32635
On Thu, Mar 12, 2020 at 4:52 PM Richard Guy Briggs <rgb@redhat.com> wrote: > On 2020-02-13 16:49, Paul Moore wrote: > > On Wed, Feb 5, 2020 at 6:51 PM Richard Guy Briggs <rgb@redhat.com> wrote: > > > On 2020-02-05 18:05, Paul Moore wrote: > > > > On Thu, Jan 30, 2020 at 2:28 PM Richard Guy Briggs <rgb@redhat.com> wrote: > > > > > On 2020-01-22 16:29, Paul Moore wrote: > > > > > > On Tue, Dec 31, 2019 at 2:51 PM Richard Guy Briggs <rgb@redhat.com> wrote: > > > > > > > > > > > > > > Track the parent container of a container to be able to filter and > > > > > > > report nesting. > > > > > > > > > > > > > > Now that we have a way to track and check the parent container of a > > > > > > > container, modify the contid field format to be able to report that > > > > > > > nesting using a carrat ("^") separator to indicate nesting. The > > > > > > > original field format was "contid=<contid>" for task-associated records > > > > > > > and "contid=<contid>[,<contid>[...]]" for network-namespace-associated > > > > > > > records. The new field format is > > > > > > > "contid=<contid>[^<contid>[...]][,<contid>[...]]". > > > > > > > > > > > > Let's make sure we always use a comma as a separator, even when > > > > > > recording the parent information, for example: > > > > > > "contid=<contid>[,^<contid>[...]][,<contid>[...]]" > > > > > > > > > > The intent here is to clearly indicate and separate nesting from > > > > > parallel use of several containers by one netns. If we do away with > > > > > that distinction, then we lose that inheritance accountability and > > > > > should really run the list through a "uniq" function to remove the > > > > > produced redundancies. This clear inheritance is something Steve was > > > > > looking for since tracking down individual events/records to show that > > > > > inheritance was not aways feasible due to rolled logs or search effort. > > > > > > > > Perhaps my example wasn't clear. I'm not opposed to the little > > > > carat/hat character indicating a container's parent, I just think it > > > > would be good to also include a comma *in*addition* to the carat/hat. > > > > > > Ah, ok. Well, I'd offer that it would be slightly shorter, slightly > > > less cluttered and having already written the parser in userspace, I > > > think the parser would be slightly simpler. > > > > > > I must admit, I was a bit puzzled by your snippet of code that was used > > > as a prefix to the next item rather than as a postfix to the given item. > > > > > > Can you say why you prefer the comma in addition? > > > > Generally speaking, I believe that a single delimiter is both easier > > for the eyes to parse, and easier/safer for machines to parse as well. > > In this particular case I think of the comma as a delimiter and the > > carat as a modifier, reusing the carat as a delimiter seems like a bad > > idea to me. > > I'm not crazy about this idea, but I'll have a look at how much work it > is to recode the userspace search tools. It also adds extra characters > and noise into the string format that seems counterproductive. If anything the parser should be *easier* (although both parsers should fall into the "trivial" category). The comma is the one and only delimiter, and if the ACID starts with a carat then it is a parent of the preceding ACID. > > > > > > > diff --git a/kernel/audit.c b/kernel/audit.c > > > > > > > index ef8e07524c46..68be59d1a89b 100644 > > > > > > > --- a/kernel/audit.c > > > > > > > +++ b/kernel/audit.c > > > > > > > > > > > > > @@ -492,6 +493,7 @@ void audit_switch_task_namespaces(struct nsproxy *ns, struct task_struct *p) > > > > > > > audit_netns_contid_add(new->net_ns, contid); > > > > > > > } > > > > > > > > > > > > > > +void audit_log_contid(struct audit_buffer *ab, u64 contid); > > > > > > > > > > > > If we need a forward declaration, might as well just move it up near > > > > > > the top of the file with the rest of the declarations. > > > > > > > > > > Ok. > > > > > > > > > > > > +void audit_log_contid(struct audit_buffer *ab, u64 contid) > > > > > > > +{ > > > > > > > + struct audit_contobj *cont = NULL, *prcont = NULL; > > > > > > > + int h; > > > > > > > > > > > > It seems safer to pass the audit container ID object and not the u64. > > > > > > > > > > It would also be faster, but in some places it isn't available such as > > > > > for ptrace and signal targets. This also links back to the drop record > > > > > refcounts to hold onto the contobj until process exit, or signal > > > > > delivery. > > > > > > > > > > What we could do is to supply two potential parameters, a contobj and/or > > > > > a contid, and have it use the contobj if it is valid, otherwise, use the > > > > > contid, as is done for names and paths supplied to audit_log_name(). > > > > > > > > Let's not do multiple parameters, that begs for misuse, let's take the > > > > wrapper function route: > > > > > > > > func a(int id) { > > > > // important stuff > > > > } > > > > > > > > func ao(struct obj) { > > > > a(obj.id); > > > > } > > > > > > > > ... and we can add a comment that you *really* should be using the > > > > variant that passes an object. > > > > > > I was already doing that where it available, and dereferencing the id > > > for the call. But I see an advantage to having both parameters supplied > > > to the function, since it saves us the trouble of dereferencing it, > > > searching for the id in the hash list and re-locating the object if the > > > object is already available. > > > > I strongly prefer we not do multiple parameters for the same "thing"; > > So do I, ideally. However... > > > I would much rather do the wrapper approach as described above. I > > would also like to see us use the audit container ID object as much as > > possible, using a bare integer should be a last resort. > > It is not clear to me that you understood what I wrote above. I can't > use the object pointer where preferable because there are a few cases > where only the ID is available. If only the ID is available, I would > have to make a best effort to look up the object pointer and am not > guaranteed to find it (invalid, stale, signal info...). If I am forced > to use only one, it becomes the ID that is used, and I no longer have > the benefit of already having the object pointer for certainty and > saving work. For all cases where I have the object pointer, which is > most cases, and most frequently used cases, I will have to dereference > the object pointer to an ID, then go through the work again to re-locate > the object pointer. This is less certain, and more work. Reluctantly, > the only practical solution I see here is to supply both, favouring the > object pointer if it is valid, then falling back on the ID from the next > parameter. It has been a while since I last looked at the patchset, but my concern over the prefered use of the ACID number vs the ACID object is that the number offers no reuse protection where the object does. I really would like us to use the object everywhere it is possible.
On 2020-03-13 12:47, Paul Moore wrote: > On Thu, Mar 12, 2020 at 4:52 PM Richard Guy Briggs <rgb@redhat.com> wrote: > > On 2020-02-13 16:49, Paul Moore wrote: > > > On Wed, Feb 5, 2020 at 6:51 PM Richard Guy Briggs <rgb@redhat.com> wrote: > > > > On 2020-02-05 18:05, Paul Moore wrote: > > > > > On Thu, Jan 30, 2020 at 2:28 PM Richard Guy Briggs <rgb@redhat.com> wrote: > > > > > > On 2020-01-22 16:29, Paul Moore wrote: > > > > > > > On Tue, Dec 31, 2019 at 2:51 PM Richard Guy Briggs <rgb@redhat.com> wrote: > > > > > > > > > > > > > > > > Track the parent container of a container to be able to filter and > > > > > > > > report nesting. > > > > > > > > > > > > > > > > Now that we have a way to track and check the parent container of a > > > > > > > > container, modify the contid field format to be able to report that > > > > > > > > nesting using a carrat ("^") separator to indicate nesting. The > > > > > > > > original field format was "contid=<contid>" for task-associated records > > > > > > > > and "contid=<contid>[,<contid>[...]]" for network-namespace-associated > > > > > > > > records. The new field format is > > > > > > > > "contid=<contid>[^<contid>[...]][,<contid>[...]]". > > > > > > > > > > > > > > Let's make sure we always use a comma as a separator, even when > > > > > > > recording the parent information, for example: > > > > > > > "contid=<contid>[,^<contid>[...]][,<contid>[...]]" > > > > > > > > > > > > The intent here is to clearly indicate and separate nesting from > > > > > > parallel use of several containers by one netns. If we do away with > > > > > > that distinction, then we lose that inheritance accountability and > > > > > > should really run the list through a "uniq" function to remove the > > > > > > produced redundancies. This clear inheritance is something Steve was > > > > > > looking for since tracking down individual events/records to show that > > > > > > inheritance was not aways feasible due to rolled logs or search effort. > > > > > > > > > > Perhaps my example wasn't clear. I'm not opposed to the little > > > > > carat/hat character indicating a container's parent, I just think it > > > > > would be good to also include a comma *in*addition* to the carat/hat. > > > > > > > > Ah, ok. Well, I'd offer that it would be slightly shorter, slightly > > > > less cluttered and having already written the parser in userspace, I > > > > think the parser would be slightly simpler. > > > > > > > > I must admit, I was a bit puzzled by your snippet of code that was used > > > > as a prefix to the next item rather than as a postfix to the given item. > > > > > > > > Can you say why you prefer the comma in addition? > > > > > > Generally speaking, I believe that a single delimiter is both easier > > > for the eyes to parse, and easier/safer for machines to parse as well. > > > In this particular case I think of the comma as a delimiter and the > > > carat as a modifier, reusing the carat as a delimiter seems like a bad > > > idea to me. > > > > I'm not crazy about this idea, but I'll have a look at how much work it > > is to recode the userspace search tools. It also adds extra characters > > and noise into the string format that seems counterproductive. > > If anything the parser should be *easier* (although both parsers > should fall into the "trivial" category). The comma is the one and > only delimiter, and if the ACID starts with a carat then it is a > parent of the preceding ACID. Ok, after a day of staring at the code and getting nowhere due to multiple distractions, I was able to rework this code fairly easily and it turned out simpler which should not surprise you. Both kernel and userspace code are now in the format you recommended. > > > > > > > > diff --git a/kernel/audit.c b/kernel/audit.c > > > > > > > > index ef8e07524c46..68be59d1a89b 100644 > > > > > > > > --- a/kernel/audit.c > > > > > > > > +++ b/kernel/audit.c > > > > > > > > > > > > > > > @@ -492,6 +493,7 @@ void audit_switch_task_namespaces(struct nsproxy *ns, struct task_struct *p) > > > > > > > > audit_netns_contid_add(new->net_ns, contid); > > > > > > > > } > > > > > > > > > > > > > > > > +void audit_log_contid(struct audit_buffer *ab, u64 contid); > > > > > > > > > > > > > > If we need a forward declaration, might as well just move it up near > > > > > > > the top of the file with the rest of the declarations. > > > > > > > > > > > > Ok. > > > > > > > > > > > > > > +void audit_log_contid(struct audit_buffer *ab, u64 contid) > > > > > > > > +{ > > > > > > > > + struct audit_contobj *cont = NULL, *prcont = NULL; > > > > > > > > + int h; > > > > > > > > > > > > > > It seems safer to pass the audit container ID object and not the u64. > > > > > > > > > > > > It would also be faster, but in some places it isn't available such as > > > > > > for ptrace and signal targets. This also links back to the drop record > > > > > > refcounts to hold onto the contobj until process exit, or signal > > > > > > delivery. > > > > > > > > > > > > What we could do is to supply two potential parameters, a contobj and/or > > > > > > a contid, and have it use the contobj if it is valid, otherwise, use the > > > > > > contid, as is done for names and paths supplied to audit_log_name(). > > > > > > > > > > Let's not do multiple parameters, that begs for misuse, let's take the > > > > > wrapper function route: > > > > > > > > > > func a(int id) { > > > > > // important stuff > > > > > } > > > > > > > > > > func ao(struct obj) { > > > > > a(obj.id); > > > > > } > > > > > > > > > > ... and we can add a comment that you *really* should be using the > > > > > variant that passes an object. > > > > > > > > I was already doing that where it available, and dereferencing the id > > > > for the call. But I see an advantage to having both parameters supplied > > > > to the function, since it saves us the trouble of dereferencing it, > > > > searching for the id in the hash list and re-locating the object if the > > > > object is already available. > > > > > > I strongly prefer we not do multiple parameters for the same "thing"; > > > > So do I, ideally. However... > > > > > I would much rather do the wrapper approach as described above. I > > > would also like to see us use the audit container ID object as much as > > > possible, using a bare integer should be a last resort. > > > > It is not clear to me that you understood what I wrote above. I can't > > use the object pointer where preferable because there are a few cases > > where only the ID is available. If only the ID is available, I would > > have to make a best effort to look up the object pointer and am not > > guaranteed to find it (invalid, stale, signal info...). If I am forced > > to use only one, it becomes the ID that is used, and I no longer have > > the benefit of already having the object pointer for certainty and > > saving work. For all cases where I have the object pointer, which is > > most cases, and most frequently used cases, I will have to dereference > > the object pointer to an ID, then go through the work again to re-locate > > the object pointer. This is less certain, and more work. Reluctantly, > > the only practical solution I see here is to supply both, favouring the > > object pointer if it is valid, then falling back on the ID from the next > > parameter. > > It has been a while since I last looked at the patchset, but my > concern over the prefered use of the ACID number vs the ACID object is > that the number offers no reuse protection where the object does. I > really would like us to use the object everywhere it is possible. Ok, so I take it from this that I go ahead with the dual format since the wrapper funciton to convert from object to ID strips away object information negating any benefit of favouring the object pointer. I'll look at the remaining calls that use a contid (rather than contobj) and convert all that I can over to storing an object using the dual counters that track process exits versus signal2 and trace references. > paul moore - RGB -- Richard Guy Briggs <rgb@redhat.com> Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada IRC: rgb, SunRaycer Voice: +1.647.777.2635, Internal: (81) 32635
On 2020-03-14 18:42, Richard Guy Briggs wrote: > On 2020-03-13 12:47, Paul Moore wrote: > > On Thu, Mar 12, 2020 at 4:52 PM Richard Guy Briggs <rgb@redhat.com> wrote: > > > On 2020-02-13 16:49, Paul Moore wrote: > > > > On Wed, Feb 5, 2020 at 6:51 PM Richard Guy Briggs <rgb@redhat.com> wrote: > > > > > On 2020-02-05 18:05, Paul Moore wrote: > > > > > > On Thu, Jan 30, 2020 at 2:28 PM Richard Guy Briggs <rgb@redhat.com> wrote: > > > > > > > On 2020-01-22 16:29, Paul Moore wrote: > > > > > > > > On Tue, Dec 31, 2019 at 2:51 PM Richard Guy Briggs <rgb@redhat.com> wrote: > > > > > > > > > > > > > > > > > > Track the parent container of a container to be able to filter and > > > > > > > > > report nesting. > > > > > > > > > > > > > > > > > > Now that we have a way to track and check the parent container of a > > > > > > > > > container, modify the contid field format to be able to report that > > > > > > > > > nesting using a carrat ("^") separator to indicate nesting. The > > > > > > > > > original field format was "contid=<contid>" for task-associated records > > > > > > > > > and "contid=<contid>[,<contid>[...]]" for network-namespace-associated > > > > > > > > > records. The new field format is > > > > > > > > > "contid=<contid>[^<contid>[...]][,<contid>[...]]". > > > > > > > > > > > > > > > > Let's make sure we always use a comma as a separator, even when > > > > > > > > recording the parent information, for example: > > > > > > > > "contid=<contid>[,^<contid>[...]][,<contid>[...]]" > > > > > > > > > > > > > > The intent here is to clearly indicate and separate nesting from > > > > > > > parallel use of several containers by one netns. If we do away with > > > > > > > that distinction, then we lose that inheritance accountability and > > > > > > > should really run the list through a "uniq" function to remove the > > > > > > > produced redundancies. This clear inheritance is something Steve was > > > > > > > looking for since tracking down individual events/records to show that > > > > > > > inheritance was not aways feasible due to rolled logs or search effort. > > > > > > > > > > > > Perhaps my example wasn't clear. I'm not opposed to the little > > > > > > carat/hat character indicating a container's parent, I just think it > > > > > > would be good to also include a comma *in*addition* to the carat/hat. > > > > > > > > > > Ah, ok. Well, I'd offer that it would be slightly shorter, slightly > > > > > less cluttered and having already written the parser in userspace, I > > > > > think the parser would be slightly simpler. > > > > > > > > > > I must admit, I was a bit puzzled by your snippet of code that was used > > > > > as a prefix to the next item rather than as a postfix to the given item. > > > > > > > > > > Can you say why you prefer the comma in addition? > > > > > > > > Generally speaking, I believe that a single delimiter is both easier > > > > for the eyes to parse, and easier/safer for machines to parse as well. > > > > In this particular case I think of the comma as a delimiter and the > > > > carat as a modifier, reusing the carat as a delimiter seems like a bad > > > > idea to me. > > > > > > I'm not crazy about this idea, but I'll have a look at how much work it > > > is to recode the userspace search tools. It also adds extra characters > > > and noise into the string format that seems counterproductive. > > > > If anything the parser should be *easier* (although both parsers > > should fall into the "trivial" category). The comma is the one and > > only delimiter, and if the ACID starts with a carat then it is a > > parent of the preceding ACID. > > Ok, after a day of staring at the code and getting nowhere due to > multiple distractions, I was able to rework this code fairly easily and > it turned out simpler which should not surprise you. Both kernel and > userspace code are now in the format you recommended. > > > > > > > > > > diff --git a/kernel/audit.c b/kernel/audit.c > > > > > > > > > index ef8e07524c46..68be59d1a89b 100644 > > > > > > > > > --- a/kernel/audit.c > > > > > > > > > +++ b/kernel/audit.c > > > > > > > > > > > > > > > > > @@ -492,6 +493,7 @@ void audit_switch_task_namespaces(struct nsproxy *ns, struct task_struct *p) > > > > > > > > > audit_netns_contid_add(new->net_ns, contid); > > > > > > > > > } > > > > > > > > > > > > > > > > > > +void audit_log_contid(struct audit_buffer *ab, u64 contid); > > > > > > > > > > > > > > > > If we need a forward declaration, might as well just move it up near > > > > > > > > the top of the file with the rest of the declarations. > > > > > > > > > > > > > > Ok. > > > > > > > > > > > > > > > > +void audit_log_contid(struct audit_buffer *ab, u64 contid) > > > > > > > > > +{ > > > > > > > > > + struct audit_contobj *cont = NULL, *prcont = NULL; > > > > > > > > > + int h; > > > > > > > > > > > > > > > > It seems safer to pass the audit container ID object and not the u64. > > > > > > > > > > > > > > It would also be faster, but in some places it isn't available such as > > > > > > > for ptrace and signal targets. This also links back to the drop record > > > > > > > refcounts to hold onto the contobj until process exit, or signal > > > > > > > delivery. > > > > > > > > > > > > > > What we could do is to supply two potential parameters, a contobj and/or > > > > > > > a contid, and have it use the contobj if it is valid, otherwise, use the > > > > > > > contid, as is done for names and paths supplied to audit_log_name(). > > > > > > > > > > > > Let's not do multiple parameters, that begs for misuse, let's take the > > > > > > wrapper function route: > > > > > > > > > > > > func a(int id) { > > > > > > // important stuff > > > > > > } > > > > > > > > > > > > func ao(struct obj) { > > > > > > a(obj.id); > > > > > > } > > > > > > > > > > > > ... and we can add a comment that you *really* should be using the > > > > > > variant that passes an object. > > > > > > > > > > I was already doing that where it available, and dereferencing the id > > > > > for the call. But I see an advantage to having both parameters supplied > > > > > to the function, since it saves us the trouble of dereferencing it, > > > > > searching for the id in the hash list and re-locating the object if the > > > > > object is already available. > > > > > > > > I strongly prefer we not do multiple parameters for the same "thing"; > > > > > > So do I, ideally. However... > > > > > > > I would much rather do the wrapper approach as described above. I > > > > would also like to see us use the audit container ID object as much as > > > > possible, using a bare integer should be a last resort. > > > > > > It is not clear to me that you understood what I wrote above. I can't > > > use the object pointer where preferable because there are a few cases > > > where only the ID is available. If only the ID is available, I would > > > have to make a best effort to look up the object pointer and am not > > > guaranteed to find it (invalid, stale, signal info...). If I am forced > > > to use only one, it becomes the ID that is used, and I no longer have > > > the benefit of already having the object pointer for certainty and > > > saving work. For all cases where I have the object pointer, which is > > > most cases, and most frequently used cases, I will have to dereference > > > the object pointer to an ID, then go through the work again to re-locate > > > the object pointer. This is less certain, and more work. Reluctantly, > > > the only practical solution I see here is to supply both, favouring the > > > object pointer if it is valid, then falling back on the ID from the next > > > parameter. > > > > It has been a while since I last looked at the patchset, but my > > concern over the prefered use of the ACID number vs the ACID object is > > that the number offers no reuse protection where the object does. I > > really would like us to use the object everywhere it is possible. > > Ok, so I take it from this that I go ahead with the dual format since > the wrapper funciton to convert from object to ID strips away object > information negating any benefit of favouring the object pointer. I'll > look at the remaining calls that use a contid (rather than contobj) and > convert all that I can over to storing an object using the dual counters > that track process exits versus signal2 and trace references. After reworking all the signal code to use the contobj and open coding unnested single contid appearances, I was able to stick with just passing a contobj to audit_contiainer_id() and audit_log_contid(), so the dual format conundrum went away. It issues the death certificate on process exit, and will issue an error indicating the contid is dead and can't be reused yet until it is reaped by a sig2 call. > > paul moore > > - RGB - RGB -- Richard Guy Briggs <rgb@redhat.com> Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada IRC: rgb, SunRaycer Voice: +1.647.777.2635, Internal: (81) 32635
On Sat, Mar 14, 2020 at 6:42 PM Richard Guy Briggs <rgb@redhat.com> wrote: > On 2020-03-13 12:47, Paul Moore wrote: ... > > It has been a while since I last looked at the patchset, but my > > concern over the prefered use of the ACID number vs the ACID object is > > that the number offers no reuse protection where the object does. I > > really would like us to use the object everywhere it is possible. > > Ok, so I take it from this that I go ahead with the dual format since > the wrapper funciton to convert from object to ID strips away object > information negating any benefit of favouring the object pointer. I'll > look at the remaining calls that use a contid (rather than contobj) and > convert all that I can over to storing an object using the dual counters > that track process exits versus signal2 and trace references. Well, as I said in the other thread, I'm not sure we need a full two counters; I think one counter and a simple flag should suffice. Otherwise that sounds good for the next iteration.
diff --git a/include/linux/audit.h b/include/linux/audit.h index ed8d5b74758d..4272b468417a 100644 --- a/include/linux/audit.h +++ b/include/linux/audit.h @@ -109,6 +109,7 @@ struct audit_contobj { struct task_struct *owner; refcount_t refcount; struct rcu_head rcu; + struct audit_contobj *parent; }; struct audit_task_info { diff --git a/kernel/audit.c b/kernel/audit.c index ef8e07524c46..68be59d1a89b 100644 --- a/kernel/audit.c +++ b/kernel/audit.c @@ -251,6 +251,7 @@ static void _audit_contobj_put(struct audit_contobj *cont) return; if (refcount_dec_and_test(&cont->refcount)) { put_task_struct(cont->owner); + _audit_contobj_put(cont->parent); list_del_rcu(&cont->list); kfree_rcu(cont, rcu); } @@ -492,6 +493,7 @@ void audit_switch_task_namespaces(struct nsproxy *ns, struct task_struct *p) audit_netns_contid_add(new->net_ns, contid); } +void audit_log_contid(struct audit_buffer *ab, u64 contid); /** * audit_log_netns_contid_list - List contids for the given network namespace * @net: the network namespace of interest @@ -523,7 +525,7 @@ void audit_log_netns_contid_list(struct net *net, struct audit_context *context) audit_log_format(ab, "contid="); } else audit_log_format(ab, ","); - audit_log_format(ab, "%llu", cont->id); + audit_log_contid(ab, cont->id); } audit_log_end(ab); out: @@ -2311,6 +2313,36 @@ void audit_log_session_info(struct audit_buffer *ab) audit_log_format(ab, "auid=%u ses=%u", auid, sessionid); } +void audit_log_contid(struct audit_buffer *ab, u64 contid) +{ + struct audit_contobj *cont = NULL, *prcont = NULL; + int h; + + if (!audit_contid_valid(contid)) { + audit_log_format(ab, "%llu", contid); + return; + } + h = audit_hash_contid(contid); + rcu_read_lock(); + list_for_each_entry_rcu(cont, &audit_contid_hash[h], list) + if (cont->id == contid) { + prcont = cont; + break; + } + if (!prcont) { + audit_log_format(ab, "%llu", contid); + goto out; + } + while (prcont) { + audit_log_format(ab, "%llu", prcont->id); + prcont = prcont->parent; + if (prcont) + audit_log_format(ab, "^"); + } +out: + rcu_read_unlock(); +} + /* * audit_log_container_id - report container info * @context: task or local context for record @@ -2326,7 +2358,8 @@ void audit_log_container_id(struct audit_context *context, u64 contid) ab = audit_log_start(context, GFP_KERNEL, AUDIT_CONTAINER_ID); if (!ab) return; - audit_log_format(ab, "contid=%llu", contid); + audit_log_format(ab, "contid="); + audit_log_contid(ab, contid); audit_log_end(ab); } EXPORT_SYMBOL(audit_log_container_id); @@ -2675,6 +2708,9 @@ int audit_set_contid(struct task_struct *task, u64 contid) newcont->id = contid; get_task_struct(current); newcont->owner = current; + newcont->parent = _audit_contobj(newcont->owner); + if (newcont->parent) + _audit_contobj_hold(newcont->parent); refcount_set(&newcont->refcount, 1); spin_lock(&audit_contobj_list_lock); list_add_rcu(&newcont->list, &audit_contid_hash[h]); @@ -2705,9 +2741,10 @@ int audit_set_contid(struct task_struct *task, u64 contid) if (!ab) return rc; - audit_log_format(ab, - "op=set opid=%d contid=%llu old-contid=%llu", - task_tgid_nr(task), contid, oldcontid); + audit_log_format(ab, "op=set opid=%d contid=", task_tgid_nr(task)); + audit_log_contid(ab, contid); + audit_log_format(ab, " old-contid="); + audit_log_contid(ab, oldcontid); audit_log_end(ab); return rc; } @@ -2723,9 +2760,9 @@ void audit_log_container_drop(void) if (!ab) return; - audit_log_format(ab, "op=drop opid=%d contid=%llu old-contid=%llu", - task_tgid_nr(current), audit_get_contid(current), - audit_get_contid(current)); + audit_log_format(ab, "op=drop opid=%d contid=%llu old-contid=", + task_tgid_nr(current), AUDIT_CID_UNSET); + audit_log_contid(ab, audit_get_contid(current)); audit_log_end(ab); } diff --git a/kernel/audit.h b/kernel/audit.h index 5e2f5c9820d8..de814fcbb38c 100644 --- a/kernel/audit.h +++ b/kernel/audit.h @@ -226,6 +226,7 @@ static inline int audit_hash_contid(u64 contid) extern int audit_match_class(int class, unsigned syscall); extern int audit_comparator(const u32 left, const u32 op, const u32 right); extern int audit_comparator64(const u64 left, const u32 op, const u64 right); +extern int audit_contid_comparator(const u64 left, const u32 op, const u64 right); extern int audit_uid_comparator(kuid_t left, u32 op, kuid_t right); extern int audit_gid_comparator(kgid_t left, u32 op, kgid_t right); extern int parent_len(const char *path); diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c index 9606f973fe33..1757896740e8 100644 --- a/kernel/auditfilter.c +++ b/kernel/auditfilter.c @@ -1297,6 +1297,21 @@ int audit_gid_comparator(kgid_t left, u32 op, kgid_t right) } } +int audit_contid_comparator(u64 left, u32 op, u64 right) +{ + struct audit_contobj *cont = NULL; + int h; + int result = 0; + + h = audit_hash_contid(left); + list_for_each_entry_rcu(cont, &audit_contid_hash[h], list) { + result = audit_comparator64(cont->id, op, right); + if (result) + break; + } + return result; +} + /** * parent_len - find the length of the parent portion of a pathname * @path: pathname of which to determine length @@ -1388,7 +1403,7 @@ int audit_filter(int msgtype, unsigned int listtype) f->op, f->val); break; case AUDIT_CONTID: - result = audit_comparator64(audit_get_contid(current), + result = audit_contid_comparator(audit_get_contid(current), f->op, f->val64); break; case AUDIT_MSGTYPE: diff --git a/kernel/auditsc.c b/kernel/auditsc.c index a658fe775b86..6bf6d8b9dfd1 100644 --- a/kernel/auditsc.c +++ b/kernel/auditsc.c @@ -630,7 +630,7 @@ static int audit_filter_rules(struct task_struct *tsk, f->op, f->val); break; case AUDIT_CONTID: - result = audit_comparator64(audit_get_contid(tsk), + result = audit_contid_comparator(audit_get_contid(tsk), f->op, f->val64); break; case AUDIT_SUBJ_USER:
Track the parent container of a container to be able to filter and report nesting. Now that we have a way to track and check the parent container of a container, modify the contid field format to be able to report that nesting using a carrat ("^") separator to indicate nesting. The original field format was "contid=<contid>" for task-associated records and "contid=<contid>[,<contid>[...]]" for network-namespace-associated records. The new field format is "contid=<contid>[^<contid>[...]][,<contid>[...]]". Signed-off-by: Richard Guy Briggs <rgb@redhat.com> --- include/linux/audit.h | 1 + kernel/audit.c | 53 +++++++++++++++++++++++++++++++++++++++++++-------- kernel/audit.h | 1 + kernel/auditfilter.c | 17 ++++++++++++++++- kernel/auditsc.c | 2 +- 5 files changed, 64 insertions(+), 10 deletions(-)