ok benno@
Claudio Jeker([email protected]) on 2018.08.06 15:50:41 +0200:
> On Mon, Aug 06, 2018 at 03:34:10PM +0200, Claudio Jeker wrote:
> > On Mon, Aug 06, 2018 at 03:26:16PM +0200, Pierre Emeriaud wrote:
> > > Not sure about this, I guess if you're using 'network bulk add|del'
> > > you're supposed to know what you're doing and not shoot yourself in
> > > the foot, but here it goes:
> > >
> > > echo | bgpctl network bulk del
> > > crashes bgpd. On 6.3 and current as of today at least.
> > >
> > > $ doas bgpd -dvf bgpd.conf
> > > startup
> > > rereading config
> > > route decision engine ready
> > > session engine ready
> > > myas = "64751"
> > > ktable_new: rdomain_0 for rtableid 0
> > > listening on 127.0.0.1
> > > SE reconfigured
> > > RDE reconfigured
> > >
> > > #### bgpd started, time to run 'echo | bgpctl network bulk del'
> > >
> > > fatal in RDE: pt_fill: unknown af
> > > peer closed imsg connection
> > > main: Lost connection to RDE
> > > peer closed imsg connection
> > > SE: Lost connection to parent
> > > session engine exiting
> > > kernel routing table 0 (Loc-RIB) decoupled
> > > ktable_destroy: freeing ktable Loc-RIB rtableid 0
> > > waiting for children to terminate
> > > terminating
> > >
> > >
> > > This somewhat fixes the empty line issue, but it's still possible to
> > > crash with any other string not beeing an ip prefix. A better
> > > workaround would be parse prefixes beforehand, or maybe just don't be
> > > a moron and don't feed bgpctl with anything stupid.
> > >
> > > Index: bgpctl.c
> > > ===================================================================
> > > RCS file: /cvs/src/usr.sbin/bgpctl/bgpctl.c,v
> > > retrieving revision 1.210
> > > diff -u -p -r1.210 bgpctl.c
> > > --- bgpctl.c 29 Jul 2018 13:02:01 -0000 1.210
> > > +++ bgpctl.c 6 Aug 2018 13:11:23 -0000
> > > @@ -1978,6 +1978,9 @@ network_bulk(struct parse_result *res)
> > > /* Don't process commented entries */
> > > if (strchr(b, '#') != NULL)
> > > break;
> > > + /* Don't process empty lines */
> > > + if (strcmp(b, "\0") == 0)
> > > + break;
> > > bzero(&net, sizeof(net));
> > > parse_prefix(b, strlen(b), &h, &len);
> > > net.prefix = h;
> > >
> >
> > Thanks for the diff and the report. I will look into this.
> >
>
> Here a diff to protect bgpd a bit better. It uses the same logic as for
> the IMSG_NETWORK_ADD case and does a very basic check of the prefix passed
> in.
>
> Skipping empty lines in bgpctl is also something we should do but that
> code needs some rework.
>
> --
> :wq Claudio
>
> Index: rde.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/rde.c,v
> retrieving revision 1.410
> diff -u -p -r1.410 rde.c
> --- rde.c 6 Aug 2018 08:13:31 -0000 1.410
> +++ rde.c 6 Aug 2018 13:47:12 -0000
> @@ -527,7 +527,7 @@ rde_dispatch_imsg_session(struct imsgbuf
> break;
> default:
> badnet:
> - log_warnx("rde_dispatch: bad network");
> + log_warnx("request to insert invalid network");
> break;
> }
> break;
> @@ -539,7 +539,23 @@ badnet:
> }
> memcpy(&netconf_s, imsg.data, sizeof(netconf_s));
> TAILQ_INIT(&netconf_s.attrset);
> - network_delete(&netconf_s, 0);
> +
> + switch (netconf_s.prefix.aid) {
> + case AID_INET:
> + if (netconf_s.prefixlen > 32)
> + goto badnetdel;
> + network_delete(&netconf_s, 0);
> + break;
> + case AID_INET6:
> + if (netconf_s.prefixlen > 128)
> + goto badnetdel;
> + network_delete(&netconf_s, 0);
> + break;
> + default:
> +badnetdel:
> + log_warnx("request to remove invalid network");
> + break;
> + }
> break;
> case IMSG_NETWORK_FLUSH:
> if (imsg.hdr.len != IMSG_HEADER_SIZE) {
>
