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 <benedictw...@google.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.faine...@gmail.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 <benedictw...@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 " ]"
> > >
> >
> >
> > --
> > Florian
