Re: [nfsv4] Review of draft-ief-nfsv4-integrity-measurement-04

[Chuck Lever <[e-mail]>]

Hi Dave, thanks for your thorough review. Comments and responses inline.

> On May 19, 2019, at 5:41 AM, David Noveck <[e-mail]> wrote:
> 
> General Comments
> 
> Context of Review
> 
> Development of this document has gone on for a considerable time and it appears to have reached something close to its eventual final form.   Therefore it is appropriate consider the path forward and specifically what would be necessary for the document to be ready for WGLC.  That issue will be the focus of this review.    
> 
> Overall Evaluation
> 
> This document is in pretty good shape but I feel that there are still a few matters, beyond the minor editoral issues that consitutute the bulk of the material in Comments by Section, that need tro be dealt with before WGLC:
> 	• I feel that some editorial work needs to be done in being more explicit about the basic  approach taken in this document.  There have, in the recent past, been concerns expressed about this approach and feel it is quite possible that such concerns will be raised as part of WGLC.   These issues are discussed in Potential Concerns about the Basic Approach but I don't feel that there is anything that the author can reasonably do to address them, partly because no specific issues have been raised, beyond the dissatisfaction/unease already mentioned.  Instead, as discussed below in Clarity about Goals and non-Goals, I feel it would be best if the author was more explicit about the limited goals of this document, continuing an existing trajectory from -00 to -04.
> 	• Although, for the most part, the current document follows the model of treating the IMA metadata as uninterpreted data whose format (beyond a necessary length limit) is not its concern, there are some areas where that division of labor is not strictly maintained, which poses issues for server implementers.   For further discussion of these areas, see Issues for Server Implementations.    
> Potential Concerns about the Basic Approach
> 
> The major area of potential concern centers around the fact that there is likely to be only a single client implementation in that the document does not  provide sufficient information for a client to be written and there is no point for anyone to write another client implementation.   This can be accurately described as a Linux-only feature and there is no point in having multiple competing implementations unless one has gone horribly wrong.
> 
> On the other hand, the document generally provides sufficient information to allow multiple server implementations to be written and tested against the Linux krnel client implmentation being written.
> 
> This is an atypical pattern for the working group and it is probably an atypical one for the IETF as a whole.   During working group last call, it is likely that concerns traceable to this issue will be raised as they have in the past.   It is important that we have a consensus on the fact that we want to do this although it is not necessary to have a consensus on why we are doing it, as people may well differ on why we are doing this and thus we may might not  have a policy about such one-client-only features going foward.

I could quibble with exactly how you stated this, but I agree that:

- the practical outcome is that for some time -- maybe forever -- there will
be a single operating system (Linux) that has the need for NFS client support
for storing and retrieving IMA metadata.

- The primary reason to create this extension is to enable broad support for
the storage of IMA metadata on NFS servers.


However I do not agree that this is a precedent or atypical pattern. The nfsv4
WG has already published RFCs where there is, practically speaking, a single
client implementation:

 - NFSv4 Security Labels; the purpose of these was to store SELinux labels, for
   which there is no standard specification. See RFC 7204 and RFC 7862.

 - NFSv4 support Linux user xattrs; there is no standard that describes what
   these are, save the Linux implementation and man pages. See RFC 8276.


> Clarity about Goal and non-Goals
> 
> As this docment has been developed, there has been a shift from an earlier state in which there were somewhat half-hearted attempts to provide for the posssibility of other clients to the current state in which it is qiuite clear that this is, as descibed in Potential Concerns about the Basic Approach a one-client-only feature.   I believe it would be helpful to be more explicit about that fact in a number of places, so that the document can address the concerns of those who have trouble with that approach head-on.

No argument from me. I'm pretty close to the subject matter, so any help with
such text would be appreciated.


> As I understand things, the Linux IMA metdata is essentally, from the server's point of view, a piece of uninterpreted data, just as user data and extended attributes are, although there are  a few remaining issues discussed in Issues for Server Implementations that need to be addressed.  However, unlike other sorts of user data, this does have an internal structure known to the client implentations and not documented in this document.   That information would not be needed in a one-client-only implementation but it is typically provided in feature descriptions, so it would be best if this point is mde explicitly.

Sure. I feel that is already stated in the Introduction. If there is a way to
make it more clear, I am open to suggestions.


> Issues for Server Implementations
> 
> The following issues are discussed in more detail in 5.2.1.  Authorizing Updates to IMA Metadata:
> 	• As described above, the basic architecture is that the server is to treat IMA metadata as uninterpreted data, yet there are some indications that this is not always so.

Here's where I can state my quibble: interpretation of this metadata is by
a security module loaded by the operating system, not by the NFS server or
client implementation themselves.

To wit: A Linux host acting as an NFS server will have the capability to
interpret IMA metadata, and in fact it does so before serving IMA-protected
file content. The actual behavior when appraisal fails depends on the local
IMA policy that is in effect.

Moreover, a Linux NFS server typically has a user execution environment
where IMA policies need to be enforced for locally-running applications.

Therefore there are some cases where it is very reasonable for an NFS server
to have the ability to interpret the IMA metadata. Section 5.3 provides
examples that might help explain these scenarios.


> 	• The issue of the proper privilege to update IMA metadata needs to be clarified.

I agree this issue still needs work, but I'm not sure how to proceed.


> Comments by Section

I'll look into each of the below comments as I prepare the next revision of
this document. The fresh revision should be ready before IETF 105.


> Title
> 
> Consideration should be given to including the word "Linux" in the title, since this is being defined as a Linux-only feature.
> 
> 1.  The Linux Integrity Measurement Architecture
> 
> In the second sentence of the first paragraph, I'm wondering about the adjective "primary".  Since I'm having trouble imagining what a secondary goal might be, I wonder if "primary" should be deleted.
> 
> With regard to the penultimate paragraph:
> 	• I don't understand the "therefore" in the second sentence.  I'm clear that the format is not described in this document, and it does not need to be since servers don't need it and there is only one client implementation.  I think we need more clarity about whether the format cannot be described in this document or whether it could but is not present because it is unnecessary.
> 	• In the third sentence suggest replacing "NFS clients" by "Linux NFS clients."
> 	• Suggest adding a final sentence such as the following :
> Information about the format of IMA metadata is not needed by server implementation as this data can be treated just as is other data whose format is defined by others (e.g. user  data and extended attributes).
>  
> 4.   Managing IMA Metadata on NFS Files
> 
> While I haven't found many issues with the material in the various subsections of this section, the material is incomplete in that it does not deal with all possible use of the IMA metdata attribute in attribute-related operation but only in those that the author blieves (probably correctly) are likely to be used.   In particular,
> 	• There is no mentiion of VERIFY/NVERIFY.   It is not clear if they are considered invalid, but I'm assuming they aren't.
> 	• There is no mention of the specfication of attributes in CREATE.   It needs to be specfied if this is valid or not.
> 	• In sections 4.2 and 4.3 the text seems to assume that the IMA metadata attribute will not be set/interrogated together with other attributes.
> One other issue that applies to multiple subsections concerns the phrase "but the object itself does not support the FATTR4_LINUX_IMA attribute".   It needs to be defined what types of objects are to have this support.   Otherwise, you have a Total Interoperability Nightmare (tm :-).

I suspect it isn't all that bad, but I agree that Section 4 needs to
discuss these three areas.


> 4.3.  Retrieving IMA Metadata
> 
> In the third paragraph, there is a stray reference to the FATTR4_FILE_PROVENANCE bit which needs to fixed.
> 
> 5.2.1.  Authorizing Updates to IMA Metadata
> 
> There are a number of issues in this section where sufficient information is not given for  a server implementation to be written or there is a reference to the possibility that the server will implement behavior based on knowledge not provided in this spec, as mentioned above in  Issues for Server Implementations.   Given the current scope of the problems, I'll reproduce the existing section in italics and respond to each element that  I believe raises problems:
>> A participating server should ensure that modifications to IMA metadata are done only by appropriately authorized agents. 
> That's unexceptionable, but the problem is that the server is given no way to determine exactly when a request is made by an "appropriately authorized agent".    As ACLs are the primary means by which authorization decision about actiions on files are determined in NFSv4, it would be best if the specfication told how the file's ACL could be used to make this determination. 
>> Such agents usually include only agents with super-user privileges.  
> Since this imples that there are also unusual situations, the "usually" is purely informational.  Again, what is needed is a specfication of how a server is to make the authorization.   The obvious choice is to allow this to be set iff the  file data can be written but if it were that simple, the spec would simply say that.

The existing implementation of IMA allows changes to the IMA metadata
only if CAP_SYS_ADMIN is set (ie, the root user privilege). I will have
to check into the rationale for this choice.


> If it is something else, what is needed is:
> 	• A specification of that other approach.
> 	• An explanation of why the obvious choice is not OK.
>> The NFS server MAY confirm that the new IMA metadata actually verifies the file content correctly before storing it.
> There are a number problems with this:
> 
> 	• It contradicts the basic idea that the server is to treat IMA metadata as uninterpreted, as it does user data,   I for one, would not want an NFSv4 server to reject a WRITE of a .c file because it didn't like my comment indenting :-)
> 	• Since the format is not defined in this document, it is basically saying that the client that the server "MAY" refuse a request to store IMA metadata pretty much whenever it feels like it and that clients have to deal wit that.
> 	• There is no specification, either here or in Section 4.2, of what error SETATTR is to return in this event.
> If this is necessary to accommodate Linux file systems that are aware of the format of this data, enforce it, and cannot be changed to treat this data according to the basic architecture treating this as uninterpreted data, consideration should be given to making this a "SHOULD NOT".

Agreed, the MAY doesn't make sense here; I think that was a misdirected
edit of another area of the document. Two alternatives to address this
issue are to remove that sentence, or change MAY to MUST NOT.


> 7.  Security Considerations
> 
> The last paragraph has the following issues:
> 	• It ignores the fact the denial-of-service triggered by mismatch of data and IMA-metadata can result from changing the data as well as changing the IMA metadata.
> 	• It doesn't define "suitable level of pivilege".
> 	• It refers the reader to section 5.2.1 for details while that section has no useful details.

--
Chuck Lever