On Fri, Mar 11, 2016 at 01:36:52PM +0100, Pavel Březina wrote: > On 03/11/2016 10:41 AM, Sumit Bose wrote: > >On Thu, Mar 10, 2016 at 12:54:15PM +0100, Pavel Březina wrote: > >>On 03/08/2016 05:55 PM, Sumit Bose wrote: > >>>Hi, > >>> > >>>This patch fixes a 2FA issues observed with sudo. See commit message for > >>>details. > >>> > >>>bye, > >>>Sumit > >>> > >>> > >>>0001-pam_sss-reorder-pam_message-array.patch > >>> > >>> > >>> From 2c38adad7b527aceb4f9cb41c7d7b4c66d4580c9 Mon Sep 17 00:00:00 2001 > >>>From: Sumit Bose<[email protected]> > >>>Date: Mon, 7 Mar 2016 17:07:16 +0100 > >>>Subject: [PATCH] pam_sss: reorder pam_message array > >>> > >>>There are different expectations about how the pam_message array is > >>>organized, details can be found in the pam_start man page. E.g. sudo was > >>>not able to handle the Linux-PAM style but expected the Solaris PAM > >>>style. With this patch both styles should work as expected. > >> > >>I don't see any detail explaining this in the man page... could you > >>elaborate please? > > > >ah, sorry, it is not pam_start but the pam_conv man page: > > > >""" > >In passing, it is worth noting that there is a descrepency between the > >way Linux-PAM handles the const struct pam_message **msg conversation > >function argument from the way that Solaris' PAM (and derivitives, known > >to include HP/UX, are there others?) does. Linux-PAM interprets the msg > >argument as entirely equivalent to the following prototype const struct > >pam_message *msg[] (which, in spirit, is consistent with the commonly > >used prototypes for argv argument to the familiar main() function: char > >**argv; and char *argv[]). Said another way Linux-PAM interprets the msg > >argument as a pointer to an array of num_msg read only 'struct > >pam_message' pointers. Solaris' PAM implementation interprets this > >argument as a pointer to a pointer to an array of num_msg pam_message > >structures. Fortunately, perhaps, for most module/application developers > >when num_msg has a value of one these two definitions are entirely > >equivalent. Unfortunately, casually raising this number to two has led > >to unanticipated compatibility problems. > > > >For what its worth the two known module writer work-arounds for trying > >to maintain source level compatibility with both PAM implementations > >are: > > > > · never call the conversation function with num_msg greater > >than one. > > > > · set up msg as doubly referenced so both types of conversation > >function can find the messages. That is, make > > > > msg[n] = & (( *msg )[n]) > >""" > >> > >>>- mesg[0] = (const struct pam_message *) m1; > >>>- mesg[1] = (const struct pam_message *) m2; > >>>+ mesg[0] = (const struct pam_message *) m; > >>>+ mesg[1] = & (( *mesg )[1]); > >> > >>Is it possible to use &m[1] instead of this? > > > >I took this version to match the suggestion from the man page, but if > >you agree I'll add a comment why this somewhat odd notation is used. > > Even &m[1] works, but it may be better to stick with manpage suggestion. > Comment is welcomed though.
Please see attaches new version. I fixed the man page reference and added an expectation for the assignment. Additionally I removed the malloc() to underline even further that we have two arrays here. bye, Sumit > _______________________________________________ > sssd-devel mailing list > [email protected] > https://lists.fedorahosted.org/admin/lists/[email protected]
From f660c91b9dffa8c6a54290ea269103eda89b6437 Mon Sep 17 00:00:00 2001 From: Sumit Bose <[email protected]> Date: Mon, 7 Mar 2016 17:07:16 +0100 Subject: [PATCH] pam_sss: reorder pam_message array There are different expectations about how the pam_message array is organized, details can be found in the pam_conv man page. E.g. sudo was not able to handle the Linux-PAM style but expected the Solaris PAM style. With this patch both styles should work as expected. Resolves https://fedorahosted.org/sssd/ticket/2971 --- src/sss_client/pam_sss.c | 36 ++++++++++++++---------------------- 1 file changed, 14 insertions(+), 22 deletions(-) diff --git a/src/sss_client/pam_sss.c b/src/sss_client/pam_sss.c index b4f7efe49017870186f1cd9e91603033a5354770..5b2307c1b59e2de5d52fdc871b12afaa90780f76 100644 --- a/src/sss_client/pam_sss.c +++ b/src/sss_client/pam_sss.c @@ -1260,8 +1260,7 @@ static int prompt_2fa(pam_handle_t *pamh, struct pam_items *pi, int ret; const struct pam_conv *conv; const struct pam_message *mesg[2] = { NULL, NULL }; - struct pam_message *m1; - struct pam_message *m2; + struct pam_message m[2] = { {0}, {0} }; struct pam_response *resp = NULL; size_t needed_size; @@ -1270,29 +1269,22 @@ static int prompt_2fa(pam_handle_t *pamh, struct pam_items *pi, return ret; } - m1 = malloc(sizeof(struct pam_message)); - if (m1 == NULL) { - D(("Malloc failed.")); - return PAM_SYSTEM_ERR; - } + m[0].msg_style = PAM_PROMPT_ECHO_OFF; + m[0].msg = prompt_fa1; + m[1].msg_style = PAM_PROMPT_ECHO_OFF; + m[1].msg = prompt_fa2; - m2 = malloc(sizeof(struct pam_message)); - if (m2 == NULL) { - D(("Malloc failed.")); - free(m1); - return PAM_SYSTEM_ERR; - } - m1->msg_style = PAM_PROMPT_ECHO_OFF; - m1->msg = prompt_fa1; - m2->msg_style = PAM_PROMPT_ECHO_OFF; - m2->msg = prompt_fa2; - - mesg[0] = (const struct pam_message *) m1; - mesg[1] = (const struct pam_message *) m2; + mesg[0] = (const struct pam_message *) m; + /* The following assignment might look a bit odd but is recommended in the + * pam_conv man page to make sure that the second argument of the PAM + * conversation function can be interpreted in two different ways. + * Basically it is important that both the actual struct pam_message and + * the pointers to the struct pam_message are arrays. Since the assignment + * makes clear that mesg[] and (*mesg)[] are arrays it should be kept this + * way and not be replaced by other equivalent assignments. */ + mesg[1] = & (( *mesg )[1]); ret = conv->conv(2, mesg, &resp, conv->appdata_ptr); - free(m1); - free(m2); if (ret != PAM_SUCCESS) { D(("Conversation failure: %s.", pam_strerror(pamh, ret))); return ret; -- 2.1.0
_______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/admin/lists/[email protected]
