[SSSD] [PATCH] Allow arbitrary-length PAM messages

Sumit Bose sbose at redhat.com
Thu Mar 25 13:56:01 UTC 2010


On Wed, Mar 24, 2010 at 06:56:10AM -0400, Stephen Gallagher wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 03/24/2010 06:49 AM, Stephen Gallagher wrote:
> > On 03/23/2010 04:51 PM, Simo Sorce wrote:
> >> On Tue, 23 Mar 2010 16:38:57 -0400
> >> Stephen Gallagher <sgallagh at redhat.com> wrote:
> > 
> >>> +        tempbuf = talloc_realloc(state, state->buf, uint8_t,
> >>> +                                 state->len + size);
> >>> +        if(!tempbuf) {
> >>> +            tevent_req_error(req, ENOMEM);
> >>>              return;
> >>>          }
> >>> +        state->buf = talloc_steal(state, tempbuf);
> > 
> >> Why the use of tempbuf ?
> > 
> >> you can simply do
> >> state->buf = talloc_realloc(state, state->buf, ...
> > 
> >> if state->buf is null you abort the whole operation anyway.
> > 
> >> The talloc_steal is also useless given that talloc_realloc doesn't
> >> change the parent.
> > 
> >> Simo.
> > 
> > 
> > 
> > You are absolutely correct, of course. New patch behaves better.
> > 
> > I also realized that I had added an extra variable to
> > pack_response_packet() in krb5_child.c that was never used (and
> > generated a compiler warning). I have removed it.
> > 
> 
> This time with the patch attached.

NACK.

Please consider the following two comments:

- can you call free(user_msg) just after do_pam_conversation() to avoid to have
  it two times?
- can you make buf in read_pipe_handler() a buf[MAX_CHILD_MSG_SIZE] or
  put the read into a while loop to avoid the multiple memory allocation
  for large messages?

Thanks.

bye,
Sumit

> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://fedorahosted.org/mailman/listinfo/sssd-devel
> 
> - -- 
> Stephen Gallagher
> RHCE 804006346421761
> 
> Delivering value year after year.
> Red Hat ranks #1 in value among software vendors.
> http://www.redhat.com/promo/vendor/
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.10 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
> 
> iEYEARECAAYFAkup78oACgkQeiVVYja6o6MpVQCfRJaX/AdoZVjgr1J2zAXp3fFa
> XLEAnjOgDp2Rshn0bR0x1nSw0CobsI9f
> =UB7m
> -----END PGP SIGNATURE-----

> From 2d5cde2c193234ad7bb254956eaa8fcb63733ac0 Mon Sep 17 00:00:00 2001
> From: Stephen Gallagher <sgallagh at redhat.com>
> Date: Tue, 23 Mar 2010 16:35:49 -0400
> Subject: [PATCH] Allow arbitrary-length PAM messages
> 
> The PAM standard allows for messages of any length to be returned
> to the client. We were discarding all messages of length greater
> than 255. This patch dynamically allocates the message buffers so
> we can pass the complete message.
> 
> This resolves https://fedorahosted.org/sssd/ticket/432
> ---
>  src/providers/child_common.c    |   27 +++++++++++++++++++--------
>  src/providers/child_common.h    |    5 ++---
>  src/providers/krb5/krb5_auth.c  |    2 +-
>  src/providers/krb5/krb5_child.c |   25 ++++++-------------------
>  src/providers/ldap/ldap_child.c |   12 ++++++------
>  src/sss_client/pam_sss.c        |   24 +++++++++++++++++++++---
>  src/util/user_info_msg.c        |   11 ++++++++---
>  7 files changed, 63 insertions(+), 43 deletions(-)
> 
> diff --git a/src/providers/child_common.c b/src/providers/child_common.c
> index 2ad0f04..252b646 100644
> --- a/src/providers/child_common.c
> +++ b/src/providers/child_common.c
> @@ -149,9 +149,8 @@ struct tevent_req *read_pipe_send(TALLOC_CTX *mem_ctx,
>      if (req == NULL) return NULL;
>  
>      state->fd = fd;
> -    state->buf = talloc_array(state, uint8_t, MAX_CHILD_MSG_SIZE);
> +    state->buf = NULL;
>      state->len = 0;
> -    if (state->buf == NULL) goto fail;
>  
>      fde = tevent_add_fd(ev, state, fd, TEVENT_FD_READ,
>                          read_pipe_handler, req);
> @@ -176,6 +175,7 @@ static void read_pipe_handler(struct tevent_context *ev,
>                                                      struct read_pipe_state);
>      ssize_t size;
>      errno_t err;
> +    uint8_t *buf;
>  
>      if (flags & TEVENT_FD_WRITE) {
>          DEBUG(1, ("read_pipe_done called with TEVENT_FD_WRITE,"
> @@ -184,9 +184,15 @@ static void read_pipe_handler(struct tevent_context *ev,
>          return;
>      }
>  
> +    buf = talloc_array(state, uint8_t, CHILD_MSG_CHUNK);
> +    if (!buf) {
> +        tevent_req_error(req, ENOMEM);
> +        return;
> +    }
> +
>      size = read(state->fd,
> -                state->buf + state->len,
> -                MAX_CHILD_MSG_SIZE - state->len);
> +                buf,
> +                CHILD_MSG_CHUNK);
>      if (size == -1) {
>          err = errno;
>          if (err == EAGAIN || err == EINTR) {
> @@ -198,13 +204,18 @@ static void read_pipe_handler(struct tevent_context *ev,
>          return;
>  
>      } else if (size > 0) {
> -        state->len += size;
> -        if (state->len > MAX_CHILD_MSG_SIZE) {
> -            DEBUG(1, ("read to much, this should never happen.\n"));
> -            tevent_req_error(req, EINVAL);
> +        state->buf = talloc_realloc(state, state->buf, uint8_t,
> +                                    state->len + size);
> +        if(!state->buf) {
> +            tevent_req_error(req, ENOMEM);
>              return;
>          }
>  
> +        safealign_memcpy(&state->buf[state->len], buf,
> +                         size, &state->len);
> +        talloc_zfree(buf);
> +        return;
> +
>      } else if (size == 0) {
>          DEBUG(6, ("EOF received, client finished\n"));
>          tevent_req_done(req);
> diff --git a/src/providers/child_common.h b/src/providers/child_common.h
> index a441df3..0b2081d 100644
> --- a/src/providers/child_common.h
> +++ b/src/providers/child_common.h
> @@ -33,12 +33,11 @@
>  #include "util/util.h"
>  
>  #define IN_BUF_SIZE         512
> -#define MAX_CHILD_MSG_SIZE  255
> +#define CHILD_MSG_CHUNK     256
>  
>  struct response {
> -    size_t max_size;
> -    size_t size;
>      uint8_t *buf;
> +    size_t size;
>  };
>  
>  struct io_buffer {
> diff --git a/src/providers/krb5/krb5_auth.c b/src/providers/krb5/krb5_auth.c
> index ce3aacd..880930a 100644
> --- a/src/providers/krb5/krb5_auth.c
> +++ b/src/providers/krb5/krb5_auth.c
> @@ -1091,7 +1091,7 @@ static void krb5_child_done(struct tevent_req *req)
>                                                  *msg_len));
>  
>      if ((p + *msg_len) != len) {
> -        DEBUG(1, ("message format error.\n"));
> +        DEBUG(1, ("message format error [%d] != [%d].\n", p+*msg_len, len));
>          goto done;
>      }
>  
> diff --git a/src/providers/krb5/krb5_child.c b/src/providers/krb5/krb5_child.c
> index 86242ef..620e4d1 100644
> --- a/src/providers/krb5/krb5_child.c
> +++ b/src/providers/krb5/krb5_child.c
> @@ -247,27 +247,15 @@ done:
>      return kerr;
>  }
>  
> -static struct response *init_response(TALLOC_CTX *mem_ctx) {
> -    struct response *r;
> -    r = talloc(mem_ctx, struct response);
> -    r->buf = talloc_size(mem_ctx, MAX_CHILD_MSG_SIZE);
> -    if (r->buf == NULL) {
> -        DEBUG(1, ("talloc_size failed.\n"));
> -        return NULL;
> -    }
> -    r->max_size = MAX_CHILD_MSG_SIZE;
> -    r->size = 0;
> -
> -    return r;
> -}
> -
>  static errno_t pack_response_packet(struct response *resp, int status, int type,
>                                      size_t len, const uint8_t *data)
>  {
>      size_t p = 0;
>  
> -    if ((3*sizeof(int32_t) + len +1) > resp->max_size) {
> -        DEBUG(1, ("response message too big.\n"));
> +    resp->buf = talloc_array(resp, uint8_t,
> +                             3*sizeof(int32_t) + len);
> +    if (!resp->buf) {
> +        DEBUG(1, ("Insufficient memory to create message.\n"));
>          return ENOMEM;
>      }
>  
> @@ -293,9 +281,9 @@ static struct response *prepare_response_message(struct krb5_req *kr,
>      size_t user_resp_len;
>      uint8_t *user_resp;
>  
> -    resp = init_response(kr);
> +    resp = talloc_zero(kr, struct response);
>      if (resp == NULL) {
> -        DEBUG(1, ("init_response failed.\n"));
> +        DEBUG(1, ("Initializing response failed.\n"));
>          return NULL;
>      }
>  
> @@ -321,7 +309,6 @@ static struct response *prepare_response_message(struct krb5_req *kr,
>              talloc_zfree(msg);
>          }
>      } else {
> -
>          if (user_error_message != NULL) {
>              ret = pack_user_info_chpass_error(kr, user_error_message,
>                                                &user_resp_len, &user_resp);
> diff --git a/src/providers/ldap/ldap_child.c b/src/providers/ldap/ldap_child.c
> index 069fbcf..6a78ca0 100644
> --- a/src/providers/ldap/ldap_child.c
> +++ b/src/providers/ldap/ldap_child.c
> @@ -97,6 +97,11 @@ static int pack_buffer(struct response *r, int result, const char *msg)
>      len = strlen(msg);
>      r->size = 2 * sizeof(uint32_t) + len;
>  
> +    r->buf = talloc_array(r, uint8_t, r->size);
> +    if(!r->buf) {
> +        return ENOMEM;
> +    }
> +
>      /* result */
>      SAFEALIGN_SET_UINT32(&r->buf[p], result, &p);
>  
> @@ -265,12 +270,7 @@ static int prepare_response(TALLOC_CTX *mem_ctx,
>      r = talloc_zero(mem_ctx, struct response);
>      if (!r) return ENOMEM;
>  
> -    r->buf = talloc_size(mem_ctx, MAX_CHILD_MSG_SIZE);
> -    if (r->buf == NULL) {
> -        DEBUG(1, ("talloc_size failed.\n"));
> -        return ENOMEM;
> -    }
> -    r->max_size = MAX_CHILD_MSG_SIZE;
> +    r->buf = NULL;
>      r->size = 0;
>  
>      if (kerr == 0) {
> diff --git a/src/sss_client/pam_sss.c b/src/sss_client/pam_sss.c
> index 07ed4e7..2b1f510 100644
> --- a/src/sss_client/pam_sss.c
> +++ b/src/sss_client/pam_sss.c
> @@ -588,7 +588,8 @@ static int user_info_chpass_error(pam_handle_t *pamh, size_t buflen,
>  {
>      int ret;
>      uint32_t msg_len;
> -    char user_msg[256];
> +    char *user_msg;
> +    size_t bufsize = 0;
>  
>      if (buflen < 2* sizeof(uint32_t)) {
>          D(("User info response data is too short"));
> @@ -602,22 +603,39 @@ static int user_info_chpass_error(pam_handle_t *pamh, size_t buflen,
>          return PAM_BUF_ERR;
>      }
>  
> -    ret = snprintf(user_msg, sizeof(user_msg), "%s%s%.*s",
> +    bufsize = strlen(_("Password change failed. ")) + 1;
> +
> +    if (msg_len > 0) {
> +        bufsize += strlen(_("Server message: ")) + msg_len;
> +    }
> +
> +    user_msg = (char *)malloc(sizeof(char) * bufsize);
> +    if (!user_msg) {
> +       D(("Out of memory."));
> +       return PAM_SYSTEM_ERR;
> +    }
> +
> +    ret = snprintf(user_msg, bufsize, "%s%s%.*s",
>                     _("Password change failed. "),
>                     msg_len > 0 ? _("Server message: ") : "",
>                     msg_len,
>                     msg_len > 0 ? (char *)(buf + 2 * sizeof(uint32_t)) : "" );
> -    if (ret < 0 || ret >= sizeof(user_msg)) {
> +    if (ret < 0 || ret > bufsize) {
>          D(("snprintf failed."));
> +
> +        free(user_msg);
>          return PAM_SYSTEM_ERR;
>      }
>  
>      ret = do_pam_conversation(pamh, PAM_TEXT_INFO, user_msg, NULL, NULL);
>      if (ret != PAM_SUCCESS) {
>          D(("do_pam_conversation failed."));
> +
> +        free(user_msg);
>          return PAM_SYSTEM_ERR;
>      }
>  
> +    free(user_msg);
>      return PAM_SUCCESS;
>  }
>  
> diff --git a/src/util/user_info_msg.c b/src/util/user_info_msg.c
> index 547e3bb..505b03d 100644
> --- a/src/util/user_info_msg.c
> +++ b/src/util/user_info_msg.c
> @@ -33,6 +33,7 @@ errno_t pack_user_info_chpass_error(TALLOC_CTX *mem_ctx,
>      uint32_t resp_type = SSS_PAM_USER_INFO_CHPASS_ERROR;
>      size_t err_len;
>      uint8_t *resp;
> +    size_t p;
>  
>      err_len = strlen(user_error_message);
>      *resp_len = 2 * sizeof(uint32_t) + err_len;
> @@ -42,9 +43,13 @@ errno_t pack_user_info_chpass_error(TALLOC_CTX *mem_ctx,
>          return ENOMEM;
>      }
>  
> -    memcpy(resp, &resp_type, sizeof(uint32_t));
> -    memcpy(resp + sizeof(uint32_t), &err_len, sizeof(uint32_t));
> -    memcpy(resp + 2 * sizeof(uint32_t), user_error_message, err_len);
> +    p = 0;
> +    SAFEALIGN_SET_UINT32(&resp[p], resp_type, &p);
> +    SAFEALIGN_SET_UINT32(&resp[p], err_len, &p);
> +    safealign_memcpy(&resp[p], user_error_message, err_len, &p);
> +    if (p != *resp_len) {
> +        DEBUG(0, ("Size mismatch\n"));
> +    }
>  
>      *_resp = resp;
>      return EOK;
> -- 
> 1.6.6.1
> 


> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://fedorahosted.org/mailman/listinfo/sssd-devel




More information about the sssd-devel mailing list