[dhcp4relay] Security and reliability fixes - #119
Open
ashutosh-agrawal wants to merge 6 commits into
Open
Conversation
…id config-thread busy-loop handle_swss_notification() registers the DHCP_SERVER_IPV4 and DHCP_SERVER_IPV4_SERVER_IP subscriber tables with swss::Select regardless of feature_dhcp_server_enabled, but only pops() them inside the feature-enabled branch. When the feature is disabled (the default), two or more writes to those tables leave their keyspace-event buffer undrained; SubscriberStateTable:: hasCachedData() then stays true, so swss::Select keeps re-queueing the selectable and select() never blocks, pinning one CPU core. The mirror gap exists for the relaymgr/interface tables when the feature is enabled. Flatten the dispatch into a single if/else-if chain that always pops() the selectable that fired and gates only the processing on feature_dhcp_server_enabled, so every buffer drains in both modes. Events for the inactive mode are discarded; the subscriber is recreated (and its snapshot re-read) whenever process_feature_notification toggles the feature, so no state is lost. (cherry picked from commit 51ce4a8c1b5ef3ecc373917aa9f4f403cf479b8e) Signed-off-by: Ashutosh Agrawal <ashu@cisco.com>
…tion (sonic-net#54) * MIGSOFTWAR-45218: dhcp4relay: fix buffer overflows in encode_relay_option Three bounds-checking fixes in encode_relay_option(): 1. circuit-id: drop option 82 entirely if circuit-id exceeds 253 bytes to prevent writing past buf[256]. 2. Remote-ID: use std::min(MAC_ADDR_STR_LEN, mac.length()) so a short MAC string is not read past its end. 3. VSS sub-option: skip the sub-option and log a warning if the VRF name exceeds vss_buf[32]-1 bytes instead of overflowing the buffer. Add unit tests for all three cases. * MIGSOFTWAR-45218: fix uint8_t wrap and test gaps flagged in review - Guard circuit_id.length() > UINT8_MAX before the uint8_t cast to prevent a silent wrap (e.g. 256 bytes → length field 0) that would produce a malformed zero-length circuit-id TLV - Add remaining arg to EncodeAndDecode unit test (compile fix) - Assert has_remote_id after loop in encode_relay_option_short_mac so the test fails if remote-id is absent rather than silently passing * MIGSOFTWAR-45218: [dhcp4relay] drop option 82 when required remote-id does not fit * MIGSOFTWAR-45218: [dhcp4relay] drop option 82 when configured sub-options do not fit * MIGSOFTWAR-45218: [dhcp4relay] fix off-by-one bound in decode_tlv * MIGSOFTWAR-45218: fix narrowing uint8_t → size_t for encode_tlv return in test (cherry picked from commit da704dbe49de7721edb2aa9e52c0ce5feda9bf65) Signed-off-by: Ashutosh Agrawal <ashu@cisco.com>
…interfaces (sonic-net#58) * MIGSOFTWAR-45222b: dhcp4relay: reject server replies on client-facing interfaces Pass ingress_intf to to_client() and drop any BOOTPREPLY arriving on a physical interface registered in vlan_map as a client-facing VLAN member. This reinstates the structural server/client interface separation that ISC dhcrelay enforced via -id/-iu flags. Tests: to_client declaration and call updated; to_client_ingress_on_client_vlan added. * MIGSOFTWAR-45222: [dhcp4relay] fix stray brace in to_client test * MIGSOFTWAR-45222: [dhcp4relay] make ingress rejection test regression-proof (cherry picked from commit 90be4074ed5eb23495a1341ea4835f43e99f0b7c) Signed-off-by: Ashutosh Agrawal <ashu@cisco.com>
…n threads (sonic-net#63) * MIGSOFTWAR-45221: [dhcp4relay] synchronize shared config state between threads The config-listener thread writes m_config (hostname, host MAC, midplane bridge), feature_dhcp_server_enabled and global_dhcp_server_ip, while the libevent packet thread reads them on the per-packet path with no lock. Concurrent std::string assignment and read is undefined behavior and can corrupt the option-82 encoding or abort the daemon. Guard the shared state with a single m_config_mutex: writers build a local copy and swap it in under the lock (keeping the lock off the midplane DB read), and packet-path readers take a snapshot under the lock. Make feature_dhcp_server_enabled a std::atomic<bool>, and update global_dhcp_server_ip under the lock before signaling the main thread so it never observes a stale value. * MIGSOFTWAR-45221: [dhcp4relay] commit m_config before DualToR event; align mock atomic decl * MIGSOFTWAR-45221: [dhcp4relay] guard all global_dhcp_server_ip access; rollback on pipe failure * MIGSOFTWAR-45221: [dhcp4relay] update subtype flags only when subtype field changes * MIGSOFTWAR-45221: clear DualToR/SmartSwitch flags when subtype field is deleted SubscriberStateTable delivers all current hash fields on every notification, so subtype_found=false in a SET means the field was deleted, not just absent from a partial update. Add an else branch that clears is_dualTor and is_SmartSwitch when subtype is not present, and fires the DualToR pipe event so the main thread stops using Loopback0/link-selection behavior. --------- (cherry picked from commit cc4e1da914123dab3a68c63a0c33bbc9eda4c45a) Signed-off-by: Ashutosh Agrawal <ashu@cisco.com>
…short frames (sonic-net#62) * MIGSOFTWAR-45223: dhcp4relay: fix heap overflow in BOOTP padding for short frames 1. send_udp(): copy caller's buffer into a local 300-byte stack array before padding to BOOTP_MIN_LEN instead of writing past the caller's allocation. 2. from_client()/to_client(): reject any DhcpLayer whose getHeaderLen() is smaller than sizeof(pcpp::dhcp_header) before dereferencing getDhcpHeader() fields or calling addOption/removeOption. 3. pkt_in_callback(): clear client_recv_buffer bytes past buffer_sz after each recvmsg to prevent header bytes of a short frame from being primed by a preceding frame in the same batch. * MIGSOFTWAR-45223: [dhcp4relay] reject short DHCP layer in pkt_in_callback before header access * MIGSOFTWAR-45223: [dhcp4relay] strengthen short-packet unit tests against false positives * MIGSOFTWAR-45223: [dhcp4relay] fix short-packet test mocks and giaddr writes * MIGSOFTWAR-45223: fix ASAN invalid-free in short-header tests; add bootp_pad unit tests pcpp::Layer v24.09 always calls delete[] m_Data when !isAllocatedToPacket(), so constructing DhcpLayer over a stack array causes an ASAN invalid-free. Switch both short-header tests to heap-allocated buffers (new uint8_t[50]()). Extract the BOOTP minimum-length padding into bootp_pad(), compiled outside real send_udp(). Add three unit tests covering the pad/no-pad/disabled paths. Update send_udp() to delegate to bootp_pad(). * MIGSOFTWAR-45223: fix dhcp4_sender.h include path in unit tests The test Makefile does not add src/ to the include path, so mock_relay.cpp must use the same ../src/ relative include style as mock_relay.h. --------- (cherry picked from commit b7cfa81be16902dbcd0f90f7fdc407a187c7a3c9) Signed-off-by: Ashutosh Agrawal <ashu@cisco.com>
Collaborator
|
/azp run |
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
…-security-fixes-clean Resolve link-selection conflict with sonic-net#121: adopt the ISC-aligned unmasked VLAN address (RFC 3527) while keeping the bounds-checked encode_tlv and thread-safe is_dualTor snapshot from this branch. Signed-off-by: Ashutosh Agrawal <ashu@cisco.com>
Collaborator
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Member
Author
|
@Xichen96 @StormLiangMS This PR consists of 5 separate security and reliability enhancements. Please let me know if you would prefer these to be split into individual PRs. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why I did it
The DHCPv4 relay has several reliability and memory-safety issues in configuration processing and packet handling:
SubscriberStateTableinstances for both relay and DHCP-server configuration are always registered withswss::Select, but events for the inactive mode were not popped. Their cached keyspace events therefore remained pending, causingselect()to return immediately and the configuration thread to consume a CPU core.std::string-backed metadata is undefined behavior and can result in corrupted Option 82 data or a daemon crash.DhcpLayerheaders were accessed before validating that a received frame contains a complete DHCP header. In addition, BOOTP minimum-length padding could extend the caller-owned packet buffer instead of a bounded local buffer. Truncated or short frames could therefore trigger out-of-bounds reads or writes.How I did it
How to verify it
Verify that: