Re: [PATCH v2 3/5] NFSD: Remove ima_file_check call

[Chuck Lever <chuck.lever@oracle.com>]

> On Mar 19, 2019, at 3:29 PM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> 
> On Fri, 2019-03-08 at 16:29 -0500, Chuck Lever wrote:
> 
> Thanks Serge for bringing this thread to my attention.  Sorry for the
> delay in responding ...
> 
>>> On Mar 8, 2019, at 4:23 PM, Bruce Fields <bfields@fieldses.org> wrote:
>>> 
>>> On Fri, Mar 08, 2019 at 04:11:06PM -0500, Chuck Lever wrote:
>>>> 
>>>> 
>>>>> On Mar 8, 2019, at 4:10 PM, bfields@fieldses.org wrote:
>>>>> 
>>>>> On Thu, Mar 07, 2019 at 10:28:54AM -0500, Chuck Lever wrote:
>>>>>> The NFS server needs to allow NFS clients to perform their own
>>>>>> attestation and measurement.
> 
> Measurement and attestation is only one aspect.  The other aspect is
> verifying the integrity of files.  Shouldn't the NFS server verify the
> integrity of a file before allowing it to be served (eg. malware)?

Hi Mimi, thanks for the review.

Architecturally, the server is not using the file's data, it is
merely part of the filesystem that stores it. But that said, there
are several concrete reasons why I feel an NFS server should not be
involved in measurement/attestation, but only with storing file
content and IMA metadata.

1. The broadest attack surface for a remote filesystem is modification
of data in flight. Attestation of the file on the server is not going
to defend against that attack, only attestation on the client will do
that. Is there a good reason to pay the cost of double attestation?

2. It is possible (perhaps even likely) that the NFS server and a
client of that server will have different IMA policies and even
different file signing authorities.

A third, perhaps related, reason is that NFS can run on non-Linux NFS
servers which would not have any attestation at all. An NFS client
should not have to rely on the server for attestation, but should
trust only its own measurement of each file, which would be done as
late as possible before use.

Lastly, the NFS protocol does not enable an NFS client to tell a
server how the file is to be used. For example, the server's policy
might block execution of an unverifiable file, but the server won't
have any way of knowing how the client is going to use that file.
The client might be opening the file to copy it or update its IMA
metadata.

Speaking of protocol, there's no special error code that reports an
integrity verification failure. The client just sees that the UID
does not have access to the file. There's no way the user or client
can do anything to clear this condition via NFS without IMA support.

If these reasons make sense, should I add them to the patch description?


>>>>> Can we really remove this call?
>>>> 
>>>> Why wouldn't we be able to?
>>> 
>>> I don't know the first thing about IMA, but surely it's there for some
>>> reason--
>> 
>> It was originally added because the number of opens and closes of @file
>> were counted, and not having that call was triggering a warning. Since
>> commit 8eb988c70e770 ("fix ima breakage") the counters are maintained
>> separately.
> 
> If that was the only reason, then the call itself would have been
> removed with the counter code.
> 
> Mimi
> 
>> 
>> 
>>> is it really OK just to skip this on opens by nfsd?
>> 
>> That's why I split this out into a separate patch. I'm hoping to get
>> some commentary from the linux-integrity community.
>> 
>> 
>>> --b.
>>> 
>>>>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>>>>> ---
>>>>>> fs/nfsd/vfs.c |    6 ------
>>>>>> 1 file changed, 6 deletions(-)
>>>>>> 
>>>>>> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
>>>>>> index 3c00072..524c6e5 100644
>>>>>> --- a/fs/nfsd/vfs.c
>>>>>> +++ b/fs/nfsd/vfs.c
>>>>>> @@ -802,12 +802,6 @@ static int nfsd_open_break_lease(struct inode *inode, int access)
>>>>>> 		goto out_nfserr;
>>>>>> 	}
>>>>>> 
>>>>>> -	host_err = ima_file_check(file, may_flags);
>>>>>> -	if (host_err) {
>>>>>> -		fput(file);
>>>>>> -		goto out_nfserr;
>>>>>> -	}
>>>>>> -
>>>>>> 	if (may_flags & NFSD_MAY_64BIT_COOKIE)
>>>>>> 		file->f_mode |= FMODE_64BITHASH;
>>>>>> 	else
>>>> 
>>>> --
>>>> Chuck Lever
>>>> 
>>>> 
>> 
>> --
>> Chuck Lever
>> 
>> 
>> 
> 

--
Chuck Lever