Linux-Security-Module Archive on lore.kernel.org
 help / color / Atom feed
From: Aaron Goidel <acgoide@tycho.nsa.gov>
To: Paul Moore <paul@paul-moore.com>, Stephen Smalley <sds@tycho.nsa.gov>
Cc: Richard Guy Briggs <rgb@redhat.com>,
	mortonm@chromium.org, john.johansen@canonical.com,
	selinux@vger.kernel.org, James Morris <jmorris@namei.org>,
	luto@amacapital.net, linux-security-module@vger.kernel.org,
	linux-audit@redhat.com, Nicholas Franck <nhfran2@tycho.nsa.gov>,
	Serge Hallyn <serge@hallyn.com>
Subject: Re: [Non-DoD Source] Re: [RFC PATCH v2] security, capability: pass object information to security_capable
Date: Thu, 15 Aug 2019 09:10:08 -0400
Message-ID: <cb2833ee-3a12-9c7d-6c5b-c7944e74b3e9@tycho.nsa.gov> (raw)
In-Reply-To: <CAHC9VhRzz52bVwMikM7C65vCCSLb0=y1HtB50o-H0G3AMHqRNw@mail.gmail.com>

On 8/14/19 5:27 PM, Paul Moore wrote:
> On Wed, Aug 14, 2019 at 5:08 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
>> On 8/14/19 3:59 PM, Paul Moore wrote:
>>> On Tue, Aug 13, 2019 at 5:27 PM Richard Guy Briggs <rgb@redhat.com> wrote:
>>>> On 2019-08-13 11:01, Aaron Goidel wrote:
>>>>> On 8/8/19 12:30 PM, Paul Moore wrote:
>>>>>> On Thu, Aug 1, 2019 at 10:43 AM Aaron Goidel <acgoide@tycho.nsa.gov> wrote:
>>>>>>> From: Nicholas Franck <nhfran2@tycho.nsa.gov>
>>>>>>>
>>>>>>> At present security_capable does not pass any object information
>>>>>>> and therefore can neither audit the particular object nor take it
>>>>>>> into account. Augment the security_capable interface to support
>>>>>>> passing supplementary data. Use this facility initially to convey
>>>>>>> the inode for capability checks relevant to inodes. This only
>>>>>>> addresses capable_wrt_inode_uidgid calls; other capability checks
>>>>>>> relevant to inodes will be addressed in subsequent changes. In the
>>>>>>> future, this will be further extended to pass object information for
>>>>>>> other capability checks such as the target task for CAP_KILL.
>>>>>>>
>>>>>>> In SELinux this new information is leveraged here to include the inode
>>>>>>> in the audit message. In the future, it could also be used to perform
>>>>>>> a per inode capability checks.
>>>>>>>
>>>>>>> It would be possible to fold the existing opts argument into this new
>>>>>>> supplementary data structure. This was omitted from this change to
>>>>>>> minimize changes.
>>>>>>>
>>>>>>> Signed-off-by: Nicholas Franck <nhfran2@tycho.nsa.gov>
>>>>>>> Signed-off-by: Aaron Goidel <acgoide@tycho.nsa.gov>
>>>>>>> ---
>>>>>>> v2:
>>>>>>> - Changed order of audit prints so optional information comes second
>>>>>>> ---
>>>>>>>     include/linux/capability.h             |  7 ++++++
>>>>>>>     include/linux/lsm_audit.h              |  5 +++-
>>>>>>>     include/linux/lsm_hooks.h              |  3 ++-
>>>>>>>     include/linux/security.h               | 23 +++++++++++++-----
>>>>>>>     kernel/capability.c                    | 33 ++++++++++++++++++--------
>>>>>>>     kernel/seccomp.c                       |  2 +-
>>>>>>>     security/apparmor/capability.c         |  8 ++++---
>>>>>>>     security/apparmor/include/capability.h |  4 +++-
>>>>>>>     security/apparmor/ipc.c                |  2 +-
>>>>>>>     security/apparmor/lsm.c                |  5 ++--
>>>>>>>     security/apparmor/resource.c           |  2 +-
>>>>>>>     security/commoncap.c                   | 11 +++++----
>>>>>>>     security/lsm_audit.c                   | 21 ++++++++++++++--
>>>>>>>     security/safesetid/lsm.c               |  3 ++-
>>>>>>>     security/security.c                    |  5 ++--
>>>>>>>     security/selinux/hooks.c               | 20 +++++++++-------
>>>>>>>     security/smack/smack_access.c          |  2 +-
>>>>>>>     17 files changed, 110 insertions(+), 46 deletions(-)
>>>>>>
>>>>>> You should CC the linux-audit list, I've added them on this mail.
>>>>>>
>>>>>> I had hoped to see some thought put into the idea of dynamically
>>>>>> emitting the proper audit records as I mentioned in the previous patch
>>>>>> set, but regardless there are some comments on this code as written
>>>>>> ...
>>>>>>
>>>>>>> diff --git a/security/lsm_audit.c b/security/lsm_audit.c
>>>>>>> index 33028c098ef3..18cc7c956b69 100644
>>>>>>> --- a/security/lsm_audit.c
>>>>>>> +++ b/security/lsm_audit.c
>>>>>>> @@ -229,9 +229,26 @@ static void dump_common_audit_data(struct audit_buffer *ab,
>>>>>>>            case LSM_AUDIT_DATA_IPC:
>>>>>>>                    audit_log_format(ab, " key=%d ", a->u.ipc_id);
>>>>>>>                    break;
>>>>>>> -       case LSM_AUDIT_DATA_CAP:
>>>>>>> -               audit_log_format(ab, " capability=%d ", a->u.cap);
>>>>>>> +       case LSM_AUDIT_DATA_CAP: {
>>>>>>> +               const struct inode *inode;
>>>>>>> +
>>>>>>> +               audit_log_format(ab, " capability=%d ", a->u.cap_struct.cap);
>>>>>>> +               if (a->u.cap_struct.cad) {
>>>>>>> +                       switch (a->u.cap_struct.cad->type) {
>>>>>>> +                       case CAP_AUX_DATA_INODE: {
>>>>>>> +                               inode = a->u.cap_struct.cad->u.inode;
>>>>>>> +
>>>>>>> +                               audit_log_format(ab, " dev=");
>>>>>>> +                               audit_log_untrustedstring(ab,
>>>>>>> +                                       inode->i_sb->s_id);
>>>>>>> +                               audit_log_format(ab, " ino=%lu",
>>>>>>> +                                       inode->i_ino);
>>>>>>> +                               break;
>>>>>>> +                       }
>>>>>>
>>>>>> Since you are declaring "inode" further up, there doesn't appear to be
>>>>>> any need for the CAP_AUX_DATA_INODE braces, please remove them.
>>>>>>
>>>>>> The general recommended practice when it comes to "sometimes" fields
>>>>>> in an audit record, is to always record them in the record, but use a
>>>>>> value of "?" when there is nothing relevant to record.  For example,
>>>>>> when *not* recording inode information you would do something like the
>>>>>> following:
>>>>>>
>>>>>>      audit_log_format(ab, " dev=? ino=?");
>>>>>>
>>>>> The issue this brings up is what happens when this is expanded to more
>>>>> cases? Assuming there will be a case here for logging audit data for task
>>>>> based capabilities (CAP_AUX_DATA_TASK), what do we want to have happen if we
>>>>> are recording *neither* inode information nor task information (say a PID)?
>>>>> If we log something in the inode case, we presumably don't want to call
>>>>> audit_log_format(ab, " dev=?, pid=?") as well. (And vice versa for when we
>>>>> log a pid and no inode).
>>>>
>>>> Yup.  This record is already a mess due to that.
>>>>
>>>> The general solution is to either use a new record type for each
>>>> variant, or to use an auxiliary record for each additional piece of
>>>> information.  The long term solution that has been suggested it to move
>>>> away from a text message format.
>>>
>>> This is why I was suggesting that Aaron look into allowing the LSMs to
>>> request generation of audit records in the earlier thread (and
>>> mentioned it again at the top of my comments in this thread).
>>
>> How would that work? The behavior we want is to capture some information
>> about the inode whenever there is a capability denial upon an attempted
>> access to that inode.  Allowing the LSM to enable audit collection on a
>> per-process basis doesn't appear to help with that goal, because this is
>> something we want for all processes.  Further, we don't really want the
>> rest of the audit collection machinery engaged here ...
> 
> I read this as "we want to audit this information, but we don't to
> turn on the very thing which is designed to do this".  At some point,
> if you want to audit things, you actually need to turn on auditing.
> 
>> ... we just want the
>> inode information for this specific inode, and we have the inode readily
>> accessible in the caller of the LSM hook already, so we don't need to do
>> it earlier.
> 
> Aaron appeared to be complaining that if we stuck to the current best
> practices regarding record formatting for the LSM generated audit
> record, the record was going to get complicated when people started
> adding additional information.  FWIW, I don't disagree.  The only real
> alternative I've seen thus far is to look into having the LSM enable
> certain records, if anyone has another idea I'm all ears/eyes.
> 
>> Further, in the future we want to be able to take the inode security
>> label into consideration as part of the capability checking, which is
>> independent of audit entirely.  So the rest of the patch will still be
>> required even if the audit solution ends up being different.
> 
> That's a different topic, I don't think there are any remaining
> objections to passing the inode information here.
> 

I'm looking at how to enable LSMs to selectively turn on audit 
collection. So there seems to be two key points: audit_alloc() and 
__audit_syscall_entry(). Would it suffice to define a single boolean 
hook that takes the task and call it from both functions, to decide 
whether to override an AUDIT_DISABLED state in audit_alloc() and to 
override a 0 audit_n_rules in __audit_syscall_entry(). In audit_alloc() 
if audit_filter_task() returned AUDIT_DISABLED and the hook returned 
true, we would change the state to AUDIT_BUILD_CONTEXT. In 
__audit_syscall_entry(), if the hook returned true, we would set dummy 
to 0. Obviously, we could have a more general hook which lets us return 
arbitrary audit states, but, it isn't clear how we would reconcile 
conflicting results from audit_filter_task() and the hook for any 
situation other than AUDIT_DISABLED. We could also potentially use a 
different hook in __audit_syscall_entry(), though I don't think that we 
want the LSMs trying to interpret the syscall number or arguments.

Do you think that is sufficiently general or would you suggest something 
different?

-- 
Aaron

  reply index

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-01 14:43 [RFC PATCH v2] security,capability: " Aaron Goidel
2019-08-08 16:30 ` Paul Moore
2019-08-13 15:01   ` [Non-DoD Source] Re: [RFC PATCH v2] security, capability: " Aaron Goidel
2019-08-13 21:27     ` Richard Guy Briggs
2019-08-14 19:59       ` Paul Moore
2019-08-14 21:08         ` Stephen Smalley
2019-08-14 21:27           ` Paul Moore
2019-08-15 13:10             ` Aaron Goidel [this message]
2019-08-16 16:29               ` [Non-DoD Source] " Paul Moore

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=cb2833ee-3a12-9c7d-6c5b-c7944e74b3e9@tycho.nsa.gov \
    --to=acgoide@tycho.nsa.gov \
    --cc=jmorris@namei.org \
    --cc=john.johansen@canonical.com \
    --cc=linux-audit@redhat.com \
    --cc=linux-security-module@vger.kernel.org \
    --cc=luto@amacapital.net \
    --cc=mortonm@chromium.org \
    --cc=nhfran2@tycho.nsa.gov \
    --cc=paul@paul-moore.com \
    --cc=rgb@redhat.com \
    --cc=sds@tycho.nsa.gov \
    --cc=selinux@vger.kernel.org \
    --cc=serge@hallyn.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Security-Module Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-security-module/0 linux-security-module/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-security-module linux-security-module/ https://lore.kernel.org/linux-security-module \
		linux-security-module@vger.kernel.org linux-security-module@archiver.kernel.org
	public-inbox-index linux-security-module


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-security-module


AGPL code for this site: git clone https://public-inbox.org/ public-inbox