-----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 <sgall...@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. _______________________________________________ sssd-devel mailing list sssd-devel@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 <sgall...@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
0001-Allow-arbitrary-length-PAM-messages.patch.sig
Description: PGP signature
_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
