linux-kernel - Re: [RFC PATCH] ima: verify mprotect change is consistent with mmap policy

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] [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
>