The Fanotify API can be used for access control by requesting permission
event notification. The user space tooling that uses it may have a
complicated policy that inherently contains additional context for the
decision. If this information were available in the audit trail, policy
writers can close the loop on debugging policy. Also, if this additional
information were available, it would enable the creation of tools that
can suggest changes to the policy similar to how audit2allow can help
refine labeled security.

This patch defines 2 additional fields within the response structure
returned from user space on a permission event. The first field is 16
bits for the context type. The context type will describe what the
meaning is of the second field. The audit system will separate the
pieces and log them individually.

The audit function was updated to log the additional information in the
AUDIT_FANOTIFY record. The following is an example of the new record
format:

type=FANOTIFY msg=audit(1600385147.372:590): resp=2 fan_type=1 fan_ctx=17

changelog:
v1:
- first version by Steve Grubb <sgrubb@redhat.com>
Link: https://lore.kernel.org/r/2042449.irdbgypaU6@x2

v2:
- enhancements suggested by Jan Kara <jack@suse.cz>
- 1/3 change %d to %u in pr_debug
- 2/3 change response from __u32 to __u16
- mod struct fanotify_response and fanotify_perm_event add extra_info_type, extra_info_buf
- extra_info_buf size max FANOTIFY_MAX_RESPONSE_EXTRA_LEN, add struct fanotify_response_audit_rule
- extend debug statements
- remove unneeded macros
- [internal] change interface to finish_permission_event() and process_access_response()
- 3/3 update format of extra information
- [internal] change interface to audit_fanotify()
- change ctx_type= to fan_type=
Link: https://lore.kernel.org/r/cover.1651174324.git.rgb@redhat.com

Richard Guy Briggs (3):
  fanotify: Ensure consistent variable type for response
  fanotify: define struct members to hold response decision context
  fanotify: Allow audit to use the full permission event response

 fs/notify/fanotify/fanotify.c      |  5 ++-
 fs/notify/fanotify/fanotify.h      |  4 +-
 fs/notify/fanotify/fanotify_user.c | 59 ++++++++++++++++++++----------
 include/linux/audit.h              |  8 ++--
 include/linux/fanotify.h           |  3 ++
 include/uapi/linux/fanotify.h      | 27 +++++++++++++-
 kernel/auditsc.c                   | 18 +++++++--
 7 files changed, 94 insertions(+), 30 deletions(-)

-- 
2.27.0

On 2022-04-28 20:44, Richard Guy Briggs wrote:
> The Fanotify API can be used for access control by requesting permission
> event notification. The user space tooling that uses it may have a
> complicated policy that inherently contains additional context for the
> decision. If this information were available in the audit trail, policy
> writers can close the loop on debugging policy. Also, if this additional
> information were available, it would enable the creation of tools that
> can suggest changes to the policy similar to how audit2allow can help
> refine labeled security.
> 
> This patch defines 2 additional fields within the response structure
> returned from user space on a permission event. The first field is 16
> bits for the context type. The context type will describe what the
> meaning is of the second field. The audit system will separate the
> pieces and log them individually.
> 
> The audit function was updated to log the additional information in the
> AUDIT_FANOTIFY record. The following is an example of the new record
> format:
> 
> type=FANOTIFY msg=audit(1600385147.372:590): resp=2 fan_type=1 fan_ctx=17

It might have been a good idea to tag this as RFC...  I have a few
questions:

1. Where did "resp=" come from?  It isn't in the field dictionary.  It
seems like a needless duplication of "res=".  If it isn't, maybe it
should have a "fan_" namespace prefix and become "fan_res="?

2. It appears I'm ok changing the "__u32 response" to "__u16" without
breaking old userspace.  Is this true on all arches?

3. What should be the action if response contains unknown flags or
types?  Is it reasonable to return -EINVAL?

4. Currently, struct fanotify_response has a fixed size, but if future
types get defined that have variable buffer sizes, how would that be
communicated or encoded?

> changelog:
> v1:
> - first version by Steve Grubb <sgrubb@redhat.com>
> Link: https://lore.kernel.org/r/2042449.irdbgypaU6@x2
> 
> v2:
> - enhancements suggested by Jan Kara <jack@suse.cz>
> - 1/3 change %d to %u in pr_debug
> - 2/3 change response from __u32 to __u16
> - mod struct fanotify_response and fanotify_perm_event add extra_info_type, extra_info_buf
> - extra_info_buf size max FANOTIFY_MAX_RESPONSE_EXTRA_LEN, add struct fanotify_response_audit_rule
> - extend debug statements
> - remove unneeded macros
> - [internal] change interface to finish_permission_event() and process_access_response()
> - 3/3 update format of extra information
> - [internal] change interface to audit_fanotify()
> - change ctx_type= to fan_type=
> Link: https://lore.kernel.org/r/cover.1651174324.git.rgb@redhat.com
> 
> Richard Guy Briggs (3):
>   fanotify: Ensure consistent variable type for response
>   fanotify: define struct members to hold response decision context
>   fanotify: Allow audit to use the full permission event response
> 
>  fs/notify/fanotify/fanotify.c      |  5 ++-
>  fs/notify/fanotify/fanotify.h      |  4 +-
>  fs/notify/fanotify/fanotify_user.c | 59 ++++++++++++++++++++----------
>  include/linux/audit.h              |  8 ++--
>  include/linux/fanotify.h           |  3 ++
>  include/uapi/linux/fanotify.h      | 27 +++++++++++++-
>  kernel/auditsc.c                   | 18 +++++++--
>  7 files changed, 94 insertions(+), 30 deletions(-)
> 
> -- 
> 2.27.0
> 

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

On Thursday, April 28, 2022 8:55:33 PM EDT Richard Guy Briggs wrote:
> On 2022-04-28 20:44, Richard Guy Briggs wrote:
> > The Fanotify API can be used for access control by requesting permission
> > event notification. The user space tooling that uses it may have a
> > complicated policy that inherently contains additional context for the
> > decision. If this information were available in the audit trail, policy
> > writers can close the loop on debugging policy. Also, if this additional
> > information were available, it would enable the creation of tools that
> > can suggest changes to the policy similar to how audit2allow can help
> > refine labeled security.
> > 
> > This patch defines 2 additional fields within the response structure
> > returned from user space on a permission event. The first field is 16
> > bits for the context type. The context type will describe what the
> > meaning is of the second field. The audit system will separate the
> > pieces and log them individually.
> > 
> > The audit function was updated to log the additional information in the
> > AUDIT_FANOTIFY record. The following is an example of the new record
> > format:
> > 
> > type=FANOTIFY msg=audit(1600385147.372:590): resp=2 fan_type=1 fan_ctx=17
> 
> It might have been a good idea to tag this as RFC...  I have a few
> questions:
> 
> 1. Where did "resp=" come from? 

This is an abbreviation for the response field of struct fanotify_response. I 
wanted to keep it short to save bytes.

> It isn't in the field dictionary.  It seems like a needless duplication of
> "res=".  If it isn't, maybe it should have a "fan_" namespace prefix and
> become "fan_res="?

At this point it's been interpretted for years.
 
> 2. It appears I'm ok changing the "__u32 response" to "__u16" without
> breaking old userspace.  Is this true on all arches?

If done carefully. Old user space won't try to use the new capabilities. The 
only trick is new user space/old kernel.

> 3. What should be the action if response contains unknown flags or
> types?  Is it reasonable to return -EINVAL?

The original patch was designed to allow the response metadata to take on 
various different meanings based on new usage. The original patch only defined 
a rule order numbering which if something else wanted to send it's own 
metadata about a decision, a new patch could layer on top of this without 
interfering.

If this is an access decision that is rejected with EINVAL (and assuming 
future decisions will also be formed the same way), the whole system is 
getting ready to lock up - even though a real answer is in the response.

> 4. Currently, struct fanotify_response has a fixed size, but if future
> types get defined that have variable buffer sizes, how would that be
> communicated or encoded?

I hadn't considered that as it would be too much of a change that I would be 
uncomfortable doing. That could be a future evolution if it's ever needed.

-Steve

On Thu, Apr 28, 2022 at 8:55 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2022-04-28 20:44, Richard Guy Briggs wrote:
> > The Fanotify API can be used for access control by requesting permission
> > event notification. The user space tooling that uses it may have a
> > complicated policy that inherently contains additional context for the
> > decision. If this information were available in the audit trail, policy
> > writers can close the loop on debugging policy. Also, if this additional
> > information were available, it would enable the creation of tools that
> > can suggest changes to the policy similar to how audit2allow can help
> > refine labeled security.
> >
> > This patch defines 2 additional fields within the response structure
> > returned from user space on a permission event. The first field is 16
> > bits for the context type. The context type will describe what the
> > meaning is of the second field. The audit system will separate the
> > pieces and log them individually.
> >
> > The audit function was updated to log the additional information in the
> > AUDIT_FANOTIFY record. The following is an example of the new record
> > format:
> >
> > type=FANOTIFY msg=audit(1600385147.372:590): resp=2 fan_type=1 fan_ctx=17
>
> It might have been a good idea to tag this as RFC...  I have a few
> questions:
>
> 1. Where did "resp=" come from?

According to the git log, it came from Steve Grubb via de8cd83e91bc
("audit: Record fanotify access control decisions").  Steve should
have known what he was doing with respect to field names so I'm
assuming he had a reason.

> It isn't in the field dictionary.  It
> seems like a needless duplication of "res=".  If it isn't, maybe it
> should have a "fan_" namespace prefix and become "fan_res="?

Regardless of what it should have been, it is "resp" now and we can't
really change it.  As far as the field dictionary is concerned, while
we should document these fields, it is important to note that when the
dictionary conflicts with the kernel, the kernel wins by definition.

> 2. It appears I'm ok changing the "__u32 response" to "__u16" without
> breaking old userspace.  Is this true on all arches?

I can't answer that for you, the fanotify folks will need to look at
that, but you likely already know that.  While I haven't gone through
the entire patchset yet, if it was me I probably would have left
response as a u32 and just added the extra fields; you are already
increasing the size of fanotify_response so why bother with shrinking
an existing field?

> 3. What should be the action if response contains unknown flags or
> types?  Is it reasonable to return -EINVAL?

Once again, not a fanotify expert, but EINVAL is intended for invalid
input so it seems like a reasonable choice.

> 4. Currently, struct fanotify_response has a fixed size, but if future
> types get defined that have variable buffer sizes, how would that be
> communicated or encoded?

If that is a concern, you should probably include a length field in
the structure before the variable length field.  You can't put it
before fd or response, so it's really a question of before or after
your new extra_info_type; I might suggest *after* extra_info_type, but
that's just me.


--
paul-moore.com

On 2022-05-02 20:16, Paul Moore wrote:
> On Thu, Apr 28, 2022 at 8:55 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > On 2022-04-28 20:44, Richard Guy Briggs wrote:
> > > The Fanotify API can be used for access control by requesting permission
> > > event notification. The user space tooling that uses it may have a
> > > complicated policy that inherently contains additional context for the
> > > decision. If this information were available in the audit trail, policy
> > > writers can close the loop on debugging policy. Also, if this additional
> > > information were available, it would enable the creation of tools that
> > > can suggest changes to the policy similar to how audit2allow can help
> > > refine labeled security.
> > >
> > > This patch defines 2 additional fields within the response structure
> > > returned from user space on a permission event. The first field is 16
> > > bits for the context type. The context type will describe what the
> > > meaning is of the second field. The audit system will separate the
> > > pieces and log them individually.
> > >
> > > The audit function was updated to log the additional information in the
> > > AUDIT_FANOTIFY record. The following is an example of the new record
> > > format:
> > >
> > > type=FANOTIFY msg=audit(1600385147.372:590): resp=2 fan_type=1 fan_ctx=17
> >
> > It might have been a good idea to tag this as RFC...  I have a few
> > questions:
> >
> > 1. Where did "resp=" come from?
> 
> According to the git log, it came from Steve Grubb via de8cd83e91bc
> ("audit: Record fanotify access control decisions").  Steve should
> have known what he was doing with respect to field names so I'm
> assuming he had a reason.
> 
> > It isn't in the field dictionary.  It
> > seems like a needless duplication of "res=".  If it isn't, maybe it
> > should have a "fan_" namespace prefix and become "fan_res="?
> 
> Regardless of what it should have been, it is "resp" now and we can't
> really change it.  As far as the field dictionary is concerned, while
> we should document these fields, it is important to note that when the
> dictionary conflicts with the kernel, the kernel wins by definition.

Agree on all counts.  It was an open-ended question.  It is also moot
since it is even expected in the audit-testsuite and would break that if
it were changed.

> > 2. It appears I'm ok changing the "__u32 response" to "__u16" without
> > breaking old userspace.  Is this true on all arches?
> 
> I can't answer that for you, the fanotify folks will need to look at
> that, but you likely already know that.  While I haven't gone through
> the entire patchset yet, if it was me I probably would have left
> response as a u32 and just added the extra fields; you are already
> increasing the size of fanotify_response so why bother with shrinking
> an existing field?

I was thinking of that, but chose to follow the lead of the fanotify
mainainer.

> > 3. What should be the action if response contains unknown flags or
> > types?  Is it reasonable to return -EINVAL?
> 
> Once again, not a fanotify expert, but EINVAL is intended for invalid
> input so it seems like a reasonable choice.

The choice of the error code wasn't in question but rather the need to
fail rather than ignore unknown flags.

> > 4. Currently, struct fanotify_response has a fixed size, but if future
> > types get defined that have variable buffer sizes, how would that be
> > communicated or encoded?
> 
> If that is a concern, you should probably include a length field in
> the structure before the variable length field.  You can't put it
> before fd or response, so it's really a question of before or after
> your new extra_info_type; I might suggest *after* extra_info_type, but
> that's just me.

After extra_info_type is what I was thinking.  The other possibility is
that a type with a variable length field could define its data size as
the first field within the variable field as set out in the format of
that varible length field so that all the fixed length fields would not
need to waste that space or bandwidth.

Thanks for the feedback.

> paul-moore.com

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635