On Tue, Jul 28, 2020 at 21:09:10 +0200, Stephan Mueller wrote:
> Am Dienstag, 28. Juli 2020, 17:47:30 CEST schrieb Antony Antony:
>
> Hi Antony,
>
> > when enabled, 1, redact XFRM SA secret in the netlink response to
> > xfrm_get_sa() or dump all sa.
> >
> > e.g
> > echo 1 > /proc/sys/net/core/xfrm_redact_secret
> > ip xfrm state
> > src 172.16.1.200 dst 172.16.1.100
> > proto esp spi 0x00000002 reqid 2 mode tunnel
> > replay-window 0
> > aead rfc4106(gcm(aes)) 0x0000000000000000000000000000000000000000 96
> >
> > the aead secret is redacted.
> >
> > /proc/sys/core/net/xfrm_redact_secret is a toggle.
> > Once enabled, either at compile or via proc, it can not be disabled.
> > Redacting secret is a FIPS 140-2 requirement.
> >
> > Cc: Stephan Mueller <smuel...@chronox.de>
> > Signed-off-by: Antony Antony <antony.ant...@secunet.com>
> > ---
> > Documentation/networking/xfrm_sysctl.rst | 7 +++
> > include/net/netns/xfrm.h | 1 +
> > net/xfrm/Kconfig | 10 ++++
> > net/xfrm/xfrm_sysctl.c | 20 +++++++
> > net/xfrm/xfrm_user.c | 76 +++++++++++++++++++++---
> > 5 files changed, 105 insertions(+), 9 deletions(-)
> >
> > diff --git a/Documentation/networking/xfrm_sysctl.rst
> > b/Documentation/networking/xfrm_sysctl.rst index 47b9bbdd0179..26432b0ff3ac
> > 100644
> > --- a/Documentation/networking/xfrm_sysctl.rst
> > +++ b/Documentation/networking/xfrm_sysctl.rst
> > @@ -9,3 +9,10 @@ XFRM Syscall
> >
> > xfrm_acq_expires - INTEGER
> > default 30 - hard timeout in seconds for acquire requests
> > +
> > +xfrm_redact_secret - INTEGER
> > + A toggle to redact xfrm SA's secret to userspace.
> > + When true the kernel, netlink message will redact SA secret
> > + to userspace. This is part of FIPS 140-2 requirement.
> > + Once the value is set to true, either at compile or at run time,
> > + it can not be set to false.
> > diff --git a/include/net/netns/xfrm.h b/include/net/netns/xfrm.h
> > index 59f45b1e9dac..0ca9328daad4 100644
> > --- a/include/net/netns/xfrm.h
> > +++ b/include/net/netns/xfrm.h
> > @@ -64,6 +64,7 @@ struct netns_xfrm {
> > u32 sysctl_aevent_rseqth;
> > int sysctl_larval_drop;
> > u32 sysctl_acq_expires;
> > + u32 sysctl_redact_secret;
> > #ifdef CONFIG_SYSCTL
> > struct ctl_table_header *sysctl_hdr;
> > #endif
> > diff --git a/net/xfrm/Kconfig b/net/xfrm/Kconfig
> > index 5b9a5ab48111..270a4e906a15 100644
> > --- a/net/xfrm/Kconfig
> > +++ b/net/xfrm/Kconfig
> > @@ -91,6 +91,16 @@ config XFRM_ESP
> > select CRYPTO_SEQIV
> > select CRYPTO_SHA256
> >
> > +config XFRM_REDACT_SECRET
> > + bool "Redact xfrm SA secret in netlink message"
> > + depends on SYSCTL
> > + default n
> > + help
> > + Enable XFRM SA secret redact in the netlink message.
> > + Redacting secret is a FIPS 140-2 requirement.
> > + Once enabled at compile, the value can not be set to false on
> > + a running system.
> > +
> > config XFRM_IPCOMP
> > tristate
> > select XFRM_ALGO
> > diff --git a/net/xfrm/xfrm_sysctl.c b/net/xfrm/xfrm_sysctl.c
> > index 0c6c5ef65f9d..a41aa325a478 100644
> > --- a/net/xfrm/xfrm_sysctl.c
> > +++ b/net/xfrm/xfrm_sysctl.c
> > @@ -4,15 +4,25 @@
> > #include <net/net_namespace.h>
> > #include <net/xfrm.h>
> >
> > +#ifdef CONFIG_SYSCTL
> > +#ifdef CONFIG_XFRM_REDACT_SECRET
> > +#define XFRM_REDACT_SECRET 1
> > +#else
> > +#define XFRM_REDACT_SECRET 0
> > +#endif
> > +#endif
> > +
> > static void __net_init __xfrm_sysctl_init(struct net *net)
> > {
> > net->xfrm.sysctl_aevent_etime = XFRM_AE_ETIME;
> > net->xfrm.sysctl_aevent_rseqth = XFRM_AE_SEQT_SIZE;
> > net->xfrm.sysctl_larval_drop = 1;
> > net->xfrm.sysctl_acq_expires = 30;
> > + net->xfrm.sysctl_redact_secret = XFRM_REDACT_SECRET;
> > }
> >
> > #ifdef CONFIG_SYSCTL
> > +
> > static struct ctl_table xfrm_table[] = {
> > {
> > .procname = "xfrm_aevent_etime",
> > @@ -38,6 +48,15 @@ static struct ctl_table xfrm_table[] = {
> > .mode = 0644,
> > .proc_handler = proc_dointvec
> > },
> > + {
> > + .procname = "xfrm_redact_secret",
> > + .maxlen = sizeof(u32),
> > + .mode = 0644,
> > + /* only handle a transition from "0" to "1" */
> > + .proc_handler = proc_dointvec_minmax,
> > + .extra1 = SYSCTL_ONE,
> > + .extra2 = SYSCTL_ONE,
> > + },
> > {}
> > };
> >
> > @@ -54,6 +73,7 @@ int __net_init xfrm_sysctl_init(struct net *net)
> > table[1].data = &net->xfrm.sysctl_aevent_rseqth;
> > table[2].data = &net->xfrm.sysctl_larval_drop;
> > table[3].data = &net->xfrm.sysctl_acq_expires;
> > + table[4].data = &net->xfrm.sysctl_redact_secret;
> >
> > /* Don't export sysctls to unprivileged users */
> > if (net->user_ns != &init_user_ns)
> > diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
> > index e6cfaa680ef3..a3e89dddea9d 100644
> > --- a/net/xfrm/xfrm_user.c
> > +++ b/net/xfrm/xfrm_user.c
> > @@ -848,21 +848,78 @@ static int copy_user_offload(struct xfrm_state_offload
> > *xso, struct sk_buff *skb return 0;
> > }
> >
> > -static int copy_to_user_auth(struct xfrm_algo_auth *auth, struct sk_buff
> > *skb) +static int copy_to_user_auth(u32 redact_secret, struct
> > xfrm_algo_auth *auth, + struct sk_buff *skb)
> > {
> > struct xfrm_algo *algo;
> > + struct xfrm_algo_auth *ap;
> > struct nlattr *nla;
> >
> > nla = nla_reserve(skb, XFRMA_ALG_AUTH,
> > sizeof(*algo) + (auth->alg_key_len + 7) / 8);
> > if (!nla)
> > return -EMSGSIZE;
> > -
> > algo = nla_data(nla);
> > strncpy(algo->alg_name, auth->alg_name, sizeof(algo->alg_name));
> > - memcpy(algo->alg_key, auth->alg_key, (auth->alg_key_len + 7) / 8);
> > +
> > + if (redact_secret && auth->alg_key_len)
> > + memset(algo->alg_key, 0, (auth->alg_key_len + 7) / 8);
> > + else
> > + memcpy(algo->alg_key, auth->alg_key,
> > + (auth->alg_key_len + 7) / 8);
> > algo->alg_key_len = auth->alg_key_len;
> >
> > + nla = nla_reserve(skb, XFRMA_ALG_AUTH_TRUNC, xfrm_alg_auth_len(auth));
> > + if (!nla)
> > + return -EMSGSIZE;
> > + ap = nla_data(nla);
> > + memcpy(ap, auth, sizeof(struct xfrm_algo_auth));
> > + if (redact_secret)
>
> You test for auth->alg_key_len above. Shouldn't there such a check here too?
It is a good idea add checks before all memset calls.
I will send a new version out soon.
thanks,
-antony
