Re: pam_oath with user_unknown=ignore

[Dirk van Deun]

On Thu, Mar 07, 2024 at 03:41:11PM +0100, Simon Josefsson wrote:
> Dirk van Deun <dvandeun@wilma.vub.ac.be> writes:
> 
> > On Wed, Mar 06, 2024 at 09:11:54PM +0100, Simon Josefsson wrote:
> >> Dirk van Deun <dvandeun@wilma.vub.ac.be> writes:
> >> 
> >> > Hi,
> >> >
> >> > I really like the fact that you can use user_unknown=ignore to
> >> > introduce pam_oath gradually, and it works fine if you use one users
> >> > file to store all the secrets; but when you use a file per user 
> >> > (like with usersfile=/oath/${USER}), users that do not have a secret
> >> > yet do need an empty file for PAM_USER_UNKNOWN to be returned;
> >> > without, you get a file-not-found kind of error instead.
> >> >
> >> > It is debatable whether this is a bug or just unexpected behaviour,
> >> > but to me it would make more sense if in such a configuration, a
> >> > missing file would also lead to PAM_USER_UNKNOWN.  
> >> 
> >> Yes this makes somewhat sense.  The patch to get your behaviour would
> >> probably be something like this:
> >> 
> >> diff --git a/pam_oath/pam_oath.c b/pam_oath/pam_oath.c
> >> index 25eb83e..f5ae490 100644
> >> --- a/pam_oath/pam_oath.c
> >> +++ b/pam_oath/pam_oath.c
> >> @@ -303,7 +303,7 @@ pam_sm_authenticate (pam_handle_t *pamh,
> >>      DBG (("authenticate first pass rc %d (%s: %s) last otp %s", rc,
> >>      oath_strerror_name (rc) ? oath_strerror_name (rc) : "UNKNOWN",
> >>      oath_strerror (rc), ctime (&last_otp)));
> >> -    if (rc == OATH_UNKNOWN_USER)
> >> +    if (rc == OATH_NO_SUCH_FILE || rc == OATH_UNKNOWN_USER)
> >>        {
> >>    retval = PAM_USER_UNKNOWN;
> >>    goto done;
> >> 
> >> Although since (IIRC) 'user_unknown=ignore' is handled by the PAM layer
> >> and not the pam_oath module, I worry that if we start to return
> >> PAM_USER_UNKNOWN on file-not-found errors, we will cause unwanted
> >> behaviour in other situations.
> >> 
> >> One ugly way to resolve this is to introduce a new keyword that cause
> >> this particular behaviour to happen, for example
> >> 'missing_usersfile_leads_to_user_unknown' causes OATH_NO_SUCH_FILE
> >> errors to be translated into PAM_USER_UNKNOWN.  What do you think?
> >> Would you like to make an attempt to make a patch?
> >> 
> >> /Simon
> >
> > We can try to do the right thing based on the usersfile parameter:
> >
> > - if usersfile does not contain a variable, then OATH_NO_SUCH_FILE
> > is still an error;
> >
> > - if usersfile starts with ${HOME}, OATH_NO_SUCH_FILE gives rise to
> > PAM_USER_UNKNOWN, except if the home directory in question does
> > not exist, as then it is safer to assume that something went wrong
> > (like homes not being mounted) and keep calling it an error;
> >
> > - if usersfile starts with a literal path that ends with a slash,
> > followed by a variable (presumably ${USER}), followed by whatever,
> > OATH_NO_SUCH_FILE gives rise to PAM_USER_UNKNOWN, except if the
> > literal path part does not exist OR is an empty directory -- the
> > is-empty test can be added because we can reasonably assume that at
> > least one user on the system has configured oath; 
> >
> > - other cases are just weird, so the error may stay an error.
> >
> > Agree ?
> 
> This sounds complicated to me since it assumes even further semantic
> understanding of the usersfile path value.  Understanding the difference
> between a missing and unreadable home directory can be difficult for
> network file systems.  Right now the error is returned on all fopen()
> failures, which includes a number of different error conditions.  It
> isn't clear to me that there aren't reasonable scenarios where a missing
> usersfile should NOT lead PAM_USER_UNKNOWN errors, but a hard error:
> maybe someone is using the current semantics and has a hierarchy of
> usersfile for users, and expect the current behaviour.
> 
> It still seems simpler to introduce a new keyword that triggers
> PAM_USER_UNKNOWN for OATH_NO_SUCH_FILE usersfile error.  Then this can
> be documented as a new supported feature.
> 
> /Simon

It's a good point that administrators may be relying on the current
extremely strict behaviour, so any new behaviour should have to be
turned on explicitly.

But I do not agree that this "relaxed" mode should just turn every
OATH_NO_SUCH_FILE into PAM_USER_UNKNOWN.  An advantage of the
current strict mode is that default behaviour is denying a login if
the usersfile cannot be opened for whatever reason: the file may
not exist, but it may also be on an unmounted partition, or the disk
may simply be broken: the conclusion is always the same: no logins
allowed.  That is quite secure.

In principle a relaxed mode can never be as secure, but in practice
we can approximate this level of security with a very simple
heuristic: if we cannot open the usersfile, but we can open the
directory in which it should be located, everything looks fine; but if
we cannot open the directory either, there might be some trouble, so
we better take no risks and deny the login.

I may have dived into details and refinements too soon in my previous
mail, and not made this point well.  Also please interpret "the
directory does not exist" in my previous mail as "cannot be accessed
for whatever reason".

Dirk van Deun
-- 
Ceterum censeo Redmond delendum