Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <b86ec24317e4a07901ee5d981d9bcf580c8fd2eb.camel@HansenPartnership.com>
Subject: Re: [PATCH 0/1] ima: check control characters in policy path
From:   James Bottomley <James.Bottomley@HansenPartnership.com>
To:     Tianxing Zhang <anakinzhang96@gmail.com>, zohar@linux.ibm.com
Cc:     linux-integrity@vger.kernel.org,
        linux-security-module@vger.kernel.org, linux-kernel@vger.kernel.org
Date:   Sat, 14 Aug 2021 08:47:53 -0400
In-Reply-To: <20210814081356.293-1-anakinzhang96@gmail.com>
References: <20210814081356.293-1-anakinzhang96@gmail.com>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.34.4 
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Sat, 2021-08-14 at 16:13 +0800, Tianxing Zhang wrote:
> Hi,
> 
> IMA policy can be updated with /sys/kernel/security/ima/policy
> interface when CONFIG_IMA_WRITE_POLICY is set. However, kernel does
> not check the file path carefully. It only checks if the path has '/'
> prefix.
> 
> When a policy file path contains control characters like '\r' or
> '\b', invalid error messages can be printed to overwrite system
> messages.

This doesn't sound like a good idea: filesystems accept control
characters in names, so the IMA file policy has to be able to specify
them.  We can debate whether filesystems should do this, but while they
do IMA has to as well.

> For example:
> 
> $ echo -e "/\rtest invalid path: ddddddddddddddddddddd" >
> /sys/kernel/security/ima/policy
> $ dmesg
> test invalid path: ddddddddddddddddddddd (-2) 
> 
> After adding this patch, we'll be able to throw out error message:
> 
> $ echo -e "/\rtest invalid path: ddddddddddddddddddddd" >
> /sys/kernel/security/ima/policy
> -bash: echo: write error: Invalid argument
> $ dmesg
> [   11.684004] ima: invalid path (control characters are not allowed)
> [   11.684071] ima: policy update failed
> 
> Any suggestions would be appreciated, thank you.

I don't quite understand what you think the problem is.  Only root can
write IMA policies so no-one other than a legitimate administrator can
use bogus paths like the above.  If the problem is producing a bogus
log message, we do have several IMA messages that print out
measured/appraised file names ... they would be vulnerable to this
since a generic user could have created them with control character
containg file names, and your proposed patch wouldn't fix that.

Wouldn't a better solution be to have a file name print that expands
the unprintable characters?

James