audit: Report suspicious O_CREAT usage

This renames the very specific audit_log_link_denied() to audit_log_path_denied() and adds the AUDIT_* type as an argument. This allows for the creation of the new AUDIT_ANOM_CREAT that can be used to report the fifo/regular file creation restrictions that were introduced in commit 30aba6656f61 ("namei: allow restricted O_CREAT of FIFOs and regular files"). Without this change, discovering that the restriction is enabled can be very challenging:
https://lore.kernel.org/lkml/CA+jJMxvkqjXHy3DnV5MVhFTL2RUhg0WQ-XVFW3ngDQOdkFq0PA@mail.gmail.com

Reported-by Jérémie Galarneau <[email protected]>

Signed-off-by Kees Cook <[email protected]>

---
This is not a complete fix because reporting was broken in commit 15564ff0a16e ("audit: make ANOM_LINK obey audit_enabled and audit_dummy_context")
which specifically goes against the intention of these records: they should _always_ be reported. If auditing isn't enabled, they should be ratelimited.

Instead of using audit, should this just go back to using pr_ratelimited()?

---
 fs/namei.c                 |  7 +++++--
 include/linux/audit.h      |  5 +++--
 include/uapi/linux/audit.h |  1 +
 kernel/audit.c             | 11 ++++++-----
 4 files changed, 15 insertions(+), 9 deletions(-)

fs/namei.c +5 -2

index 671c3c1a3425..0e60f81e1d5a 100644
@@ -925,7 +925,7 @@ static inline int may_follow_link(struct nameidata *nd)
 		return -ECHILD;
 
 	audit_inode(nd->name, nd->stack[0].link.dentry, 0);
-	audit_log_link_denied("follow_link");
+	audit_log_path_denied(AUDIT_ANOM_LINK, "follow_link");
 	return -EACCES;
 }
 

@@ -993,7 +993,7 @@ static int may_linkat(struct path *link)
 	if (safe_hardlink_source(inode) || inode_owner_or_capable(inode))
 		return 0;
 
-	audit_log_link_denied("linkat");
+	audit_log_path_denied(AUDIT_ANOM_LINK, "linkat");
 	return -EPERM;
 }
 

@@ -1031,6 +1031,9 @@ static int may_create_in_sticky(struct dentry * const dir,
 	    (dir->d_inode->i_mode & 0020 &&
 	     ((sysctl_protected_fifos >= 2 && S_ISFIFO(inode->i_mode)) ||
 	      (sysctl_protected_regular >= 2 && S_ISREG(inode->i_mode))))) {
+		audit_log_path_denied(AUDIT_ANOM_CREAT,
+				      S_ISFIFO(inode->i_mode) ? "fifo"
+							      : "regular");
 		return -EACCES;
 	}
 	return 0;

include/linux/audit.h +2 -1

index aee3dc9eb378..b3715e2ee1c5 100644
@@ -156,7 +156,8 @@ extern void		    audit_log_d_path(struct audit_buffer *ab,
 					     const struct path *path);
 extern void		    audit_log_key(struct audit_buffer *ab,
 					  char *key);
-extern void		    audit_log_link_denied(const char *operation);
+extern void		    audit_log_path_denied(int type,
+						  const char *operation);
 extern void		    audit_log_lost(const char *message);
 
 extern int audit_log_task_context(struct audit_buffer *ab);

include/linux/audit.h +0 -0

 { }
 static inline int audit_log_task_context(struct audit_buffer *ab)
 {

include/uapi/linux/audit.h +1 -0

index c89c6495983d..3ad935527177 100644
@@ -143,6 +143,7 @@
 #define AUDIT_ANOM_PROMISCUOUS      1700 /* Device changed promiscuous mode */
 #define AUDIT_ANOM_ABEND            1701 /* Process ended abnormally */
 #define AUDIT_ANOM_LINK		    1702 /* Suspicious use of file links */
+#define AUDIT_ANOM_CREAT	    1703 /* Suspicious file creation */
 #define AUDIT_INTEGRITY_DATA	    1800 /* Data integrity verification */
 #define AUDIT_INTEGRITY_METADATA    1801 /* Metadata integrity verification */
 #define AUDIT_INTEGRITY_STATUS	    1802 /* Integrity enable status */

kernel/audit.c +6 -5

index da8dc0db5bd3..ed7402ac81b6 100644
@@ -2155,18 +2155,19 @@ void audit_log_task_info(struct audit_buffer *ab)
 EXPORT_SYMBOL(audit_log_task_info);
 
 /**
- * audit_log_link_denied - report a link restriction denial
- * @operation: specific link operation
+ * audit_log_path_denied - report a path restriction denial
+ * @type: audit message type (AUDIT_ANOM_LINK, AUDIT_ANOM_CREAT, etc)
+ * @operation: specific operation name
  */
-void audit_log_link_denied(const char *operation)
+void audit_log_path_denied(int type, const char *operation)
 {
 	struct audit_buffer *ab;
 
 	if (!audit_enabled || audit_dummy_context())
 		return;
 
-	/* Generate AUDIT_ANOM_LINK with subject, operation, outcome. */
-	ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_ANOM_LINK);
+	/* Generate log with subject, operation, outcome. */
+	ab = audit_log_start(audit_context(), GFP_KERNEL, type);
 	if (!ab)
 		return;
 	audit_log_format(ab, "op=%s", operation);

Signature · 5 lines

2.17.1


-- 
Kees Cook

This renames the very specific audit_log_link_denied() to audit_log_path_denied() and adds the AUDIT_* type as an argument. This allows for the creation of the new AUDIT_ANOM_CREAT that can be used to report the fifo/regular file creation restrictions that were introduced in commit 30aba6656f61 ("namei: allow restricted O_CREAT of FIFOs and regular files"). Without this change, discovering that the restriction is enabled can be very challenging:
https://lore.kernel.org/lkml/CA+jJMxvkqjXHy3DnV5MVhFTL2RUhg0WQ-XVFW3ngDQOdkFq0PA@mail.gmail.com

Reported-by Jérémie Galarneau <[email protected]>

Signed-off-by Kees Cook <[email protected]>

---
This is not a complete fix because reporting was broken in commit 15564ff0a16e ("audit: make ANOM_LINK obey audit_enabled and audit_dummy_context")
which specifically goes against the intention of these records: they should _always_ be reported. If auditing isn't enabled, they should be ratelimited.

Instead of using audit, should this just go back to using pr_ratelimited()?

Quoted 20 lines · Kees Cook

This renames the very specific audit_log_link_denied() to audit_log_path_denied() and adds the AUDIT_* type as an argument. This allows for the creation of the new AUDIT_ANOM_CREAT that can be used to report the fifo/regular file creation restrictions that were introduced in commit 30aba6656f61 ("namei: allow restricted O_CREAT of FIFOs and regular files"). Without this change, discovering that the restriction is enabled can be very challenging:
https://lore.kernel.org/lkml/CA+jJMxvkqjXHy3DnV5MVhFTL2RUhg0WQ-XVFW3ngDQO
[email protected]

Reported-by Jérémie Galarneau <[email protected]>

Signed-off-by Kees Cook <[email protected]>

---
This is not a complete fix because reporting was broken in commit 15564ff0a16e ("audit: make ANOM_LINK obey audit_enabled and audit_dummy_context")
which specifically goes against the intention of these records: they should _always_ be reported. If auditing isn't enabled, they should be ratelimited.

Quoted 2 lines

Instead of using audit, should this just go back to using pr_ratelimited()?

I'm going to ignore the rename and other aspects of this patch for the moment so we can focus on the topic of if/when/how these records should be emitted by the kernel.

Unfortunately, people tend to get very upset if audit emits *any* records when they haven't explicitly enabled audit, the significance of the record doesn't seem to matter, which is why you see patches like 15564ff0a16e ("audit: make ANOM_LINK obey audit_enabled and audit_dummy_context"). We could consider converting some records to printk()s, rate-limited or not, but we need to balance this with the various security certifications which audit was created to satisfy. In some cases a printk() isn't sufficient.

Steve is probably the only one who really keeps track of the various auditing requirements of the different security certifications; what say you Steve on this issue with ANOM_CREAT records?

Quoted 37 lines · EDT Paul Moore

Quoted 20 lines · Kees Cook

This renames the very specific audit_log_link_denied() to audit_log_path_denied() and adds the AUDIT_* type as an argument. This allows for the creation of the new AUDIT_ANOM_CREAT that can be used to report the fifo/regular file creation restrictions that were introduced in commit 30aba6656f61 ("namei: allow restricted O_CREAT of FIFOs and regular files"). Without this change, discovering that the restriction is enabled can be very challenging:
https://lore.kernel.org/lkml/CA+jJMxvkqjXHy3DnV5MVhFTL2RUhg0WQ-XVFW3ngDQO
[email protected]

Reported-by Jérémie Galarneau <[email protected]>

Signed-off-by Kees Cook <[email protected]>

---
This is not a complete fix because reporting was broken in commit 15564ff0a16e ("audit: make ANOM_LINK obey audit_enabled and audit_dummy_context")
which specifically goes against the intention of these records: they should _always_ be reported. If auditing isn't enabled, they should be ratelimited.

Quoted 2 lines

Instead of using audit, should this just go back to using pr_ratelimited()?

I'm going to ignore the rename and other aspects of this patch for the moment so we can focus on the topic of if/when/how these records should be emitted by the kernel.

Unfortunately, people tend to get very upset if audit emits *any* records when they haven't explicitly enabled audit, the significance of the record doesn't seem to matter, which is why you see patches like 15564ff0a16e ("audit: make ANOM_LINK obey audit_enabled and audit_dummy_context"). We could consider converting some records to printk()s, rate-limited or not, but we need to balance this with the various security certifications which audit was created to satisfy. In some cases a printk() isn't sufficient.

Quoted 3 lines

Steve is probably the only one who really keeps track of the various auditing requirements of the different security certifications; what say you Steve on this issue with ANOM_CREAT records?

Common Criteria and other security standards I track do not call out for anomoly detection. So, there are no requirements on this. That said, we do have other anomaly detections because they give early warning that something strange is happening. I think adding this event is a nice improvement as long as it obeys audit_enabled before emitting an event - for example, look at the AUDIT_ANOM_ABEND event.

Quoted 42 lines · Steve Grubb

Quoted 37 lines · EDT Paul Moore

Quoted 20 lines · Kees Cook

This renames the very specific audit_log_link_denied() to audit_log_path_denied() and adds the AUDIT_* type as an argument. This allows for the creation of the new AUDIT_ANOM_CREAT that can be used to report the fifo/regular file creation restrictions that were introduced in commit 30aba6656f61 ("namei: allow restricted O_CREAT of FIFOs and regular files"). Without this change, discovering that the restriction is enabled can be very challenging:
https://lore.kernel.org/lkml/CA+jJMxvkqjXHy3DnV5MVhFTL2RUhg0WQ-XVFW3ngDQO
[email protected]

Reported-by Jérémie Galarneau <[email protected]>

Signed-off-by Kees Cook <[email protected]>

---
This is not a complete fix because reporting was broken in commit 15564ff0a16e ("audit: make ANOM_LINK obey audit_enabled and audit_dummy_context")
which specifically goes against the intention of these records: they should _always_ be reported. If auditing isn't enabled, they should be ratelimited.

Quoted 2 lines

Instead of using audit, should this just go back to using pr_ratelimited()?

I'm going to ignore the rename and other aspects of this patch for the moment so we can focus on the topic of if/when/how these records should be emitted by the kernel.

Unfortunately, people tend to get very upset if audit emits *any* records when they haven't explicitly enabled audit, the significance of the record doesn't seem to matter, which is why you see patches like 15564ff0a16e ("audit: make ANOM_LINK obey audit_enabled and audit_dummy_context"). We could consider converting some records to printk()s, rate-limited or not, but we need to balance this with the various security certifications which audit was created to satisfy. In some cases a printk() isn't sufficient.

Quoted 3 lines

Steve is probably the only one who really keeps track of the various auditing requirements of the different security certifications; what say you Steve on this issue with ANOM_CREAT records?

Quoted 6 lines

Common Criteria and other security standards I track do not call out for anomoly detection. So, there are no requirements on this. That said, we do have other anomaly detections because they give early warning that something strange is happening. I think adding this event is a nice improvement as long as it obeys audit_enabled before emitting an event - for example, look at the AUDIT_ANOM_ABEND event.

Okay, so the patch is good as-is? (The "report things always" issue I will deal with separately. For now I'd just like to gain this anomaly detection corner case...)

Paul, what do you see as next steps here?

This renames the very specific audit_log_link_denied() to audit_log_path_denied() and adds the AUDIT_* type as an argument. This allows for the creation of the new AUDIT_ANOM_CREAT that can be used to report the fifo/regular file creation restrictions that were introduced in commit 30aba6656f61 ("namei: allow restricted O_CREAT of FIFOs and regular files"). Without this change, discovering that the restriction is enabled can be very challenging:
https://lore.kernel.org/lkml/CA+jJMxvkqjXHy3DnV5MVhFTL2RUhg0WQ-XVFW3ngDQOdkFq0PA@mail.gmail.com

Reported-by Jérémie Galarneau <[email protected]>

Signed-off-by Kees Cook <[email protected]>

---
This is not a complete fix because reporting was broken in commit 15564ff0a16e ("audit: make ANOM_LINK obey audit_enabled and audit_dummy_context")
which specifically goes against the intention of these records: they should _always_ be reported. If auditing isn't enabled, they should be ratelimited.

Instead of using audit, should this just go back to using pr_ratelimited()?

---
 fs/namei.c                 |  7 +++++--
 include/linux/audit.h      |  5 +++--
 include/uapi/linux/audit.h |  1 +
 kernel/audit.c             | 11 ++++++-----
 4 files changed, 15 insertions(+), 9 deletions(-)