Fix robustness issues, harden bounds checks, and improve portability

- Fix TLV loop off-by-one: use >= so zero-length TLVs at buffer tail are processed
- Fix uint16_t overflows in pp_info_add_aws_vpce_id, pp_info_add_netns, tlv_array_append_tlv_new_usascii
- Fix SSL sub-TLV bounds: require full header before read, validate sub-TLV length, reject trailing bytes
- Fix uint16_t overflow in pp2_tlv_offset: use uint32_t to prevent infinite loop on large TLV lengths
- Fix SSL TLV minimum length check: validate pp2_tlv_len >= 5 before accessing fields
- Fix NOOP padding heap overflow: add padding_bytes > 0 guard
- Fix v1 parser: change || to && for UNKNOWN header, use PP1_MAX_LENGTH - 1 to preserve null terminator
- Add NULL checks in pp_info_add_netns and pp_info_add_aws_vpce_id
- Eliminate switch fallthrough and remove -Wimplicit-fallthrough=0 from Makefile for portability
- Refactor Azure parser to use tlv_array_append_tlv_new() wrapper
- Add explicit casts for pointer subtraction and strlen results to suppress MSVC 64-bit warnings
- Add 5 unit tests for TLV overflow, SSL too short, SSL trailing bytes, v1 invalid family, v1 no CRLF

Author: kosmas-valianos

Makefile

@@ -16,7 +16,7 @@
 # along with this program.  If not, see <https://www.gnu.org/licenses/>.
 #
 
-CFLAGS := -Wall -Wextra -Wshadow -Wimplicit-fallthrough=0 -ansi -fshort-enums -fpic
+CFLAGS := -Wall -Wextra -Wshadow -ansi -fshort-enums -fpic
 
 all: build tests example
 

src/proxy_protocol.c

@@ -286,7 +286,12 @@ static uint8_t tlv_array_append_tlv_new(tlv_array_t *tlv_array, uint8_t type, ui
 
 static uint8_t tlv_array_append_tlv_new_usascii(tlv_array_t *tlv_array, uint8_t type, uint16_t length, const void *value)
 {
-    pp2_tlv_t *tlv = tlv_new(type, length + 1, value);
+    pp2_tlv_t *tlv;
+    if (length == UINT16_MAX)
+    {
+        return 0;
+    }
+    tlv = tlv_new(type, length + 1, value);
     if (!tlv)
     {
         return 0;
@@ -409,20 +414,42 @@ uint8_t pp_info_add_ssl(pp_info_t *pp_info, const char *version, const char *cip
 
 uint8_t pp_info_add_netns(pp_info_t *pp_info, const char *netns)
 {
-    return tlv_array_append_tlv_new(&pp_info->pp2_info.tlv_array, PP2_TYPE_NETNS, (uint16_t) strlen(netns), netns);
+    size_t netns_len;
+    if (!netns)
+    {
+        return 0;
+    }
+    netns_len = strlen(netns);
+    if (netns_len > UINT16_MAX)
+    {
+        return 0;
+    }
+    return tlv_array_append_tlv_new(&pp_info->pp2_info.tlv_array, PP2_TYPE_NETNS, (uint16_t) netns_len, netns);
 }
 
 uint8_t pp_info_add_aws_vpce_id(pp_info_t *pp_info, const char *vpce_id)
 {
-    uint16_t length = sizeof_pp2_tlv_aws_t + (uint16_t) strlen(vpce_id);
-    pp2_tlv_aws_t *pp2_tlv_aws = malloc(length);
+    size_t vpce_id_len;
+    uint16_t length;
+    pp2_tlv_aws_t *pp2_tlv_aws;
     uint8_t rc;
+    if (!vpce_id)
+    {
+        return 0;
+    }
+    vpce_id_len = strlen(vpce_id);
+    if (vpce_id_len > UINT16_MAX - sizeof_pp2_tlv_aws_t)
+    {
+        return 0;
+    }
+    length = (uint16_t)(sizeof_pp2_tlv_aws_t + vpce_id_len);
+    pp2_tlv_aws = malloc(length);
     if (!pp2_tlv_aws)
     {
         return 0;
     }
     pp2_tlv_aws->type = PP2_SUBTYPE_AWS_VPCE_ID;
-    memcpy(pp2_tlv_aws->value, vpce_id, strlen(vpce_id));
+    memcpy(pp2_tlv_aws->value, vpce_id, vpce_id_len);
     rc = tlv_array_append_tlv_new(&pp_info->pp2_info.tlv_array, PP2_TYPE_AWS, length, pp2_tlv_aws);
     free(pp2_tlv_aws);
     return rc;
@@ -795,7 +822,7 @@ static uint8_t *pp2_create_hdr(const pp_info_t *pp_info, uint16_t *pp2_hdr_len,
         memcpy(pp2_hdr + index, tlv_array->tlvs[i], tlv_len);
         index += tlv_len;
     }
-    if (pp_info->pp2_info.alignment_power > 1)
+    if (pp_info->pp2_info.alignment_power > 1 && padding_bytes > 0)
     {
         pp2_tlv_t tlv = { 0 };
         tlv.type = PP2_TYPE_NOOP;
@@ -1055,12 +1082,12 @@ static int32_t pp2_parse_hdr(const uint8_t *buffer, uint32_t buffer_length, pp_i
     }
 
     /* TLVs */
-    /* Any TLV vector must be at least 3 bytes */
-    while (tlv_vectors_len > sizeof_pp2_tlv_t)
+    /* Any TLV vector must be at least 3 bytes (sizeof_pp2_tlv_t) */
+    while (tlv_vectors_len >= sizeof_pp2_tlv_t)
     {
         const pp2_tlv_t *pp2_tlv = (const pp2_tlv_t*) buffer;
         uint16_t pp2_tlv_len = pp2_tlv->length_hi << 8 | pp2_tlv->length_lo;
-        uint16_t pp2_tlv_offset = sizeof_pp2_tlv_t + pp2_tlv_len;
+        uint32_t pp2_tlv_offset = (uint32_t) sizeof_pp2_tlv_t + pp2_tlv_len;
         if (pp2_tlv_offset > tlv_vectors_len)
         {
             return -ERR_PP2_TLV_LENGTH;
@@ -1094,7 +1121,7 @@ static int32_t pp2_parse_hdr(const uint8_t *buffer, uint32_t buffer_length, pp_i
              * Instead of zeroing the field in the buffer, compute CRC in 3 segments:
              * before the checksum value, 4 zero bytes, after the checksum value. */
             total_hdr_len = sizeof(proxy_hdr_v2_t) + len;
-            offset_to_chksum_value = (const uint8_t*)pp2_tlv->value - pp2_hdr;
+            offset_to_chksum_value = (uint32_t)((const uint8_t*)pp2_tlv->value - pp2_hdr);
             after_chksum_offset = offset_to_chksum_value + sizeof(uint32_t);
 
             crc = crc32c_continue(0xffffffff, pp2_hdr, offset_to_chksum_value);
@@ -1129,25 +1156,39 @@ static int32_t pp2_parse_hdr(const uint8_t *buffer, uint32_t buffer_length, pp_i
             break;
         case PP2_TYPE_SSL:
         {
-            const pp2_tlv_ssl_t *pp2_tlv_ssl = (const pp2_tlv_ssl_t*) pp2_tlv->value;
+            const pp2_tlv_ssl_t *pp2_tlv_ssl;
             uint16_t pp2_tlvs_ssl_len = 0, pp2_sub_tlv_offset = 0;
             uint8_t tlv_ssl_version_found = 0;
 
+            if (pp2_tlv_len < sizeof(((pp2_tlv_ssl_t*)0)->client) + sizeof(((pp2_tlv_ssl_t*)0)->verify))
+            {
+                return -ERR_PP2_TYPE_SSL;
+            }
+
+            pp2_tlv_ssl = (const pp2_tlv_ssl_t*) pp2_tlv->value;
+
             /* Set the pp2_ssl_info */
             pp_info->pp2_info.pp2_ssl_info.ssl = !!(pp2_tlv_ssl->client & PP2_CLIENT_SSL);
             pp_info->pp2_info.pp2_ssl_info.cert_in_connection = !!(pp2_tlv_ssl->client & PP2_CLIENT_CERT_CONN);
             pp_info->pp2_info.pp2_ssl_info.cert_in_session = !!(pp2_tlv_ssl->client & PP2_CLIENT_CERT_SESS);
             pp_info->pp2_info.pp2_ssl_info.cert_verified = !pp2_tlv_ssl->verify;
 
             pp2_tlvs_ssl_len = pp2_tlv_len - sizeof(pp2_tlv_ssl->client) - sizeof(pp2_tlv_ssl->verify);
-            while (pp2_sub_tlv_offset < pp2_tlvs_ssl_len)
+            while (pp2_sub_tlv_offset + sizeof_pp2_tlv_t <= pp2_tlvs_ssl_len)
             {
                 const pp2_tlv_t *pp2_sub_tlv_ssl = (const pp2_tlv_t*) ((const uint8_t*) pp2_tlv_ssl->sub_tlv + pp2_sub_tlv_offset);
                 uint16_t pp2_sub_tlv_ssl_len = pp2_sub_tlv_ssl->length_hi << 8 | pp2_sub_tlv_ssl->length_lo;
+                if ((uint32_t) sizeof_pp2_tlv_t + pp2_sub_tlv_ssl_len > (uint32_t)(pp2_tlvs_ssl_len - pp2_sub_tlv_offset))
+                {
+                    return -ERR_PP2_TYPE_SSL;
+                }
+                if (pp2_sub_tlv_ssl->type == PP2_SUBTYPE_SSL_VERSION)
+                {
+                    tlv_ssl_version_found = 1;
+                }
                 switch (pp2_sub_tlv_ssl->type)
                 {
                 case PP2_SUBTYPE_SSL_VERSION: /* US-ASCII */
-                    tlv_ssl_version_found = 1;
                 case PP2_SUBTYPE_SSL_CIPHER:  /* US-ASCII */
                 case PP2_SUBTYPE_SSL_SIG_ALG: /* US-ASCII */
                 case PP2_SUBTYPE_SSL_KEY_ALG: /* US-ASCII */
@@ -1171,7 +1212,7 @@ static int32_t pp2_parse_hdr(const uint8_t *buffer, uint32_t buffer_length, pp_i
 
                 pp2_sub_tlv_offset += sizeof_pp2_tlv_t + pp2_sub_tlv_ssl_len;
             }
-            if (pp2_sub_tlv_offset > pp2_tlvs_ssl_len || (pp_info->pp2_info.pp2_ssl_info.ssl && !tlv_ssl_version_found))
+            if (pp2_sub_tlv_offset != pp2_tlvs_ssl_len || (pp_info->pp2_info.pp2_ssl_info.ssl && !tlv_ssl_version_found))
             {
                 return -ERR_PP2_TYPE_SSL;
             }
@@ -1247,15 +1288,15 @@ static int32_t pp1_parse_hdr(const uint8_t *buffer, uint32_t buffer_length, pp_i
     char src_port_str[6] = { 0 };
     char dst_port_str[6] = { 0 };
 
-    memcpy(block, buffer, buffer_length < PP1_MAX_LENGTH ? buffer_length : PP1_MAX_LENGTH);
+    memcpy(block, buffer, buffer_length < PP1_MAX_LENGTH - 1 ? buffer_length : PP1_MAX_LENGTH - 1);
 
     block_end = strstr(block, CRLF);
     if (!block_end)
     {
         return -ERR_PP1_CRLF;
     }
     block_end += strlen(CRLF);
-    pp1_hdr_len = block_end - block;
+    pp1_hdr_len = (int32_t)(block_end - block);
 
     /* PROXY */
     if (memcmp(block, "PROXY", 5))
@@ -1276,7 +1317,7 @@ static int32_t pp1_parse_hdr(const uint8_t *buffer, uint32_t buffer_length, pp_i
     if (!inet_family)
     {
         /* Unknown connection (short form) */
-        if (pp1_hdr_len == 15 || !memcmp(ptr, "UNKNOWN", 7))
+        if (pp1_hdr_len == 15 && !memcmp(ptr, "UNKNOWN", 7))
         {
             pp_info->address_family = ADDR_FAMILY_UNSPEC;
             pp_info->transport_protocol = TRANSPORT_PROTOCOL_UNSPEC;
@@ -1323,7 +1364,7 @@ static int32_t pp1_parse_hdr(const uint8_t *buffer, uint32_t buffer_length, pp_i
     {
         return sa_family == AF_INET ? -ERR_PP1_IPV4_SRC_IP : -ERR_PP1_IPV6_SRC_IP;
     }
-    src_address_length = src_address_end - ptr;
+    src_address_length = (uint16_t)(src_address_end - ptr);
     memcpy(pp_info->src_addr, ptr, src_address_length);
     if (inet_pton(sa_family, pp_info->src_addr, &src_sin_addr) != 1)
     {
@@ -1344,7 +1385,7 @@ static int32_t pp1_parse_hdr(const uint8_t *buffer, uint32_t buffer_length, pp_i
     {
         return sa_family == AF_INET ? -ERR_PP1_IPV4_DST_IP : -ERR_PP1_IPV6_DST_IP;
     }
-    dst_address_length = dst_address_end - ptr;
+    dst_address_length = (uint16_t)(dst_address_end - ptr);
     memcpy(pp_info->dst_addr, ptr, dst_address_length);
     if (inet_pton(sa_family, pp_info->dst_addr, &dst_sin_addr) != 1)
     {
@@ -1365,7 +1406,7 @@ static int32_t pp1_parse_hdr(const uint8_t *buffer, uint32_t buffer_length, pp_i
     {
         return -ERR_PP1_SRC_PORT;
     }
-    src_port_length = src_port_end - ptr;
+    src_port_length = (uint16_t)(src_port_end - ptr);
     if (src_port_length == 0 || src_port_length > 5)
     {
         return -ERR_PP1_SRC_PORT;
@@ -1390,7 +1431,7 @@ static int32_t pp1_parse_hdr(const uint8_t *buffer, uint32_t buffer_length, pp_i
     {
         return -ERR_PP1_DST_PORT;
     }
-    dst_port_length = dst_port_end - ptr;
+    dst_port_length = (uint16_t)(dst_port_end - ptr);
     if (dst_port_length == 0 || dst_port_length > 5)
     {
         return -ERR_PP1_DST_PORT;

tests/test.c

@@ -173,6 +173,48 @@ uint8_t pp2_hdr_ssl[] = {
 
 static const uint32_t test_azure_linkid = 5678;
 
+/* v2 header with TLV length 0xFFFD (65533) causing uint16_t overflow in TLV offset calculation */
+uint8_t pp2_hdr_tlv_overflow[] = {
+            0x0d, 0x0a, 0x0d, 0x0a, /* Start of v2 signature */
+            0x00, 0x0d, 0x0a, 0x51,
+            0x55, 0x49, 0x54, 0x0a, /* End of v2 signature */
+            0x21, 0x11, 0x00, 0x10, /* ver_cmd, fam and len (16) */
+            0xc0, 0xa8, 0x0a, 0x64, /* Source IP */
+            0xc0, 0xa8, 0x0b, 0x5a, /* Destination IP */
+            0xa5, 0x5c, 0x1f, 0x90, /* Source port, Destination port */
+            0x01, 0xff, 0xfd, 0x00, /* PP2_TYPE_ALPN TLV with length 0xFFFD, 1 byte value */
+};
+
+/* v2 header with PP2_TYPE_SSL TLV too short (length 2, needs at least 5) */
+uint8_t pp2_hdr_ssl_too_short[] = {
+            0x0d, 0x0a, 0x0d, 0x0a, /* Start of v2 signature */
+            0x00, 0x0d, 0x0a, 0x51,
+            0x55, 0x49, 0x54, 0x0a, /* End of v2 signature */
+            0x21, 0x11, 0x00, 0x11, /* ver_cmd, fam and len (17) */
+            0xc0, 0xa8, 0x0a, 0x64, /* Source IP */
+            0xc0, 0xa8, 0x0b, 0x5a, /* Destination IP */
+            0xa5, 0x5c, 0x1f, 0x90, /* Source port, Destination port */
+            0x20, 0x00, 0x02,       /* PP2_TYPE_SSL TLV with length 2 */
+            0x00, 0x00,             /* Truncated SSL data (too short) */
+};
+
+/* v2 header with PP2_TYPE_SSL sub-TLV trailing bytes (1 valid sub-TLV + 2 orphan bytes) */
+uint8_t pp2_hdr_ssl_trailing_bytes[] = {
+            0x0d, 0x0a, 0x0d, 0x0a, /* Start of v2 signature */
+            0x00, 0x0d, 0x0a, 0x51,
+            0x55, 0x49, 0x54, 0x0a, /* End of v2 signature */
+            0x21, 0x11, 0x00, 0x1c, /* ver_cmd, fam and len (28) */
+            0xc0, 0xa8, 0x0a, 0x64, /* Source IP */
+            0xc0, 0xa8, 0x0b, 0x5a, /* Destination IP */
+            0xa5, 0x5c, 0x1f, 0x90, /* Source port, Destination port */
+            0x20, 0x00, 0x0d,       /* PP2_TYPE_SSL TLV with length 13 */
+            0x01,                   /* client (PP2_CLIENT_SSL set) */
+            0x00, 0x00, 0x00, 0x00, /* verify (0 = verified) */
+            0x21, 0x00, 0x03,       /* PP2_SUBTYPE_SSL_VERSION, length 3 */
+            0x31, 0x2e, 0x32,       /* "1.2" */
+            0xff, 0xff,             /* 2 trailing orphan bytes (not a valid sub-TLV header) */
+};
+
 static uint8_t pp_add_tlvs(pp_info_t *pp_info, const test_tlv_t (*add_tlvs)[10])
 {
     uint8_t i;
@@ -391,14 +433,14 @@ int main(void)
         {
             .name = "v1 PROXY protocol header: UNKNOWN - short",
             .raw_bytes_in = (const uint8_t*) "PROXY UNKNOWN\r\n",
-            .raw_bytes_in_length = strlen((const char*) tests[0].raw_bytes_in),
-            .rc_expected = strlen((const char*) tests[0].raw_bytes_in),
+            .raw_bytes_in_length = (uint32_t) strlen((const char*) tests[0].raw_bytes_in),
+            .rc_expected = (int32_t) strlen((const char*) tests[0].raw_bytes_in),
         },
         {
             .name = "v1 PROXY protocol header: UNKNOWN - full",
             .raw_bytes_in = (const uint8_t*) "PROXY UNKNOWN ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff 65535 65535\r\n",
-            .raw_bytes_in_length = strlen((const char*) tests[1].raw_bytes_in),
-            .rc_expected = strlen((const char*) tests[1].raw_bytes_in),
+            .raw_bytes_in_length = (uint32_t) strlen((const char*) tests[1].raw_bytes_in),
+            .rc_expected = (int32_t) strlen((const char*) tests[1].raw_bytes_in),
         },
         {
             .name = "v2 PROXY protocol header: PROXY, TCP over IPv4. TLVs: PP2_TYPE_CRC32C, PP2_TYPE_AWS(PP2_SUBTYPE_AWS_VPCE_ID)",
@@ -869,6 +911,68 @@ int main(void)
             },
             .error_expected = -ERR_PP2_IPV6_DST_IP,
         },
+        {
+            .name = "v2 PROXY protocol header: -ERR_PP2_TLV_LENGTH (TLV length overflow)",
+            .raw_bytes_in = pp2_hdr_tlv_overflow,
+            .raw_bytes_in_length = sizeof(pp2_hdr_tlv_overflow),
+            .rc_expected = -ERR_PP2_TLV_LENGTH,
+            .pp_info_out_expected = {
+                .address_family = ADDR_FAMILY_INET,
+                .transport_protocol = TRANSPORT_PROTOCOL_STREAM,
+                .src_addr = "192.168.10.100",
+                .dst_addr = "192.168.11.90",
+                .src_port = 42332,
+                .dst_port = 8080,
+            },
+        },
+        {
+            .name = "v2 PROXY protocol header: -ERR_PP2_TYPE_SSL (SSL TLV too short)",
+            .raw_bytes_in = pp2_hdr_ssl_too_short,
+            .raw_bytes_in_length = sizeof(pp2_hdr_ssl_too_short),
+            .rc_expected = -ERR_PP2_TYPE_SSL,
+            .pp_info_out_expected = {
+                .address_family = ADDR_FAMILY_INET,
+                .transport_protocol = TRANSPORT_PROTOCOL_STREAM,
+                .src_addr = "192.168.10.100",
+                .dst_addr = "192.168.11.90",
+                .src_port = 42332,
+                .dst_port = 8080,
+            },
+        },
+        {
+            .name = "v2 PROXY protocol header: -ERR_PP2_TYPE_SSL (SSL sub-TLV trailing bytes)",
+            .raw_bytes_in = pp2_hdr_ssl_trailing_bytes,
+            .raw_bytes_in_length = sizeof(pp2_hdr_ssl_trailing_bytes),
+            .rc_expected = -ERR_PP2_TYPE_SSL,
+            .pp_info_out_expected = {
+                .address_family = ADDR_FAMILY_INET,
+                .transport_protocol = TRANSPORT_PROTOCOL_STREAM,
+                .src_addr = "192.168.10.100",
+                .dst_addr = "192.168.11.90",
+                .src_port = 42332,
+                .dst_port = 8080,
+                .pp2_info = {
+                    .pp2_ssl_info = {
+                        .ssl = 1,
+                        .cert_verified = 1,
+                    },
+                },
+            },
+        },
+        {
+            .name = "v1 PROXY protocol header: -ERR_PP1_TRANSPORT_FAMILY (invalid family, 15-byte header)",
+            .raw_bytes_in = (const uint8_t*) "PROXY XXXXXXX\r\n",
+            .raw_bytes_in_length = 15,
+            .rc_expected = -ERR_PP1_TRANSPORT_FAMILY,
+        },
+        {
+            .name = "v1 PROXY protocol header: -ERR_PP1_CRLF (buffer >= 108 bytes, no CRLF)",
+            .raw_bytes_in = (const uint8_t*)
+                            "PROXY TCP4 255.255.255.255 255.255.255.255 65535 65535 "  /* 55 */
+                            "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA",    /* 53, total 108 */
+            .raw_bytes_in_length = 108,
+            .rc_expected = -ERR_PP1_CRLF,
+        },
     };
 
     /* Run tests */