Mitigate ARP poisoning by preventing overwrite#58
Mitigate ARP poisoning by preventing overwrite#58danielinux wants to merge 1 commit intowolfSSL:masterfrom
Conversation
When receiving unsolicited ARP traffic, wolfIP accepts neighbors addresses. This prevents overwriting/poisoning ARP tables when an attacker is trying to impersonate an existing neighbor. For latency/performance optimizations, wolfIP is currently still populating the neighbor cache based on valid ARP requests from neighbors, if neighbors were not known.
There was a problem hiding this comment.
Pull request overview
This PR hardens wolfIP’s ARP neighbor learning to mitigate ARP-table poisoning by preventing existing neighbor entries from being overwritten by unsolicited ARP traffic, while still allowing updates when an ARP reply corresponds to a recently-sent ARP request.
Changes:
- Add tracking for recently-issued ARP requests (pending requests with TTL) to distinguish solicited vs unsolicited ARP replies.
- Prevent ARP requests and unsolicited ARP replies from overwriting an existing neighbor cache entry.
- Add unit tests covering: no-overwrite on unsolicited reply, overwrite allowed when reply matches a pending request, and no-overwrite on ARP request.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/wolfip.c | Adds pending-request tracking and changes ARP neighbor update rules to block unsolicited overwrites. |
| src/test/unit/unit.c | Adds/adjusts ARP unit tests to validate the new overwrite-prevention behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| uint64_t oldest_ts = 0; | ||
| if (!s) | ||
| return; | ||
| for (i = 0; i < WOLFIP_ARP_PENDING_MAX; i++) { | ||
| struct arp_pending_req *p = &s->arp.pending[i]; | ||
| if (p->ip == ip && p->if_idx == (uint8_t)if_idx) { | ||
| p->ts = s->last_tick; | ||
| return; | ||
| } | ||
| if (p->ip == IPADDR_ANY && slot < 0) | ||
| slot = i; | ||
| if (slot < 0) { | ||
| if (i == 0 || p->ts < oldest_ts) { | ||
| oldest_ts = p->ts; | ||
| slot = i; | ||
| } | ||
| } | ||
| } | ||
| if (slot < 0) | ||
| slot = 0; |
There was a problem hiding this comment.
arp_pending_record's slot-selection logic will effectively always pick slot 0 once the table has any non-empty entry (because slot gets set at i==0 and the later entries are never considered). This prevents tracking multiple simultaneous pending ARP requests and can cause legitimate replies to be treated as unsolicited and ignored. Consider selecting the first empty slot if available; otherwise, scan for the truly oldest ts across all entries (and/or expire old entries here) before choosing a replacement slot.
| uint64_t oldest_ts = 0; | |
| if (!s) | |
| return; | |
| for (i = 0; i < WOLFIP_ARP_PENDING_MAX; i++) { | |
| struct arp_pending_req *p = &s->arp.pending[i]; | |
| if (p->ip == ip && p->if_idx == (uint8_t)if_idx) { | |
| p->ts = s->last_tick; | |
| return; | |
| } | |
| if (p->ip == IPADDR_ANY && slot < 0) | |
| slot = i; | |
| if (slot < 0) { | |
| if (i == 0 || p->ts < oldest_ts) { | |
| oldest_ts = p->ts; | |
| slot = i; | |
| } | |
| } | |
| } | |
| if (slot < 0) | |
| slot = 0; | |
| int first_free = -1; | |
| int oldest_idx = -1; | |
| uint64_t oldest_ts = UINT64_MAX; | |
| if (!s) | |
| return; | |
| for (i = 0; i < WOLFIP_ARP_PENDING_MAX; i++) { | |
| struct arp_pending_req *p = &s->arp.pending[i]; | |
| /* If we already have a pending request for this IP on this interface, | |
| * just refresh its timestamp. */ | |
| if (p->ip == ip && p->if_idx == (uint8_t)if_idx) { | |
| p->ts = s->last_tick; | |
| return; | |
| } | |
| /* Track the first empty slot for reuse. */ | |
| if (p->ip == IPADDR_ANY) { | |
| if (first_free < 0) { | |
| first_free = i; | |
| } | |
| continue; | |
| } | |
| /* Track the oldest in-use entry as a candidate for replacement. */ | |
| if (p->ts < oldest_ts) { | |
| oldest_ts = p->ts; | |
| oldest_idx = i; | |
| } | |
| } | |
| if (first_free >= 0) { | |
| slot = first_free; | |
| } | |
| else if (oldest_idx >= 0) { | |
| slot = oldest_idx; | |
| } | |
| else { | |
| /* Fallback: if table is somehow uninitialized, use slot 0. */ | |
| slot = 0; | |
| } |
| ip4 sip = ee32(arp->sip); | ||
| int idx = arp_neighbor_index(s, if_idx, sip); | ||
| int pending = arp_pending_match_and_clear(s, if_idx, sip); | ||
| /* Security trade-off: allow quick-path add, but block unsolicited overwrite. */ | ||
| if (pending || idx < 0) { | ||
| arp_store_neighbor(s, if_idx, sip, arp->sma); | ||
| } |
There was a problem hiding this comment.
With the new if (pending || idx < 0) gate, an existing neighbor entry will never be refreshed unless we have a recent pending ARP request. However, the stack only calls arp_request() when arp_lookup() fails, so once an entry exists it can become permanently stale (e.g., peer MAC changes / VRRP failover) and wolfIP will keep sending to the old MAC indefinitely. Consider adding an aging/refresh mechanism for neighbor entries (timestamp + TTL) so that existing entries are periodically revalidated (record pending + accept update) or expired.
When receiving unsolicited ARP traffic, wolfIP
accepts neighbors addresses. This prevents overwriting/poisoning ARP tables when an attacker is trying to impersonate an existing neighbor.
For latency/performance optimizations, wolfIP is currently still populating the neighbor cache based on valid ARP requests from neighbors, if neighbors were not known.
Fenrir/226