Re: [Otpasswd-talk] More observations

[Tomasz bla Fortuna]

Dnia Mon, 4 Jan 2010 21:25:06 -0600
Hannes Beinert <address@hidden> napisał(a):

> A few more observations:
> 
> 1. What would you think about moving the --flag list functionality to
> a primary command-line option?  It seems to me that (-l, --list, or
> perhaps --info, -i) would be more intuitive for a user to obtain
> information about his user state.  The "list" functionality isn't
> really a "flag", per se.

Yeah, I already had this idea but couldn't locate a letter for it (-l
is taken by latex, --show by skip. But info seems great!).
> 
> 2. When changing a password with the -p option, I can see why it is
> considered an error when no argument is supplied, however instead of
> spitting out the entire help text, what about a short usage or simple
> error?

I made argument optional. But this is queer as it now requires equal
sign. 
-p <- will unset password
-p="bleh" <- will set password (same with long version)
-p bleh <- this is an error which is not nice as it doesn't work good
with other options. 

I think I'll prompt user for password finally just like passwd does.
With second question and correction checking.

> 
> 3. When a password is successfully changed, the user state is
> (partially) displayed.  How about only doing this if the -v option is
> specified?  I kind of like the interface to the passwd command -- just
> do it, and go away.  :-)

True. Same applies to flags. Maybe not with -v but -i combined with -f.
For now, it doesn't matter.

> 
> 4. How does the code handle the situation where there is no static
> password?  Is there now no password on all that functionality, or is
> that functionality no longer accessible to the user?

Authentication using static password is not yet written. I thought like
this:
- If feature requires spass (OOB request) and user doesn't have it -
  forbid.
- If logon requires spass and user doesn't have it - forbid.
- If policy allows but not enforces logons with spass and user doesn't
  have it ignore.

I'm not sure if it answers your question.
 
> 5. I think there *might* be some value in having an
> enforce/noenforce-type option available for the pam_otpasswd module.
> I realize this can be specified as a policy, however I could see that
> a system admin might feel good about having it hard-coded in the PAM
> config so that this option could not be changed by a compromised or
> corrupted configuration. 

I guess this should be fixed better. Configuration in first place
should not be compromised and I'm not writting the code while assuming
it can be changed by attacker. What's more - if configuration can be
compromised then in currently assumed mode (suid to otpasswd) also
utility executable can be compromised and that's not funny.

There's mode of operation I'm thinking about in which configuration and
executable would be owned by root, not readable by others and safe.

> My thought is also that if a "noenforce"
> override is applied to the pam_otpasswd line, then it could almost be
> included in PAM configs by default.  No harm would be done if the
> system were not completely configured, for example -- it would just
> by-pass any auth request.

Hm. I'm currently careful about expanding pam module options. This
duplicates kind of program functionality and user might later change
something in config and wonder why it doesn't work - voillá it was
enforced in pam config. Assumptions that config file might be changed
by attacker is dangerous because of many other not-well-though reasons.

> 
> 6. The PAM developer's manual indicates that modules should accept the
> generic options 'debug' and 'use_first_pass', and potentially silently
> ignore them.  I think pam_helpers.c will complain about the latter
> option.
> http://www.kernel.org/pub/linux/libs/pam/Linux-PAM-html/mwg-see-options.html
debug and silent is accepted for this reason. I didn't know that
use_first_pass is required but current version will ignore it (except
for warning in syslog maybe).

> 7. The pam_otpasswd module has a debug logging mode, which presumably
> is aimed at you (the developer :-).  Would there be any reason to add
> an "audit" option which is aimed at the sysadmin to provide a little
> more day-to-day logging?  (As it happens, I appear to already have
> documented this option :-)

There's some strange delay in this messages. If you won't get message
from me with info about this option being already implemented shout
aloud. ;-) (Same applies to point 1. I've written this somewhere but
can't find it).


> 
> 8. When pam_otpasswd doesn't recognize an option, it returns a
> PAM_AUTH_ERR.  I'm not sure that this is the best approach, since any
> minor misconfiguration could lock-out any authentications using the
> module.  I believe that the typical PAM behavior is to log the option
> error, and then just continue processing.

Fixed.
 
> 9. It wasn't entirely clear to me (I'm just in the process of looking
> over the module) -- are the suggested system logging levels being
> observed, as per
> http://www.kernel.org/pub/linux/libs/pam/Linux-PAM-html/mwg-see-programming-syslog.html

I've read this. OTPasswd directs more info to the user unless silent
option is passed (in which case auth will act exactly as described
there).

More information can be usually printed as we require pam_unix auth
first.

Point 1) moved info about invalid parameter back to LOG_ERR from WARN.

Implementing 2), 3), 4) can be done when we'll try to make this log
system more robust. This should be taken into account but is not
critical.

5) Can easily be done. But...

You described two dimensional logging. Keeping in mind those things
might require three-dimentional logging which would clearly be
an overshoot. I'd really like to keep log system small, clean, simple,
BUT functional. Currently I'm rather after one-dimensional approach but
much more fine-grained. 

Possibly with some additional method of determining message header
(ERROR:, WARN: etc.) where it's appropriate (say, message must be shown
when user passes audit/sets LOGGING to '2', but is not a warning nor an
error so give it 'INFO' label or no label at all).

> ?
> 
> 10. You return PAM_IGNORE when pam_sm_close_session() is called -- I'm
> never sure if it should be PAM_IGNORE or PAM_SUCCESS.  What are your
> thoughts on this?

Consider example session stacks:
otpasswd required
ignore will fail, success will succeed. I guess this should never be
used as a single session entry.

otpasswd sufficient
then PAM_SUCCESS would cause success no matter what other session
entries will return. PAM_IGNORE is safer here as it is less invasive.

It's hardly a practical question really. PAM_IGNORE won't break any
system I guess. ;-)

> 
> 11. In pam_helpers.c, I'm unclear what PAM return code is given when
> the user is not known to OTPasswd.  Specifically, the code is roughly:
> 
>         /* We must know the user of whom we must find state data */
>         retval = pam_get_user(pamh, &user, NULL);
>         :
>         /* Initialize state with given username */
>         if (ppp_init(s, user) != 0) {
>                 /* This will fail if we're unable to locate home
> directory */ goto error;
>         }
>         :
> error:
>         print_fini();
>         return retval;
> }
>
> It almost looks to me as though whem pam_get_user() returns
> PAM_SUCCESS, and ppp_init() fails, then the function returns
> PAM_SUCCESS?  What am I missing?
I guess I've fixed this in the meantime. Currently this code looks like
this:

        retval = pam_get_user(pamh, &user, NULL);
        if (retval != PAM_SUCCESS && user) {
                print(PRINT_ERROR, "pam_get_user %s", 
pam_strerror(pamh,retval));
                retval = PAM_USER_UNKNOWN;
                goto error;
        }
...
        /* Initialize state with given username */
        retval = ppp_init(s, user); 
        if (retval != 0) {
                /* This will fail if, for example, we're 
                 * unable to locate home directory */
                retval = PAM_USER_UNKNOWN; // here it is!
                goto error;
        }

From git annotate, this seems to be a change from yesterday (this line)

> 
> 12. I noticed that in the man page for pam_unix(8), they discuss
> SIGCHLD handling.  Is this anything that pam_otpasswd should be
> worried about?  It sounds like they ran into trouble with applications
> receiving the SIGCHLD after the helper binary quit, and the signal
> handler for the application that called PAM freaked out.

It might be required before running OOB script, but otherwise for sure
 not. Added to Changelog/TODO.



> Alright.  That's it for now.  :-)
> 
> Hannes.
> 
> 


-- 
Tomasz bla Fortuna
jid: bla(at)af.gliwice.pl
pgp: 0x90746E79 @ pgp.mit.edu
www: http://bla.thera.be

signature.asc
Description: PGP signature