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) {
