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(-)
diff --git a/fs/namei.c b/fs/namei.c
index 671c3c1a3425..0e60f81e1d5a 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -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;
diff --git a/include/linux/audit.h b/include/linux/audit.h
index aee3dc9eb378..b3715e2ee1c5 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -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);
@@ -217,7 +218,7 @@ static inline void audit_log_d_path(struct audit_buffer *ab,
{ }
static inline void audit_log_key(struct audit_buffer *ab, char *key)
{ }
-static inline void audit_log_link_denied(const char *string)
+static inline void audit_log_path_denied(int type, const char *string);
{ }
static inline int audit_log_task_context(struct audit_buffer *ab)
{
diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
index c89c6495983d..3ad935527177 100644
--- a/include/uapi/linux/audit.h
+++ b/include/uapi/linux/audit.h
@@ -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 */
diff --git a/kernel/audit.c b/kernel/audit.c
index da8dc0db5bd3..ed7402ac81b6 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -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);
--
2.17.1
--
Kees Cook
Hi Kees,
I love your patch! Yet something to improve:
[auto build test ERROR on linus/master]
[cannot apply to v5.3 next-20190924]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Kees-Cook/audit-Report-suspicious-O_CREAT-usage/20190926-050423
config: i386-tinyconfig (attached as .config)
compiler: gcc-7 (Debian 7.4.0-13) 7.4.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <[email protected]>
All errors (new ones prefixed by >>):
In file included from include/linux/fsnotify.h:16:0,
from fs///attr.c:15:
>> include/linux/audit.h:222:1: error: expected identifier or '(' before '{' token
{ }
^
include/linux/audit.h:221:20: warning: 'audit_log_path_denied' declared 'static' but never defined [-Wunused-function]
static inline void audit_log_path_denied(int type, const char *string);
^~~~~~~~~~~~~~~~~~~~~
vim +222 include/linux/audit.h
b48345aafb2038 Richard Guy Briggs 2019-05-10 187
96368701e1c890 Paul Moore 2016-01-13 188 #else /* CONFIG_AUDIT */
96368701e1c890 Paul Moore 2016-01-13 189 static inline __printf(4, 5)
96368701e1c890 Paul Moore 2016-01-13 190 void audit_log(struct audit_context *ctx, gfp_t gfp_mask, int type,
96368701e1c890 Paul Moore 2016-01-13 191 const char *fmt, ...)
96368701e1c890 Paul Moore 2016-01-13 192 { }
96368701e1c890 Paul Moore 2016-01-13 193 static inline struct audit_buffer *audit_log_start(struct audit_context *ctx,
96368701e1c890 Paul Moore 2016-01-13 194 gfp_t gfp_mask, int type)
96368701e1c890 Paul Moore 2016-01-13 195 {
96368701e1c890 Paul Moore 2016-01-13 196 return NULL;
96368701e1c890 Paul Moore 2016-01-13 197 }
96368701e1c890 Paul Moore 2016-01-13 198 static inline __printf(2, 3)
96368701e1c890 Paul Moore 2016-01-13 199 void audit_log_format(struct audit_buffer *ab, const char *fmt, ...)
96368701e1c890 Paul Moore 2016-01-13 200 { }
96368701e1c890 Paul Moore 2016-01-13 201 static inline void audit_log_end(struct audit_buffer *ab)
96368701e1c890 Paul Moore 2016-01-13 202 { }
96368701e1c890 Paul Moore 2016-01-13 203 static inline void audit_log_n_hex(struct audit_buffer *ab,
96368701e1c890 Paul Moore 2016-01-13 204 const unsigned char *buf, size_t len)
96368701e1c890 Paul Moore 2016-01-13 205 { }
96368701e1c890 Paul Moore 2016-01-13 206 static inline void audit_log_n_string(struct audit_buffer *ab,
96368701e1c890 Paul Moore 2016-01-13 207 const char *buf, size_t n)
96368701e1c890 Paul Moore 2016-01-13 208 { }
96368701e1c890 Paul Moore 2016-01-13 209 static inline void audit_log_n_untrustedstring(struct audit_buffer *ab,
96368701e1c890 Paul Moore 2016-01-13 210 const char *string, size_t n)
96368701e1c890 Paul Moore 2016-01-13 211 { }
96368701e1c890 Paul Moore 2016-01-13 212 static inline void audit_log_untrustedstring(struct audit_buffer *ab,
96368701e1c890 Paul Moore 2016-01-13 213 const char *string)
96368701e1c890 Paul Moore 2016-01-13 214 { }
96368701e1c890 Paul Moore 2016-01-13 215 static inline void audit_log_d_path(struct audit_buffer *ab,
96368701e1c890 Paul Moore 2016-01-13 216 const char *prefix,
96368701e1c890 Paul Moore 2016-01-13 217 const struct path *path)
96368701e1c890 Paul Moore 2016-01-13 218 { }
96368701e1c890 Paul Moore 2016-01-13 219 static inline void audit_log_key(struct audit_buffer *ab, char *key)
96368701e1c890 Paul Moore 2016-01-13 220 { }
21e61058cf0f22 Kees Cook 2019-09-25 221 static inline void audit_log_path_denied(int type, const char *string);
96368701e1c890 Paul Moore 2016-01-13 @222 { }
96368701e1c890 Paul Moore 2016-01-13 223 static inline int audit_log_task_context(struct audit_buffer *ab)
96368701e1c890 Paul Moore 2016-01-13 224 {
96368701e1c890 Paul Moore 2016-01-13 225 return 0;
96368701e1c890 Paul Moore 2016-01-13 226 }
2a1fe215e7300c Paul Moore 2018-11-26 227 static inline void audit_log_task_info(struct audit_buffer *ab)
96368701e1c890 Paul Moore 2016-01-13 228 { }
4b7d248b3a1de4 Richard Guy Briggs 2019-01-22 229
:::::: The code at line 222 was first introduced by commit
:::::: 96368701e1c89057bbf39222e965161c68a85b4b audit: force seccomp event logging to honor the audit_enabled flag
:::::: TO: Paul Moore <[email protected]>
:::::: CC: Paul Moore <[email protected]>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Kees,
I love your patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[cannot apply to v5.3 next-20190924]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Kees-Cook/audit-Report-suspicious-O_CREAT-usage/20190926-050423
config: i386-tinyconfig (attached as .config)
compiler: gcc-7 (Debian 7.4.0-13) 7.4.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <[email protected]>
All warnings (new ones prefixed by >>):
In file included from include/linux/fsnotify.h:16:0,
from fs/namei.c:25:
include/linux/audit.h:222:1: error: expected identifier or '(' before '{' token
{ }
^
In file included from include/linux/fsnotify.h:16:0,
from fs/namei.c:25:
>> include/linux/audit.h:221:20: warning: 'audit_log_path_denied' used but never defined
static inline void audit_log_path_denied(int type, const char *string);
^~~~~~~~~~~~~~~~~~~~~
vim +/audit_log_path_denied +221 include/linux/audit.h
187
188 #else /* CONFIG_AUDIT */
189 static inline __printf(4, 5)
190 void audit_log(struct audit_context *ctx, gfp_t gfp_mask, int type,
191 const char *fmt, ...)
192 { }
193 static inline struct audit_buffer *audit_log_start(struct audit_context *ctx,
194 gfp_t gfp_mask, int type)
195 {
196 return NULL;
197 }
198 static inline __printf(2, 3)
199 void audit_log_format(struct audit_buffer *ab, const char *fmt, ...)
200 { }
201 static inline void audit_log_end(struct audit_buffer *ab)
202 { }
203 static inline void audit_log_n_hex(struct audit_buffer *ab,
204 const unsigned char *buf, size_t len)
205 { }
206 static inline void audit_log_n_string(struct audit_buffer *ab,
207 const char *buf, size_t n)
208 { }
209 static inline void audit_log_n_untrustedstring(struct audit_buffer *ab,
210 const char *string, size_t n)
211 { }
212 static inline void audit_log_untrustedstring(struct audit_buffer *ab,
213 const char *string)
214 { }
215 static inline void audit_log_d_path(struct audit_buffer *ab,
216 const char *prefix,
217 const struct path *path)
218 { }
219 static inline void audit_log_key(struct audit_buffer *ab, char *key)
220 { }
> 221 static inline void audit_log_path_denied(int type, const char *string);
> 222 { }
223 static inline int audit_log_task_context(struct audit_buffer *ab)
224 {
225 return 0;
226 }
227 static inline void audit_log_task_info(struct audit_buffer *ab)
228 { }
229
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Wed, Sep 25, 2019 at 02:02:33PM -0700, Kees Cook wrote:
> 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(-)
>
> diff --git a/fs/namei.c b/fs/namei.c
> index 671c3c1a3425..0e60f81e1d5a 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -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;
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index aee3dc9eb378..b3715e2ee1c5 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -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);
> @@ -217,7 +218,7 @@ static inline void audit_log_d_path(struct audit_buffer *ab,
> { }
> static inline void audit_log_key(struct audit_buffer *ab, char *key)
> { }
> -static inline void audit_log_link_denied(const char *string)
> +static inline void audit_log_path_denied(int type, const char *string);
Oops, typo above (should be no trailing ";"). Thanks 0day-bot! I didn't
build without CONFIG_AUDIT. :)
-Kees
> { }
> static inline int audit_log_task_context(struct audit_buffer *ab)
> {
> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> index c89c6495983d..3ad935527177 100644
> --- a/include/uapi/linux/audit.h
> +++ b/include/uapi/linux/audit.h
> @@ -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 */
> diff --git a/kernel/audit.c b/kernel/audit.c
> index da8dc0db5bd3..ed7402ac81b6 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -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);
> --
> 2.17.1
>
>
> --
> Kees Cook
--
Kees Cook
On Wed, Sep 25, 2019 at 5:02 PM Kees Cook <[email protected]> wrote:
>
> 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()?
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?
--
paul moore
http://www.paul-moore.com
On Thursday, September 26, 2019 11:31:32 AM EDT Paul Moore wrote:
> On Wed, Sep 25, 2019 at 5:02 PM Kees Cook <[email protected]> wrote:
> > 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.
> >
> > 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?
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.
-Steve
On Mon, Sep 30, 2019 at 09:50:00AM -0400, Steve Grubb wrote:
> On Thursday, September 26, 2019 11:31:32 AM EDT Paul Moore wrote:
> > On Wed, Sep 25, 2019 at 5:02 PM Kees Cook <[email protected]> wrote:
> > > 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.
> > >
> > > 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?
>
> 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?
--
Kees Cook
On Mon, Sep 30, 2019 at 2:29 PM Kees Cook <[email protected]> wrote:
> On Mon, Sep 30, 2019 at 09:50:00AM -0400, Steve Grubb wrote:
> > On Thursday, September 26, 2019 11:31:32 AM EDT Paul Moore wrote:
> > > On Wed, Sep 25, 2019 at 5:02 PM Kees Cook <[email protected]> wrote:
> > > > 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.
> > > >
> > > > 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?
> >
> > 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?
I'll reply back on the original post so I can more easily comment on
the details of patch.
--
paul moore
http://www.paul-moore.com
On Wed, Sep 25, 2019 at 5:02 PM Kees Cook <[email protected]> wrote:
>
> 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(-)
...
> diff --git a/fs/namei.c b/fs/namei.c
> index 671c3c1a3425..0e60f81e1d5a 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -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;
Other callers typically pass an operation value more closely tied to
the syscall/function name, and that somewhat makes sense since the
syscall/function name is often verb-ish. The code above, while
helpful in the sense that it distinguishes between types of inodes, it
doesn't give much indication about the "operation" which failed. I'm
open to suggestions, but something like "sticky_create_fifo" seems
more consistent which current usage. Thoughts?
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index aee3dc9eb378..b3715e2ee1c5 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -217,7 +218,7 @@ static inline void audit_log_d_path(struct audit_buffer *ab,
> { }
> static inline void audit_log_key(struct audit_buffer *ab, char *key)
> { }
> -static inline void audit_log_link_denied(const char *string)
> +static inline void audit_log_path_denied(int type, const char *string);
> { }
I probably wouldn't make you respin just for this, but since you may
need to respin this anyway, you might as well fix the above.
--
paul moore
http://www.paul-moore.com