[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