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  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:   Tue, 05 May 2020 11:33:05 -0400
From:   Mimi Zohar <zohar@...ux.ibm.com>
To:     Lakshmi Ramasubramanian <nramas@...ux.microsoft.com>,
        linux-integrity@...r.kernel.org
Cc:     Jann Horn <jannh@...gle.com>,
        Stephen Smalley <stephen.smalley.work@...il.com>,
        Eric Biggers <ebiggers@...nel.org>,
        linux-security-module@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH] ima: verify mprotect change is consistent with mmap
 policy

On Mon, 2020-05-04 at 15:51 -0700, Lakshmi Ramasubramanian wrote:
> On 5/4/20 2:17 PM, Mimi Zohar wrote:
> 
> Hi Mimi,
> 
> > +int ima_file_mprotect(struct vm_area_struct *vma, unsigned long prot)
> > +{
> > +	struct ima_template_desc *template;
> > +	struct inode *inode;
> > +	int result = 0;
> > +	int action;
> > +	u32 secid;
> > +	int pcr;
> > +
> > +	if (vma->vm_file && (prot & PROT_EXEC) && !(vma->vm_flags & VM_EXEC)) {
> 
> Just a suggestion:
> Maybe you could do the negative of the above check and return, so that 
> the block within the if statement doesn't have to be indented.

Good suggestion.

> 
> > +		inode = file_inode(vma->vm_file);
> > +
> > +		security_task_getsecid(current, &secid);
> > +		action = ima_get_action(inode, current_cred(), secid, MAY_EXEC,
> > +					MMAP_CHECK, &pcr, &template, 0);
> > +
> > +		if (action & IMA_APPRAISE_SUBMASK)
> > +			result = -EPERM;
> > +
> > +		if ((action & IMA_APPRAISE_SUBMASK) || (action & IMA_MEASURE)) {
> 
> action is checked for IMA_APPRAISE_SUBMASK bits in the previous if 
> statement. Does it need to be checked again in the above if statement?

Agreed, the code should be cleaned up here too.  In either the
measurement or the appraisal case, mprotect modifying the execute mmap
flag should be audited, but only in the appraisal case is the request
denied.

Mimi

> 
> > +			struct file *file = vma->vm_file;
> > +			char *pathbuf = NULL;
> > +			const char *pathname;
> > +			char filename[NAME_MAX];
> > +
> > +			pathname = ima_d_path(&file->f_path, &pathbuf,
> > +					      filename);
> > +			integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode,
> > +					    pathname, "collect_data",
> > +					    "failed-mprotect", result, 0);
> > +
> > +			if (pathbuf)
> > +				__putname(pathbuf);
> > +		}
> > +	}
> > +	return result;
> > +}
> 
> thanks,
>   -lakshmi
> 

Powered by blists - more mailing lists