Improve the parser of TCP/session packets.

Add more safeguards against malformed packets and fix existing ones. Also,
rename a few variables and constants to match their real meaning. For
example, rename gen_msg_tlv() to gen_msg_hdr() because this function
generates an LDP header, not a TLV.

Finally, clean-up all the send_* functions so they all follow the same
pattern.

8 files changed, 56 insertions, 70 deletions

diff --git a/usr.sbin/ldpd/address.c b/usr.sbin/ldpd/address.c
index edebfc4c02e..209a894c6c6 100644
--- a/usr.sbin/ldpd/address.c
+++ b/usr.sbin/ldpd/address.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: address.c,v 1.19 2016/05/23 15:49:31 renato Exp $ */
+/*	$OpenBSD: address.c,v 1.20 2016/05/23 16:04:04 renato Exp $ */
 
 /*
  * Copyright (c) 2009 Michele Marchetto <michele@openbsd.org>
@@ -90,8 +90,8 @@ recv_address(struct nbr *nbr, char *buf, u_int16_t len)
 	else
 		type = IMSG_ADDRESS_DEL;
 
-	buf += sizeof(struct ldp_msg);
-	len -= sizeof(struct ldp_msg);
+	buf += LDP_MSG_SIZE;
+	len -= LDP_MSG_SIZE;
 
 	if (len < sizeof(alt)) {
 		session_shutdown(nbr, S_BAD_MSG_LEN, addr.msgid, addr.type);
@@ -132,7 +132,7 @@ recv_address(struct nbr *nbr, char *buf, u_int16_t len)
 		return (-1);
 	}
 
-	return (ntohs(addr.length));
+	return (0);
 }
 
 void
diff --git a/usr.sbin/ldpd/hello.c b/usr.sbin/ldpd/hello.c
index 17b13ad6482..ed2369fe3b0 100644
--- a/usr.sbin/ldpd/hello.c
+++ b/usr.sbin/ldpd/hello.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: hello.c,v 1.31 2016/05/23 15:53:40 renato Exp $ */
+/*	$OpenBSD: hello.c,v 1.32 2016/05/23 16:04:04 renato Exp $ */
 
 /*
  * Copyright (c) 2009 Michele Marchetto <michele@openbsd.org>
@@ -72,19 +72,16 @@ send_hello(enum hello_type type, struct iface *iface, struct tnbr *tnbr)
 		break;
 	}
 
-	if ((buf = ibuf_open(LDP_MAX_LEN)) == NULL)
-		fatal(__func__);
-
-	size = LDP_HDR_SIZE + sizeof(struct ldp_msg) +
-	    sizeof(struct hello_prms_tlv) +
+	size = LDP_HDR_SIZE + LDP_MSG_SIZE + sizeof(struct hello_prms_tlv) +
 	    sizeof(struct hello_prms_opt4_tlv);
 
-	gen_ldp_hdr(buf, size);
+	/* generate message */
+	if ((buf = ibuf_open(size)) == NULL)
+		fatal(__func__);
 
+	gen_ldp_hdr(buf, size);
 	size -= LDP_HDR_SIZE;
-
-	gen_msg_tlv(buf, MSG_TYPE_HELLO, size);
-
+	gen_msg_hdr(buf, MSG_TYPE_HELLO, size);
 	gen_hello_prms_tlv(buf, holdtime, flags);
 	gen_opt4_hello_prms_tlv(buf, TLV_TYPE_IPV4TRANSADDR,
 	    leconf->trans_addr.s_addr);
diff --git a/usr.sbin/ldpd/init.c b/usr.sbin/ldpd/init.c
index 0fa6aca3bd2..f223b6801bd 100644
--- a/usr.sbin/ldpd/init.c
+++ b/usr.sbin/ldpd/init.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: init.c,v 1.20 2016/05/23 16:01:59 renato Exp $ */
+/*	$OpenBSD: init.c,v 1.21 2016/05/23 16:04:04 renato Exp $ */
 
 /*
  * Copyright (c) 2009 Michele Marchetto <michele@openbsd.org>
@@ -49,19 +49,14 @@ send_init(struct nbr *nbr)
 
 	log_debug("%s: neighbor ID %s", __func__, inet_ntoa(nbr->id));
 
-	if ((buf = ibuf_open(LDP_MAX_LEN)) == NULL)
+	size = LDP_HDR_SIZE + LDP_MSG_SIZE + SESS_PRMS_SIZE;
+	if ((buf = ibuf_open(size)) == NULL)
 		fatal(__func__);
 
-	size = LDP_HDR_SIZE + sizeof(struct ldp_msg) + SESS_PRMS_SIZE;
-
 	gen_ldp_hdr(buf, size);
-
 	size -= LDP_HDR_SIZE;
-
-	gen_msg_tlv(buf, MSG_TYPE_INIT, size);
-
-	size -= sizeof(struct ldp_msg);
-
+	gen_msg_hdr(buf, MSG_TYPE_INIT, size);
+	size -= LDP_MSG_SIZE;
 	gen_init_prms_tlv(buf, nbr, size);
 
 	evbuf_enqueue(&nbr->tcp->wbuf, buf);
@@ -87,8 +82,7 @@ recv_init(struct nbr *nbr, char *buf, u_int16_t len)
 	}
 	bcopy(buf, &sess, sizeof(sess));
 
-	if (ntohs(sess.length) != SESS_PRMS_SIZE - TLV_HDR_LEN ||
-	    ntohs(sess.length) > len - TLV_HDR_LEN) {
+	if (ntohs(sess.length) != SESS_PRMS_SIZE - TLV_HDR_LEN) {
 		session_shutdown(nbr, S_BAD_TLV_LEN, init.msgid, init.type);
 		return (-1);
 	}
@@ -122,7 +116,7 @@ recv_init(struct nbr *nbr, char *buf, u_int16_t len)
 
 	nbr_fsm(nbr, NBR_EVT_INIT_RCVD);
 
-	return (ntohs(init.length));
+	return (0);
 }
 
 int
diff --git a/usr.sbin/ldpd/keepalive.c b/usr.sbin/ldpd/keepalive.c
index e22937a5a9b..2d231a71d21 100644
--- a/usr.sbin/ldpd/keepalive.c
+++ b/usr.sbin/ldpd/keepalive.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: keepalive.c,v 1.13 2016/05/23 15:14:07 renato Exp $ */
+/*	$OpenBSD: keepalive.c,v 1.14 2016/05/23 16:04:04 renato Exp $ */
 
 /*
  * Copyright (c) 2009 Michele Marchetto <michele@openbsd.org>
@@ -42,16 +42,13 @@ send_keepalive(struct nbr *nbr)
 	struct ibuf	*buf;
 	u_int16_t	 size;
 
-	if ((buf = ibuf_open(LDP_MAX_LEN)) == NULL)
+	size = LDP_HDR_SIZE + LDP_MSG_SIZE;
+	if ((buf = ibuf_open(size)) == NULL)
 		fatal(__func__);
 
-	size = LDP_HDR_SIZE + sizeof(struct ldp_msg);
-
 	gen_ldp_hdr(buf, size);
-
 	size -= LDP_HDR_SIZE;
-
-	gen_msg_tlv(buf, MSG_TYPE_KEEPALIVE, size);
+	gen_msg_hdr(buf, MSG_TYPE_KEEPALIVE, size);
 
 	evbuf_enqueue(&nbr->tcp->wbuf, buf);
 }
@@ -71,5 +68,5 @@ recv_keepalive(struct nbr *nbr, char *buf, u_int16_t len)
 	if (nbr->state != NBR_STA_OPER)
 		nbr_fsm(nbr, NBR_EVT_KEEPALIVE_RCVD);
 
-	return (ntohs(ka.length));
+	return (0);
 }
diff --git a/usr.sbin/ldpd/labelmapping.c b/usr.sbin/ldpd/labelmapping.c
index 617e5f82ca8..9b24bdb7890 100644
--- a/usr.sbin/ldpd/labelmapping.c
+++ b/usr.sbin/ldpd/labelmapping.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: labelmapping.c,v 1.35 2016/05/23 15:14:07 renato Exp $ */
+/*	$OpenBSD: labelmapping.c,v 1.36 2016/05/23 16:04:04 renato Exp $ */
 
 /*
  * Copyright (c) 2009 Michele Marchetto <michele@openbsd.org>
@@ -389,7 +389,7 @@ recv_labelmessage(struct nbr *nbr, char *buf, u_int16_t len, u_int16_t type)
 		free(me);
 	}
 
-	return (ntohs(lm.length));
+	return (0);
 
 err:
 	mapping_list_clr(&mh);
diff --git a/usr.sbin/ldpd/ldp.h b/usr.sbin/ldpd/ldp.h
index de93c1c6904..48a397bb0f0 100644
--- a/usr.sbin/ldpd/ldp.h
+++ b/usr.sbin/ldpd/ldp.h
@@ -1,4 +1,4 @@
-/*	$OpenBSD: ldp.h,v 1.21 2016/05/23 15:53:40 renato Exp $ */
+/*	$OpenBSD: ldp.h,v 1.22 2016/05/23 16:04:04 renato Exp $ */
 
 /*
  * Copyright (c) 2009 Michele Marchetto <michele@openbsd.org>
@@ -102,8 +102,9 @@ struct ldp_hdr {
 	u_int16_t		lspace_id;
 } __packed;
 
-#define	LDP_HDR_SIZE		10
-#define	LDP_HDR_PDU_LEN		6
+#define	LDP_HDR_SIZE		10	/* actual size of the LDP header */
+#define	LDP_HDR_PDU_LEN		6	/* minimum "PDU Length" */
+#define LDP_HDR_DEAD_LEN	4
 
 /* TLV record */
 struct tlv {
@@ -120,7 +121,9 @@ struct ldp_msg {
 	/* Optional Parameters */
 } __packed;
 
-#define LDP_MSG_LEN		8
+#define LDP_MSG_SIZE		8	/* minimum size of LDP message */
+#define LDP_MSG_LEN		4	/* minimum "Message Length" */
+#define LDP_MSG_DEAD_LEN	4
 
 #define	UNKNOWN_FLAG		0x8000
 #define	FORWARD_FLAG		0xc000
diff --git a/usr.sbin/ldpd/notification.c b/usr.sbin/ldpd/notification.c
index 0e42ea53c21..6d4ca1884f9 100644
--- a/usr.sbin/ldpd/notification.c
+++ b/usr.sbin/ldpd/notification.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: notification.c,v 1.20 2016/05/23 15:59:55 renato Exp $ */
+/*	$OpenBSD: notification.c,v 1.21 2016/05/23 16:04:04 renato Exp $ */
 
 /*
  * Copyright (c) 2009 Michele Marchetto <michele@openbsd.org>
@@ -48,11 +48,8 @@ send_notification_full(struct tcp_conn *tcp, struct notify_msg *nm)
 		log_debug("%s: nbr ID %s, status %s", __func__,
 		    inet_ntoa(tcp->nbr->id), notification_name(nm->status));
 
-	if ((buf = ibuf_open(LDP_MAX_LEN)) == NULL)
-		fatal(__func__);
-
 	/* calculate size */
-	size = LDP_HDR_SIZE + sizeof(struct ldp_msg) + STATUS_SIZE;
+	size = LDP_HDR_SIZE + LDP_MSG_SIZE + STATUS_SIZE;
 	if (nm->flags & F_NOTIF_PW_STATUS)
 		size += PW_STATUS_TLV_LEN;
 	if (nm->flags & F_NOTIF_FEC) {
@@ -66,12 +63,12 @@ send_notification_full(struct tcp_conn *tcp, struct notify_msg *nm)
 		}
 	}
 
-	gen_ldp_hdr(buf, size);
+	if ((buf = ibuf_open(size)) == NULL)
+		fatal(__func__);
 
+	gen_ldp_hdr(buf, size);
 	size -= LDP_HDR_SIZE;
-
-	gen_msg_tlv(buf, MSG_TYPE_NOTIFICATION, size);
-
+	gen_msg_hdr(buf, MSG_TYPE_NOTIFICATION, size);
 	gen_status_tlv(buf, nm->status, nm->messageid, nm->type);
 	/* optional tlvs */
 	if (nm->flags & F_NOTIF_PW_STATUS)
@@ -239,7 +236,7 @@ recv_notification(struct nbr *nbr, char *buf, u_int16_t len)
 		ldpe_imsg_compose_lde(IMSG_NOTIFICATION, nbr->peerid, 0,
 		    &nm, sizeof(nm));
 
-	return (ntohs(not.length));
+	return (0);
 }
 
 int
diff --git a/usr.sbin/ldpd/packet.c b/usr.sbin/ldpd/packet.c
index d8c96ca1fa8..cdae6fbbf22 100644
--- a/usr.sbin/ldpd/packet.c
+++ b/usr.sbin/ldpd/packet.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: packet.c,v 1.46 2016/05/23 16:01:59 renato Exp $ */
+/*	$OpenBSD: packet.c,v 1.47 2016/05/23 16:04:04 renato Exp $ */
 
 /*
  * Copyright (c) 2009 Michele Marchetto <michele@openbsd.org>
@@ -54,11 +54,8 @@ gen_ldp_hdr(struct ibuf *buf, u_int16_t size)
 
 	bzero(&ldp_hdr, sizeof(ldp_hdr));
 	ldp_hdr.version = htons(LDP_VERSION);
-
-	/* We want just the size of the value */
-	size -= TLV_HDR_LEN;
-
-	ldp_hdr.length = htons(size);
+	/* exclude the 'Version' and 'PDU Length' fields from the total */
+	ldp_hdr.length = htons(size - LDP_HDR_DEAD_LEN);
 	ldp_hdr.lsr_id = leconf->rtr_id.s_addr;
 	ldp_hdr.lspace_id = 0;
 
@@ -344,11 +341,11 @@ session_read(int fd, short event, void *arg)
 		 * "Prior to completion of the negotiation, the maximum
 		 * allowable length is 4096 bytes".
 		 */
-		if (nbr->state == NBR_STA_OPER)
+		if (nbr && nbr->state == NBR_STA_OPER)
 			max_pdu_len = nbr->max_pdu_len;
 		else
 			max_pdu_len = LDP_MAX_LEN;
-		if (pdu_len < (LDP_HDR_PDU_LEN + LDP_MSG_LEN) ||
+		if (pdu_len < (LDP_HDR_PDU_LEN + LDP_MSG_SIZE) ||
 		    pdu_len > max_pdu_len) {
 			if (nbr)
 				session_shutdown(nbr, S_BAD_PDU_LEN, 0, 0);
@@ -360,6 +357,7 @@ session_read(int fd, short event, void *arg)
 			free(buf);
 			return;
 		}
+		pdu_len -= LDP_HDR_PDU_LEN;
 
 		if (nbr) {
 			if (ldp_hdr->lsr_id != nbr->id.s_addr ||
@@ -451,25 +449,25 @@ session_read(int fd, short event, void *arg)
 			/* switch LDP packet type */
 			switch (type) {
 			case MSG_TYPE_NOTIFICATION:
-				msg_size = recv_notification(nbr, pdu, pdu_len);
+				ret = recv_notification(nbr, pdu, msg_size);
 				break;
 			case MSG_TYPE_INIT:
-				msg_size = recv_init(nbr, pdu, pdu_len);
+				ret = recv_init(nbr, pdu, msg_size);
 				break;
 			case MSG_TYPE_KEEPALIVE:
-				msg_size = recv_keepalive(nbr, pdu, pdu_len);
+				ret = recv_keepalive(nbr, pdu, msg_size);
 				break;
 			case MSG_TYPE_ADDR:
 			case MSG_TYPE_ADDRWITHDRAW:
-				msg_size = recv_address(nbr, pdu, pdu_len);
+				ret = recv_address(nbr, pdu, msg_size);
 				break;
 			case MSG_TYPE_LABELMAPPING:
 			case MSG_TYPE_LABELREQUEST:
 			case MSG_TYPE_LABELWITHDRAW:
 			case MSG_TYPE_LABELRELEASE:
 			case MSG_TYPE_LABELABORTREQ:
-				msg_size = recv_labelmessage(nbr, pdu,
-				    pdu_len, type);
+				ret = recv_labelmessage(nbr, pdu, msg_size,
+				    type);
 				break;
 			default:
 				log_debug("%s: unknown LDP packet from nbr %s",
@@ -481,19 +479,19 @@ session_read(int fd, short event, void *arg)
 					return;
 				}
 				/* unknown flag is set, ignore the message */
-				msg_size = ntohs(ldp_msg->length);
+				ret = 0;
 				break;
 			}
 
-			if (msg_size == -1) {
+			if (ret == -1) {
 				/* parser failed, giving up */
 				free(buf);
 				return;
 			}
 
 			/* Analyse the next message */
-			pdu += msg_size + TLV_HDR_LEN;
-			len -= msg_size + TLV_HDR_LEN;
+			pdu += msg_size;
+			len -= msg_size;
 		}
 		free(buf);
 		if (len != 0) {
@@ -558,7 +556,7 @@ session_get_pdu(struct ibuf_read *r, char **b)
 		return (0);
 
 	memcpy(&l, r->buf, sizeof(l));
-	dlen = ntohs(l.length) + TLV_HDR_LEN;
+	dlen = ntohs(l.length) + LDP_HDR_DEAD_LEN;
 	if (dlen > av)
 		return (0);