Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pid wrapping caused sss_cli_check_socket to close the file descriptor opened by the process #6592

Closed
answer9030 opened this issue Feb 24, 2023 · 8 comments
Labels
Closed: Fixed Issue was closed as fixed.

Comments

@answer9030
Copy link

We encountered a problem: the tcp socket of a database child process was replaced by a unix socket. After locating, it was found that the "sss_cli_check_socket" function closed the tcp socket opened by the process due to the wrapping of the pid.
The problem recurrence logic is as follows:
1) A unix socket connection to sssd_nss is established when process A calls getpwnam(), then process A exits after fork process B.
2) Process B closes all file descriptors, creates a tcp socket, and then fork process C. Process B will restart process C after process C exits abnormally.
3) Process C has restarted abnormally many times, resulting in the pid of process C being equal to that of process A. At this time, process C calls the getpwnam() function, causing the tcp socket to be replaced by a unix socket.

how to solve this problem?

@answer9030
Copy link
Author

The following code can reproduce the problem:

#define _GNU_SOURCE
#include <sched.h>
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <string.h>
#include <errno.h>
#include <sys/wait.h>
#include <sys/time.h>
#include <sys/types.h>          /* See NOTES */
#include <sys/socket.h>
#include <arpa/inet.h>
#include <pwd.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>

#if 0 // Disable PAM
// Add PAM
#include <security/pam_appl.h>
#endif 


static int tcp_socket_init(void)
{
	int ret;
	int fd = socket(AF_INET, SOCK_STREAM, 0);
	if(fd < 0) {
		fprintf(stderr, "socket failure:%s\n", strerror(errno));
		return fd;
	}

	struct sockaddr_in listen_addr = {
		.sin_family = AF_INET,
		.sin_port = ntohs(6666),
		.sin_addr = {
			.s_addr = 0,
		},
	};
	ret = bind(fd, (struct sockaddr*)&listen_addr, sizeof(listen_addr));
	if(ret < 0) {
		fprintf(stderr, "bind failure:%s\n", strerror(errno));
		close(fd);
		return ret;
	}

	ret = listen(fd, 1024);
	if(ret < 0) {
		fprintf(stderr, "listen failure:%s\n", strerror(errno));
		close(fd);
		return ret;
	}

	return fd;
}

void loop()
{
	while(1)
	{}
}

static int enter_daemon(void)
{
	int pid = fork();
	if(pid < 0) {
		fprintf(stderr, "fork failure:%s\n", strerror(errno));
		return pid;
	}
	else if(pid == 0) {
		//the chile process close all file descriptors
		for(int fd = 3; fd <= 32; ++fd) {
			close(fd);
		}
		
		setgid(getpid());
		return 0;
	}
	else {
		//parent process exits
		fprintf(stdout, "original process %d goto exiting...\n", getpid());
		exit(EXIT_SUCCESS);
	}

	return 0;
}


int main(int argc, char *argv[])
{
	//establish a unix socket connection with sssd_nss when calling getpwnam()
	getpwnam("any");

	if(enter_daemon() < 0) {
		return -1;
	}

	int stub_fd = open("/proc/self/cmdline", O_RDONLY);

	//create a tcp socket
	int net_fd = tcp_socket_init();

	while(1) {
		int pid = fork();
		if(pid < 0) {
			fprintf(stderr, "fork failure:%s\n", strerror(errno));
			return 0;
		}
		else if(pid == 0) {
			//child process call getpwnam()
			fprintf(stdout, "before getpwnam, %d\n", getpid());
			getpwnam("any");
			fprintf(stdout, "after getpwnam, %d\n", getpid());
			
			loop();
		}
		else {
			//parent process monitors child process
			int child_state;
			pid_t child = waitpid(pid, &child_state, 0);
			if(child < 0) {
				fprintf(stderr, "waitpid failure:%s\n", strerror(errno));
			}
			else {
				fprintf(stdout, "Test finish,%d quit...\n", pid);
			}
		}
	}

	return 0;
}


@alexey-tikhonov
Copy link
Member

alexey-tikhonov commented Feb 27, 2023

What SSSD version do you use and what is the OS?

@alexey-tikhonov
Copy link
Member

As a work around you can call any NSS function explicitly after enter_daemon() and before next fork() -- to let sss_cli_check_socket() update static pid_t mypid.

@answer9030
Copy link
Author

answer9030 commented Feb 28, 2023

we can not modify the application now, is it possible to modify sssd in the following way to reduce the risk of pid wrapping.

static enum sss_status sss_cli_check_socket(int *errnop,
                                            const char *socket_name,
                                            int timeout)
{
    static pid_t mypid;
    static struct stat selfsb;
    struct stat mysb, self_sb;
    int mysd;
    int ret;

    memset(&self_sb, 0, sizeof(self_sb));
    lstat("/proc/self/", &self_sb);

    if (getpid() != mypid || self_sb.st_ino != selfsb.st_ino) {
        ret = fstat(sss_cli_sd, &mysb);
        if (ret == 0) {
            if (S_ISSOCK(mysb.st_mode) &&
                mysb.st_dev == sss_cli_sb.st_dev &&
                mysb.st_ino == sss_cli_sb.st_ino) {
                sss_cli_close_socket();
            }
        }
        sss_cli_sd = -1;
        mypid = getpid();
        lstat("/proc/self/", &selfsb);
    }

@answer9030
Copy link
Author

patch as follows:

--- a/sssd-2.8.2/src/sss_client/common.c	2022-12-09 20:39:40.000000000 +0800
+++ b/sssd-2.8.2/src/sss_client/common.c	2023-02-27 11:42:10.000000000 +0800
@@ -664,11 +664,15 @@
                                             int timeout)
 {
     static pid_t mypid;
-    struct stat mysb;
+    static struct stat selfsb;
+    struct stat mysb, self_sb;
     int mysd;
     int ret;
 
-    if (getpid() != mypid) {
+    memset(&self_sb, 0, sizeof(self_sb));
+    lstat("/proc/self/", &self_sb);
+
+    if (getpid() != mypid || self_sb.st_ino != selfsb.st_ino) {
         ret = fstat(sss_cli_sd, &mysb);
         if (ret == 0) {
             if (S_ISSOCK(mysb.st_mode) &&
@@ -679,6 +683,7 @@
         }
         sss_cli_sd = -1;
         mypid = getpid();
+        lstat("/proc/self/", &selfsb);
     }
 
     /* check if the socket has been closed on the other side */

@alexey-tikhonov
Copy link
Member

In general, makes sense, imo.
Did you test if this solves an issue for you?

Patch would benefit from more telling variable names and also handling of lstat() failures.

Would you mind to open a PR?

@answer9030
Copy link
Author

Yes,I tested it with the above test case and solved the problem of pid wrapping.
I opened a PR, #6605

@pbrezina
Copy link
Member

Pushed PR: #6616

  • master
    • 5c363bf - Fixed the problem of calling getpid() and lstat() twice in sss_cli_check_socket()
    • 0e25f0d - Fixed pid wrapping in sss_cli_check_socket

@pbrezina pbrezina added the Closed: Fixed Issue was closed as fixed. label Mar 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Closed: Fixed Issue was closed as fixed.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants