On 07/17/2013 06:47 PM, Jakub Hrozek wrote:
>On Wed, Jul 17, 2013 at 02:04:16PM +0200, Ondrej Kos wrote:
>>On 07/16/2013 08:10 PM, Jakub Hrozek wrote:
>>>On Tue, Jul 09, 2013 at 11:15:09PM +0200, Jakub Hrozek wrote:
>>>>On Fri, Jun 28, 2013 at 02:23:01PM +0200, Lukas Slebodnik wrote:
>>>>>On (24/06/13 17:04), Ondrej Kos wrote:
>>>>>>The problem here wasn't in returned error code, but in
faultly read
>>>>>>DBUS message, due to condition in sss_authtok_set_string.
>>>>>>
>>>>>>When password is empty, it passes 0 as length, which is
>>>>>>misinterpreted, and the function tries to determine the length
of
>>>>>>string by itself, reaching over boundaries of authtok string.
>>>>>>
>>>>>>trac issue:
https://fedorahosted.org/sssd/ticket/1814
>>>>>>
>>>>>>Patch is attached
>>>>>>
>>>>>>Ondra
>>>>>
>>>>>We found out that it is right approach (after long discussion :-)
>>>>>
>>>>>ACK
>>>>
>>>>The approach might be good but on entering a blank password I'm
getting
>>>>"System Error" with this patch, while without the patch I was
getting
>>>>"Authentication failure". PAM_AUTH_ERR is the correct return
code for
>>>>this use case.
>>>
>>>Hi,
>>>
>>>Ondra asked me to re-test atop the current git HEAD. Here is what I see
>>>when I type "su - jhrozek" and then just type "Enter":
>>>
>>>[sssd[be[redhat.com]]] [be_pam_handler] (0x0100): Got request with the
following data
>>>[sssd[be[redhat.com]]] [pam_print_data] (0x0100): command: PAM_AUTHENTICATE
>>>[sssd[be[redhat.com]]] [pam_print_data] (0x0100): domain:
redhat.com
>>>[sssd[be[redhat.com]]] [pam_print_data] (0x0100): user: jhrozek
>>>[sssd[be[redhat.com]]] [pam_print_data] (0x0100): service: su-l
>>>[sssd[be[redhat.com]]] [pam_print_data] (0x0100): tty: pts/20
>>>[sssd[be[redhat.com]]] [pam_print_data] (0x0100): ruser: jhrozek
>>>[sssd[be[redhat.com]]] [pam_print_data] (0x0100): rhost:
>>>[sssd[be[redhat.com]]] [pam_print_data] (0x0100): authtok type: 0
>>>[sssd[be[redhat.com]]] [pam_print_data] (0x0100): newauthtok type: 0
>>>[sssd[be[redhat.com]]] [pam_print_data] (0x0100): priv: 0
>>>[sssd[be[redhat.com]]] [pam_print_data] (0x0100): cli_pid: 22305
>>>[sssd[be[redhat.com]]] [krb5_pam_handler] (0x1000): Wait queue of user
[jhrozek] is empty, running request immediately.
>>>[sssd[be[redhat.com]]] [krb5_auth_send] (0x0020): Wrong authtok type for user
[jhrozek]. Expected [1], got [0]
>>>[sssd[be[redhat.com]]] [check_wait_queue] (0x1000): Wait queue for user
[jhrozek] is empty.
>>>[sssd[be[redhat.com]]] [be_pam_handler_callback] (0x0100): Backend returned:
(0, 4, <NULL>) [Success]
>>>[sssd[be[redhat.com]]] [be_pam_handler_callback] (0x0100): Sending result
[
4][redhat.com]
>>>[sssd[be[redhat.com]]] [be_pam_handler_callback] (0x0100): Sent result
[
4][redhat.com]
>>>
>>>
>>>4 == System Error
>>>_______________________________________________
>>>sssd-devel mailing list
>>>sssd-devel(a)lists.fedorahosted.org
>>>https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>>>
>>
>>Hi,
>>
>>Now I see what you meant, new patches rebased on top of current master are
>>attached.
>>
>>The third patch replaces mutiple calls of sss_authtok_get_type with variable
>>and single call, just a small refactoring.
>>
>>Ondra
>>
>>--
>>Ondrej Kos
>>Associate Software Engineer
>>Identity Management - SSSD
>>Red Hat Czech
>
>[PATCH 1/3] Do not try to set password when authtok_length is zero
>Ack
>
>> From 3968df17f835a8dac892b3756f2a4f35a86a0baa Mon Sep 17 00:00:00 2001
>>From: Ondrej Kos <okos(a)redhat.com>
>>Date: Wed, 17 Jul 2013 13:42:57 +0200
>>Subject: [PATCH 2/3] KRB: Handle empty password gracefully
>>
>>https://fedorahosted.org/sssd/ticket/1814
>>
>>Return authentication error when empty password is passed.
>>---
>> src/providers/krb5/krb5_auth.c | 13 +++++++++++++
>> 1 file changed, 13 insertions(+)
>>
>>diff --git a/src/providers/krb5/krb5_auth.c b/src/providers/krb5/krb5_auth.c
>>index
22495f57079d979b6c3484acf916ecc0736292d1..5b4d4abb6c9f97cac394853d73eb4e5f8b045c44 100644
>>--- a/src/providers/krb5/krb5_auth.c
>>+++ b/src/providers/krb5/krb5_auth.c
>>@@ -495,6 +495,19 @@ struct tevent_req *krb5_auth_send(TALLOC_CTX *mem_ctx,
>> case SSS_PAM_AUTHENTICATE:
>> case SSS_PAM_CHAUTHTOK:
>> if (sss_authtok_get_type(pd->authtok) !=
SSS_AUTHTOK_TYPE_PASSWORD) {
>>+ /* handle empty password gracefully */
>>+ if (sss_authtok_get_type(pd->authtok) ==
SSS_AUTHTOK_TYPE_EMPTY &&
>>+ sss_authtok_get_size(pd->authtok) == 0) {
>
>The second part is not needed, empty authtok cannot have size != 0 by
>definiton.
>
>>+
>>+ DEBUG(SSSDBG_CRIT_FAILURE,
>>+ ("Illegal zero-length authtok for user
[%s]\n",
>>+ pd->user));
>>+ state->pam_status = PAM_AUTH_ERR;
>>+ state->dp_err = DP_ERR_OK;
>>+ ret = EOK;
>>+ goto done;
>>+ }
>>+
>> DEBUG(SSSDBG_CRIT_FAILURE,
>> ("Wrong authtok type for user [%s]. " \
>> "Expected [%d], got [%d]\n", pd->user,
>>--
>>1.8.1.4
>>
>
>> From e9f44ae4e487b0cddfb676c7f52bcef9780c4393 Mon Sep 17 00:00:00 2001
>>From: Ondrej Kos <okos(a)redhat.com>
>>Date: Wed, 17 Jul 2013 13:58:36 +0200
>>Subject: [PATCH 3/3] KRB: Replace multiple calls with variable
>
>Meh, the compiler would optimize the call anyway :-) But in general I'm
>not opposed.
>_______________________________________________
>sssd-devel mailing list
>sssd-devel(a)lists.fedorahosted.org
>https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
Resending all three patches, modified and rebased.
Ondra