Message-ID: <CANrj0bZvCYK0+7uRtKj0HoPZjjR1hi0=w8LqskRmYxFVM6vG2Q@mail.gmail.com>
Date:   Tue, 15 Jan 2019 18:52:43 -0800
From:   Benedict Wong <benedictwong@...gle.com>
To:     netdev@...r.kernel.org
Cc:     Nathan Harold <nharold@...gle.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Lorenzo Colitti <lorenzo@...gle.com>,
        Maciej Żenczykowski <maze@...gle.com>
Subject: Re: [PATCH iproute2] xfrm: add option to hide keys in state output

Friendly ping for review. If there are no concerns, I think this would
be useful especially in the logging/bugreport use cases.

On Mon, Jan 7, 2019 at 3:10 PM Benedict Wong <benedictwong@...gle.com> wrote:
>
> (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