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

[no subject]

But: if I looks to some more-or-less "big" modules, I see so many
strange things...  I just don't understand: why people written this
stuff this way.  It is _very_ hard to support/enhance/change/debug
some existing modules...  I fell sometimes that this is just badly
written code, without any (good) concepts at basis. Ok, this need
explanation, and it is rather big.  Let's try to explain.

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.

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?
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).

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.

Again, "password asking".  One good thing that can do pam_unix is to
enshure that new password is not the same as one of previous ones
(remember=XXX).  Really good thing.  But -- where those old passwords
should be stored?  This is not a trivial question. All are ok while
your usere are on only one machine, -- /etc/security/opasswd is good.
But if you have network with nis/ldap/etc, and user can change password
on any machine?  What one should do when account should be deleted?
And so on...  This question is "hardest" in this set of Qs, since I have
no real solution to it using current pam abilities.  One possible solution
is to have "loop" in pam config file.  Loop while user entered valid new
password.  When he enter some password, first check if it is ok (of good
quality, pam_cracklib or something).  If yes, give it to some "storage"
module and ask if this password already was entered.  Module can answer
"yes" (user have that pass before), "no" (user have no such pass before)
and "dunno" ("i have no info about that user").  In the last case, pam
should look to next "storing" module available and ask the same question.
(the same approach should be used in actual password storing time, but
a bit different: try to enumerate modules (files, nis, ldap, ...) while
some says "I know about the user").  If no module says that password
already has been here, then prompt user to confirm and voila, else
start again (retries).  That loop is not possible currently, and pam_cracklib
can't deal with ldap etc.

Ok, enouth for "concepts".  Let's summarize things.

Should be one pam_unix module _without_ password functions.  Should be many
small password modules (pam_passwd_files, pam_passwd_ldap etc, some can be
combined) that should only store supplied in PAM_AUTHTOK (?) password.
Them should (?) encrypt it (pam_passwd_smbpasswd should use it's own "encryption"
routines).  Should be one good "asking" module like pam_cracklib.
And should be some mechanism to implement storing of old passwords using
that "storing" modules like pam_passwd_files etc.

Is that sounds good?

And finally I want to explain my statement at beginning.  Some explanations already
here -- in previous paragraphs.  I mean that each module was designed without
thinking about cooperation and reuse of efforts.  But there is another issue exists.
When I looked to pam_cracklib (that I tried to change a bit) I found tons of small
bugs (mostly leaks, almost nowhere checked returns of malloc, etc).  And those
bugs as I can understand comes from just badly written code -- it is just ugly.
Examples are many.  I will show pam_unix here as an example, as I don't remember
good ones in cracklib module, but pam_unix is new to my mind.

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...

Let's see how many times pam_unix does shadow (spwd) "retrieving".  This is a big
pease of code with a lot of set[reug]id's (that logic I can't even understand...)
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!...

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?

Compare also salt generation in 'crypt' and 'md5' ways...

Also, count how many times getpnam/getspnam are called.  I'll not be sirprized if
some days later pam_unix will not have same problems as pam_pwdb with scalability.
Currently we have no problem with this (well, almost no), but due to good
implementation of getpwnam/etc in glibc and ability to cache with nscd.
But getspXXX should not be cached!

Uhhhhh.....  This list can be continued, but I'll stop here.

I tried to change things a bit (crypt/md5 only), but fell that I should do a cleanup
before.  I tried to do cleanup, and now I have only one file (pam_unix.c) that is about
35Kb long (combined pam_unix_*.c and support.c).  It is only preliminary version,
not good to be shared with someone; it was made during only one night; and it was made
with cleaness and accuracy in mind.

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.

Waaay!..  The last note.  I do not want to blame someone.  I want a good working
software, I can do something myself.  But I want to understand also...

Comments, pleeeease!....   :)


P.S. Excuse me for my not-so-good English.

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