Re: [Otpasswd-talk] More observations

[Hannes Beinert]

On Tue, Jan 5, 2010 at 17:17, Tomasz bla Fortuna <address@hidden> wrote:
> 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!).

I saw that you had made that change in the repo already.  ;-)

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

Hrm.  Yes.  That's not very nice.  "-p=x" is a really unusual way to
specify an option value.  I realize that getopt needs to disambiguate
the command line, but...  dunno.

I think your last idea is best, honestly.  It's not very good security
policy to put such a high-value password on the command-line, anyway.
I should've thought about that when I first saw it, but I didn't think
of it.  Yes, I think that prompting for the password is best -- that's
what all the good security applications do, generally speaking (ssh,
gpg, passwd, etc).

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

Yes, it does.  The bottom line is that if an OTPasswd function
requires an spass and the user doesn't have it, then deny the function
if there's any doubt.  Cool.

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

Very true.

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

Also true.  And it's true that overriding functionality in the PAM
config could also be confusing to an admin.

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

No worries.  I was just going through the PAM documentation as I was
writing the man page, so these questions just came to my mind.  :-)

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

Yes, as I said in my previous email, it appears that possibly the
savannah server is a little uneven.  I'll cc you directly from now on.
 I should really have noticed that this was happening.  :-/

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

I know.  Again, no worries.  As the english idiom goes, I'm just
tossing it out there (the ideas), to see what sticks.  ;-)

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

In my life, I can barely cope with more than 0- or 1-dimensions.  Your
approach is probably pretty wise.  ;-)

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

After some reading, I think I have to revisit my earlier opinion.
pam_otpasswd is in business to authenticate in the auth stack, however
it really has nothing to contribute in the session stack.  I think I
now believe that pam_sm_open_session() should *also* return
PAM_IGNORE.  If the admin writes a stack in which pam_otpasswd is the
only module, then I think it probably *should* fail.

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

Agreed.

Currently, pam_otpasswd is configured that pam_sm_open_session()
returns PAM_SUCCESS, and pam_sm_close_session() returns PAM_IGNORE.
Would it make sense that pam_sm_open_session() *also* returns
PAM_IGNORE?  I'm thinking (now) that this is the correct approach.

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

No, it shouldn't.

(Although, I swear, my old SuSE 9.0 Linux-PAM did, I think, break with
PAM_IGNORE, but that may have been another issue.  Maybe even my
stupidity. :-)

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

Heh.  So I stumbled across that presumably just before it was fixed.  :-)

Yawn.  :-)

Hannes.