On Thu, Jun 27, 2013 at 01:32:59PM +0200, Ondrej Kos wrote:
> On 06/27/2013 01:17 PM, Pavel Březina wrote:
>> On 06/25/2013 02:55 PM, 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
>>>> --
>>>> Ondrej Kos
>>>> Associate Software Engineer
>>>> Identity Management - SSSD
>>>> Red Hat Czech
>>>
>>> >From 7f7e34e3d64b2b7f3c02225ed506791106c8d8a9 Mon Sep 17 00:00:00 2001
>>>> From: Ondrej Kos <okos(a)redhat.com>
>>>> Date: Mon, 24 Jun 2013 16:58:23 +0200
>>>> Subject: [PATCH] Do not try to set password when authtok_length is zero
>>>>
>>>>
https://fedorahosted.org/sssd/ticket/1814
>>>>
>>>> When the authtok_length is zero, it shouldn't call
>>>> sss_authtok_set_password, because it tries to determine lenght of passed
>>>> string by itself and would read parts of DBus message behind boundaries
>>>> of authtok.
>>>> ---
>>>> src/responder/pam/pamsrv_cmd.c | 8 ++++++--
>>>> 1 file changed, 6 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/src/responder/pam/pamsrv_cmd.c
>>>> b/src/responder/pam/pamsrv_cmd.c
>>>> index
>>>>
ff86a13a5ac13856a65d1618056caf4657cb473a..bf9a686230e0deb39f7387ed3f51c08f97575007
>>>> 100644
>>>> --- a/src/responder/pam/pamsrv_cmd.c
>>>> +++ b/src/responder/pam/pamsrv_cmd.c
>>>> @@ -65,8 +65,12 @@ static int extract_authtok_v2(TALLOC_CTX *mem_ctx,
>>>> struct sss_auth_token *tok,
>>>> sss_authtok_set_empty(tok);
>>>> break;
>>>> case SSS_AUTHTOK_TYPE_PASSWORD:
>>>> - ret = sss_authtok_set_password(tok, (const char
>>>> *)auth_token_data,
>>>> - auth_token_length);
>>>> + if (auth_token_length == 0) {
>>>> + sss_authtok_set_empty(tok);
>>>> + } else {
>>>> + ret = sss_authtok_set_password(tok, (const char
>>>> *)auth_token_data,
>>>> + auth_token_length);
>>>> + }
>>>> break;
>>>> default:
>>>> return EINVAL;
>>>> --
>>>> 1.8.1.4
>>>>
>>> I tried this patch and you did not reach krb5 authentication in
>>> krb5_child,
>>> but reason is:
>>> #>[krb5_auth_send] (0x0020): Wrong authtok type for user [usersssd01].
>>> #> Expected [1], got [0]
>>> So it failed in sssd_be
>>>
>>> I tried next patch:
>>> + if (auth_token_length == 0) {
>>> + ret = sss_authtok_set_password(tok, "", 0);
>>> + } else {
>>> + ret = sss_authtok_set_password(tok, (const char
>>> *)auth_token_data,
>>> + auth_token_length);
>>> + }
>>> and then it will fail in sssd_pam
>>> #> [pam_parse_in_data_v3] (0x0020): pam_parse_in_data_v2 failed,
>>> because sss_authtok_set_password could not be run with empty password.
>>>
>>> So there is question:
>>> where should we fail? (in sssd_be or sssd_pam)
>>
>> The question is: do we want to allow empty password for non krb
>> authentication? If yes, than we should fail in sssd_be. If no, we can
>> fail in sssd_pam.
>>
>> The current authtoken code suggest that we do not allow empty passwords
>> at all. See: sss_authtok_set_string().
>>
>> Does LDAP support bind with empty password?
>>
>
> LDAP with empty password results into anonymous bind, we shouldn't
> support empty passwords in any case.
yes, but in general I think the decision should be made in the backend.
The PAM modules should just send what it got (or not got).
bye,
Sumit