On Tue, Jan 05, 2021 at 04:50:05PM +0000, Al Viro wrote:
> LSM_AUDIT_DATA_DENTRY is easy to handle - wrap
> audit_log_untrustedstring(ab, a->u.dentry->d_name.name);
> into grabbing/dropping a->u.dentry->d_lock and we are done.
Incidentally, LSM_AUDIT_DATA_DENTRY in mainline is *not* safe wrt
rename() - for long-named dentries it is possible to get preempted
in the middle of
audit_log_untrustedstring(ab, a->u.dentry->d_name.name);
and have the bugger renamed, with old name ending up freed. The
same goes for LSM_AUDIT_DATA_INODE...
Folks, ->d_name.name is not automatically stable and the memory it
points to is not always guaranteed to last as long as dentry itself does.
In something like ->rename(), ->mkdir(), etc. - sure, we have the parent
->i_rwsem held exclusive and nothing's going to rename dentry under us.
But there's a reason why e.g. d_path() has to be careful. And there
are selinux and smack hooks using LSM_AUDIT_DATA_DENTRY in locking
environment that does not exclude renames.
AFAICS, that race goes back to 2004: 605303cc340d ("[PATCH] selinux: Add dname
to audit output when a path cannot be generated.") in historical tree is where
its first ancestor appears...
The minimal fix is to grab ->d_lock around these audit_log_untrustedstring()
calls, and IMO that's -stable fodder. It is a slow path (we are spewing an
audit record, not to mention anything else), so I don't believe it's worth
trying to do anything fancier than that.
How about the following? If nobody objects, I'll drop it into #fixes and
send a pull request in a few days...
dump_common_audit_data(): fix racy accesses to ->d_name
We are not guaranteed the locking environment that would prevent
dentry getting renamed right under us. And it's possible for
old long name to be freed after rename, leading to UAF here.
Cc: stable@xxxxxxxxxx # v2.6.2+
Signed-off-by: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
---
diff --git a/security/lsm_audit.c b/security/lsm_audit.c
index 7d8026f3f377..a0cd28cd31a8 100644
--- a/security/lsm_audit.c
+++ b/security/lsm_audit.c
@@ -275,7 +275,9 @@ static void dump_common_audit_data(struct audit_buffer *ab,
struct inode *inode;
audit_log_format(ab, " name=");
+ spin_lock(&a->u.dentry->d_lock);
audit_log_untrustedstring(ab, a->u.dentry->d_name.name);
+ spin_unlock(&a->u.dentry->d_lock);
inode = d_backing_inode(a->u.dentry);
if (inode) {
@@ -293,8 +295,9 @@ static void dump_common_audit_data(struct audit_buffer *ab,
dentry = d_find_alias(inode);
if (dentry) {
audit_log_format(ab, " name=");
- audit_log_untrustedstring(ab,
- dentry->d_name.name);
+ spin_lock(&dentry->d_lock);
+ audit_log_untrustedstring(ab, dentry->d_name.name);
+ spin_unlock(&dentry->d_lock);
dput(dentry);
}
audit_log_format(ab, " dev=");
