PAM_SUCCESS returned for non-duo users instead of PAM_IGNORE

[st0rmi]

Summary

The pam_duo module returns PAM_SUCCESS when encountering a user that is not part of the configured groups. I believe it would be better to return PAM_IGNORE instead in these cases.

We are using pam_duo as the only authentication factor for using sudo (we use SSH keys for logon) and are therefore configuring pam_duo as sufficient. This, however, also allows all other users with sudo permissions to bypass password authentication, as PAM_SUCCESS is returned for them

Steps to reproduce

Authenticate as a user that is not configured for pam_duo.

Specs

Tested on Ubuntu 22.04 amd64, but should be the same across all OSes supported by pam_duo.

[AaronAtDuo]

@st0rmi Just to make sure I'm looking at the right place, you are talking about the return value at https://github.com/duosecurity/duo_unix/blob/master/pam_duo/pam_duo.c#L221? Where if the user doesn't match any groups we return PAM_SUCCESS?

[st0rmi]

@AaronAtDuo Yup, that's the line that I believe is the culprit. I am not 100% sure, but I believe a PAM_IGNORE might be better here.

[AaronAtDuo]

@st0rmi This seems like a reasonable idea. The tricky part is that if we release with this change, it could alter the behavior of existing PAM stacks, possibly even opening security vulnerabilities if people aren't expecting it.

Off the top of my head, the safest way to deal with that problem feels like we should keep the old behavior as default but allow (probably by config) an opt-in to the new behavior. I'm worried that hardly anyone will opt in to the new behavior though, somewhat undermining the effectiveness of updating it...

[st0rmi]

@AaronAtDuo That such a change might break existing PAM configurations was a concern I had as well. I think handling this behavior via config is probably the safest option. It might also be an option to take the "fail safe"/"fail secure" config that already exists for other behaviors into account.

[Xaraxia]

We do some shenanigans on our site to ensure we don't go through DUO for non-DUO users at all, specifically because of this problem. So +1 from here.

[st0rmi]

It could also be an option to initially make this a non-default configuration (i.e. user has to manually set this behavior in the config file) and then change it to be the default behavior during the next major version update.

I could go ahead and create a PR for this, unless you need some more time to discuss this change internally first. What are your thoughts on this?

[AaronAtDuo]

Interesting - looks like about 8 years ago we had the same thought, but reverted it
3eb3e35

[Xaraxia]

By the PAM semantics, however, the commit AaronAtDuo mentions above was correct prior to the change. I note this makes it difficult to manage with the DUO config instead of inside PAM (it's certainly possible to bypass based on group with PAM, because that's what I'm doing now, but I acknowledge it makes the config more complicated).

Can I second a config option, please? I understand why the default behaviour is designed to support your DUO configuration, but for those people who have multiple MFA stacks it matters. There are complex reasons why we have multiple MFA stacks and it's not an option to use DUO for certain users. At the moment I have to put all DUO users into a posix group and detect that group, otherwise bypass DUO.

The alternative to a config option is to change your documentation so that people who want to use the DUO config have to use [ ignore=ok ]. That should result in the same behaviour that you have now (as [success=ok] in a standard PAM stack). Then you can return ignore. This does require a config change on the client side and would need more communication of the change.