From d6cf1b85a917cf9d3e3d8f3333083ba11d6556fc Mon Sep 17 00:00:00 2001 From: Davide Beatrici Date: Sat, 13 Jul 2019 23:29:16 +0200 Subject: [PATCH] Virtual: fix race condition in DHCP server which resulted in multiple clients receiving the same IP A race condition in the DHCP server caused it to offer the same IP address to multiple clients when they connected at the same time, because an offered IP address was considered free until the final step (DHCP_ACK). This commit introduces a list to keep track of the pending leases created during DHCP_OFFER, so that an IP address is guaranteed to be offered to a single client. --- src/Cedar/Virtual.c | 130 +++++++++++++++++++++++++++++++++++++------- src/Cedar/Virtual.h | 3 + 2 files changed, 113 insertions(+), 20 deletions(-) diff --git a/src/Cedar/Virtual.c b/src/Cedar/Virtual.c index 218082c4..1fc2c9c6 100644 --- a/src/Cedar/Virtual.c +++ b/src/Cedar/Virtual.c @@ -8956,8 +8956,8 @@ void FreeDhcpServer(VH *v) return; } - // Remove the all lease entries - for (i = 0;i < LIST_NUM(v->DhcpLeaseList);i++) + // Empty the leases lists + for (i = 0; i < LIST_NUM(v->DhcpLeaseList); ++i) { DHCP_LEASE *d = LIST_DATA(v->DhcpLeaseList, i); FreeDhcpLease(d); @@ -8965,6 +8965,15 @@ void FreeDhcpServer(VH *v) ReleaseList(v->DhcpLeaseList); v->DhcpLeaseList = NULL; + + for (i = 0; i < LIST_NUM(v->DhcpPendingLeaseList); ++i) + { + DHCP_LEASE *d = LIST_DATA(v->DhcpPendingLeaseList, i); + FreeDhcpLease(d); + } + + ReleaseList(v->DhcpPendingLeaseList); + v->DhcpPendingLeaseList = NULL; } // Initialize the DHCP server @@ -8978,6 +8987,29 @@ void InitDhcpServer(VH *v) // Create a list v->DhcpLeaseList = NewList(CompareDhcpLeaseList); + v->DhcpPendingLeaseList = NewList(CompareDhcpLeaseList); +} + +// Search for a pending DHCP lease item by the IP address +DHCP_LEASE *SearchDhcpPendingLeaseByIp(VH *v, UINT ip) +{ + UINT i; + // Validate arguments + if (v == NULL) + { + return NULL; + } + + for (i = 0; i < LIST_NUM(v->DhcpPendingLeaseList); ++i) + { + DHCP_LEASE *d = LIST_DATA(v->DhcpPendingLeaseList, i); + if (d->IpAddress == ip) + { + return d; + } + } + + return NULL; } // Search for a DHCP lease item by the IP address @@ -8990,7 +9022,7 @@ DHCP_LEASE *SearchDhcpLeaseByIp(VH *v, UINT ip) return NULL; } - for (i = 0;i < LIST_NUM(v->DhcpLeaseList);i++) + for (i = 0; i < LIST_NUM(v->DhcpLeaseList); ++i) { DHCP_LEASE *d = LIST_DATA(v->DhcpLeaseList, i); if (d->IpAddress == ip) @@ -9002,6 +9034,22 @@ DHCP_LEASE *SearchDhcpLeaseByIp(VH *v, UINT ip) return NULL; } +// Search for a pending DHCP lease item by the MAC address +DHCP_LEASE *SearchDhcpPendingLeaseByMac(VH *v, UCHAR *mac) +{ + DHCP_LEASE *d, t; + // Validate arguments + if (v == NULL || mac == NULL) + { + return NULL; + } + + Copy(&t.MacAddress, mac, 6); + d = Search(v->DhcpPendingLeaseList, &t); + + return d; +} + // Search for a DHCP lease item by the MAC address DHCP_LEASE *SearchDhcpLeaseByMac(VH *v, UCHAR *mac) { @@ -9099,9 +9147,8 @@ void PollingDhcpServer(VH *v) } v->LastDhcpPolling = v->Now; - // Remove expired entries -FIRST_LIST: - for (i = 0;i < LIST_NUM(v->DhcpLeaseList);i++) +LIST_CLEANUP: + for (i = 0; i < LIST_NUM(v->DhcpLeaseList); ++i) { DHCP_LEASE *d = LIST_DATA(v->DhcpLeaseList, i); @@ -9109,7 +9156,21 @@ FIRST_LIST: { FreeDhcpLease(d); Delete(v->DhcpLeaseList, d); - goto FIRST_LIST; + goto LIST_CLEANUP; + } + } + +PENDING_LIST_CLEANUP: + // Remove expired entries + for (i = 0; i < LIST_NUM(v->DhcpPendingLeaseList); ++i) + { + DHCP_LEASE *d = LIST_DATA(v->DhcpPendingLeaseList, i); + + if (d->ExpireTime < v->Now) + { + FreeDhcpLease(d); + Delete(v->DhcpPendingLeaseList, d); + goto PENDING_LIST_CLEANUP; } } } @@ -9151,6 +9212,11 @@ UINT ServeDhcpDiscover(VH *v, UCHAR *mac, UINT request_ip) { // IP address is specified DHCP_LEASE *d = SearchDhcpLeaseByIp(v, request_ip); + if (d == NULL) + { + d = SearchDhcpPendingLeaseByIp(v, request_ip); + } + if (d != NULL) { // If an entry for the same IP address already exists, @@ -9187,6 +9253,11 @@ UINT ServeDhcpDiscover(VH *v, UCHAR *mac, UINT request_ip) // If there is any entry with the same MAC address // that are already registered, use it with priority DHCP_LEASE *d = SearchDhcpLeaseByMac(v, mac); + if (d == NULL) + { + d = SearchDhcpPendingLeaseByMac(v, mac); + } + if (d != NULL) { // Examine whether the found IP address is in the allocation region @@ -9234,7 +9305,7 @@ UINT GetFreeDhcpIpAddress(VH *v) for (i = ip_start; i <= ip_end;i++) { UINT ip = Endian32(i); - if (SearchDhcpLeaseByIp(v, ip) == NULL) + if (SearchDhcpLeaseByIp(v, ip) == NULL && SearchDhcpPendingLeaseByIp(v, ip) == NULL) { // A free IP address is found return ip; @@ -9284,7 +9355,7 @@ UINT GetFreeDhcpIpAddressByRandom(VH *v, UCHAR *mac) new_ip = Endian32(ip_start + (rand_int % (ip_end - ip_start + 1))); - if (SearchDhcpLeaseByIp(v, new_ip) == NULL) + if (SearchDhcpLeaseByIp(v, new_ip) == NULL && SearchDhcpPendingLeaseByIp(v, new_ip) == NULL) { // A free IP address is found return new_ip; @@ -9401,8 +9472,9 @@ void VirtualDhcpServer(VH *v, PKT *p) if (opt->Opcode == DHCP_REQUEST) { DHCP_LEASE *d; - char mac[MAX_SIZE]; - char str[MAX_SIZE]; + char client_mac[MAX_SIZE]; + char client_ip[MAX_SIZE]; + // Remove old records with the same IP address d = SearchDhcpLeaseByIp(v, ip); if (d != NULL) @@ -9411,17 +9483,22 @@ void VirtualDhcpServer(VH *v, PKT *p) Delete(v->DhcpLeaseList, d); } + d = SearchDhcpPendingLeaseByIp(v, ip); + if (d != NULL) + { + FreeDhcpLease(d); + Delete(v->DhcpPendingLeaseList, d); + } + // Create a new entry - d = NewDhcpLease(v->DhcpExpire, p->MacAddressSrc, - ip, v->DhcpMask, - opt->Hostname); + d = NewDhcpLease(v->DhcpExpire, p->MacAddressSrc, ip, v->DhcpMask, opt->Hostname); d->Id = ++v->DhcpId; Add(v->DhcpLeaseList, d); - MacToStr(mac, sizeof(mac), d->MacAddress); - IPToStr32(str, sizeof(str), d->IpAddress); + MacToStr(client_mac, sizeof(client_mac), d->MacAddress); + IPToStr32(client_ip, sizeof(client_ip), d->IpAddress); - NLog(v, "LH_NAT_DHCP_CREATED", d->Id, mac, str, d->Hostname, v->DhcpExpire / 1000); + NLog(v, "LH_NAT_DHCP_CREATED", d->Id, client_mac, client_ip, d->Hostname, v->DhcpExpire / 1000); } // Respond @@ -9510,12 +9587,25 @@ void VirtualDhcpServer(VH *v, PKT *p) char client_mac[MAX_SIZE]; char client_ip[64]; IP ips; + BinToStr(client_mac, sizeof(client_mac), p->MacAddressSrc, 6); UINTToIP(&ips, ip); IPToStr(client_ip, sizeof(client_ip), &ips); - Debug("DHCP %s : %s given %s\n", - ret.Opcode == DHCP_OFFER ? "DHCP_OFFER" : "DHCP_ACK", - client_mac, client_ip); + + if (ret.Opcode == DHCP_OFFER) + { + // DHCP_OFFER + DHCP_LEASE *d = NewDhcpLease(5000, p->MacAddressSrc, ip, v->DhcpMask, opt->Hostname); + d->Id = LIST_NUM(v->DhcpPendingLeaseList); + Add(v->DhcpPendingLeaseList, d); + + Debug("VirtualDhcpServer(): %s has been marked as pending for %s\n", client_ip, client_mac); + } + else + { + // DHCP_ACK + Debug("VirtualDhcpServer(): %s has been assigned to %s\n", client_ip, client_mac); + } } // Build a DHCP option diff --git a/src/Cedar/Virtual.h b/src/Cedar/Virtual.h index 9b3acc68..80c71463 100644 --- a/src/Cedar/Virtual.h +++ b/src/Cedar/Virtual.h @@ -296,6 +296,7 @@ struct VH UINT DhcpDns2; // DNS server address 2 char DhcpDomain[MAX_HOST_NAME_LEN + 1]; // Assigned domain name LIST *DhcpLeaseList; // DHCP lease list + LIST *DhcpPendingLeaseList; // Pending DHCP lease list UINT64 LastDhcpPolling; // Time which the DHCP list polled last bool SaveLog; // Save a log DHCP_CLASSLESS_ROUTE_TABLE PushRoute; // Pushing routing table @@ -511,7 +512,9 @@ int CompareDhcpLeaseList(void *p1, void *p2); DHCP_LEASE *NewDhcpLease(UINT expire, UCHAR *mac_address, UINT ip, UINT mask, char *hostname); void FreeDhcpLease(DHCP_LEASE *d); DHCP_LEASE *SearchDhcpLeaseByMac(VH *v, UCHAR *mac); +DHCP_LEASE *SearchDhcpPendingLeaseByMac(VH *v, UCHAR *mac); DHCP_LEASE *SearchDhcpLeaseByIp(VH *v, UINT ip); +DHCP_LEASE *SearchDhcpPendingLeaseByIp(VH *v, UINT ip); UINT ServeDhcpDiscover(VH *v, UCHAR *mac, UINT request_ip); UINT GetFreeDhcpIpAddress(VH *v); UINT GetFreeDhcpIpAddressByRandom(VH *v, UCHAR *mac);