linux-kernel - Re: [PATCH ghak90 V6 03/10] audit: read container ID of a process

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 19 Jul 2019 13:05:52 -0400
From:   Richard Guy Briggs <rgb@...hat.com>
To:     "Eric W. Biederman" <ebiederm@...ssion.com>
Cc:     containers@...ts.linux-foundation.org, linux-api@...r.kernel.org,
        Linux-Audit Mailing List <linux-audit@...hat.com>,
        linux-fsdevel@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>,
        netdev@...r.kernel.org, netfilter-devel@...r.kernel.org,
        Paul Moore <paul@...l-moore.com>, sgrubb@...hat.com,
        omosnace@...hat.com, dhowells@...hat.com, simo@...hat.com,
        eparis@...isplace.org, serge@...lyn.com, nhorman@...driver.com
Subject: Re: [PATCH ghak90 V6 03/10] audit: read container ID of a process

On 2019-07-19 11:03, Eric W. Biederman wrote:
> Richard Guy Briggs <rgb@...hat.com> writes:
> 
> > Add support for reading the audit container identifier from the proc
> > filesystem.
> >
> > This is a read from the proc entry of the form
> > /proc/PID/audit_containerid where PID is the process ID of the task
> > whose audit container identifier is sought.
> >
> > The read expects up to a u64 value (unset: 18446744073709551615).
> >
> > This read requires CAP_AUDIT_CONTROL.
> 
> This scares me.    As this seems to make it easy to reuse an audit
> containerid for non-audit purporses.

At this point, given that capable(CAP_AUDIT_CONTROL) is not available to
any userspaced container orchestrator/engine, it is moot anywaysand we
will need another method.

> I would think it would be safer and easier to poke audit and ask it to
> log a message with your audit container id.

For it to be useful to a container orchestrator/engine, I think that
would depend on whether we are setting the value, or it is being
assigned by the kernel.  At this stage it is set by the orchestrator so
this could work.

> Eric
> 
> > Signed-off-by: Richard Guy Briggs <rgb@...hat.com>
> > Acked-by: Serge Hallyn <serge@...lyn.com>
> > Acked-by: Neil Horman <nhorman@...driver.com>
> > Reviewed-by: Ondrej Mosnacek <omosnace@...hat.com>
> > ---
> >  fs/proc/base.c | 25 ++++++++++++++++++++++---
> >  1 file changed, 22 insertions(+), 3 deletions(-)
> >
> > diff --git a/fs/proc/base.c b/fs/proc/base.c
> > index 43fd0c4b87de..acc70239d0cb 100644
> > --- a/fs/proc/base.c
> > +++ b/fs/proc/base.c
> > @@ -1211,7 +1211,7 @@ static ssize_t oom_score_adj_write(struct file *file, const char __user *buf,
> >  };
> >  
> >  #ifdef CONFIG_AUDIT
> > -#define TMPBUFLEN 11
> > +#define TMPBUFLEN 21
> >  static ssize_t proc_loginuid_read(struct file * file, char __user * buf,
> >  				  size_t count, loff_t *ppos)
> >  {
> > @@ -1295,6 +1295,24 @@ static ssize_t proc_sessionid_read(struct file * file, char __user * buf,
> >  	.llseek		= generic_file_llseek,
> >  };
> >  
> > +static ssize_t proc_contid_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];
> > +
> > +	if (!task)
> > +		return -ESRCH;
> > +	/* if we don't have caps, reject */
> > +	if (!capable(CAP_AUDIT_CONTROL))
> > +		return -EPERM;
> > +	length = scnprintf(tmpbuf, TMPBUFLEN, "%llu", audit_get_contid(task));
> > +	put_task_struct(task);
> > +	return simple_read_from_buffer(buf, count, ppos, tmpbuf, length);
> > +}
> > +
> >  static ssize_t proc_contid_write(struct file *file, const char __user *buf,
> >  				   size_t count, loff_t *ppos)
> >  {
> > @@ -1325,6 +1343,7 @@ static ssize_t proc_contid_write(struct file *file, const char __user *buf,
> >  }
> >  
> >  static const struct file_operations proc_contid_operations = {
> > +	.read		= proc_contid_read,
> >  	.write		= proc_contid_write,
> >  	.llseek		= generic_file_llseek,
> >  };
> > @@ -3067,7 +3086,7 @@ static int proc_stack_depth(struct seq_file *m, struct pid_namespace *ns,
> >  #ifdef CONFIG_AUDIT
> >  	REG("loginuid",   S_IWUSR|S_IRUGO, proc_loginuid_operations),
> >  	REG("sessionid",  S_IRUGO, proc_sessionid_operations),
> > -	REG("audit_containerid", S_IWUSR, proc_contid_operations),
> > +	REG("audit_containerid", S_IWUSR|S_IRUSR, proc_contid_operations),
> >  #endif
> >  #ifdef CONFIG_FAULT_INJECTION
> >  	REG("make-it-fail", S_IRUGO|S_IWUSR, proc_fault_inject_operations),
> > @@ -3466,7 +3485,7 @@ static int proc_tid_comm_permission(struct inode *inode, int mask)
> >  #ifdef CONFIG_AUDIT
> >  	REG("loginuid",  S_IWUSR|S_IRUGO, proc_loginuid_operations),
> >  	REG("sessionid",  S_IRUGO, proc_sessionid_operations),
> > -	REG("audit_containerid", S_IWUSR, proc_contid_operations),
> > +	REG("audit_containerid", S_IWUSR|S_IRUSR, proc_contid_operations),
> >  #endif
> >  #ifdef CONFIG_FAULT_INJECTION
> >  	REG("make-it-fail", S_IRUGO|S_IWUSR, proc_fault_inject_operations),

- RGB

--
Richard Guy Briggs <rgb@...hat.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