From 5abcf997c684dadf53b561c4d8079c810c94220c Mon Sep 17 00:00:00 2001
From: claudio <claudio@openbsd.org>
Date: Sun, 16 May 2021 09:09:11 +0000
Subject: [PATCH] Capabilities are only supported on a session when both sides
 announce that capability. Change capa_neg_calc() to check always both the
 ann(ounce) and peer capa struct to figure out what was negotiated. This
 change affects route refersh and graceful restart (where before setting the
 capability in the config to 'no' would not fully disable the feature). Also
 ignore and warn about unexpected route refresh messages. OK benno@

---
 usr.sbin/bgpd/session.c | 35 ++++++++++++++++++++++-------------
 1 file changed, 22 insertions(+), 13 deletions(-)

diff --git a/usr.sbin/bgpd/session.c b/usr.sbin/bgpd/session.c
index b8b6b6dc11b..c8acfad57e3 100644
--- a/usr.sbin/bgpd/session.c
+++ b/usr.sbin/bgpd/session.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: session.c,v 1.414 2021/05/06 09:18:54 claudio Exp $ */
+/*	$OpenBSD: session.c,v 1.415 2021/05/16 09:09:11 claudio Exp $ */
 
 /*
  * Copyright (c) 2003, 2004, 2005 Henning Brauer <henning@openbsd.org>
@@ -1636,7 +1636,7 @@ session_neighbor_rrefresh(struct peer *p)
 {
 	u_int8_t	i;
 
-	if (!p->capa.peer.refresh)
+	if (!p->capa.neg.refresh)
 		return (-1);
 
 	for (i = 0; i < AID_MAX; i++) {
@@ -2257,6 +2257,11 @@ parse_refresh(struct peer *peer)
 		return (0);
 	}
 
+	if (!peer->capa.neg.refresh) {
+		log_peer_warnx(&peer->conf, "peer sent unexpected refresh");
+		return (0);
+	}
+
 	if (imsg_rde(IMSG_REFRESH, peer->conf.id, &aid, sizeof(aid)) == -1)
 		return (-1);
 
@@ -2546,16 +2551,15 @@ capa_neg_calc(struct peer *p)
 {
 	u_int8_t	i, hasmp = 0;
 
-	/* refresh: does not realy matter here, use peer setting */
-	p->capa.neg.refresh = p->capa.peer.refresh;
+	/* a capability is accepted only if both sides announced it */
 
-	/* as4byte: both side must announce capability */
-	if (p->capa.ann.as4byte && p->capa.peer.as4byte)
-		p->capa.neg.as4byte = 1;
-	else
-		p->capa.neg.as4byte = 0;
+	p->capa.neg.refresh =
+	    (p->capa.ann.refresh && p->capa.peer.refresh) != 0;
+
+	p->capa.neg.as4byte =
+	    (p->capa.ann.as4byte && p->capa.peer.as4byte) != 0;
 
-	/* MP: both side must announce capability */
+	/* MP: both side must agree on the AFI,SAFI pair */
 	for (i = 0; i < AID_MAX; i++) {
 		if (p->capa.ann.mp[i] && p->capa.peer.mp[i])
 			p->capa.neg.mp[i] = 1;
@@ -2569,18 +2573,21 @@ capa_neg_calc(struct peer *p)
 		p->capa.neg.mp[AID_INET] = 1;
 
 	/*
-	 * graceful restart: only the peer capabilities are of interest here.
+	 * graceful restart: the peer capabilities are of interest here.
 	 * It is necessary to compare the new values with the previous ones
 	 * and act acordingly. AFI/SAFI that are not part in the MP capability
 	 * are treated as not being present.
+	 * Also make sure that a flush happens if the session stopped
+	 * supporting graceful restart.
 	 */
 
 	for (i = 0; i < AID_MAX; i++) {
 		int8_t	negflags;
 
 		/* disable GR if the AFI/SAFI is not present */
-		if (p->capa.peer.grestart.flags[i] & CAPA_GR_PRESENT &&
-		    p->capa.neg.mp[i] == 0)
+		if (p->capa.ann.grestart.restart == 0 ||
+		    (p->capa.peer.grestart.flags[i] & CAPA_GR_PRESENT &&
+		    p->capa.neg.mp[i] == 0))
 			p->capa.peer.grestart.flags[i] = 0;	/* disable */
 		/* look at current GR state and decide what to do */
 		negflags = p->capa.neg.grestart.flags[i];
@@ -2600,6 +2607,8 @@ capa_neg_calc(struct peer *p)
 	}
 	p->capa.neg.grestart.timeout = p->capa.peer.grestart.timeout;
 	p->capa.neg.grestart.restart = p->capa.peer.grestart.restart;
+	if (p->capa.ann.grestart.restart == 0)
+		p->capa.neg.grestart.restart = 0;
 
 	return (0);
 }
-- 
2.20.1