From 53a8d99baaed2e7059df29b115fce47961e9c256 Mon Sep 17 00:00:00 2001
From: Ying Xie <yxieca@users.noreply.github.com>
Date: Wed, 26 Jan 2022 09:58:52 -0800
Subject: [PATCH] [dhcp6relay] a couple memory access protections (#9851)

Why I did it
the strcpy and buffer allocation is not safe, it corrupts 1 byte on the stack. Depending on the memory layout, it may or may not cause issue immediately.
message type is not validated before updating the counter. Which could cause segment fault.

How I did it
Remove the unsafe strcpy, use config->interface.c_str() instead.
Check message type before updating counters.

How to verify it
The issue (1) caused segment fault on a specific platform. The fix was validated there. Issue (2) was precautionary. Added log in case it triggers.
---
 src/dhcp6relay/src/configInterface.cpp |  1 +
 src/dhcp6relay/src/relay.cpp           | 15 +++++++--------
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/src/dhcp6relay/src/configInterface.cpp b/src/dhcp6relay/src/configInterface.cpp
index 0af4f49a28..ca78d80e63 100644
--- a/src/dhcp6relay/src/configInterface.cpp
+++ b/src/dhcp6relay/src/configInterface.cpp
@@ -119,6 +119,7 @@ void processRelayNotification(std::deque<swss::KeyOpFieldsValuesTuple> &entries,
         relay_config intf;
         intf.is_option_79 = true;
         intf.interface = vlan;
+        intf.db = nullptr;
         for (auto &fieldValue: fieldValues) {
             std::string f = fvField(fieldValue);
             std::string v = fvValue(fieldValue);
diff --git a/src/dhcp6relay/src/relay.cpp b/src/dhcp6relay/src/relay.cpp
index 83ec69c956..2ebdd99d48 100644
--- a/src/dhcp6relay/src/relay.cpp
+++ b/src/dhcp6relay/src/relay.cpp
@@ -226,9 +226,11 @@ void send_udp(int sock, uint8_t *buffer, struct sockaddr_in6 target, uint32_t n,
     std::string counterVlan = counter_table;
     if(sendto(sock, buffer, n, 0, (const struct sockaddr *)&target, sizeof(target)) == -1)
         syslog(LOG_ERR, "sendto: Failed to send to target address\n");
-    else {
+    else if (counterMap.find(msg_type) != counterMap.end()) {
         counters[msg_type]++;
         update_counter(config->db, counterVlan.append(config->interface), msg_type);
+    } else {
+        syslog(LOG_WARNING, "unexpected message type %d(0x%x)\n", msg_type, msg_type);
     }
 }
 
@@ -518,10 +520,9 @@ void relay_relay_forw(int sock, const uint8_t *msg, int32_t len, const ip6_hdr *
  *
  * @return              none
  */
- void relay_relay_reply(int sock, const uint8_t *msg, int32_t len, relay_config *configs) {
+ void relay_relay_reply(int sock, const uint8_t *msg, int32_t len, relay_config *config) {
     static uint8_t buffer[4096];
     uint8_t type = 0;
-    char ifname[configs->interface.size()];
     struct sockaddr_in6 target_addr;
     auto current_buffer_position = buffer;
     auto current_position = msg;
@@ -546,14 +547,13 @@ void relay_relay_forw(int sock, const uint8_t *msg, int32_t len, const ip6_hdr *
         }
     }
 
-    strcpy(ifname, configs->interface.c_str());
     memcpy(&target_addr.sin6_addr, &dhcp_relay_header->peer_address, sizeof(struct in6_addr));
     target_addr.sin6_family = AF_INET6;
     target_addr.sin6_flowinfo = 0;
     target_addr.sin6_port = htons(CLIENT_PORT);
-    target_addr.sin6_scope_id = if_nametoindex(ifname);
+    target_addr.sin6_scope_id = if_nametoindex(config->interface.c_str());
 
-    send_udp(sock, buffer, target_addr, current_buffer_position - buffer, configs, type);
+    send_udp(sock, buffer, target_addr, current_buffer_position - buffer, config, type);
 } 
 
 
@@ -758,8 +758,7 @@ void loop_relay(std::vector<relay_config> *vlans, swss::DBConnector *db) {
         int filter = 0;
         int local_sock = 0; 
         int server_sock = 0;
-        const char *ifname = config->interface.c_str();
-        int index = if_nametoindex(ifname);
+        int index = if_nametoindex(config->interface.c_str());
         config->db = db;
 
         std::string counterVlan = counter_table;