Skip to content

Commit 3dbc42f

Browse files
somethingwithproofsomethingwithproof
committed
fix(ping): eliminate seteuid race by pre-opening ICMP socket at init
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>
1 parent 205f522 commit 3dbc42f

3 files changed

Lines changed: 53 additions & 107 deletions

File tree

‎ping.c‎

Lines changed: 28 additions & 107 deletions
Original file line numberDiff line numberDiff line change
@@ -257,7 +257,7 @@ int ping_snmp(host_t *host, ping_t *ping) {
257257
*
258258
*/
259259
int ping_icmp(host_t *host, ping_t *ping) {
260-
int icmp_socket;
260+
int icmp_fd;
261261

262262
double begin_time, end_time, total_time;
263263
double host_timeout;
@@ -286,49 +286,21 @@ int ping_icmp(host_t *host, ping_t *ping) {
286286
SPINE_LOG_DEBUG(("DEBUG: Device[%i] Entering ICMP Ping", host->id));
287287
}
288288

289-
/* get ICMP socket */
290-
retry_count = 0;
291-
while (TRUE) {
292-
#if !(defined(__CYGWIN__) && !defined(SOLAR_PRIV))
293-
if (hasCaps() != TRUE) {
294-
thread_mutex_lock(LOCK_SETEUID);
295-
if (seteuid(0) == -1) {
296-
SPINE_LOG_DEBUG(("WARNING: Spine unable to obtain root privileges."));
297-
}
298-
}
299-
#endif
300-
301-
if ((icmp_socket = socket(AF_INET, SOCK_RAW, IPPROTO_ICMP)) == -1) {
302-
usleep(500000);
303-
retry_count++;
304-
305-
if (retry_count > 4) {
306-
snprintf(ping->ping_response, SMALL_BUFSIZE, "ICMP: Ping unable to create ICMP Socket");
307-
snprintf(ping->ping_status, 50, "down");
308-
#if !(defined(__CYGWIN__) && !defined(SOLAR_PRIV))
309-
if (hasCaps() != TRUE) {
310-
if (seteuid(getuid()) == -1) {
311-
SPINE_LOG_DEBUG(("WARNING: Spine unable to drop from root to local user."));
312-
}
313-
thread_mutex_unlock(LOCK_SETEUID);
314-
}
315-
#endif
316-
317-
return HOST_DOWN;
318-
}
319-
} else {
320-
break;
321-
}
289+
/* Use the pre-opened global ICMP socket (opened single-threaded in main
290+
* before any worker threads start, eliminating the seteuid race).
291+
* dup() gives each thread its own fd so select()/close() don't interfere. */
292+
if (icmp_socket < 0) {
293+
snprintf(ping->ping_response, SMALL_BUFSIZE, "ICMP: raw socket not available");
294+
snprintf(ping->ping_status, 50, "down");
295+
return HOST_DOWN;
322296
}
323297

324-
#if !(defined(__CYGWIN__) && !defined(SOLAR_PRIV))
325-
if (hasCaps() != TRUE) {
326-
if (seteuid(getuid()) == -1) {
327-
SPINE_LOG_DEBUG(("WARNING: Spine unable to drop from root to local user."));
328-
}
329-
thread_mutex_unlock(LOCK_SETEUID);
298+
icmp_fd = dup(icmp_socket);
299+
if (icmp_fd < 0) {
300+
snprintf(ping->ping_response, SMALL_BUFSIZE, "ICMP: socket dup failed: %s", strerror(errno));
301+
snprintf(ping->ping_status, 50, "down");
302+
return HOST_DOWN;
330303
}
331-
#endif
332304

333305
/* convert the host timeout to a double precision number in seconds */
334306
host_timeout = host->ping_timeout;
@@ -359,7 +331,7 @@ int ping_icmp(host_t *host, ping_t *ping) {
359331
icmp->icmp_cksum = get_checksum(packet, packet_len);
360332

361333
/* hostname must be nonblank */
362-
if ((strlen(host->hostname) != 0) && (icmp_socket != -1)) {
334+
if (strlen(host->hostname) != 0) {
363335
/* initialize variables */
364336
snprintf(ping->ping_status, 50, "down");
365337
snprintf(ping->ping_response, SMALL_BUFSIZE, "default");
@@ -375,7 +347,7 @@ int ping_icmp(host_t *host, ping_t *ping) {
375347
snprintf(ping->ping_response, SMALL_BUFSIZE, "ICMP: Ping timed out");
376348
snprintf(ping->ping_status, 50, "down");
377349
free(packet);
378-
close(icmp_socket);
350+
close(icmp_fd);
379351
return HOST_DOWN;
380352
}
381353

@@ -390,27 +362,27 @@ int ping_icmp(host_t *host, ping_t *ping) {
390362
timeout.tv_usec = ((int) (host_timeout - total_time) % 1000) * 1000;
391363

392364
/* set the socket send and receive timeout */
393-
setsockopt(icmp_socket, SOL_SOCKET, SO_RCVTIMEO, (char*)&timeout, sizeof(timeout));
394-
setsockopt(icmp_socket, SOL_SOCKET, SO_SNDTIMEO, (char*)&timeout, sizeof(timeout));
365+
setsockopt(icmp_fd, SOL_SOCKET, SO_RCVTIMEO, (char*)&timeout, sizeof(timeout));
366+
setsockopt(icmp_fd, SOL_SOCKET, SO_SNDTIMEO, (char*)&timeout, sizeof(timeout));
395367

396368
/* send packet to destination */
397-
return_code = sendto(icmp_socket, packet, packet_len, 0, (struct sockaddr *) &fromname, sizeof(fromname));
369+
return_code = sendto(icmp_fd, packet, packet_len, 0, (struct sockaddr *) &fromname, sizeof(fromname));
398370

399371
fromlen = sizeof(fromname);
400372

401373
/* wait for a response on the socket */
402374
/* reinitialize fd_set -- select(2) clears bits in place on return */
403375
keep_listening:
404376
FD_ZERO(&socket_fds);
405-
if (icmp_socket >= FD_SETSIZE) {
406-
SPINE_LOG(("ERROR: Device[%i] ICMP socket %d exceeds FD_SETSIZE %d", host->id, icmp_socket, FD_SETSIZE));
377+
if (icmp_fd >= FD_SETSIZE) {
378+
SPINE_LOG(("ERROR: Device[%i] ICMP socket %d exceeds FD_SETSIZE %d", host->id, icmp_fd, FD_SETSIZE));
407379
snprintf(ping->ping_status, 50, "down");
408380
snprintf(ping->ping_response, SMALL_BUFSIZE, "ICMP: fd exceeds FD_SETSIZE");
409-
close(icmp_socket);
381+
close(icmp_fd);
410382
return HOST_DOWN;
411383
}
412-
FD_SET(icmp_socket,&socket_fds);
413-
return_code = select(icmp_socket + 1, &socket_fds, NULL, NULL, &timeout);
384+
FD_SET(icmp_fd,&socket_fds);
385+
return_code = select(icmp_fd + 1, &socket_fds, NULL, NULL, &timeout);
414386

415387
/* record end time */
416388
end_time = get_time_as_double();
@@ -420,9 +392,9 @@ int ping_icmp(host_t *host, ping_t *ping) {
420392

421393
if (total_time < host_timeout) {
422394
#if !(defined(__CYGWIN__))
423-
return_code = recvfrom(icmp_socket, socket_reply, BUFSIZE, MSG_WAITALL, (struct sockaddr *) &recvname, &fromlen);
395+
return_code = recvfrom(icmp_fd, socket_reply, BUFSIZE, MSG_WAITALL, (struct sockaddr *) &recvname, &fromlen);
424396
#else
425-
return_code = recvfrom(icmp_socket, socket_reply, BUFSIZE, MSG_PEEK, (struct sockaddr *) &recvname, &fromlen);
397+
return_code = recvfrom(icmp_fd, socket_reply, BUFSIZE, MSG_PEEK, (struct sockaddr *) &recvname, &fromlen);
426398
#endif
427399

428400
if (return_code < 0) {
@@ -451,24 +423,7 @@ int ping_icmp(host_t *host, ping_t *ping) {
451423
snprintf(ping->ping_response, SMALL_BUFSIZE, "ICMP: Device is Alive");
452424
snprintf(ping->ping_status, 50, "%.5f", total_time);
453425
free(packet);
454-
#if !(defined(__CYGWIN__) && !defined(SOLAR_PRIV))
455-
if (hasCaps() != TRUE) {
456-
thread_mutex_lock(LOCK_SETEUID);
457-
if (seteuid(0) == -1) {
458-
SPINE_LOG_DEBUG(("WARNING: Spine unable to obtain root privileges."));
459-
}
460-
}
461-
#endif
462-
close(icmp_socket);
463-
#if !(defined(__CYGWIN__) && !defined(SOLAR_PRIV))
464-
if (hasCaps() != TRUE) {
465-
if (seteuid(getuid()) == -1) {
466-
SPINE_LOG_DEBUG(("WARNING: Spine unable to drop from root to local user."));
467-
}
468-
thread_mutex_unlock(LOCK_SETEUID);
469-
}
470-
#endif
471-
426+
close(icmp_fd);
472427
return HOST_UP;
473428
} else {
474429
/* received a response other than an echo reply */
@@ -502,48 +457,14 @@ int ping_icmp(host_t *host, ping_t *ping) {
502457
snprintf(ping->ping_response, SMALL_BUFSIZE, "ICMP: Destination hostname invalid");
503458
snprintf(ping->ping_status, 50, "down");
504459
free(packet);
505-
#if !(defined(__CYGWIN__) && !defined(SOLAR_PRIV))
506-
if (hasCaps() != TRUE) {
507-
thread_mutex_lock(LOCK_SETEUID);
508-
if (seteuid(0) == -1) {
509-
SPINE_LOG_DEBUG(("WARNING: Spine unable to obtain root privileges."));
510-
}
511-
}
512-
#endif
513-
close(icmp_socket);
514-
#if !(defined(__CYGWIN__) && !defined(SOLAR_PRIV))
515-
if (hasCaps() != TRUE) {
516-
if (seteuid(getuid()) == -1) {
517-
SPINE_LOG_DEBUG(("WARNING: Spine unable to drop from root to local user."));
518-
}
519-
thread_mutex_unlock(LOCK_SETEUID);
520-
}
521-
#endif
460+
close(icmp_fd);
522461
return HOST_DOWN;
523462
}
524463
} else {
525464
snprintf(ping->ping_response, SMALL_BUFSIZE, "ICMP: Destination address not specified");
526465
snprintf(ping->ping_status, 50, "down");
527466
free(packet);
528-
if (icmp_socket != -1) {
529-
#if !(defined(__CYGWIN__) && !defined(SOLAR_PRIV))
530-
if (hasCaps() != TRUE) {
531-
thread_mutex_lock(LOCK_SETEUID);
532-
if (seteuid(0) == -1) {
533-
SPINE_LOG_DEBUG(("WARNING: Spine unable to obtain root privileges."));
534-
}
535-
}
536-
#endif
537-
close(icmp_socket);
538-
#if !(defined(__CYGWIN__) && !defined(SOLAR_PRIV))
539-
if (hasCaps() != TRUE) {
540-
if (seteuid(getuid()) == -1) {
541-
SPINE_LOG_DEBUG(("WARNING: Spine unable to drop from root to local user."));
542-
}
543-
thread_mutex_unlock(LOCK_SETEUID);
544-
}
545-
#endif
546-
}
467+
close(icmp_fd);
547468
return HOST_DOWN;
548469
}
549470
}

‎spine.c‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -113,6 +113,7 @@ int *debug_devices;
113113

114114
pool_t *db_pool_local;
115115
pool_t *db_pool_remote;
116+
int icmp_socket = -1;
116117

117118
poller_thread_t** details = NULL;
118119

@@ -722,6 +723,29 @@ int main(int argc, char *argv[]) {
722723
/* initialize thread initialization semaphore */
723724
sem_init(&thread_init_sem, 0, 1);
724725

726+
/* Open the ICMP raw socket while still single-threaded and privileged.
727+
* seteuid(0) is process-wide; doing it here avoids the race where
728+
* multiple threads could inherit euid=0 while one holds LOCK_SETEUID. */
729+
#if !(defined(__CYGWIN__) && !defined(SOLAR_PRIV))
730+
if (hasCaps() != TRUE) {
731+
if (seteuid(0) == -1) {
732+
SPINE_LOG(("WARNING: Unable to obtain root privileges for ICMP socket."));
733+
}
734+
}
735+
#endif
736+
icmp_socket = socket(AF_INET, SOCK_RAW, IPPROTO_ICMP);
737+
#if !(defined(__CYGWIN__) && !defined(SOLAR_PRIV))
738+
if (hasCaps() != TRUE) {
739+
if (seteuid(getuid()) == -1) {
740+
SPINE_LOG(("WARNING: Unable to drop root privileges after ICMP socket open."));
741+
}
742+
}
743+
#endif
744+
if (icmp_socket < 0) {
745+
SPINE_LOG(("WARNING: Unable to open ICMP raw socket: %s. ICMP ping disabled.", strerror(errno)));
746+
set.icmp_avail = FALSE;
747+
}
748+
725749
/* specify the point of timeout for timedwait semaphores */
726750
//until_spec.tv_sec = (time_t)(set.poller_interval + begin_time - 0.2);
727751
//until_spec.tv_nsec = 0;

‎spine.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -634,5 +634,6 @@ extern sem_t available_threads;
634634
extern sem_t available_scripts;
635635
extern pool_t *db_pool_remote;
636636
extern pool_t *db_pool_local;
637+
extern int icmp_socket;
637638

638639
#endif /* not _SPINE_H_ */

0 commit comments

Comments
 (0)