From 0096d392ea82ea173b4c72961d6d2b530f4b55ae Mon Sep 17 00:00:00 2001
From: Alistair King <alistair@kentik.com>
Date: Wed, 24 Mar 2021 14:07:21 +1300
Subject: [PATCH] Fix parsing of missing OPEN/UPDATE messages

It seems that FRR is sending a BGP message with the OPEN type set, but
no OPEN message
(https://github.com/FRRouting/frr/blob/8baa41e571f4a741c29116b35b28a8f7dff586f7/bgpd/bgp_bmp.c#L395)

From my reading of the RFC this is incorrect, but previously we would
try and parse the subsequent bytes as if they were part of the OPEN
message.

Also applies the same length check to the UPDATE message parsing since
they apparently cannot be empty either.
---
 lib/bgp/parsebgp_bgp_open.c   | 19 ++++++++++++++++---
 lib/bgp/parsebgp_bgp_update.c |  5 +++++
 2 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/lib/bgp/parsebgp_bgp_open.c b/lib/bgp/parsebgp_bgp_open.c
index e718a98..c89dcb3 100644
--- a/lib/bgp/parsebgp_bgp_open.c
+++ b/lib/bgp/parsebgp_bgp_open.c
@@ -39,8 +39,7 @@ static parsebgp_error_t parse_capabilities(parsebgp_opts_t *opts,
   size_t len = *lenp, nread = 0;
   parsebgp_bgp_open_capability_t *cap;
 
-  while ((remain - nread) > 0) {
-
+  while (nread < remain) {
     PARSEBGP_MAYBE_REALLOC(msg->capabilities,
       msg->_capabilities_alloc_cnt, msg->capabilities_cnt + 1);
     cap = &msg->capabilities[msg->capabilities_cnt++];
@@ -150,7 +149,7 @@ static parsebgp_error_t parse_params(parsebgp_opts_t *opts,
 
   msg->capabilities_cnt = 0;
 
-  while ((remain - nread) > 0) {
+  while (nread < remain) {
     // Ensure this is a capabilities parameter
     PARSEBGP_DESERIALIZE_UINT8(buf, len, nread, u8);
     if (u8 != 2) {
@@ -185,6 +184,11 @@ parsebgp_error_t parsebgp_bgp_open_decode(parsebgp_opts_t *opts,
   size_t len = *lenp, nread = 0, slen;
   parsebgp_error_t err;
 
+  if (remain == 0) {
+    // Cannot have an OPEN message type with no length
+    PARSEBGP_RETURN_INVALID_MSG_ERR;
+  }
+
   // Version
   PARSEBGP_DESERIALIZE_UINT8(buf, len, nread, msg->version);
 
@@ -206,6 +210,15 @@ parsebgp_error_t parsebgp_bgp_open_decode(parsebgp_opts_t *opts,
     return PARSEBGP_OK;
   }
 
+  if (len <= nread) {
+    // nothing left in the buffer
+    return PARSEBGP_PARTIAL_MSG;
+  }
+  if (remain <= nread) {
+    // reported message length is too short
+    PARSEBGP_RETURN_INVALID_MSG_ERR;
+  }
+
   // Parse the capabilities
   slen = len - nread;
   if ((err = parse_params(opts, msg, buf, &slen, (remain - nread))) !=
diff --git a/lib/bgp/parsebgp_bgp_update.c b/lib/bgp/parsebgp_bgp_update.c
index 68357a6..4d8f3a9 100644
--- a/lib/bgp/parsebgp_bgp_update.c
+++ b/lib/bgp/parsebgp_bgp_update.c
@@ -1079,6 +1079,11 @@ parsebgp_error_t parsebgp_bgp_update_decode(parsebgp_opts_t *opts,
   size_t len = *lenp, nread = 0, slen = 0;
   parsebgp_error_t err;
 
+  if (remain == 0) {
+      // Cannot have an UPDATE message type with no length
+    PARSEBGP_RETURN_INVALID_MSG_ERR;
+  }
+
   // Withdrawn Routes Length
   PARSEBGP_DESERIALIZE_UINT16(buf, len, nread, msg->withdrawn_nlris.len);