fix(ping): eliminate seteuid race by pre-opening ICMP socket at init#521
Closed
somethingwithproof wants to merge 3 commits intoCacti:developfrom
Closed
fix(ping): eliminate seteuid race by pre-opening ICMP socket at init#521somethingwithproof wants to merge 3 commits intoCacti:developfrom
somethingwithproof wants to merge 3 commits intoCacti:developfrom
Conversation
…RITICAL) Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
…HIGH) Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
seteuid(0) is process-wide; the previous approach of acquiring LOCK_SETEUID per-thread serialized the seteuid calls but left a window where other threads inherited euid=0 while the mutex was held. Open the ICMP raw socket once during single-threaded initialization in spine.c main(), before any worker threads start. Store it as a global (icmp_socket). ping_icmp() now dup()s that fd per call so each thread has an independent fd for select()/setsockopt()/close() without interfering with other threads. All seteuid()/LOCK_SETEUID blocks are removed from ping_icmp(). If the socket could not be opened at startup, icmp_avail is set to FALSE and the poller falls back to UDP ping as before. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
somethingwithproof
force-pushed
the
fix/seteuid-icmp-race
branch
from
e12ec4f to
3dbc42f
Compare
Contributor
Author
|
Consolidated into mega PR #522 for independent mergeability. |
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
1 participant
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.
Problem
ping_icmp()calledseteuid(0)inside each worker thread to open a raw ICMP socket.seteuidis process-wide, so while one thread heldLOCK_SETEUIDwith euid=0, other threads that did not check the mutex could inherit the elevated privilege. The mutex serialized the seteuid calls but did not close the privilege window for concurrent threads.Fix
Open the ICMP raw socket once in
spine.c main(), before any worker threads start. The single seteuid(0)/seteuid(getuid()) pair now happens in a single-threaded context with no race.ping_icmp()callsdup(icmp_socket)to get a per-thread fd, soselect(),setsockopt(), andclose()in each thread operate on independent descriptors without interfering with other threads or the global socket.If the socket cannot be opened at startup (permissions, capability check),
icmp_availis set toFALSEand spine falls back to UDP ping as before.Changes
spine.c: addint icmp_socket = -1global; open raw socket before thread pool startsspine.h:extern int icmp_socketping.c: remove allseteuid/LOCK_SETEUIDblocks fromping_icmp; replace local socket open loop withdup(icmp_socket); rename localicmp_sockettoicmp_fdthroughoutVerification
Built clean on macOS aarch64 (gcc -std=gnu23, no errors or warnings).
Net change: 49 insertions, 103 deletions.