Skip to content

Commit

Permalink
[dhcp6relay] a couple memory access protections (#9851)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
yxieca authored and judyjoseph committed Feb 22, 2022
1 parent 4df0aa3 commit 53a8d99
Show file tree
Hide file tree
Showing 2 changed files with 8 additions and 8 deletions.
1 change: 1 addition & 0 deletions src/dhcp6relay/src/configInterface.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
15 changes: 7 additions & 8 deletions src/dhcp6relay/src/relay.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}

Expand Down Expand Up @@ -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;
Expand All @@ -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);
}


Expand Down Expand Up @@ -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;
Expand Down

0 comments on commit 53a8d99

Please sign in to comment.