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, 24 insertions, 14 deletions

diff --git a/ospfd/ospf_api.c b/ospfd/ospf_api.c
index 7e7236a3b..81de88275 100644
--- a/ospfd/ospf_api.c
+++ b/ospfd/ospf_api.c
@@ -58,7 +58,7 @@
 
 
 /* For debugging only, will be removed */
-void api_opaque_lsa_print(struct lsa_header *data)
+void api_opaque_lsa_print(struct ospf_lsa *lsa)
 {
 	struct opaque_lsa {
 		struct lsa_header header;
@@ -69,11 +69,11 @@ void api_opaque_lsa_print(struct lsa_header *data)
 	int opaquelen;
 	int i;
 
-	ospf_lsa_header_dump(data);
+	ospf_lsa_header_dump(lsa->data);
 
-	olsa = (struct opaque_lsa *)data;
+	olsa = (struct opaque_lsa *)lsa->data;
 
-	opaquelen = ntohs(data->length) - OSPF_LSA_HEADER_SIZE;
+	opaquelen = lsa->size - OSPF_LSA_HEADER_SIZE;
 	zlog_debug("apiserver_lsa_print: opaquelen=%d", opaquelen);
 
 	for (i = 0; i < opaquelen; i++) {
@@ -111,11 +111,16 @@ struct msg *msg_new(uint8_t msgtype, void *msgbody, uint32_t seqnum,
 struct msg *msg_dup(struct msg *msg)
 {
 	struct msg *new;
+	size_t size;
 
 	assert(msg);
 
+	size = ntohs(msg->hdr.msglen);
+	if (size > OSPF_MAX_LSA_SIZE)
+		return NULL;
+
 	new = msg_new(msg->hdr.msgtype, STREAM_DATA(msg->s),
-		      ntohl(msg->hdr.msgseq), ntohs(msg->hdr.msglen));
+		      ntohl(msg->hdr.msgseq), size);
 	return new;
 }
 
@@ -400,7 +405,7 @@ struct msg *msg_read(int fd)
 	}
 
 	/* Allocate new message */
-	msg = msg_new(hdr.msgtype, buf, ntohl(hdr.msgseq), ntohs(hdr.msglen));
+	msg = msg_new(hdr.msgtype, buf, ntohl(hdr.msgseq), bodylen);
 
 	return msg;
 }
@@ -408,29 +413,34 @@ struct msg *msg_read(int fd)
 int msg_write(int fd, struct msg *msg)
 {
 	uint8_t buf[OSPF_API_MAX_MSG_SIZE];
-	int l;
+	uint16_t l;
 	int wlen;
 
 	assert(msg);
 	assert(msg->s);
 
-	/* Length of message including header */
-	l = sizeof(struct apimsghdr) + ntohs(msg->hdr.msglen);
+	/* Length of OSPF LSA payload */
+	l = ntohs(msg->hdr.msglen);
+	if (l > OSPF_MAX_LSA_SIZE) {
+		zlog_warn("%s: wrong LSA size %d", __func__, l);
+		return -1;
+	}
 
 	/* Make contiguous memory buffer for message */
 	memcpy(buf, &msg->hdr, sizeof(struct apimsghdr));
-	memcpy(buf + sizeof(struct apimsghdr), STREAM_DATA(msg->s),
-	       ntohs(msg->hdr.msglen));
+	memcpy(buf + sizeof(struct apimsghdr), STREAM_DATA(msg->s), l);
 
+	/* Total length of OSPF API Message */
+	l += sizeof(struct apimsghdr);
 	wlen = writen(fd, buf, l);
 	if (wlen < 0) {
-		zlog_warn("msg_write: writen %s", safe_strerror(errno));
+		zlog_warn("%s: writen %s", __func__, safe_strerror(errno));
 		return -1;
 	} else if (wlen == 0) {
-		zlog_warn("msg_write: Connection closed by peer");
+		zlog_warn("%s: Connection closed by peer", __func__);
 		return -1;
 	} else if (wlen != l) {
-		zlog_warn("msg_write: Cannot write API message");
+		zlog_warn("%s: Cannot write API message", __func__);
 		return -1;
 	}
 	return 0;