On 8/14/19 5:27 PM, Paul Moore wrote:
On Wed, Aug 14, 2019 at 5:08 PM Stephen Smalley <sds@xxxxxxxxxxxxx> wrote:
On 8/14/19 3:59 PM, Paul Moore wrote:
On Tue, Aug 13, 2019 at 5:27 PM Richard Guy Briggs <rgb@xxxxxxxxxx> 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@xxxxxxxxxxxxx> wrote:
From: Nicholas Franck <nhfran2@xxxxxxxxxxxxx>
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@xxxxxxxxxxxxx>
Signed-off-by: Aaron Goidel <acgoide@xxxxxxxxxxxxx>
---
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
