Skip to content

Commit 5afef51

Browse files
yxiecaqiluo-msft
authored andcommitted
[dhcp6relay] a couple memory access protections (sonic-net#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.
1 parent 0b9077d commit 5afef51

File tree

2 files changed

+8
-8
lines changed

2 files changed

+8
-8
lines changed

src/dhcp6relay/src/configInterface.cpp

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -119,6 +119,7 @@ void processRelayNotification(std::deque<swss::KeyOpFieldsValuesTuple> &entries,
119119
relay_config intf;
120120
intf.is_option_79 = true;
121121
intf.interface = vlan;
122+
intf.db = nullptr;
122123
for (auto &fieldValue: fieldValues) {
123124
std::string f = fvField(fieldValue);
124125
std::string v = fvValue(fieldValue);

src/dhcp6relay/src/relay.cpp

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -226,9 +226,11 @@ void send_udp(int sock, uint8_t *buffer, struct sockaddr_in6 target, uint32_t n,
226226
std::string counterVlan = counter_table;
227227
if(sendto(sock, buffer, n, 0, (const struct sockaddr *)&target, sizeof(target)) == -1)
228228
syslog(LOG_ERR, "sendto: Failed to send to target address\n");
229-
else {
229+
else if (counterMap.find(msg_type) != counterMap.end()) {
230230
counters[msg_type]++;
231231
update_counter(config->db, counterVlan.append(config->interface), msg_type);
232+
} else {
233+
syslog(LOG_WARNING, "unexpected message type %d(0x%x)\n", msg_type, msg_type);
232234
}
233235
}
234236

@@ -478,10 +480,9 @@ void relay_client(int sock, const uint8_t *msg, int32_t len, const ip6_hdr *ip_h
478480
*
479481
* @return none
480482
*/
481-
void relay_relay_reply(int sock, const uint8_t *msg, int32_t len, relay_config *configs) {
483+
void relay_relay_reply(int sock, const uint8_t *msg, int32_t len, relay_config *config) {
482484
static uint8_t buffer[4096];
483485
uint8_t type = 0;
484-
char ifname[configs->interface.size()];
485486
struct sockaddr_in6 target_addr;
486487
auto current_buffer_position = buffer;
487488
auto current_position = msg;
@@ -506,14 +507,13 @@ void relay_client(int sock, const uint8_t *msg, int32_t len, const ip6_hdr *ip_h
506507
}
507508
}
508509

509-
strcpy(ifname, configs->interface.c_str());
510510
memcpy(&target_addr.sin6_addr, &dhcp_relay_header->peer_address, sizeof(struct in6_addr));
511511
target_addr.sin6_family = AF_INET6;
512512
target_addr.sin6_flowinfo = 0;
513513
target_addr.sin6_port = htons(CLIENT_PORT);
514-
target_addr.sin6_scope_id = if_nametoindex(ifname);
514+
target_addr.sin6_scope_id = if_nametoindex(config->interface.c_str());
515515

516-
send_udp(sock, buffer, target_addr, current_buffer_position - buffer, configs, type);
516+
send_udp(sock, buffer, target_addr, current_buffer_position - buffer, config, type);
517517
}
518518

519519

@@ -707,8 +707,7 @@ void loop_relay(std::vector<relay_config> *vlans, swss::DBConnector *db) {
707707
int filter = 0;
708708
int local_sock = 0;
709709
int server_sock = 0;
710-
const char *ifname = config->interface.c_str();
711-
int index = if_nametoindex(ifname);
710+
int index = if_nametoindex(config->interface.c_str());
712711
config->db = db;
713712

714713
std::string counterVlan = counter_table;

0 commit comments

Comments
 (0)