Feature request: behavior in situation of missing conf file and not member of groups directive

[haoshu]

Hi,

Can I suggest some change on behavior of login_duo and pam_duo?

Summary

1. Both login_duo and pam_duo will proceed ahead if the specified conf file doesn't exist. Can we follow the fail-secure principle and change the behavior to abort further processing?
2. If the user is not a member of the groups specified in conf file, pam_duo module will automatically return success code, which cause issue when using PAM control flag sufficient. Can we add another directive in pam_duo conf file to determine the behavior: either success(pass) or failure(skip) for those not a member of the group?

Steps to reproduce

1. Accidentally specify a missing conf file and monitor the behavior of login_duo and pam_duo.

• login_duo: login_duo -c MISSING_FILE
• pam_duo: pam_duo.so conf=MISSING_FILE

2. Specify groups directive in duo_pam conf file & PAM control flag sufficient in PAM conf file; then monitor the behavior of pam_duo.

/pam_duo.conf
[duo]
groups = DUO_GROUP

/etc/pam.d/sshd
# Good to Go without further authentication if pass Duo auth
auth       sufficient      pam_duo.so
# Non-Duo users flow down to next pam module
auth       requisite       pam_google_authenticator.so

Specs

• OS version (ie CENTOS 7 or Ubuntu 14): OpenWrt 23 and CentOS 9
• OS arch (ie 32 or 64): both
• Using pam_duo or login_duo: both

[haoshu]

Hi,

I am not sure if I made it clear enough how severe the issue#1 is in my previous post the other day, I will let the code speak for itself. I use login_duo as example, pam_duo is applicable for the same issue, because it follow the same logic.

1. In do_auth() of login_duo.c, duo_config_default() initialize the default conf as follows. Plz notice that failmode = DUO_FAIL_SAFE is hard-coded.

duo_config_default(struct duo_config *cfg)
{
    memset(cfg, 0, sizeof(struct duo_config));
    cfg->failmode = DUO_FAIL_SAFE;
    cfg->prompts = MAX_PROMPTS;
    cfg->local_ip_fallback = 0;
    cfg->https_timeout = -1;
    cfg->fips_mode = 0;
    cfg->gecos_username_pos = -1;
    cfg->gecos_delim = ',';
}

2. If conf file don't exist, duo_parse_config() return -1

duo_parse_config(const char *filename,
    int (*callback)(void *arg, const char *section,
    const char *name, const char *val), void *arg)
{
    FILE *fp;
    struct stat st;
    int fd, ret;

    if ((fd = open(filename, O_RDONLY)) < 0) {
        return (-1);
    }

3. An error message(through stderr) is generated in the switch-case statement and eventually return EXIT_SUCCESS

if ((i = duo_parse_config(config, __ini_handler, &cfg)) != 0 ||
        ...
        case -1:
            fprintf(stderr, "Couldn't open %s: %s\n",
                config, strerror(errno));
            break;
        ...
        /* Implicit "safe" failmode for local configuration errors */
        if (cfg.failmode == DUO_FAIL_SAFE) {
            return (EXIT_SUCCESS);
        }

4. login_duo pass through w/o even an error record on syslog about the issue.

I don't actually know how widely duo_unix is used in production. As a system admin I accept, I don't like, but I accept that I make mistake. Accidentally removing a conf file from the production system do happen, just like what happened to Notam the other day.
I don't know if this issue deserve a security advisory, but, honestly speaking, Duo's current design is against my better instincts and it don't make me feel secure.

About my point#2 on pam_duo's behavior for those not a member of the group, can you plz review you proposal.
By X/Open's spec on PAM, every implementation of PAM shall be capable to handle return code PAM_IGNORE along with PAM_SUCCESS from pam_sm_authenticate().
https://pubs.opengroup.org/onlinepubs/8329799/toc.pdf

What I am looking for is to add another directive and give sysadmin discretion to make the call on return code of duo_check_groups() either PAM_SUCCESS or PAM_IGNORE. Code snippet is attached as follows.

    } else if (matched == 0) {
        duo_syslog(LOG_INFO, "User %s bypassed Duo 2FA due to user's UNIX group", user);
        close_config(&cfg);
        return (PAM_SUCCESS);
    }

[AaronAtDuo]

@haoshu Regarding item 1:
FYI, Duo Unix is used pretty widely in production, though use of a custom configuration file is very rare. Most people use the default config files.

My concern with your suggestion is that if
A) pam_duo fails closed in the case of a missing or invalid config
and
B) The config file gets deleted or otherwise made unusable
now you are locked out of your system because PAM will fail.

This could be especially bad during initial setup and configuration of duo unix, when the chance of making a mistake is fairly high and could lead to a lot of lock outs, especially for less experienced administrators.

[AaronAtDuo]

Regarding item 2, we'll review your idea and get back to you with questions.

[haoshu]

@AaronAtDuo
Got your point on item 1. I agree, fail-secure come with price, and as what you pointed out, first-time user experience for sysadmin can be terrible to make Duo fail-secure by nature. Can I suggest a tiny change: login_duo shall at least generate an error syslog(instead of stderr) about this issue, just like what pam_duo do(non-debug mode) in the same circumstance.

Appreciate your consideration on item 2. Though I have no visibility on the other Unix variants, Linux-PAM handle PAM_IGNORE pretty well.
https://github.com/linux-pam/linux-pam/blob/master/libpam/pam_handlers.c