[SSSD] [PATCH] PAM: close socket fd with pam_set_data

Jakub Hrozek jhrozek at redhat.com
Thu Oct 11 18:23:19 UTC 2012


On Thu, Oct 11, 2012 at 09:30:29AM -0400, Simo Sorce wrote:
> On Thu, 2012-10-11 at 10:00 +0200, Jakub Hrozek wrote:
> > From 98c8a6b92db2872083473b4ce0761bffc919e847 Mon Sep 17 00:00:00 2001
> > From: Jakub Hrozek <jhrozek at redhat.com>
> > Date: Thu, 4 Oct 2012 19:08:08 +0200
> > Subject: [PATCH] PAM: close socket fd with pam_set_data
> > 
> > https://fedorahosted.org/sssd/ticket/1569
> > ---
> >  src/sss_client/common.c  |  6 ++++++
> >  src/sss_client/pam_sss.c | 26 ++++++++++++++++++++++++++
> >  src/sss_client/sss_cli.h |  2 ++
> >  3 files changed, 34 insertions(+)
> > 
> > diff --git a/src/sss_client/common.c b/src/sss_client/common.c
> > index
> > 1ef3ba15e5a86952a05de29c9db212fc829111bc..a4d523cdf45665c2e1b4984cc9b6db14c3d05340 100644
> > --- a/src/sss_client/common.c
> > +++ b/src/sss_client/common.c
> > @@ -794,6 +794,12 @@ errno_t check_server_cred(int sockfd)
> >  #endif
> >      return 0;
> >  }
> > +
> > +int *sss_pam_get_socket(void)
> > +{
> > +    return &sss_cli_sd;
> > +}
> > +
> >  int sss_pam_make_request(enum sss_cli_command cmd,
> >                        struct sss_cli_req_data *rd,
> >                        uint8_t **repbuf, size_t *replen,
> > diff --git a/src/sss_client/pam_sss.c b/src/sss_client/pam_sss.c
> > index
> > efbc48b6ef0458bb79dad53f81c584611f2be22d..92c90742e1931cf063a769a13befa8ec3567969a 100644
> > --- a/src/sss_client/pam_sss.c
> > +++ b/src/sss_client/pam_sss.c
> > @@ -54,6 +54,7 @@
> >  #define FLAGS_USE_AUTHTOK    (1 << 2)
> >  
> >  #define PWEXP_FLAG "pam_sss:password_expired_flag"
> > +#define FD_DESTRUCTOR "pam_sss:fd_destructor"
> >  
> >  #define PW_RESET_MSG_FILENAME_TEMPLATE SSSD_CONF_DIR"/customize/%
> > s/pam_sss_pw_reset_message.%s"
> >  #define PW_RESET_MSG_MAX_SIZE 4096
> > @@ -122,6 +123,24 @@ static void free_exp_data(pam_handle_t *pamh,
> > void *ptr, int err)
> >      ptr = NULL;
> >  }
> >  
> > +static void close_fd(pam_handle_t *pamh, void *ptr, int err)
> > +{
> > +    int fd = *((int *) ptr);
> > +
> > +    if (err & PAM_DATA_REPLACE) {
> > +        /* Nothing to do */
> > +        return;
> > +    }
> > +
> > +    if (fd == -1) {
> > +        /* fd not yet initialized */
> > +        return;
> > +    }
> > +
> > +    D(("Closing the fd"));
> > +    close(fd);
> > +}
> 
> NACK, ^^^^^^^^^

I'm sorry Simo but your Nack came too late after I pushed the patch and
before I sent a pushmail, so I'll attach a fix.

As discussed off-list already I will CC you with any future client
patches. 

> you are closing the fd and leaving sss_cli_sd untouched, which means it
> will still point to a random file descriptor.
> 

fixed

> Also I do not understand why this whole dance in passing pointers and
> the sss_pam_get_socket() function, seem all very unnecessary.
> .
> sss_cli_sd is a global variable which means:
> a) you can access it directly

Yes and no, it's local to the sss_client/common.c module, so I moved
closing the socket there.

> b) you need mutexes to access it (see sss_pam_make_request()).

fixed

The attached fix should amend the code as you suggested.
-------------- next part --------------
>From 83593a2d3452faeb59acc229d5c102ab58b8f76e Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Thu, 11 Oct 2012 20:18:18 +0200
Subject: [PATCH] PAM: fix handling the client fd in pam destructor

* Protect the fd with a mutex when closing
* Set it to a safe value after closing
---
 src/sss_client/common.c  | 17 ++++++++++++-----
 src/sss_client/pam_sss.c | 11 ++---------
 src/sss_client/sss_cli.h |  8 ++++----
 3 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/src/sss_client/common.c b/src/sss_client/common.c
index a4d523cdf45665c2e1b4984cc9b6db14c3d05340..7cfa3e0ef58e91315100e7571dff60c3bd21e9d9 100644
--- a/src/sss_client/common.c
+++ b/src/sss_client/common.c
@@ -795,11 +795,6 @@ errno_t check_server_cred(int sockfd)
     return 0;
 }
 
-int *sss_pam_get_socket(void)
-{
-    return &sss_cli_sd;
-}
-
 int sss_pam_make_request(enum sss_cli_command cmd,
                       struct sss_cli_req_data *rd,
                       uint8_t **repbuf, size_t *replen,
@@ -879,6 +874,18 @@ out:
     return ret;
 }
 
+void sss_pam_close_fd(void)
+{
+    sss_pam_lock();
+
+    if (sss_cli_sd != -1) {
+        close(sss_cli_sd);
+        sss_cli_sd = -1;
+    }
+
+    sss_pam_unlock();
+}
+
 int sss_sudo_make_request(enum sss_cli_command cmd,
                           struct sss_cli_req_data *rd,
                           uint8_t **repbuf, size_t *replen,
diff --git a/src/sss_client/pam_sss.c b/src/sss_client/pam_sss.c
index 90d4c0a331f4bac81426d148e529b41060b19250..3734c8f083cc3093dfb15f15d5af3096df631678 100644
--- a/src/sss_client/pam_sss.c
+++ b/src/sss_client/pam_sss.c
@@ -125,20 +125,13 @@ static void free_exp_data(pam_handle_t *pamh, void *ptr, int err)
 
 static void close_fd(pam_handle_t *pamh, void *ptr, int err)
 {
-    int fd = *((int *) ptr);
-
     if (err & PAM_DATA_REPLACE) {
         /* Nothing to do */
         return;
     }
 
-    if (fd == -1) {
-        /* fd not yet initialized */
-        return;
-    }
-
     D(("Closing the fd"));
-    close(fd);
+    sss_pam_close_fd();
 }
 
 static size_t add_authtok_item(enum pam_item_type type,
@@ -1098,7 +1091,7 @@ static int send_and_receive(pam_handle_t *pamh, struct pam_items *pi,
     errnop = 0;
     ret = sss_pam_make_request(task, &rd, &repbuf, &replen, &errnop);
 
-    sret = pam_set_data(pamh, FD_DESTRUCTOR, sss_pam_get_socket(), close_fd);
+    sret = pam_set_data(pamh, FD_DESTRUCTOR, NULL, close_fd);
     if (sret != PAM_SUCCESS) {
         D(("pam_set_data failed, client might leaks fds"));
     }
diff --git a/src/sss_client/sss_cli.h b/src/sss_client/sss_cli.h
index f3cb44adbcefb31c60c88c71e9251b351dc3a6c7..372bcee596a72e85d02516acdb10b775c4ddc2df 100644
--- a/src/sss_client/sss_cli.h
+++ b/src/sss_client/sss_cli.h
@@ -478,10 +478,10 @@ enum nss_status sss_nss_make_request(enum sss_cli_command cmd,
                                      int *errnop);
 
 int sss_pam_make_request(enum sss_cli_command cmd,
-                                     struct sss_cli_req_data *rd,
-                                     uint8_t **repbuf, size_t *replen,
-                                     int *errnop);
-int *sss_pam_get_socket(void);
+                         struct sss_cli_req_data *rd,
+                         uint8_t **repbuf, size_t *replen,
+                         int *errnop);
+void sss_pam_close_fd(void);
 
 int sss_pac_make_request(enum sss_cli_command cmd,
                          struct sss_cli_req_data *rd,
-- 
1.7.11.4



More information about the sssd-devel mailing list