Re: [RFC PATCH ghak32 V2 13/13] debug audit: read container ID of a process

[Paul Moore]

On Tue, May 22, 2018 at 1:35 PM, Richard Guy Briggs  wrote:
> On 2018-05-21 16:06, Paul Moore wrote:
>> On Mon, May 21, 2018 at 3:19 PM, Eric W. Biederman  
>> wrote:
>> > Steve Grubb  writes:
>> >> On Friday, March 16, 2018 5:00:40 AM EDT Richard Guy Briggs wrote:
>> >>> Add support for reading the container ID from the proc filesystem.
>> >>
>> >> I think this could be useful in general. Please consider this to be part 
>> >> of
>> >> the full patch set and not something merely used to debug the patches.
>> >
>> > Only with an audit specific name.
>> >
>> > As it is:
>> >
>> > Nacked-by: "Eric W. Biederman" 
>> >
>> > The truth is the containerid name really stinks and is quite confusing
>> > and does not imply that the label applies only to audit.  And little
>> > things like this make me extremely uncofortable with it.
>>
>> It also makes the audit container ID (notice how I *always* call it
>> the *audit* container ID? that is not an accident) available for
>> userspace applications to abuse.  Perhaps in the future we can look at
>> ways to make this more available to applications, but this patch is
>> not the answer.
>
> Do you have a productive suggestion?

I haven't given it much thought beyond our discussions and until we
get the basic audit container ID support in place (all the other parts
of this patchset) I doubt I'll be giving it much thought.

-- 
paul moore
www.paul-moore.com

Re: [RFC PATCH ghak32 V2 13/13] debug audit: read container ID of a process

[Richard Guy Briggs]

On 2018-05-21 16:06, Paul Moore wrote:
> On Mon, May 21, 2018 at 3:19 PM, Eric W. Biederman  
> wrote:
> > Steve Grubb  writes:
> >> On Friday, March 16, 2018 5:00:40 AM EDT Richard Guy Briggs wrote:
> >>> Add support for reading the container ID from the proc filesystem.
> >>
> >> I think this could be useful in general. Please consider this to be part of
> >> the full patch set and not something merely used to debug the patches.
> >
> > Only with an audit specific name.
> >
> > As it is:
> >
> > Nacked-by: "Eric W. Biederman" 
> >
> > The truth is the containerid name really stinks and is quite confusing
> > and does not imply that the label applies only to audit.  And little
> > things like this make me extremely uncofortable with it.
> 
> It also makes the audit container ID (notice how I *always* call it
> the *audit* container ID? that is not an accident) available for
> userspace applications to abuse.  Perhaps in the future we can look at
> ways to make this more available to applications, but this patch is
> not the answer.

Do you have a productive suggestion?

> paul moore

- RGB

--
Richard Guy Briggs 
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

Re: [RFC PATCH ghak32 V2 13/13] debug audit: read container ID of a process

[Paul Moore]

On Mon, May 21, 2018 at 3:19 PM, Eric W. Biederman
 wrote:
> Steve Grubb  writes:
>
>> On Friday, March 16, 2018 5:00:40 AM EDT Richard Guy Briggs wrote:
>>> Add support for reading the container ID from the proc filesystem.
>>
>> I think this could be useful in general. Please consider this to be part of
>> the full patch set and not something merely used to debug the patches.
>
> Only with an audit specific name.
>
> As it is:
>
> Nacked-by: "Eric W. Biederman" 
>
> The truth is the containerid name really stinks and is quite confusing
> and does not imply that the label applies only to audit.  And little
> things like this make me extremely uncofortable with it.

It also makes the audit container ID (notice how I *always* call it
the *audit* container ID? that is not an accident) available for
userspace applications to abuse.  Perhaps in the future we can look at
ways to make this more available to applications, but this patch is
not the answer.

-- 
paul moore
www.paul-moore.com

Re: [RFC PATCH ghak32 V2 13/13] debug audit: read container ID of a process

[Eric W. Biederman]

Steve Grubb  writes:

> On Friday, March 16, 2018 5:00:40 AM EDT Richard Guy Briggs wrote:
>> Add support for reading the container ID from the proc filesystem.
>
> I think this could be useful in general. Please consider this to be part of 
> the full patch set and not something merely used to debug the patches.

Only with an audit specific name.

As it is:

Nacked-by: "Eric W. Biederman" 

The truth is the containerid name really stinks and is quite confusing
and does not imply that the label applies only to audit.  And little
things like this make me extremely uncofortable with it.

Eric

Re: [RFC PATCH ghak32 V2 13/13] debug audit: read container ID of a process

[Steve Grubb]

On Friday, March 16, 2018 5:00:40 AM EDT Richard Guy Briggs wrote:
> Add support for reading the container ID from the proc filesystem.

I think this could be useful in general. Please consider this to be part of 
the full patch set and not something merely used to debug the patches.

-Steve

> This is a read from the proc entry of the form /proc/PID/containerid
> where PID is the process ID of the task whose container ID is sought.
> 
> The read expects up to a u64 value (unset: 18446744073709551615).
> 
> Signed-off-by: Richard Guy Briggs 
> ---
>  fs/proc/base.c | 20 ++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 6ce4fbe..f66d1e2 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -1300,6 +1300,21 @@ static ssize_t proc_sessionid_read(struct file *
> file, char __user * buf, .llseek  = generic_file_llseek,
>  };
> 
> +static ssize_t proc_containerid_read(struct file *file, char __user *buf,
> +   size_t count, loff_t *ppos)
> +{
> + struct inode *inode = file_inode(file);
> + struct task_struct *task = get_proc_task(inode);
> + ssize_t length;
> + char tmpbuf[TMPBUFLEN*2];
> +
> + if (!task)
> + return -ESRCH;
> + length = scnprintf(tmpbuf, TMPBUFLEN*2, "%llu",
> audit_get_containerid(task)); +   put_task_struct(task);
> + return simple_read_from_buffer(buf, count, ppos, tmpbuf, length);
> +}
> +
>  static ssize_t proc_containerid_write(struct file *file, const char __user
> *buf, size_t count, loff_t *ppos)
>  {
> @@ -1330,6 +1345,7 @@ static ssize_t proc_containerid_write(struct file
> *file, const char __user *buf, }
> 
>  static const struct file_operations proc_containerid_operations = {
> + .read   = proc_containerid_read,
>   .write  = proc_containerid_write,
>   .llseek = generic_file_llseek,
>  };
> @@ -2996,7 +3012,7 @@ static int proc_pid_patch_state(struct seq_file *m,
> struct pid_namespace *ns, #ifdef CONFIG_AUDITSYSCALL
>   REG("loginuid",   S_IWUSR|S_IRUGO, proc_loginuid_operations),
>   REG("sessionid",  S_IRUGO, proc_sessionid_operations),
> - REG("containerid", S_IWUSR, proc_containerid_operations),
> + REG("containerid", S_IWUSR|S_IRUSR, proc_containerid_operations),
>  #endif
>  #ifdef CONFIG_FAULT_INJECTION
>   REG("make-it-fail", S_IRUGO|S_IWUSR, proc_fault_inject_operations),
> @@ -3391,7 +3407,7 @@ static int proc_tid_comm_permission(struct inode
> *inode, int mask) #ifdef CONFIG_AUDITSYSCALL
>   REG("loginuid",  S_IWUSR|S_IRUGO, proc_loginuid_operations),
>   REG("sessionid",  S_IRUGO, proc_sessionid_operations),
> - REG("containerid", S_IWUSR, proc_containerid_operations),
> + REG("containerid", S_IWUSR|S_IRUSR, proc_containerid_operations),
>  #endif
>  #ifdef CONFIG_FAULT_INJECTION
>   REG("make-it-fail", S_IRUGO|S_IWUSR, proc_fault_inject_operations),

[RFC PATCH ghak32 V2 13/13] debug audit: read container ID of a process

[Richard Guy Briggs]

Add support for reading the container ID from the proc filesystem.

This is a read from the proc entry of the form /proc/PID/containerid
where PID is the process ID of the task whose container ID is sought.

The read expects up to a u64 value (unset: 18446744073709551615).

Signed-off-by: Richard Guy Briggs 
---
 fs/proc/base.c | 20 ++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 6ce4fbe..f66d1e2 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -1300,6 +1300,21 @@ static ssize_t proc_sessionid_read(struct file * file, 
char __user * buf,
.llseek = generic_file_llseek,
 };
 
+static ssize_t proc_containerid_read(struct file *file, char __user *buf,
+ size_t count, loff_t *ppos)
+{
+   struct inode *inode = file_inode(file);
+   struct task_struct *task = get_proc_task(inode);
+   ssize_t length;
+   char tmpbuf[TMPBUFLEN*2];
+
+   if (!task)
+   return -ESRCH;
+   length = scnprintf(tmpbuf, TMPBUFLEN*2, "%llu", 
audit_get_containerid(task));
+   put_task_struct(task);
+   return simple_read_from_buffer(buf, count, ppos, tmpbuf, length);
+}
+
 static ssize_t proc_containerid_write(struct file *file, const char __user 
*buf,
   size_t count, loff_t *ppos)
 {
@@ -1330,6 +1345,7 @@ static ssize_t proc_containerid_write(struct file *file, 
const char __user *buf,
 }
 
 static const struct file_operations proc_containerid_operations = {
+   .read   = proc_containerid_read,
.write  = proc_containerid_write,
.llseek = generic_file_llseek,
 };
@@ -2996,7 +3012,7 @@ static int proc_pid_patch_state(struct seq_file *m, 
struct pid_namespace *ns,
 #ifdef CONFIG_AUDITSYSCALL
REG("loginuid",   S_IWUSR|S_IRUGO, proc_loginuid_operations),
REG("sessionid",  S_IRUGO, proc_sessionid_operations),
-   REG("containerid", S_IWUSR, proc_containerid_operations),
+   REG("containerid", S_IWUSR|S_IRUSR, proc_containerid_operations),
 #endif
 #ifdef CONFIG_FAULT_INJECTION
REG("make-it-fail", S_IRUGO|S_IWUSR, proc_fault_inject_operations),
@@ -3391,7 +3407,7 @@ static int proc_tid_comm_permission(struct inode *inode, 
int mask)
 #ifdef CONFIG_AUDITSYSCALL
REG("loginuid",  S_IWUSR|S_IRUGO, proc_loginuid_operations),
REG("sessionid",  S_IRUGO, proc_sessionid_operations),
-   REG("containerid", S_IWUSR, proc_containerid_operations),
+   REG("containerid", S_IWUSR|S_IRUSR, proc_containerid_operations),
 #endif
 #ifdef CONFIG_FAULT_INJECTION
REG("make-it-fail", S_IRUGO|S_IWUSR, proc_fault_inject_operations),
-- 
1.8.3.1