Date:   Mon, 7 Jan 2019 15:10:25 -0800
From:   Benedict Wong <benedictwong@...gle.com>
To:     Florian Fainelli <f.fainelli@...il.com>
Cc:     netdev@...r.kernel.org, Nathan Harold <nharold@...gle.com>,
        Lorenzo Colitti <lorenzo@...gle.com>,
        Maciej Żenczykowski <maze@...gle.com>
Subject: Re: [PATCH iproute2] xfrm: add option to hide keys in state output

(Accidentally sent previously as direct reply. Re-sending as reply-all)

> ... would not it be better to not request the
> kernel not to dump the keys to begin with ...

I think it's still valid to have it in iproute2, since it does allow for
backward compatibility against older kernels. Adding it to the
kernels would mean it hits 5.1+ at the very most.

> ... that way a process ptracing
> iproute2 or whatever, would also not have a chance to ex-filtrate those
> keys? ...

That is a good point. I suspect the kernel will still need to have the
keys dumped default in order to maintain backwards compatibility, so
this does also add some safety. The primary goal of this patch was to
allow someone with (permanent) access to the to automatically
generate a debug log that doesn't compromise (eg a privileged system
daemon, or a superuser). Just my 2c though :)

On Mon, Jan 7, 2019 at 2:23 PM Florian Fainelli <f.fainelli@...il.com> wrote:
>
> On 1/7/19 1:31 PM, Benedict Wong 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.
>
> I have not yet looked at the kernel side, but assuming the keys are
> stored on the kernel side as well (which is quite conceivably what is
> happening for software IPsec), would not it be better to not request the
> kernel not to dump the keys to begin with, that way a process ptracing
> iproute2 or whatever, would also not have a chance to ex-filtrate those
> keys? If this is not how this is implemented, apologies :)
>
> >
> > Signed-off-by: Benedict Wong <benedictwong@...gle.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 " ]"
> >
>
>
> --
> Florian

Powered by blists - more mailing lists