| Message ID | 6768f7c7d9804216853e6e9c59e44f8a10f46b99.1577830902.git.rgb@redhat.com |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | Address NETFILTER_CFG issues | expand |
Richard Guy Briggs <rgb@redhat.com> wrote: > diff --git a/include/linux/audit.h b/include/linux/audit.h > index 96cabb095eed..5eab4d898c26 100644 > --- a/include/linux/audit.h > +++ b/include/linux/audit.h > @@ -379,7 +379,7 @@ extern int __audit_log_bprm_fcaps(struct linux_binprm *bprm, > extern void __audit_fanotify(unsigned int response); > extern void __audit_tk_injoffset(struct timespec64 offset); > extern void __audit_ntp_log(const struct audit_ntp_data *ad); > -extern void __audit_nf_cfg(const char *name, u8 af, int nentries); > +extern void __audit_nf_cfg(const char *name, u8 af, int nentries, int op); Consider adding an enum instead of int op. > if (audit_enabled) > - audit_nf_cfg(repl->name, AF_BRIDGE, repl->nentries); > + audit_nf_cfg(repl->name, AF_BRIDGE, repl->nentries, 1); audit_nf_cfg(repl->name, AF_BRIDGE, repl->nentries, AUDIT_XT_OP_REPLACE); ... would be a bit more readable than '1'. The name is just an example, you can pick something else.
On Mon, Jan 6, 2020 at 3:23 PM Florian Westphal <fw@strlen.de> wrote: > Richard Guy Briggs <rgb@redhat.com> wrote: > > diff --git a/include/linux/audit.h b/include/linux/audit.h > > index 96cabb095eed..5eab4d898c26 100644 > > --- a/include/linux/audit.h > > +++ b/include/linux/audit.h > > @@ -379,7 +379,7 @@ extern int __audit_log_bprm_fcaps(struct linux_binprm *bprm, > > extern void __audit_fanotify(unsigned int response); > > extern void __audit_tk_injoffset(struct timespec64 offset); > > extern void __audit_ntp_log(const struct audit_ntp_data *ad); > > -extern void __audit_nf_cfg(const char *name, u8 af, int nentries); > > +extern void __audit_nf_cfg(const char *name, u8 af, int nentries, int op); > > Consider adding an enum instead of int op. > > > if (audit_enabled) > > - audit_nf_cfg(repl->name, AF_BRIDGE, repl->nentries); > > + audit_nf_cfg(repl->name, AF_BRIDGE, repl->nentries, 1); > > audit_nf_cfg(repl->name, AF_BRIDGE, repl->nentries, AUDIT_XT_OP_REPLACE); > > ... would be a bit more readable than '1'. > > The name is just an example, you can pick something else. I agree. Also, please just merge this into patch 4; I don't see a solid reason why it shouldn't be there. -- paul moore www.paul-moore.com
On 2020-01-06 21:23, Florian Westphal wrote: > Richard Guy Briggs <rgb@redhat.com> wrote: > > diff --git a/include/linux/audit.h b/include/linux/audit.h > > index 96cabb095eed..5eab4d898c26 100644 > > --- a/include/linux/audit.h > > +++ b/include/linux/audit.h > > @@ -379,7 +379,7 @@ extern int __audit_log_bprm_fcaps(struct linux_binprm *bprm, > > extern void __audit_fanotify(unsigned int response); > > extern void __audit_tk_injoffset(struct timespec64 offset); > > extern void __audit_ntp_log(const struct audit_ntp_data *ad); > > -extern void __audit_nf_cfg(const char *name, u8 af, int nentries); > > +extern void __audit_nf_cfg(const char *name, u8 af, int nentries, int op); > > Consider adding an enum instead of int op. > > > if (audit_enabled) > > - audit_nf_cfg(repl->name, AF_BRIDGE, repl->nentries); > > + audit_nf_cfg(repl->name, AF_BRIDGE, repl->nentries, 1); > > audit_nf_cfg(repl->name, AF_BRIDGE, repl->nentries, AUDIT_XT_OP_REPLACE); > > ... would be a bit more readable than '1'. > > The name is just an example, you can pick something else. Thanks Florian. Another question occurs to me about table default policy. I'd observed previously that if nentries was zero due to an empty table, then it was due to table registration calls, which resulted from module loading. The default policy is NF_ACCEPT (because Rusty didn't want more email, go figure...). It occurred to me later that some table loads took a command line parameter to be able to change the default policy verdict from NF_ACCEPT to NF_DROP. In particular, filter FORWARD hook tables. Is there a straightforward way to be able to detect this in all the audit_nf_cfg() callers to be able to log it? In particular, in: net/bridge/netfilter/ebtables.c: ebt_register_table() net/bridge/netfilter/ebtables.c: do_replace_finish() net/bridge/netfilter/ebtables.c: __ebt_unregister_table() net/netfilter/x_tables.c: xt_replace_table() net/netfilter/x_tables.c: xt_unregister_table() It appears to be stored in the second entry of struct ipt_replace and struct ip6t_replace, of types struct ipt_standard and struct ip6t_standard in target.verdict, which doesn't appear to be obvious or easily accessible from xt_replace_table(). In ebtables, it appears to be in the policy member of struct ebt_entries. Both potential solutions are awkward, adding a parameter to pass that value in, but also trying to reach into the protocol-specific entry table to find that value. Would you have a recommendation? This assumes that reporting that default policy value is even desired or required. - RGB -- Richard Guy Briggs <rgb@redhat.com> Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada IRC: rgb, SunRaycer Voice: +1.647.777.2635, Internal: (81) 32635
Richard Guy Briggs <rgb@redhat.com> wrote: > The default policy is NF_ACCEPT (because Rusty didn't want > more email, go figure...). It occurred to me later that some table > loads took a command line parameter to be able to change the default > policy verdict from NF_ACCEPT to NF_DROP. In particular, filter FORWARD > hook tables. Is there a straightforward way to be able to detect this > in all the audit_nf_cfg() callers to be able to log it? In particular, > in: > net/bridge/netfilter/ebtables.c: ebt_register_table() > net/bridge/netfilter/ebtables.c: do_replace_finish() > net/bridge/netfilter/ebtables.c: __ebt_unregister_table() > net/netfilter/x_tables.c: xt_replace_table() > net/netfilter/x_tables.c: xt_unregister_table() The module parameter or the policy? The poliy can be changed via the xtables tools. Given you can have: *filter :INPUT ACCEPT [0:0] :FORWARD DROP [0:0] :OUTPUT ACCEPT [0:0] -A FORWARD -j ACCEPT COMMIT ... which effectily gives a FORWARD ACCEPT policy I'm not sure logging the policy is useful. Furthermore, ebtables has polices even for user-defined chains. > Both potential solutions are awkward, adding a parameter to pass that > value in, but also trying to reach into the protocol-specific entry > table to find that value. Would you have a recommendation? This > assumes that reporting that default policy value is even desired or > required. See above, I don't think its useful. If it is needed, its probably best to define an informational struct containing the policy (accept/drop) value for the each hook points (prerouting to postrouting), fill that from the backend specific code (as thats the only place that exposes the backend specific structs ...) and then pass that to the audit logging functions.
On 2020-02-13 13:34, Florian Westphal wrote: > Richard Guy Briggs <rgb@redhat.com> wrote: > > The default policy is NF_ACCEPT (because Rusty didn't want > > more email, go figure...). It occurred to me later that some table > > loads took a command line parameter to be able to change the default > > policy verdict from NF_ACCEPT to NF_DROP. In particular, filter FORWARD > > hook tables. Is there a straightforward way to be able to detect this > > in all the audit_nf_cfg() callers to be able to log it? In particular, > > in: > > net/bridge/netfilter/ebtables.c: ebt_register_table() > > net/bridge/netfilter/ebtables.c: do_replace_finish() > > net/bridge/netfilter/ebtables.c: __ebt_unregister_table() > > net/netfilter/x_tables.c: xt_replace_table() > > net/netfilter/x_tables.c: xt_unregister_table() > > The module parameter or the policy? > > The poliy can be changed via the xtables tools. > Given you can have: > > *filter > :INPUT ACCEPT [0:0] > :FORWARD DROP [0:0] > :OUTPUT ACCEPT [0:0] > -A FORWARD -j ACCEPT > COMMIT > > ... which effectily gives a FORWARD ACCEPT policy I'm not sure logging > the policy is useful. > > Furthermore, ebtables has polices even for user-defined chains. > > > Both potential solutions are awkward, adding a parameter to pass that > > value in, but also trying to reach into the protocol-specific entry > > table to find that value. Would you have a recommendation? This > > assumes that reporting that default policy value is even desired or > > required. > > See above, I don't think its useful. If it is needed, its probably best > to define an informational struct containing the policy (accept/drop) > value for the each hook points (prerouting to postrouting), fill > that from the backend specific code (as thats the only place that > exposes the backend specific structs ...) and then pass that to > the audit logging functions. Ok, for this set, I'll drop the idea. If it becomes apparent later than it is necessary, it can be added as a field at the end of the record. Thanks for looking at this. - RGB -- Richard Guy Briggs <rgb@redhat.com> Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada IRC: rgb, SunRaycer Voice: +1.647.777.2635, Internal: (81) 32635
diff --git a/include/linux/audit.h b/include/linux/audit.h index 96cabb095eed..5eab4d898c26 100644 --- a/include/linux/audit.h +++ b/include/linux/audit.h @@ -379,7 +379,7 @@ extern int __audit_log_bprm_fcaps(struct linux_binprm *bprm, extern void __audit_fanotify(unsigned int response); extern void __audit_tk_injoffset(struct timespec64 offset); extern void __audit_ntp_log(const struct audit_ntp_data *ad); -extern void __audit_nf_cfg(const char *name, u8 af, int nentries); +extern void __audit_nf_cfg(const char *name, u8 af, int nentries, int op); static inline void audit_ipc_obj(struct kern_ipc_perm *ipcp) { @@ -515,10 +515,10 @@ static inline void audit_ntp_log(const struct audit_ntp_data *ad) __audit_ntp_log(ad); } -static inline void audit_nf_cfg(const char *name, u8 af, int nentries) +static inline void audit_nf_cfg(const char *name, u8 af, int nentries, int op) { if (!audit_dummy_context()) - __audit_nf_cfg(name, af, nentries); + __audit_nf_cfg(name, af, nentries, op); } extern int audit_n_rules; @@ -654,7 +654,7 @@ static inline void audit_ntp_log(const struct audit_ntp_data *ad) static inline void audit_ptrace(struct task_struct *t) { } -static inline void audit_nf_cfg(const char *name, u8 af, int nentries) +static inline void audit_nf_cfg(const char *name, u8 af, int nentries, int op) { } #define audit_n_rules 0 diff --git a/kernel/auditsc.c b/kernel/auditsc.c index 4e1df4233cd3..999ac184246b 100644 --- a/kernel/auditsc.c +++ b/kernel/auditsc.c @@ -2545,7 +2545,7 @@ void __audit_ntp_log(const struct audit_ntp_data *ad) audit_log_ntp_val(ad, "adjust", AUDIT_NTP_ADJUST); } -void __audit_nf_cfg(const char *name, u8 af, int nentries) +void __audit_nf_cfg(const char *name, u8 af, int nentries, int op) { struct audit_buffer *ab; struct audit_context *context = audit_context(); @@ -2555,8 +2555,9 @@ void __audit_nf_cfg(const char *name, u8 af, int nentries) ab = audit_log_start(context, GFP_KERNEL, AUDIT_NETFILTER_CFG); if (!ab) return; /* audit_panic or being filtered */ - audit_log_format(ab, "table=%s family=%u entries=%u", - name, af, nentries); + audit_log_format(ab, "table=%s family=%u entries=%u op=%s", + name, af, nentries, + op ? "replace" : "register"); audit_log_end(ab); } EXPORT_SYMBOL_GPL(__audit_nf_cfg); diff --git a/net/bridge/netfilter/ebtables.c b/net/bridge/netfilter/ebtables.c index 58126547b175..baff2f05af43 100644 --- a/net/bridge/netfilter/ebtables.c +++ b/net/bridge/netfilter/ebtables.c @@ -1049,7 +1049,7 @@ static int do_replace_finish(struct net *net, struct ebt_replace *repl, vfree(counterstmp); if (audit_enabled) - audit_nf_cfg(repl->name, AF_BRIDGE, repl->nentries); + audit_nf_cfg(repl->name, AF_BRIDGE, repl->nentries, 1); return ret; free_unlock: @@ -1220,7 +1220,7 @@ int ebt_register_table(struct net *net, const struct ebt_table *input_table, } if (audit_enabled) - audit_nf_cfg(repl->name, AF_BRIDGE, repl->nentries); + audit_nf_cfg(repl->name, AF_BRIDGE, repl->nentries, 0); return ret; free_unlock: mutex_unlock(&ebt_mutex); diff --git a/net/netfilter/x_tables.c b/net/netfilter/x_tables.c index c0416ae52f7f..4ae4f7bf8946 100644 --- a/net/netfilter/x_tables.c +++ b/net/netfilter/x_tables.c @@ -1402,7 +1402,8 @@ struct xt_table_info *xt_replace_table(struct xt_table *table, } if (audit_enabled) - audit_nf_cfg(table->name, table->af, private->number); + audit_nf_cfg(table->name, table->af, private->number, + private->number); return private; }
Add the operation performed (register or replace) to the NETFILTER_CFG record. Here is the sample record: type=NETFILTER_CFG msg=audit(1494981627.248:9764): family=7 table=broute entries=0 op=replace See: https://github.com/linux-audit/audit-kernel/issues/25 Signed-off-by: Richard Guy Briggs <rgb@redhat.com> --- include/linux/audit.h | 8 ++++---- kernel/auditsc.c | 7 ++++--- net/bridge/netfilter/ebtables.c | 4 ++-- net/netfilter/x_tables.c | 3 ++- 4 files changed, 12 insertions(+), 10 deletions(-)