CVE-2020-12887: Missing Release of Memory after Effective Lifetime (CWE-401)
value: Memory leaks were discovered in the CoAP library in Arm Mbed OS 5.15.3 when using the Arm mbed-coap library 5.1.5. The CoAP parser is responsible for parsing received CoAP packets. The function sn_coap_parser_options_parse() parses the CoAP option number field of all options present in the input packet. Each option number is calculated as a sum of the previous option number and a delta of the current option. The delta and the previous option number are expressed as unsigned 16-bit integers. Due to lack of overflow detection, it is possible to craft a packet that wraps the option number around and results in the same option number being processed again in a single packet. Certain options allocate memory by calling a memory allocation function. In the cases of COAP_OPTION_URI_QUERY, COAP_OPTION_URI_PATH, COAP_OPTION_LOCATION_QUERY, and COAP_OPTION_ETAG, there is no check on whether memory has already been allocated, which in conjunction with the option number integer overflow may lead to multiple assignments of allocated memory to a single pointer. This has been demonstrated to lead to memory leak by buffer orphaning. As a result, the memory is never freed.
CWE description: The product does not sufficiently track and release allocated memory after it has been used, which slowly consumes remaining memory.
CVSS v3 base score: 7.5
Published: 2020-06-18T19:15Z
Repository: https://github.com/mjurczak/mbed-coap
Language: C
Files changed: source/sn_coap_parser.c
Commit date: 2020-05-11 22:46:09 +0200
Commit message: Implemented measures to prevent memory leaks in sn_coap_parser_options_parse().

Added a helper uint16_t addition function with overflow detection. The function is used when calculating the extended length and option delta. The overlow detection is needed to avoid wrap-around of option number or length.
Additional checks in options using sn_coap_parser_options_parse_multiple_options() have been implemented to avoid overwriting of pointers pointing to previously allocated memory.

Diff: diff --git a/source/sn_coap_parser.c b/source/sn_coap_parser.c
index d222de4..a2577cd 100644
--- a/source/sn_coap_parser.c
+++ b/source/sn_coap_parser.c
@@ -258,10 +258,33 @@ static uint32_t sn_coap_parser_options_parse_uint(uint8_t **packet_data_pptr, ui
         value |= *(*packet_data_pptr)++;
     }
     return value;
 }
 
+/**
+ * \brief Add u16 integers with overflow detection
+ *
+ * \param a            first term of addition
+ * \param b            second term of addion
+ * \param result       pointer to the result variable
+ *
+ * \return Return 0 if there was no overflow, -1 otherwise
+ */
+static int8_t sn_coap_parser_add_u16_limit(uint16_t a, uint16_t b, uint16_t *result)
+{
+    uint16_t c;
+
+    c = a + b;
+    if (c < a || c < b)
+    {
+        return -1;
+    }
+
+    *result = c;
+
+    return 0;
+}
 
 /**
  * \brief Performs data packet pointer boundary check
  *
  * \param packet_data_ptr           current data packet read pointer
@@ -395,15 +418,19 @@ static int8_t parse_ext_option(uint16_t *dst, uint8_t **packet_data_pptr, uint8_
             /* packet_data_pptr would overflow! */
             tr_error("sn_coap_parser_options_parse - **packet_data_pptr overflow !");
             return -1;
         }
         else {
-                option_number += option_ext;
-                *message_left = sn_coap_parser_move_packet_ptr(packet_data_pptr,
-                                                               packet_data_start_ptr,
-                                                               packet_len,
-                                                               1);
+            if(sn_coap_parser_add_u16_limit(option_number, option_ext, &option_number) != 0)
+            {
+                return -1;
+            }
+
+            *message_left = sn_coap_parser_move_packet_ptr(packet_data_pptr,
+                                                            packet_data_start_ptr,
+                                                            packet_len,
+                                                            1);
         }
     } else if (option_number == 14) {
             int8_t read_result = sn_coap_parser_read_packet_u16(&option_number,
                                                                 *packet_data_pptr,
                                                                 packet_data_start_ptr,
@@ -412,15 +439,19 @@ static int8_t parse_ext_option(uint16_t *dst, uint8_t **packet_data_pptr, uint8_
                 /* packet_data_pptr would overflow! */
                 tr_error("sn_coap_parser_options_parse - **packet_data_pptr overflow !");
                 return -1;
             }
             else {
-            option_number += 269;
-            *message_left = sn_coap_parser_move_packet_ptr(packet_data_pptr,
-                                                           packet_data_start_ptr,
-                                                           packet_len,
-                                                           2);
+                if(sn_coap_parser_add_u16_limit(option_number, 269, &option_number) != 0)
+                {
+                    return -1;
+                }
+
+                *message_left = sn_coap_parser_move_packet_ptr(packet_data_pptr,
+                                                            packet_data_start_ptr,
+                                                            packet_len,
+                                                            2);
             }
     }
     /* Option number 15 reserved for payload marker. This is handled as a error! */
     else if (option_number == 15) {
         tr_error("sn_coap_parser_options_parse - invalid option number(15)!");
@@ -497,11 +528,14 @@ static int8_t sn_coap_parser_options_parse(struct coap_s *handle, uint8_t **pack
                                                 &message_left);
         if (option_parse_result != 0) {
             return -1;
         }
         /* Add previous option to option delta and get option number */
-        option_number += previous_option_number;
+        if(sn_coap_parser_add_u16_limit(option_number, previous_option_number, &option_number) != 0)
+        {
+            return -1;
+        }
 
         /* Add possible option length extension to resolve full length of the option */
         option_parse_result = parse_ext_option(&option_len,
                                                 packet_data_pptr,
                                                 packet_data_start_ptr,
@@ -575,10 +609,15 @@ static int8_t sn_coap_parser_options_parse(struct coap_s *handle, uint8_t **pack
                 }
                 message_left = sn_coap_parser_move_packet_ptr(packet_data_pptr, packet_data_start_ptr, packet_len, option_len);
                 break;
 
             case COAP_OPTION_ETAG:
+                if (dst_coap_msg_ptr->options_list_ptr->etag_ptr)
+                {
+                    tr_error("sn_coap_parser_options_parse - COAP_OPTION_ETAG exists!");
+                    return -1;
+                }
                 /* This is managed independently because User gives this option in one character table */
                 ret_status = sn_coap_parser_options_parse_multiple_options(handle, packet_data_pptr,
                              message_left,
                              &dst_coap_msg_ptr->options_list_ptr->etag_ptr,
                              (uint16_t *)&dst_coap_msg_ptr->options_list_ptr->etag_len,
@@ -626,10 +665,15 @@ static int8_t sn_coap_parser_options_parse(struct coap_s *handle, uint8_t **pack
                 }
                 dst_coap_msg_ptr->options_list_ptr->uri_port = sn_coap_parser_options_parse_uint(packet_data_pptr, option_len);
                 break;
 
             case COAP_OPTION_LOCATION_QUERY:
+                if (dst_coap_msg_ptr->options_list_ptr->location_query_ptr)
+                {
+                    tr_error("sn_coap_parser_options_parse - COAP_OPTION_LOCATION_QUERY exists!");
+                    return -1;
+                }
                 ret_status = sn_coap_parser_options_parse_multiple_options(handle, packet_data_pptr, message_left,
                              &dst_coap_msg_ptr->options_list_ptr->location_query_ptr, &dst_coap_msg_ptr->options_list_ptr->location_query_len,
                              COAP_OPTION_LOCATION_QUERY, option_len);
                 if (ret_status < 0) {
                     tr_error("sn_coap_parser_options_parse - COAP_OPTION_LOCATION_QUERY not valid!");
@@ -637,10 +681,15 @@ static int8_t sn_coap_parser_options_parse(struct coap_s *handle, uint8_t **pack
                 }
 
                 break;
 
             case COAP_OPTION_URI_PATH:
+                if (dst_coap_msg_ptr->uri_path_ptr)
+                {
+                    tr_error("sn_coap_parser_options_parse - COAP_OPTION_URI_PATH exists!");
+                    return -1;
+                }
                 ret_status = sn_coap_parser_options_parse_multiple_options(handle, packet_data_pptr, message_left,
                              &dst_coap_msg_ptr->uri_path_ptr, &dst_coap_msg_ptr->uri_path_len,
                              COAP_OPTION_URI_PATH, option_len);
                 if (ret_status < 0) {
                     tr_error("sn_coap_parser_options_parse - COAP_OPTION_URI_PATH not valid!");

Vulnerable code:                 option_number += option_ext;
                *message_left = sn_coap_parser_move_packet_ptr(packet_data_pptr,
                                                               packet_data_start_ptr,
                                                               packet_len,
                                                               1);
            option_number += 269;
            *message_left = sn_coap_parser_move_packet_ptr(packet_data_pptr,
                                                           packet_data_start_ptr,
                                                           packet_len,
                                                           2);
        option_number += previous_option_number;
Fixed code: /**
 * \brief Add u16 integers with overflow detection
 *
 * \param a            first term of addition
 * \param b            second term of addion
 * \param result       pointer to the result variable
 *
 * \return Return 0 if there was no overflow, -1 otherwise
 */
static int8_t sn_coap_parser_add_u16_limit(uint16_t a, uint16_t b, uint16_t *result)
{
    uint16_t c;

    c = a + b;
    if (c < a || c < b)
    {
        return -1;
    }

    *result = c;

    return 0;
}
            if(sn_coap_parser_add_u16_limit(option_number, option_ext, &option_number) != 0)
            {
                return -1;
            }

            *message_left = sn_coap_parser_move_packet_ptr(packet_data_pptr,
                                                            packet_data_start_ptr,
                                                            packet_len,
                                                            1);
                if(sn_coap_parser_add_u16_limit(option_number, 269, &option_number) != 0)
                {
                    return -1;
                }

                *message_left = sn_coap_parser_move_packet_ptr(packet_data_pptr,
                                                            packet_data_start_ptr,
                                                            packet_len,
                                                            2);
        if(sn_coap_parser_add_u16_limit(option_number, previous_option_number, &option_number) != 0)
        {
            return -1;
        }
                if (dst_coap_msg_ptr->options_list_ptr->etag_ptr)
                {
                    tr_error("sn_coap_parser_options_parse - COAP_OPTION_ETAG exists!");
                    return -1;
                }
                if (dst_coap_msg_ptr->options_list_ptr->location_query_ptr)
                {
                    tr_error("sn_coap_parser_options_parse - COAP_OPTION_LOCATION_QUERY exists!");
                    return -1;
                }
                if (dst_coap_msg_ptr->uri_path_ptr)
                {
                    tr_error("sn_coap_parser_options_parse - COAP_OPTION_URI_PATH exists!");
                    return -1;
                }