[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: PAM concepts (was: pam_{unix,pwdb}: crypt/md5 necessary?)

On Thu, 3 Aug 2000, Michael Tokarev wrote:

> pam_{unix,pwdb} modules.  Some questions was popped up when I liiked to
> them closely and remember a history.  The simplest question is --
> Why two of them with more-or-less similar functionality?  Unixes
> now have some sort of nsswitch -- so -- why the pwdb just comes in
> not utilizing the features of plain getpwnam() & Co!?  The idea was
> so simple -- use existing interface, that's all... Yes, there are
> some systems that have no nsswitch magic, maybe this was issue?
> But still pwdb and unix is just a duplication of efforts from my
> point of view. I can be wrong.

Work on pam_pwdb started before glibc was widely used on Linux (and Linux 
libc5 didn't have nsswitch), and for a while, pam_pwdb was better-maintained
and more feature-rich than pam_unix.  The situation has changed now: pam_pwdb
has been almost completely abandoned.  RedHat still uses it as their default,
but they're also talking about switching to pam_unix in the future.  When they
do, pam_unix will be the de facto authoritative module.

> Okay, let's see pam_unix.  It uses generic getpwnam/getspnam interface,
> and it have no knowledge about actual source of information this routines
> returns (this stated in comments there).  So -- and this is conceptual
> question -- why it should deal with password stack at all?  It will be
> unusable if nsswitch will point to ldap for example...  Why not just
> drop password stuff from it and implement some little modules for each
> king of real data storage (passwd+shadow, nis, ldap, etc), and _only_
> password, leaving other (account, auth, session) basic things to pam_unix?

I think it supports it because it was convenient at the time.  pam_unix can
already do updates to the file and NIS back-ends, and I think it would be nice
if it could also handle NIS+ (the SuSE pam_unix may already do this?).  This
also makes things more convenient for the administrator, who can change a
system from flatfile authentication to NIS just by changing /etc/nsswitch.conf
(no need to mess with the PAM configuration).

> It will do pretty well here, not requiring to learn about how to _store_
> passwords.  And each little password-only module should only _store_
> password, not do other things like auth etc.  Ths scheme will be true
> modular, and each module will implement minimum functionality, without
> any duplications (with nsswitch, one should write one nsswitch module
> and one little "set-password" module, not the whole pam stuff).

Is this how the pam_ldap module works?  I don't know if it has an 'auth' mode
as well (perhaps there's a more secure way to authenticate against LDAP than
by sending the password with getpwnam()).

> Ok, let's see next thing.  Asking new password.  Why pam_unix, pam_pwdb
> attempted to ask new password in their passwd routines?  Why in this
> case pam_cracklib?  Remember the previous statement -- many little
> "storing" modules? -- in this respect, there should be definitely one
> "asking" module like cracklib, and "storing" modules should just store
> PAM_AUTHTOK, not asking one...  Cracklib does a good job in enshuring
> password quality, and there is no need to duplicate this many times.

Some people don't want to use pam_cracklib on their machines, and they
shouldn't be required to do so.  If pam_cracklib is /required/, then you don't
really have modularity, do you?  Hence, all pam password modules are able to
prompt for the new password, so that they can be stacked however the
administrator chooses.

In the past, I've found pam_cracklib to be very flaky.  The messages it passes
back are sometimes ambiguous, and not terribly helpful to users.  And when the
users have problems, they go to the administrators... :)  Password
strength checking is nice in theory, but I wouldn't want to try pam_cracklib
at an ISP with 10,000 users and 20 employees. ;)

There are also persistent problems with cracklib under RedHat, because
cracklib itself has one dictionary path compiled in, and RedHat installs the
dictionaries to another.

I think that modularity, in this case, means writing modules which are wholly
independent of one another.  PAM modules should interoperate nicely, but no
PAM module should depend on other modules to do its job.

> Pam_unix_auth.c.  Look to define AUTH_RETURN and it's usage.  It will far,
> far easy to split pam_sm_authenticate() to two parts, one static that does
> the real work and just "return retval" in error cases (without AUTH_RETURN),
> and one pam_sm_authenticate itself that calls that static routine and does
> things in AUTH_RETURN only once.

> This will clean things a lot since we will have no hidden side-effects in
> macros. Thus, less choice to do a mistake...

Have you found hidden side-effects in this code?

> Let's see how many times pam_unix does shadow (spwd) "retrieving".

A few of these calls are there because of the code's heritage (e.g.,
_unix_blankpasswd() in support.c, which is designed rather inefficiently).
Cleaning this up really hasn't been a priority for me, but I'm sure that if
you cleaned the _unix_blankpasswd() function up to behave more sanely, your
patch would be welcomed.

Most of the other getspnam() calls are necessary.  getspnam() is not called
too many times, in light of what the code does.

> This is a big pease of code with a lot of set[reug]id's (that logic I can't
> even understand...)

The calls are necessary because glibc's NIS+ nsswitch module won't let a
program see the password field for any user other than the one which matches
the program's euid.  Because we don't know what permissions the program will
be running with when pam_unix is invoked, we have to do a complex dance with
the uid's to make sure that they are put back exactly the way they were when
we're done.  At the time I wrote this code, I did formally prove that it did
the right thing.  But please don't ask me to explain it to you now :)

> And it is repeated at least 8 times!  Why!?  Why this hard
> code is not reused across one module!?  In the other side, there is "shared"
> routines (like isblankpas) in support.c that are used only once, and should
> be in the module that uses them only, and declared as static!...

_unix_blankpasswd() is used in both pam_unix_auth.c and pam_unix_passwd.c.

> Look also to pam_unix_passwd.c.  Salt manipulation is a great place!..
> I see two macros:

>   #define ascii_to_bin(c) ((c)>='a'?(c-59):(c)>='A'?((c)-53):(c)-'.')
>   #define bin_to_ascii(c) ((c)>=38?((c)-38+'a'):(c)>=12?((c)-12+'A'):(c)+'.')

> and a big crappy routine:

>   static int i64c(int i)
>   {
>         if (i < 0)
>                 return ('.');
>         else if (i > 63)
>                 return ('z');
>         if (i == 0)
>                 return ('.');
>         if (i == 1)
>                 return ('/');
>         if (i >= 2 && i <= 11)
>                 return ('0' - 2 + i);
>         if (i >= 12 && i <= 37)
>                 return ('A' - 12 + i);
>         if (i >= 38 && i <= 63)
>                 return ('a' - 38 + i);
>         return ('\0');
>  }

> The i64c is just a ugly way to implement the same thing as bin_to_ascii()
> does, isn't it?

Different assumptions in the way things are done.  bin_to_ascii() assumes that
the input is between 0 and 63; it also assumes that the output should be in
the ascii character set.  If it's safe to make those assumptions, then it's
safe to remove i64c() from the code.

Personally, I would say that the macro is uglier than the function, because I
find the function easier to read.

> At this point I have another question.  After some more nights, I will have
> new module (call it pam_unix3, if you like) that is not a pam_unix but does
> exactly the same (and plus some more, and maybe minus password changing
> stuff).  This will _not_ be a patch, since it is basically a complete
> rewrite.  So, -- what to do with this!? :)

> I can't just patch pam_unix, as I can't patch pam_cracklib (the same
> situation)...

> And, -- yes, this new module will have _no_ security history at all unlike
> current pam_unix and pam_pwdb that already in production.

pam_unix is a fairly mature module now.  By writing from scratch, you also
lose that, and it becomes difficult to see if your module is always doing the
Right Thing.

I think we would benefit more by just cleaning up the pam_unix we have, if
there are problems with it.  It's already used by a great number of people,
and we know it works well.  Breaking with that history means going through the
beta testing phase all over again.

Steve Langasek
postmodern programmer

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index] []