ospfd: Correct Coverity defects

When browsing or parsing OSPF LSA TLVs, we need to use the LSA length which is
part of the LSA header. This length, encoded in 16 bits, must be first
converted to host byte order with ntohs() function. However, Coverity Scan
considers that ntohs() function return TAINTED data. Thus, when the length is
used to control for() loop, Coverity Scan marks this part of the code as defect
with "Untrusted Loop Bound" due to the usage of Tainted variable. Similar
problems occur when browsing sub-TLV where length is extracted with ntohs().

To overcome this limitation, a size attribute has been added to the ospf_lsa
structure. The size is set when lsa->data buffer is allocated. In addition,
when an OSPF packet is received, the size of the payload is controlled before
contains is processed. For OSPF LSA, this allow a secure buffer allocation.
Thus, new size attribute contains the exact buffer allocation allowing a
strict control during TLV browsing.

This patch adds extra control to bound for() loop during TLV browsing to
avoid potential problem as suggested by Coverity Scan. Controls are based
on new size attribute of the ospf_lsa structure to avoid any ambiguity.

Signed-off-by: Olivier Dugeon <olivier.dugeon@orange.com>

1 files changed, 64 insertions, 19 deletions

diff --git a/ospfd/ospf_ext.c b/ospfd/ospf_ext.c
index 754e2bcba..2d08eeece 100644
--- a/ospfd/ospf_ext.c
+++ b/ospfd/ospf_ext.c
@@ -1715,13 +1715,23 @@ static void ospf_ext_lsa_schedule(struct ext_itf *exti, enum lsa_opcode op)
  * ------------------------------------
  */
 
+#define check_tlv_size(size, msg)                                              \
+	do {                                                                   \
+		if (ntohs(tlvh->length) != size) {                             \
+			vty_out(vty, "  Wrong %s TLV size: %d(%d). Abort!\n",  \
+				msg, ntohs(tlvh->length), size);               \
+			return size + TLV_HDR_SIZE;                            \
+		}                                                              \
+	} while (0)
+
 /* Cisco experimental SubTLV */
 static uint16_t show_vty_ext_link_rmt_itf_addr(struct vty *vty,
 					       struct tlv_header *tlvh)
 {
-	struct ext_subtlv_rmt_itf_addr *top;
+	struct ext_subtlv_rmt_itf_addr *top =
+		(struct ext_subtlv_rmt_itf_addr *)tlvh;
 
-	top = (struct ext_subtlv_rmt_itf_addr *)tlvh;
+	check_tlv_size(EXT_SUBTLV_RMT_ITF_ADDR_SIZE, "Remote Itf. Address");
 
 	vty_out(vty,
 		"  Remote Interface Address Sub-TLV: Length %u\n	Address: %pI4\n",
@@ -1736,6 +1746,8 @@ static uint16_t show_vty_ext_link_adj_sid(struct vty *vty,
 {
 	struct ext_subtlv_adj_sid *top = (struct ext_subtlv_adj_sid *)tlvh;
 
+	check_tlv_size(EXT_SUBTLV_ADJ_SID_SIZE, "Adjacency SID");
+
 	vty_out(vty,
 		"  Adj-SID Sub-TLV: Length %u\n\tFlags: 0x%x\n\tMT-ID:0x%x\n\tWeight: 0x%x\n\t%s: %u\n",
 		ntohs(top->header.length), top->flags, top->mtid, top->weight,
@@ -1755,6 +1767,8 @@ static uint16_t show_vty_ext_link_lan_adj_sid(struct vty *vty,
 	struct ext_subtlv_lan_adj_sid *top =
 		(struct ext_subtlv_lan_adj_sid *)tlvh;
 
+	check_tlv_size(EXT_SUBTLV_LAN_ADJ_SID_SIZE, "Lan-Adjacency SID");
+
 	vty_out(vty,
 		"  LAN-Adj-SID Sub-TLV: Length %u\n\tFlags: 0x%x\n\tMT-ID:0x%x\n\tWeight: 0x%x\n\tNeighbor ID: %pI4\n\t%s: %u\n",
 		ntohs(top->header.length), top->flags, top->mtid, top->weight,
@@ -1768,8 +1782,15 @@ static uint16_t show_vty_ext_link_lan_adj_sid(struct vty *vty,
 	return TLV_SIZE(tlvh);
 }
 
-static uint16_t show_vty_unknown_tlv(struct vty *vty, struct tlv_header *tlvh)
+static uint16_t show_vty_unknown_tlv(struct vty *vty, struct tlv_header *tlvh,
+				     size_t buf_size)
 {
+	if (TLV_SIZE(tlvh) > buf_size) {
+		vty_out(vty, "    TLV size %d exceeds buffer size. Abort!",
+			TLV_SIZE(tlvh));
+		return buf_size;
+	}
+
 	vty_out(vty, "    Unknown TLV: [type(0x%x), length(0x%x)]\n",
 		ntohs(tlvh->type), ntohs(tlvh->length));
 
@@ -1777,13 +1798,22 @@ static uint16_t show_vty_unknown_tlv(struct vty *vty, struct tlv_header *tlvh)
 }
 
 /* Extended Link Sub TLVs */
-static uint16_t show_vty_link_info(struct vty *vty, struct tlv_header *ext)
+static uint16_t show_vty_link_info(struct vty *vty, struct tlv_header *ext,
+				   size_t buf_size)
 {
 	struct ext_tlv_link *top = (struct ext_tlv_link *)ext;
 	struct tlv_header *tlvh;
-	uint16_t length = ntohs(top->header.length) - 3 * sizeof(uint32_t);
+	uint16_t length = ntohs(top->header.length);
 	uint16_t sum = 0;
 
+	/* Verify that TLV length is valid against remaining buffer size */
+	if (length > buf_size) {
+		vty_out(vty,
+			"  Extended Link TLV size %d exceeds buffer size. Abort!\n",
+			length);
+		return buf_size;
+	}
+
 	vty_out(vty,
 		"  Extended Link TLV: Length %u\n	Link Type: 0x%x\n"
 		"	Link ID: %pI4\n",
@@ -1791,9 +1821,11 @@ static uint16_t show_vty_link_info(struct vty *vty, struct tlv_header *ext)
 		&top->link_id);
 	vty_out(vty, "	Link data: %pI4\n", &top->link_data);
 
+	/* Skip Extended TLV and parse sub-TLVs */
+	length -= EXT_TLV_LINK_SIZE;
 	tlvh = (struct tlv_header *)((char *)(ext) + TLV_HDR_SIZE
 				     + EXT_TLV_LINK_SIZE);
-	for (; sum < length; tlvh = TLV_HDR_NEXT(tlvh)) {
+	for (; sum < length && tlvh; tlvh = TLV_HDR_NEXT(tlvh)) {
 		switch (ntohs(tlvh->type)) {
 		case EXT_SUBTLV_ADJ_SID:
 			sum += show_vty_ext_link_adj_sid(vty, tlvh);
@@ -1805,7 +1837,7 @@ static uint16_t show_vty_link_info(struct vty *vty, struct tlv_header *ext)
 			sum += show_vty_ext_link_rmt_itf_addr(vty, tlvh);
 			break;
 		default:
-			sum += show_vty_unknown_tlv(vty, tlvh);
+			sum += show_vty_unknown_tlv(vty, tlvh, length - sum);
 			break;
 		}
 	}
@@ -1821,16 +1853,16 @@ static void ospf_ext_link_show_info(struct vty *vty, struct ospf_lsa *lsa)
 	uint16_t length = 0, sum = 0;
 
 	/* Initialize TLV browsing */
-	length = ntohs(lsah->length) - OSPF_LSA_HEADER_SIZE;
+	length = lsa->size - OSPF_LSA_HEADER_SIZE;
 
-	for (tlvh = TLV_HDR_TOP(lsah); sum < length;
+	for (tlvh = TLV_HDR_TOP(lsah); sum < length && tlvh;
 	     tlvh = TLV_HDR_NEXT(tlvh)) {
 		switch (ntohs(tlvh->type)) {
 		case EXT_TLV_LINK:
-			sum += show_vty_link_info(vty, tlvh);
+			sum += show_vty_link_info(vty, tlvh, length - sum);
 			break;
 		default:
-			sum += show_vty_unknown_tlv(vty, tlvh);
+			sum += show_vty_unknown_tlv(vty, tlvh, length - sum);
 			break;
 		}
 	}
@@ -1843,6 +1875,8 @@ static uint16_t show_vty_ext_pref_pref_sid(struct vty *vty,
 	struct ext_subtlv_prefix_sid *top =
 		(struct ext_subtlv_prefix_sid *)tlvh;
 
+	check_tlv_size(EXT_SUBTLV_PREFIX_SID_SIZE, "Prefix SID");
+
 	vty_out(vty,
 		"  Prefix SID Sub-TLV: Length %u\n\tAlgorithm: %u\n\tFlags: 0x%x\n\tMT-ID:0x%x\n\t%s: %u\n",
 		ntohs(top->header.length), top->algorithm, top->flags,
@@ -1857,28 +1891,39 @@ static uint16_t show_vty_ext_pref_pref_sid(struct vty *vty,
 }
 
 /* Extended Prefix SubTLVs */
-static uint16_t show_vty_pref_info(struct vty *vty, struct tlv_header *ext)
+static uint16_t show_vty_pref_info(struct vty *vty, struct tlv_header *ext,
+				   size_t buf_size)
 {
 	struct ext_tlv_prefix *top = (struct ext_tlv_prefix *)ext;
 	struct tlv_header *tlvh;
-	uint16_t length = ntohs(top->header.length) - 2 * sizeof(uint32_t);
+	uint16_t length = ntohs(top->header.length);
 	uint16_t sum = 0;
 
+	/* Verify that TLV length is valid against remaining buffer size */
+	if (length > buf_size) {
+		vty_out(vty,
+			"  Extended Link TLV size %d exceeds buffer size. Abort!\n",
+			length);
+		return buf_size;
+	}
+
 	vty_out(vty,
 		"  Extended Prefix TLV: Length %u\n\tRoute Type: %u\n"
 		"\tAddress Family: 0x%x\n\tFlags: 0x%x\n\tAddress: %pI4/%u\n",
 		ntohs(top->header.length), top->route_type, top->af, top->flags,
 		&top->address, top->pref_length);
 
+	/* Skip Extended Prefix TLV and parse sub-TLVs */
+	length -= EXT_TLV_PREFIX_SIZE;
 	tlvh = (struct tlv_header *)((char *)(ext) + TLV_HDR_SIZE
 				     + EXT_TLV_PREFIX_SIZE);
-	for (; sum < length; tlvh = TLV_HDR_NEXT(tlvh)) {
+	for (; sum < length && tlvh; tlvh = TLV_HDR_NEXT(tlvh)) {
 		switch (ntohs(tlvh->type)) {
 		case EXT_SUBTLV_PREFIX_SID:
 			sum += show_vty_ext_pref_pref_sid(vty, tlvh);
 			break;
 		default:
-			sum += show_vty_unknown_tlv(vty, tlvh);
+			sum += show_vty_unknown_tlv(vty, tlvh, length - sum);
 			break;
 		}
 	}
@@ -1894,16 +1939,16 @@ static void ospf_ext_pref_show_info(struct vty *vty, struct ospf_lsa *lsa)
 	uint16_t length = 0, sum = 0;
 
 	/* Initialize TLV browsing */
-	length = ntohs(lsah->length) - OSPF_LSA_HEADER_SIZE;
+	length = lsa->size - OSPF_LSA_HEADER_SIZE;
 
-	for (tlvh = TLV_HDR_TOP(lsah); sum < length;
+	for (tlvh = TLV_HDR_TOP(lsah); sum < length && tlvh;
 	     tlvh = TLV_HDR_NEXT(tlvh)) {
 		switch (ntohs(tlvh->type)) {
 		case EXT_TLV_PREFIX:
-			sum += show_vty_pref_info(vty, tlvh);
+			sum += show_vty_pref_info(vty, tlvh, length - sum);
 			break;
 		default:
-			sum += show_vty_unknown_tlv(vty, tlvh);
+			sum += show_vty_unknown_tlv(vty, tlvh, length - sum);
 			break;
 		}
 	}