[RFC iproute2] xfrm: add option to hide keys in state output

[RFC iproute2] xfrm: add option to hide keys in state output

[Benedict Wong]

ip xfrm state show currently dumps keys unconditionally. This limits its
use in logging, as security information can be leaked.

This patch adds a nokeys option to ip xfrm ( state show | monitor ), which
prevents the printing of keys. This allows ip xfrm state show to be used
in logging without exposing keys.

Signed-off-by: Benedict Wong <benedictwong@google.com>
---
 ip/ipxfrm.c        | 45 +++++++++++++++++++++++++--------------------
 ip/xfrm.h          |  5 +++--
 ip/xfrm_monitor.c  |  7 +++++--
 ip/xfrm_state.c    | 27 ++++++++++++++++++++++-----
 man/man8/ip-xfrm.8 | 15 ++++++++++++++-
 5 files changed, 69 insertions(+), 30 deletions(-)

diff --git a/ip/ipxfrm.c b/ip/ipxfrm.c
index 2dea4e37..1334ca9f 100644
--- a/ip/ipxfrm.c
+++ b/ip/ipxfrm.c
@@ -497,7 +497,8 @@ void xfrm_selector_print(struct xfrm_selector *sel, __u16 family,
 }
 
 static void __xfrm_algo_print(struct xfrm_algo *algo, int type, int len,
-			      FILE *fp, const char *prefix, int newline)
+			      FILE *fp, const char *prefix, int newline,
+			      bool nokeys)
 {
 	int keylen;
 	int i;
@@ -521,7 +522,9 @@ static void __xfrm_algo_print(struct xfrm_algo *algo, int type, int len,
 		goto fin;
 	}
 
-	if (keylen > 0) {
+	if (nokeys)
+		fprintf(fp, "<<Keys hidden>>");
+	else if (keylen > 0) {
 		fprintf(fp, "0x");
 		for (i = 0; i < keylen; i++)
 			fprintf(fp, "%.2x", (unsigned char)algo->alg_key[i]);
@@ -536,13 +539,13 @@ static void __xfrm_algo_print(struct xfrm_algo *algo, int type, int len,
 }
 
 static inline void xfrm_algo_print(struct xfrm_algo *algo, int type, int len,
-				   FILE *fp, const char *prefix)
+				   FILE *fp, const char *prefix, bool nokeys)
 {
-	return __xfrm_algo_print(algo, type, len, fp, prefix, 1);
+	return __xfrm_algo_print(algo, type, len, fp, prefix, 1, nokeys);
 }
 
 static void xfrm_aead_print(struct xfrm_algo_aead *algo, int len,
-			    FILE *fp, const char *prefix)
+			    FILE *fp, const char *prefix, bool nokeys)
 {
 	struct xfrm_algo *base_algo = alloca(sizeof(*base_algo) + algo->alg_key_len / 8);
 
@@ -550,7 +553,8 @@ static void xfrm_aead_print(struct xfrm_algo_aead *algo, int len,
 	base_algo->alg_key_len = algo->alg_key_len;
 	memcpy(base_algo->alg_key, algo->alg_key, algo->alg_key_len / 8);
 
-	__xfrm_algo_print(base_algo, XFRMA_ALG_AEAD, len, fp, prefix, 0);
+	__xfrm_algo_print(base_algo,
+			  XFRMA_ALG_AEAD, len, fp, prefix, 0, nokeys);
 
 	fprintf(fp, " %d", algo->alg_icv_len);
 
@@ -558,7 +562,7 @@ static void xfrm_aead_print(struct xfrm_algo_aead *algo, int len,
 }
 
 static void xfrm_auth_trunc_print(struct xfrm_algo_auth *algo, int len,
-				  FILE *fp, const char *prefix)
+				  FILE *fp, const char *prefix, bool nokeys)
 {
 	struct xfrm_algo *base_algo = alloca(sizeof(*base_algo) + algo->alg_key_len / 8);
 
@@ -566,7 +570,8 @@ static void xfrm_auth_trunc_print(struct xfrm_algo_auth *algo, int len,
 	base_algo->alg_key_len = algo->alg_key_len;
 	memcpy(base_algo->alg_key, algo->alg_key, algo->alg_key_len / 8);
 
-	__xfrm_algo_print(base_algo, XFRMA_ALG_AUTH_TRUNC, len, fp, prefix, 0);
+	__xfrm_algo_print(base_algo,
+			  XFRMA_ALG_AUTH_TRUNC, len, fp, prefix, 0, nokeys);
 
 	fprintf(fp, " %d", algo->alg_trunc_len);
 
@@ -679,7 +684,7 @@ done:
 }
 
 void xfrm_xfrma_print(struct rtattr *tb[], __u16 family,
-		      FILE *fp, const char *prefix)
+		      FILE *fp, const char *prefix, bool nokeys)
 {
 	if (tb[XFRMA_MARK]) {
 		struct rtattr *rta = tb[XFRMA_MARK];
@@ -700,36 +705,36 @@ void xfrm_xfrma_print(struct rtattr *tb[], __u16 family,
 	if (tb[XFRMA_ALG_AUTH] && !tb[XFRMA_ALG_AUTH_TRUNC]) {
 		struct rtattr *rta = tb[XFRMA_ALG_AUTH];
 
-		xfrm_algo_print(RTA_DATA(rta),
-				XFRMA_ALG_AUTH, RTA_PAYLOAD(rta), fp, prefix);
+		xfrm_algo_print(RTA_DATA(rta), XFRMA_ALG_AUTH,
+				RTA_PAYLOAD(rta), fp, prefix, nokeys);
 	}
 
 	if (tb[XFRMA_ALG_AUTH_TRUNC]) {
 		struct rtattr *rta = tb[XFRMA_ALG_AUTH_TRUNC];
 
 		xfrm_auth_trunc_print(RTA_DATA(rta),
-				      RTA_PAYLOAD(rta), fp, prefix);
+				      RTA_PAYLOAD(rta), fp, prefix, nokeys);
 	}
 
 	if (tb[XFRMA_ALG_AEAD]) {
 		struct rtattr *rta = tb[XFRMA_ALG_AEAD];
 
 		xfrm_aead_print(RTA_DATA(rta),
-				RTA_PAYLOAD(rta), fp, prefix);
+				RTA_PAYLOAD(rta), fp, prefix, nokeys);
 	}
 
 	if (tb[XFRMA_ALG_CRYPT]) {
 		struct rtattr *rta = tb[XFRMA_ALG_CRYPT];
 
-		xfrm_algo_print(RTA_DATA(rta),
-				XFRMA_ALG_CRYPT, RTA_PAYLOAD(rta), fp, prefix);
+		xfrm_algo_print(RTA_DATA(rta), XFRMA_ALG_CRYPT,
+				RTA_PAYLOAD(rta), fp, prefix, nokeys);
 	}
 
 	if (tb[XFRMA_ALG_COMP]) {
 		struct rtattr *rta = tb[XFRMA_ALG_COMP];
 
-		xfrm_algo_print(RTA_DATA(rta),
-				XFRMA_ALG_COMP, RTA_PAYLOAD(rta), fp, prefix);
+		xfrm_algo_print(RTA_DATA(rta), XFRMA_ALG_COMP,
+				RTA_PAYLOAD(rta), fp, prefix, nokeys);
 	}
 
 	if (tb[XFRMA_ENCAP]) {
@@ -897,7 +902,7 @@ static int xfrm_selector_iszero(struct xfrm_selector *s)
 
 void xfrm_state_info_print(struct xfrm_usersa_info *xsinfo,
 			    struct rtattr *tb[], FILE *fp, const char *prefix,
-			    const char *title)
+			    const char *title, bool nokeys)
 {
 	char buf[STRBUF_SIZE] = {};
 	int force_spi = xfrm_xfrmproto_is_ipsec(xsinfo->id.proto);
@@ -943,7 +948,7 @@ void xfrm_state_info_print(struct xfrm_usersa_info *xsinfo,
 		fprintf(fp, " (0x%s)", strxf_mask8(xsinfo->flags));
 	fprintf(fp, "%s", _SL_);
 
-	xfrm_xfrma_print(tb, xsinfo->family, fp, buf);
+	xfrm_xfrma_print(tb, xsinfo->family, fp, buf, nokeys);
 
 	if (!xfrm_selector_iszero(&xsinfo->sel)) {
 		char sbuf[STRBUF_SIZE];
@@ -1071,7 +1076,7 @@ void xfrm_policy_info_print(struct xfrm_userpolicy_info *xpinfo,
 	if (show_stats > 0)
 		xfrm_lifetime_print(&xpinfo->lft, &xpinfo->curlft, fp, buf);
 
-	xfrm_xfrma_print(tb, xpinfo->sel.family, fp, buf);
+	xfrm_xfrma_print(tb, xpinfo->sel.family, fp, buf, false);
 }
 
 int xfrm_id_parse(xfrm_address_t *saddr, struct xfrm_id *id, __u16 *family,
diff --git a/ip/xfrm.h b/ip/xfrm.h
index 72390d79..9ba5ca61 100644
--- a/ip/xfrm.h
+++ b/ip/xfrm.h
@@ -105,6 +105,7 @@ struct xfrm_filter {
 extern struct xfrm_filter filter;
 
 int xfrm_state_print(struct nlmsghdr *n, void *arg);
+int xfrm_state_print_nokeys(struct nlmsghdr *n, void *arg);
 int xfrm_policy_print(struct nlmsghdr *n, void *arg);
 int do_xfrm_state(int argc, char **argv);
 int do_xfrm_policy(int argc, char **argv);
@@ -124,10 +125,10 @@ const char *strxf_ptype(__u8 ptype);
 void xfrm_selector_print(struct xfrm_selector *sel, __u16 family,
 			 FILE *fp, const char *prefix);
 void xfrm_xfrma_print(struct rtattr *tb[], __u16 family,
-		      FILE *fp, const char *prefix);
+		      FILE *fp, const char *prefix, bool nokeys);
 void xfrm_state_info_print(struct xfrm_usersa_info *xsinfo,
 			    struct rtattr *tb[], FILE *fp, const char *prefix,
-			   const char *title);
+			   const char *title, bool nokeys);
 void xfrm_policy_info_print(struct xfrm_userpolicy_info *xpinfo,
 			    struct rtattr *tb[], FILE *fp, const char *prefix,
 			    const char *title);
diff --git a/ip/xfrm_monitor.c b/ip/xfrm_monitor.c
index 76905ed3..17117f41 100644
--- a/ip/xfrm_monitor.c
+++ b/ip/xfrm_monitor.c
@@ -35,10 +35,11 @@
 
 static void usage(void) __attribute__((noreturn));
 static int listen_all_nsid;
+static bool nokeys;
 
 static void usage(void)
 {
-	fprintf(stderr, "Usage: ip xfrm monitor [all-nsid] [ all | OBJECTS | help ]\n");
+	fprintf(stderr, "Usage: ip xfrm monitor [ nokeys ] [ all-nsid ] [ all | OBJECTS | help ]\n");
 	fprintf(stderr, "OBJECTS := { acquire | expire | SA | aevent | policy | report }\n");
 	exit(-1);
 }
@@ -197,7 +198,7 @@ static int xfrm_report_print(struct nlmsghdr *n, void *arg)
 
 	parse_rtattr(tb, XFRMA_MAX, XFRMREP_RTA(xrep), len);
 
-	xfrm_xfrma_print(tb, family, fp, "  ");
+	xfrm_xfrma_print(tb, family, fp, "  ", nokeys);
 
 	if (oneline)
 		fprintf(fp, "\n");
@@ -352,6 +353,8 @@ int do_xfrm_monitor(int argc, char **argv)
 		if (matches(*argv, "file") == 0) {
 			NEXT_ARG();
 			file = *argv;
+		} else if (strcmp(*argv, "nokeys") == 0) {
+			nokeys = true;
 		} else if (strcmp(*argv, "all") == 0) {
 			/* fall out */
 		} else if (matches(*argv, "all-nsid") == 0) {
diff --git a/ip/xfrm_state.c b/ip/xfrm_state.c
index e8c01746..d8f9350e 100644
--- a/ip/xfrm_state.c
+++ b/ip/xfrm_state.c
@@ -65,7 +65,9 @@ static void usage(void)
 	fprintf(stderr, "Usage: ip xfrm state allocspi ID [ mode MODE ] [ mark MARK [ mask MASK ] ]\n");
 	fprintf(stderr, "        [ reqid REQID ] [ seq SEQ ] [ min SPI max SPI ]\n");
 	fprintf(stderr, "Usage: ip xfrm state { delete | get } ID [ mark MARK [ mask MASK ] ]\n");
-	fprintf(stderr, "Usage: ip xfrm state { deleteall | list } [ ID ] [ mode MODE ] [ reqid REQID ]\n");
+	fprintf(stderr, "Usage: ip xfrm state { deleteall } [ ID ] [ mode MODE ] [ reqid REQID ]\n");
+	fprintf(stderr, "        [ flag FLAG-LIST ]\n");
+	fprintf(stderr, "Usage: ip xfrm state { list } [ nokeys ] [ ID ] [ mode MODE ] [ reqid REQID ]\n");
 	fprintf(stderr, "        [ flag FLAG-LIST ]\n");
 	fprintf(stderr, "Usage: ip xfrm state flush [ proto XFRM-PROTO ]\n");
 	fprintf(stderr, "Usage: ip xfrm state count\n");
@@ -908,7 +910,7 @@ static int xfrm_state_filter_match(struct xfrm_usersa_info *xsinfo)
 	return 1;
 }
 
-int xfrm_state_print(struct nlmsghdr *n, void *arg)
+static int __do_xfrm_state_print(struct nlmsghdr *n, void *arg, bool nokeys)
 {
 	FILE *fp = (FILE *)arg;
 	struct rtattr *tb[XFRMA_MAX+1];
@@ -979,7 +981,7 @@ int xfrm_state_print(struct nlmsghdr *n, void *arg)
 		xsinfo = RTA_DATA(tb[XFRMA_SA]);
 	}
 
-	xfrm_state_info_print(xsinfo, tb, fp, NULL, NULL);
+	xfrm_state_info_print(xsinfo, tb, fp, NULL, NULL, nokeys);
 
 	if (n->nlmsg_type == XFRM_MSG_EXPIRE) {
 		fprintf(fp, "\t");
@@ -994,6 +996,16 @@ int xfrm_state_print(struct nlmsghdr *n, void *arg)
 	return 0;
 }
 
+int xfrm_state_print(struct nlmsghdr *n, void *arg)
+{
+	return __do_xfrm_state_print(n, arg, false);
+}
+
+int xfrm_state_print_nokeys(struct nlmsghdr *n, void *arg)
+{
+	return __do_xfrm_state_print(n, arg, true);
+}
+
 static int xfrm_state_get_or_delete(int argc, char **argv, int delete)
 {
 	struct rtnl_handle rth;
@@ -1145,13 +1157,16 @@ static int xfrm_state_list_or_deleteall(int argc, char **argv, int deleteall)
 {
 	char *idp = NULL;
 	struct rtnl_handle rth;
+	bool nokeys = false;
 
 	if (argc > 0)
 		filter.use = 1;
 	filter.xsinfo.family = preferred_family;
 
 	while (argc > 0) {
-		if (strcmp(*argv, "mode") == 0) {
+		if (strcmp(*argv, "nokeys") == 0) {
+			nokeys = true;
+		} else if (strcmp(*argv, "mode") == 0) {
 			NEXT_ARG();
 			xfrm_mode_parse(&filter.xsinfo.mode, &argc, &argv);
 
@@ -1267,7 +1282,9 @@ static int xfrm_state_list_or_deleteall(int argc, char **argv, int deleteall)
 			exit(1);
 		}
 
-		if (rtnl_dump_filter(&rth, xfrm_state_print, stdout) < 0) {
+		rtnl_filter_t filter = nokeys ?
+				xfrm_state_print_nokeys : xfrm_state_print;
+		if (rtnl_dump_filter(&rth, filter, stdout) < 0) {
 			fprintf(stderr, "Dump terminated\n");
 			exit(1);
 		}
diff --git a/man/man8/ip-xfrm.8 b/man/man8/ip-xfrm.8
index 839e06aa..0fdf97d9 100644
--- a/man/man8/ip-xfrm.8
+++ b/man/man8/ip-xfrm.8
@@ -89,7 +89,7 @@ ip-xfrm \- transform configuration
 .IR MASK " ] ]"
 
 .ti -8
-.BR "ip xfrm state" " { " deleteall " | " list " } ["
+.BR "ip xfrm state" " { " deleteall " } ["
 .IR ID " ]"
 .RB "[ " mode
 .IR MODE " ]"
@@ -98,6 +98,17 @@ ip-xfrm \- transform configuration
 .RB "[ " flag
 .IR FLAG-LIST " ]"
 
+.ti -8
+.BR "ip xfrm state" " { " list " } ["
+.IR ID " ]"
+.RB "[ " nokeys " ]"
+.RB "[ " mode
+.IR MODE " ]"
+.RB "[ " reqid
+.IR REQID " ]"
+.RB "[ " flag
+.IR FLAG-LIST " ]"
+
 .ti -8
 .BR "ip xfrm state flush" " [ " proto
 .IR XFRM-PROTO " ]"
@@ -381,6 +392,8 @@ ip-xfrm \- transform configuration
 .BR "ip xfrm monitor" " ["
 .BI all-nsid
 ] [
+.BI nokeys
+] [
 .BI all
  |
 .IR LISTofXFRM-OBJECTS " ]"
-- 
2.20.1.97.g81188d93c3-goog

Re: [RFC iproute2] xfrm: add option to hide keys in state output

[Stephen Hemminger]

On Fri,  4 Jan 2019 15:19:10 -0800
Benedict Wong <benedictwong@google.com> wrote:

> ip xfrm state show currently dumps keys unconditionally. This limits its
> use in logging, as security information can be leaked.
> 
> This patch adds a nokeys option to ip xfrm ( state show | monitor ), which
> prevents the printing of keys. This allows ip xfrm state show to be used
> in logging without exposing keys.
> 
> Signed-off-by: Benedict Wong <benedictwong@google.com>
> ---
>  ip/ipxfrm.c        | 45 +++++++++++++++++++++++++--------------------
>  ip/xfrm.h          |  5 +++--
>  ip/xfrm_monitor.c  |  7 +++++--
>  ip/xfrm_state.c    | 27 ++++++++++++++++++++++-----
>  man/man8/ip-xfrm.8 | 15 ++++++++++++++-
>  5 files changed, 69 insertions(+), 30 deletions(-)
> 
> diff --git a/ip/ipxfrm.c b/ip/ipxfrm.c
> index 2dea4e37..1334ca9f 100644
> --- a/ip/ipxfrm.c
> +++ b/ip/ipxfrm.c
> @@ -497,7 +497,8 @@ void xfrm_selector_print(struct xfrm_selector *sel, __u16 family,
>  }
>  
>  static void __xfrm_algo_print(struct xfrm_algo *algo, int type, int len,
> -			      FILE *fp, const char *prefix, int newline)
> +			      FILE *fp, const char *prefix, int newline,
> +			      bool nokeys)
>  {
>  	int keylen;
>  	int i;
> @@ -521,7 +522,9 @@ static void __xfrm_algo_print(struct xfrm_algo *algo, int type, int len,
>  		goto fin;
>  	}
>  
> -	if (keylen > 0) {
> +	if (nokeys)
> +		fprintf(fp, "<<Keys hidden>>")


This reminds me, xfrm never got converted to use JSON output formatting.

Re: [RFC iproute2] xfrm: add option to hide keys in state output

[Benedict Wong]

Noted. Should I wait until xfrm is converted to JSON output formatting?

Or if there are no structural and stylistic issues, should I re-send
this as a patch?

On Fri, Jan 4, 2019 at 4:21 PM Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> On Fri,  4 Jan 2019 15:19:10 -0800
> Benedict Wong <benedictwong@google.com> wrote:
>
> > ip xfrm state show currently dumps keys unconditionally. This limits its
> > use in logging, as security information can be leaked.
> >
> > This patch adds a nokeys option to ip xfrm ( state show | monitor ), which
> > prevents the printing of keys. This allows ip xfrm state show to be used
> > in logging without exposing keys.
> >
> > Signed-off-by: Benedict Wong <benedictwong@google.com>
> > ---
> >  ip/ipxfrm.c        | 45 +++++++++++++++++++++++++--------------------
> >  ip/xfrm.h          |  5 +++--
> >  ip/xfrm_monitor.c  |  7 +++++--
> >  ip/xfrm_state.c    | 27 ++++++++++++++++++++++-----
> >  man/man8/ip-xfrm.8 | 15 ++++++++++++++-
> >  5 files changed, 69 insertions(+), 30 deletions(-)
> >
> > diff --git a/ip/ipxfrm.c b/ip/ipxfrm.c
> > index 2dea4e37..1334ca9f 100644
> > --- a/ip/ipxfrm.c
> > +++ b/ip/ipxfrm.c
> > @@ -497,7 +497,8 @@ void xfrm_selector_print(struct xfrm_selector *sel, __u16 family,
> >  }
> >
> >  static void __xfrm_algo_print(struct xfrm_algo *algo, int type, int len,
> > -                           FILE *fp, const char *prefix, int newline)
> > +                           FILE *fp, const char *prefix, int newline,
> > +                           bool nokeys)
> >  {
> >       int keylen;
> >       int i;
> > @@ -521,7 +522,9 @@ static void __xfrm_algo_print(struct xfrm_algo *algo, int type, int len,
> >               goto fin;
> >       }
> >
> > -     if (keylen > 0) {
> > +     if (nokeys)
> > +             fprintf(fp, "<<Keys hidden>>")
>
>
> This reminds me, xfrm never got converted to use JSON output formatting.

Re: [RFC iproute2] xfrm: add option to hide keys in state output

[Stephen Hemminger]

On Mon, 7 Jan 2019 12:37:41 -0800
Benedict Wong <benedictwong@google.com> wrote:

> Noted. Should I wait until xfrm is converted to JSON output formatting?
> 
> Or if there are no structural and stylistic issues, should I re-send
> this as a patch?
> 

No the patch should go in now.