On Mon, Oct 26, 2020 at 01:07:48PM +0100, Pascal Mathis wrote:
> >Synopsis: OpenBGPD crashes when reloading with duplicate neighbor
> >Category: system
> >Environment:
> System : OpenBSD 6.8
> Details : OpenBSD 6.8 (GENERIC) #97: Sun Oct 4 18:00:46 MDT 2020
>
> [email protected]:/usr/src/sys/arch/amd64/compile/GENERIC
>
> Architecture: OpenBSD.amd64
> Machine : amd64
> >Description:
> When the OpenBGPD configuration mistakenly contains a neighbor
> with a duplicate address, executing "bgpctl reload" results in
> bgpd exiting with a fatal error:
>
> bgpd[75744]: fatal in bgpd: yyparse: peer tree is corrupt
> bgpd[44477]: peer closed imsg connection
> bgpd[73636]: peer closed imsg connection
> bgpd[73636]: fatal in RDE: Lost connection to parent
> bgpd[44477]: SE: Lost connection to parent
>
> While having two neighbors with the same address and port
> configuration is an obvious mistake and probably not useful
> under any circumstances, the daemon should not crash when
> executing "bgpctl reload" and gracefully reject reloading the
> configuration. Interestingly enough, bgpd is able to start up
> from scratch with a duplicate neighbor configuration, resulting
> in the daemon showing both neighbors as active:
>
> Neighbor AS MsgRcvd MsgSent OutQ Up/Down
> State/PrfRcvd
> 192.0.2.1 2 0 0 0 Never
> Connect
> 192.0.2.1 2 0 0 0 Never
> Connect
>
> If I am not missing any specific use case, this is probably also
> a bug / undesired behavior, as having the same neighbor twice
> does not seem like a good choice either. I would expect (1) bgpd
> not to crash when reloading with a configuration mistake and (2)
> bgpd to not start in the first place when a duplicate neighbor
> exists, although that one might be up for debate.
> >How-To-Repeat:
> This issue can be reproduced using this /etc/bgpd.conf:
>
> AS 1
> neighbor 192.0.2.1 { remote-as 2 }
> neighbor 192.0.2.1 { remote-as 2 }
>
> (Re-)starting bgpd using rcctl works fine and "bgpctl show"
> shows the duplicate neighbor twice. However when now executing
> "bgpctl reload", bgpd crashes with a fatal error. It can be
> reliably reproduced every single time and does not seem to
> matter if the remote-as is different.
> >Fix:
> No specific fix known. The issue does not appear when the
> configuration has no duplicates while reloading, but this is
> exactly what this bug is about - catching such a mistake.
Indeed, the parser needs to detect duplicate peer early on.
This diff should do that. It also fixes a few mem-leaks (of curpeer and
curgroup) and brings getid more in sync with the getpeerbyip version of
session.c.
With this duplicate peers will cause a syntax error on startup and
reloads.
--
:wq Claudio
Index: parse.y
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/parse.y,v
retrieving revision 1.408
diff -u -p -r1.408 parse.y
--- parse.y 10 May 2020 13:38:46 -0000 1.408
+++ parse.y 26 Oct 2020 15:30:34 -0000
@@ -1178,8 +1178,10 @@ neighbor : { curpeer = new_peer(); }
curpeer_filter[0] = NULL;
curpeer_filter[1] = NULL;
- if (neighbor_consistent(curpeer) == -1)
+ if (neighbor_consistent(curpeer) == -1) {
+ free(curpeer);
YYERROR;
+ }
if (RB_INSERT(peer_head, new_peers, curpeer) != NULL)
fatalx("%s: peer tree is corrupt", __func__);
curpeer = curgroup;
@@ -1194,11 +1196,13 @@ group : GROUP string {
yyerror("group name \"%s\" too long: max %zu",
$2, sizeof(curgroup->conf.group) - 1);
free($2);
+ free(curgroup);
YYERROR;
}
free($2);
if (get_id(curgroup)) {
yyerror("get_id failed");
+ free(curgroup);
YYERROR;
}
} '{' groupopts_l '}' {
@@ -3987,7 +3991,9 @@ get_id(struct peer *newpeer)
/* neighbor */
if (cur_peers)
RB_FOREACH(p, peer_head, cur_peers)
- if (memcmp(&p->conf.remote_addr,
+ if (p->conf.remote_masklen ==
+ newpeer->conf.remote_masklen &&
+ memcmp(&p->conf.remote_addr,
&newpeer->conf.remote_addr,
sizeof(p->conf.remote_addr)) == 0)
break;
@@ -4196,6 +4202,7 @@ int
neighbor_consistent(struct peer *p)
{
struct bgpd_addr *local_addr;
+ struct peer *xp;
switch (p->conf.remote_addr.aid) {
case AID_INET:
@@ -4251,6 +4258,21 @@ neighbor_consistent(struct peer *p)
if (p->conf.reflector_client && p->conf.ebgp) {
yyerror("EBGP neighbors are not allowed in route "
"reflector clusters");
+ return (-1);
+ }
+
+ /* check for duplicate peer definitions */
+ RB_FOREACH(xp, peer_head, new_peers)
+ if (xp->conf.remote_masklen ==
+ p->conf.remote_masklen &&
+ memcmp(&xp->conf.remote_addr,
+ &p->conf.remote_addr,
+ sizeof(p->conf.remote_addr)) == 0)
+ break;
+ if (xp != NULL) {
+ char *descr = log_fmt_peer(&p->conf);
+ yyerror("duplicate %s", descr);
+ free(descr);
return (-1);
}
