Improve error handling on session_read

* Don't try to send a Shutdown message if the connection is already
  closed or a read error occured;
* As per RFC 5036, send a Shutdown message if an unexpected message is
  received during the initialization process;
* Check if the whole LSR ID of received messages is correct;
* On ldpe_dispatch_main(), ignore the messages from the lde process
  whose associated neighbor is not in the operational state.
Diff from Renato Westphal

4 files changed, 66 insertions, 69 deletions

diff --git a/usr.sbin/ldpd/address.c b/usr.sbin/ldpd/address.c
index 9bba91c194a..1f9e18c8397 100644
--- a/usr.sbin/ldpd/address.c
+++ b/usr.sbin/ldpd/address.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: address.c,v 1.8 2013/06/01 18:47:07 claudio Exp $ */
+/*	$OpenBSD: address.c,v 1.9 2013/06/01 19:01:32 claudio Exp $ */
 
 /*
  * Copyright (c) 2009 Michele Marchetto <michele@openbsd.org>
@@ -84,12 +84,6 @@ recv_address(struct nbr *nbr, char *buf, u_int16_t len)
 	struct address_list_tlv	alt;
 	enum imsg_type		type;
 
-	if (nbr->state != NBR_STA_OPER) {
-		log_debug("recv_address: neighbor ID %s not operational",
-		    inet_ntoa(nbr->id));
-		return (-1);
-	}
-
 	bcopy(buf, &addr, sizeof(addr));
 	log_debug("recv_address: neighbor ID %s%s", inet_ntoa(nbr->id),
 	    ntohs(addr.type) == MSG_TYPE_ADDR ? "" : " address withdraw");
diff --git a/usr.sbin/ldpd/labelmapping.c b/usr.sbin/ldpd/labelmapping.c
index b44d4f56b90..268e44b88da 100644
--- a/usr.sbin/ldpd/labelmapping.c
+++ b/usr.sbin/ldpd/labelmapping.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: labelmapping.c,v 1.21 2013/06/01 18:47:07 claudio Exp $ */
+/*	$OpenBSD: labelmapping.c,v 1.22 2013/06/01 19:01:32 claudio Exp $ */
 
 /*
  * Copyright (c) 2009 Michele Marchetto <michele@openbsd.org>
@@ -96,12 +96,6 @@ recv_labelmapping(struct nbr *nbr, char *buf, u_int16_t len)
 	int			feclen, lbllen, tlen;
 	u_int8_t		addr_type;
 
-	if (nbr->state != NBR_STA_OPER) {
-		log_debug("recv_labelmapping: neighbor ID %s not operational",
-		    inet_ntoa(nbr->id));
-		return (-1);
-	}
-
 	bcopy(buf, &lm, sizeof(lm));
 
 	buf += sizeof(struct ldp_msg);
@@ -201,12 +195,6 @@ recv_labelrequest(struct nbr *nbr, char *buf, u_int16_t len)
 	int		feclen, tlen;
 	u_int8_t	addr_type;
 
-	if (nbr->state != NBR_STA_OPER) {
-		log_debug("recv_labelrequest: neighbor ID %s not operational",
-		    inet_ntoa(nbr->id));
-		return (-1);
-	}
-
 	bcopy(buf, &lr, sizeof(lr));
 
 	buf += sizeof(struct ldp_msg);
@@ -304,12 +292,6 @@ recv_labelwithdraw(struct nbr *nbr, char *buf, u_int16_t len)
 	int		feclen, tlen, numfec = 0;
 	u_int8_t	addr_type;
 
-	if (nbr->state != NBR_STA_OPER) {
-		log_debug("recv_labelwithdraw: neighbor ID %s not operational",
-		    inet_ntoa(nbr->id));
-		return (-1);
-	}
-
 	bcopy(buf, &lw, sizeof(lw));
 
 	buf += sizeof(struct ldp_msg);
@@ -442,12 +424,6 @@ recv_labelrelease(struct nbr *nbr, char *buf, u_int16_t len)
 	int		feclen, tlen, numfec = 0;
 	u_int8_t	addr_type;
 
-	if (nbr->state != NBR_STA_OPER) {
-		log_debug("recv_labelrelease: neighbor ID %s not operational",
-		    inet_ntoa(nbr->id));
-		return (-1);
-	}
-
 	bcopy(buf, &lr, sizeof(lr));
 
 	buf += sizeof(struct ldp_msg);
@@ -557,12 +533,6 @@ recv_labelabortreq(struct nbr *nbr, char *buf, u_int16_t len)
 	int		feclen, tlen;
 	u_int8_t	addr_type;
 
-	if (nbr->state != NBR_STA_OPER) {
-		log_debug("recv_labelabortreq: neighbor ID %s not operational",
-		    inet_ntoa(nbr->id));
-		return (-1);
-	}
-
 	log_debug("recv_labelabortreq: neighbor ID %s", inet_ntoa(nbr->id));
 
 	bcopy(buf, &la, sizeof(la));
diff --git a/usr.sbin/ldpd/ldpe.c b/usr.sbin/ldpd/ldpe.c
index f3521502f1f..8a2d1716b86 100644
--- a/usr.sbin/ldpd/ldpe.c
+++ b/usr.sbin/ldpd/ldpe.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: ldpe.c,v 1.16 2012/04/12 17:33:43 claudio Exp $ */
+/*	$OpenBSD: ldpe.c,v 1.17 2013/06/01 19:01:32 claudio Exp $ */
 
 /*
  * Copyright (c) 2005 Claudio Jeker <claudio@openbsd.org>
@@ -399,6 +399,8 @@ ldpe_dispatch_lde(int fd, short event, void *bula)
 				    "neighbor");
 				return;
 			}
+			if (nbr->state != NBR_STA_OPER)
+				return;
 
 			switch (imsg.hdr.type) {
 			case IMSG_MAPPING_ADD:
@@ -421,6 +423,8 @@ ldpe_dispatch_lde(int fd, short event, void *bula)
 				    "neighbor");
 				return;
 			}
+			if (nbr->state != NBR_STA_OPER)
+				return;
 
 			switch (imsg.hdr.type) {
 			case IMSG_MAPPING_ADD_END:
@@ -445,6 +449,8 @@ ldpe_dispatch_lde(int fd, short event, void *bula)
 				    "neighbor");
 				return;
 			}
+			if (nbr->state != NBR_STA_OPER)
+				return;
 
 			send_notification_nbr(nbr, nm.status,
 			    htonl(nm.messageid), htonl(nm.type));
diff --git a/usr.sbin/ldpd/packet.c b/usr.sbin/ldpd/packet.c
index 879c95b7b1b..bd1be15fb29 100644
--- a/usr.sbin/ldpd/packet.c
+++ b/usr.sbin/ldpd/packet.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: packet.c,v 1.23 2013/06/01 18:47:07 claudio Exp $ */
+/*	$OpenBSD: packet.c,v 1.24 2013/06/01 19:01:32 claudio Exp $ */
 
 /*
  * Copyright (c) 2009 Michele Marchetto <michele@openbsd.org>
@@ -39,8 +39,6 @@
 #include "log.h"
 #include "ldpe.h"
 
-int		 ldp_hdr_sanity_check(struct ldp_hdr *, u_int16_t,
-		    const struct iface *);
 struct iface	*find_iface(struct ldpd_conf *, unsigned int, struct in_addr);
 ssize_t		 session_get_pdu(struct ibuf_read *, char **);
 
@@ -175,8 +173,12 @@ disc_recv_packet(int fd, short event, void *bula)
 		return;
 	}
 
-	if (ldp_hdr_sanity_check(&ldp_hdr, len, iface) == -1)
+	if (ldp_hdr.lspace_id != 0) {
+		log_debug("disc_recv_packet: invalid label space "
+		    "ID %s, interface %s", ldp_hdr.lspace_id,
+		    iface->name);
 		return;
+	}
 
 	if (ntohs(ldp_hdr.length) >
 	    len - sizeof(ldp_hdr.version) - sizeof(ldp_hdr.length)) {
@@ -193,7 +195,6 @@ disc_recv_packet(int fd, short event, void *bula)
 
 	bcopy(buf + LDP_HDR_SIZE, &ldp_msg, sizeof(ldp_msg));
 
-
 	/* switch LDP packet type */
 	switch (ntohs(ldp_msg.type)) {
 	case MSG_TYPE_HELLO:
@@ -205,23 +206,6 @@ disc_recv_packet(int fd, short event, void *bula)
 	}
 }
 
-int
-ldp_hdr_sanity_check(struct ldp_hdr *ldp_hdr, u_int16_t len,
-    const struct iface *iface)
-{
-	struct in_addr		 addr;
-
-	if (ldp_hdr->lspace_id != 0) {
-		addr.s_addr = ldp_hdr->lspace_id;
-		log_debug("ldp_hdr_sanity_check: invalid label space "
-		    "ID %s, interface %s", inet_ntoa(addr),
-		    iface->name);
-		return (-1);
-	}
-
-	return (0);
-}
-
 struct iface *
 find_iface(struct ldpd_conf *xconf, unsigned int ifindex, struct in_addr src)
 {
@@ -297,7 +281,6 @@ void
 session_read(int fd, short event, void *arg)
 {
 	struct nbr	*nbr = arg;
-	struct iface	*iface = nbr->iface;
 	struct ldp_hdr	*ldp_hdr;
 	struct ldp_msg	*ldp_msg;
 	char		*buf, *pdu;
@@ -313,7 +296,8 @@ session_read(int fd, short event, void *arg)
 	if ((n = read(fd, nbr->rbuf->buf + nbr->rbuf->wpos,
 	    sizeof(nbr->rbuf->buf) - nbr->rbuf->wpos)) == -1) {
 		if (errno != EINTR && errno != EAGAIN) {
-			session_shutdown(nbr, S_SHUTDOWN, 0, 0);
+			log_warn("session_read: read error");
+			nbr_fsm(nbr, NBR_EVT_CLOSE_SESSION);
 			return;
 		}
 		/* retry read */
@@ -321,7 +305,8 @@ session_read(int fd, short event, void *arg)
 	}
 	if (n == 0) {
 		/* connection closed */
-		session_shutdown(nbr, S_SHUTDOWN, 0, 0);
+		log_debug("session_read: connection closed by remote end");
+		nbr_fsm(nbr, NBR_EVT_CLOSE_SESSION);
 		return;
 	}
 	nbr->rbuf->wpos += n;
@@ -342,7 +327,8 @@ session_read(int fd, short event, void *arg)
 			return;
 		}
 
-		if (ldp_hdr_sanity_check(ldp_hdr, len, iface) == -1) {
+		if (ldp_hdr->lsr_id != nbr->id.s_addr ||
+		    ldp_hdr->lspace_id != 0) {
 			session_shutdown(nbr, S_BAD_LDP_ID, 0, 0);
 			free(buf);
 			return;
@@ -353,17 +339,59 @@ session_read(int fd, short event, void *arg)
 
 		while (len >= LDP_MSG_LEN) {
 			ldp_msg = (struct ldp_msg *)pdu;
+			u_int16_t type = ntohs(ldp_msg->type);
 
 			pdu_len = ntohs(ldp_msg->length) + TLV_HDR_LEN;
 			if (pdu_len > len ||
 			    pdu_len < LDP_MSG_LEN - TLV_HDR_LEN) {
-				session_shutdown(nbr, S_BAD_TLV_LEN, 0, 0);
+				session_shutdown(nbr, S_BAD_TLV_LEN,
+				    ldp_msg->msgid, ldp_msg->type);
 				free(buf);
 				return;
 			}
 
+			/* check for error conditions earlier */
+			switch (type) {
+			case MSG_TYPE_NOTIFICATION:
+				/* notifications are always processed */
+				break;
+			case MSG_TYPE_INIT:
+				if ((nbr->state != NBR_STA_INITIAL) &&
+				    (nbr->state != NBR_STA_OPENSENT)) {
+					session_shutdown(nbr, S_SHUTDOWN,
+					    ldp_msg->msgid, ldp_msg->type);
+					free(buf);
+					return;
+				}
+				break;
+			case MSG_TYPE_KEEPALIVE:
+				if ((nbr->state == NBR_STA_INITIAL) ||
+				    (nbr->state == NBR_STA_OPENSENT)) {
+					session_shutdown(nbr, S_SHUTDOWN,
+					    ldp_msg->msgid, ldp_msg->type);
+					free(buf);
+					return;
+				}
+				break;
+			case MSG_TYPE_ADDR:
+			case MSG_TYPE_ADDRWITHDRAW:
+			case MSG_TYPE_LABELMAPPING:
+			case MSG_TYPE_LABELREQUEST:
+			case MSG_TYPE_LABELWITHDRAW:
+			case MSG_TYPE_LABELRELEASE:
+			case MSG_TYPE_LABELABORTREQ:
+			default:
+				if (nbr->state != NBR_STA_OPER) {
+					session_shutdown(nbr, S_SHUTDOWN,
+					    ldp_msg->msgid, ldp_msg->type);
+					free(buf);
+					return;
+				}
+				break;
+			}
+
 			/* switch LDP packet type */
-			switch (ntohs(ldp_msg->type)) {
+			switch (type) {
 			case MSG_TYPE_NOTIFICATION:
 				msg_size = recv_notification(nbr, pdu, pdu_len);
 				break;
@@ -390,10 +418,9 @@ session_read(int fd, short event, void *arg)
 				msg_size = recv_labelrelease(nbr, pdu, pdu_len);
 				break;
 			case MSG_TYPE_LABELABORTREQ:
-			case MSG_TYPE_HELLO:
 			default:
 				log_debug("session_read: unknown LDP packet "
-				    "type interface %s", iface->name);
+				    "from nbr %s", inet_ntoa(nbr->id));
 				free(buf);
 				return;
 			}