[SECURITY][PATCH] PAM change reverses test for valid password

[Andrew Bartlett]

Jeremy Allison wrote:
> 
> On Tue, May 01, 2001 at 06:40:26PM +1000, Andrew Bartlett wrote:
> > The change to the PAM setup to use NT_STATUS constants had an
> > unfortunate side-effect - it reversed the check for a password being
> > valid, ie WRONG passwords are considered correct.
> >
> > This affects both plaintext logins and SWAT.  My patch (on which the
> > changes were based) included changes that made this not entirely obvious
> > when the patch was not applied in full.  (I'll be more careful about my
> > comments in future).
> >
> > The error was introduced into the CVS tree at Mon Apr 30 20:37:44 2001
> > UTC
> >
> > This patch also fixes some bugs introduced in the PAM changes that made
> > correct plain-text authentication impossible.
> >
> 
> Thanks Andrew for the tidyups, I've committed them in 2.2
> and HEAD. What did you think of the other changes to the
> patch ? I removed the use of the global variables, and
> added the userdata pointer stuff in the dynamic pwconv
> code instead. I also changed the style of :
> 
> if (do_something == success)
>                 if (do_something_else == success)
>                                 if (do_another_thing == success)
>                                                 return success;
> return fail;
> 
> To the (much preferable in Tridge's and my opinion) style
> of :
> 
> if (do_something != success)
>                 return Fail;
> if (do_something_else != success)
>                 return Fail;
> if (do_another_thing != success)
>                 return Fail;
> return success;
> 
> As this style of code is *much* easier to understand.
> 
> Jeremy.
> 

I like it.  I never liked the global variable stuff (just never liked
global variables), but did not know how to fix it.

You should also watch out with the NT_STATUS stuff, if I understand my C
correctly False == 0 == NT_STATUS_NOPROBLEMO, ie access permitted.  As
such a 'return False' can be rather drastic.  

I am also thinking about reorginising the big case statements into one
function - ie have a return of every PAM constant, as found in the PAM
headers.  This way we cover all our bases and don't just trust that
modules will only return error codes listed in the PAM web-page for that
particular activity...  (and we know how accurate the PAM web-site is
:-)

You should also note my NT_STATUS constant selection procedure was not
exactly scientific:  I have a 7 page printout of nterr.h, and just made
my selections...  (I do know however that the account expired codes
work, and most of them make sense.)

I understand the rejection of the rest of the (original) patch, ie too
many changes that can't be tested without 50 recompiles - but it did add
one feature:  It prevented us running our mini password-cracker agaist
PAM when accounts were disabled.

Andrew Bartlett
abartlet at pcug.org.au
-- 
Andrew Bartlett
abartlet at pcug.org.au