Re: [OATH-Toolkit-help] pam_oath and pam_unix' try_first_pass

[Simon Josefsson]

Christian Hesse <address@hidden> writes:

> Simon Josefsson <address@hidden> on Thu, 2012/05/31 10:40:
>> Christian Hesse <address@hidden> writes:
>> 
>> > Christian Hesse <address@hidden> on Wed, 2012/05/09 11:55:
>> >> Hello everybody,
>> >> 
>> >> This is from man pam_unix:
>> >> > try_first_pass
>> >> >            Before prompting the user for their password, the module
>> >> > first tries the previous stacked module's password in case that
>> >> > satisfies this module as well.
>> >> 
>> >> I think pam_oath has a similar option.
>> >> So I tested with these in my pam configuration:
>> >> 
>> >> auth sufficient pam_unix.so
>> >> -auth required pam_oath.so usersfile=/etc/users.oath digits=6
>> >> try_first_pass
>> >> 
>> >> and
>> >> 
>> >> -auth sufficient pam_oath.so usersfile=/etc/users.oath digits=6
>> >> auth required pam_unix.so try_first_pass
>> >> 
>> >> My understanding is that I get one prompt that accepts unix and oath
>> >> password. But obviously this does not work, authentication fails for the
>> >> line containing try_first_pass.
>> >> Anything I misunderstood? How is this expected to work?
>> >
>> > I did some debugging... Looks like pam_get_item() at line 161 writes a
>> > memory address to password that is not guaranteed to persist. The memory
>> > is overwritten or cleared in pam_set_item() at line 275, thus the
>> > following strncpy() does not copy anything useful.
>> >
>> > The attached patch fixes it for me, though I am not sure this is a 'clean'
>> > way...
>> 
>> Thanks for debugging!  Could you analyze a bit further why the patch
>> solves the problem -- maybe the password returned via
>> 
>>       retval = pam_get_item (pamh, PAM_AUTHTOK, (const void **) &password);
>> 
>> stop working later down in that function?  Could you pin-point where?
>> Maybe it goes away after this call?
>> 
>>       retval = pam_set_item (pamh, PAM_AUTHTOK, onlypasswd);
>>
>> That could make some sense.  The PAM library might re-allocate or use a
>> static buffer for the password, and this call overwrites the earlier
>> value.
>
> As said before... Yes, that is the problem. ;) See above, I wrote about line
> 275, that is the function you mentioned.

Heh, I guess I read code better than e-mails. :-)

>> Is there an easy way to reproduce the problem you discovered?
>
> This is perfectly reproducible with version 1.12.2, it works with my patch
> applied.
>
> Probably it's enough to just reorder the code so we do not have to allocate
> memory for password. New patch is attached, works for me as well.

I like this variant better.  Applied to master, thank you!

/Simon