From: Todd C. Miller Date: Tue, 10 Jul 2012 16:42:33 +0000 (-0400) Subject: Move log_denial() calls and logic to log_failure(). X-Git-Tag: SUDO_1_8_6^2~99 X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=8b03f3e7d0e510a9c303532bbb39c06601ed6c2c;p=sudo Move log_denial() calls and logic to log_failure(). Move authentication failure logging to log_auth_failure(). Both of these call audit_failure() for us. This subtly changes logging for commands that are denied by sudoers but where the user failed to enter the correct password. Previously, these would be logged as "N incorrect password attempts" but now are logged as "command not allowed". Fixes bug #563 --- diff --git a/plugins/sudoers/auth/sudo_auth.c b/plugins/sudoers/auth/sudo_auth.c index 3216582d4..56ba26893 100644 --- a/plugins/sudoers/auth/sudo_auth.c +++ b/plugins/sudoers/auth/sudo_auth.c @@ -100,6 +100,10 @@ extern char **NewArgv; /* XXX - for auditing */ static void pass_warn(void); +/* + * Initialize sudoers authentication method(s). + * Returns 0 on success and -1 on error. + */ int sudo_auth_init(struct passwd *pw) { @@ -108,7 +112,7 @@ sudo_auth_init(struct passwd *pw) debug_decl(sudo_auth_init, SUDO_DEBUG_AUTH) if (auth_switch[0].name == NULL) - debug_return_int(true); + debug_return_int(0); /* Make sure we haven't mixed standalone and shared auth methods. */ standalone = IS_STANDALONE(&auth_switch[0]); @@ -134,18 +138,20 @@ sudo_auth_init(struct passwd *pw) if (NEEDS_USER(auth)) restore_perms(); + /* Disable if it failed to init unless there was a fatal error. */ if (status == AUTH_FAILURE) SET(auth->flags, FLAG_DISABLED); - else if (status == AUTH_FATAL) { - /* XXX log */ - audit_failure(NewArgv, "authentication failure"); + else if (status == AUTH_FATAL) break; /* assume error msg already printed */ - } } } - debug_return_int(status == AUTH_FATAL ? -1 : true); + debug_return_int(status == AUTH_FATAL ? -1 : 0); } +/* + * Cleanup all authentication methods. + * Returns 0 on success and -1 on error. + */ int sudo_auth_cleanup(struct passwd *pw) { @@ -164,22 +170,23 @@ sudo_auth_cleanup(struct passwd *pw) if (NEEDS_USER(auth)) restore_perms(); - if (status == AUTH_FATAL) { - /* XXX log */ - audit_failure(NewArgv, "authentication failure"); + if (status == AUTH_FATAL) break; /* assume error msg already printed */ - } } } - debug_return_int(status == AUTH_FATAL ? -1 : true); + debug_return_int(status == AUTH_FATAL ? -1 : 0); } +/* + * Verify the specified user. + * Returns true if verified, false if not or -1 on error. + */ int -verify_user(struct passwd *pw, char *prompt) +verify_user(struct passwd *pw, char *prompt, int validated) { int counter = def_passwd_tries + 1; int success = AUTH_FAILURE; - int flags, status, rval; + int status, rval; char *p; sudo_auth *auth; sigaction_t sa, osa; @@ -216,11 +223,8 @@ verify_user(struct passwd *pw, char *prompt) if (status == AUTH_FAILURE) SET(auth->flags, FLAG_DISABLED); - else if (status == AUTH_FATAL) { - /* XXX log */ - audit_failure(NewArgv, "authentication failure"); - debug_return_int(-1);/* assume error msg already printed */ - } + else if (status == AUTH_FATAL) + goto done; /* assume error msg already printed */ } } @@ -263,21 +267,14 @@ done: break; case AUTH_INTR: case AUTH_FAILURE: - if (counter != def_passwd_tries) { - if (def_mail_badpass || def_mail_always) - flags = 0; - else - flags = NO_MAIL; - log_fatal(flags, ngettext("%d incorrect password attempt", - "%d incorrect password attempts", - def_passwd_tries - counter), def_passwd_tries - counter); - } - audit_failure(NewArgv, "authentication failure"); + if (counter != def_passwd_tries) + validated |= FLAG_BAD_PASSWORD; + log_auth_failure(validated, def_passwd_tries - counter); rval = false; break; case AUTH_FATAL: default: - audit_failure(NewArgv, "authentication failure"); + log_auth_failure(validated | FLAG_AUTH_ERROR, 0); rval = -1; break; } @@ -285,43 +282,46 @@ done: debug_return_int(rval); } +/* + * Call authentication method begin session hooks. + * Returns 1 on success and -1 on error. + */ int sudo_auth_begin_session(struct passwd *pw, char **user_env[]) { sudo_auth *auth; - int status; + int status = AUTH_SUCCESS; debug_decl(auth_begin_session, SUDO_DEBUG_AUTH) for (auth = auth_switch; auth->name; auth++) { if (auth->begin_session && !IS_DISABLED(auth)) { status = (auth->begin_session)(pw, user_env, auth); - if (status == AUTH_FATAL) { - /* XXX log */ - audit_failure(NewArgv, "authentication failure"); - debug_return_bool(-1); /* assume error msg already printed */ - } + if (status == AUTH_FATAL) + break; /* assume error msg already printed */ } } - debug_return_bool(true); + debug_return_int(status == AUTH_FATAL ? -1 : 1); } +/* + * Call authentication method end session hooks. + * Returns 1 on success and -1 on error. + */ int sudo_auth_end_session(struct passwd *pw) { sudo_auth *auth; - int status; + int status = AUTH_SUCCESS; debug_decl(auth_end_session, SUDO_DEBUG_AUTH) for (auth = auth_switch; auth->name; auth++) { if (auth->end_session && !IS_DISABLED(auth)) { status = (auth->end_session)(pw, auth); - if (status == AUTH_FATAL) { - /* XXX log */ - debug_return_bool(-1); /* assume error msg already printed */ - } + if (status == AUTH_FATAL) + break; /* assume error msg already printed */ } } - debug_return_bool(true); + debug_return_int(status == AUTH_FATAL ? -1 : 1); } static void diff --git a/plugins/sudoers/check.c b/plugins/sudoers/check.c index 61ed21f1f..bcef2a689 100644 --- a/plugins/sudoers/check.c +++ b/plugins/sudoers/check.c @@ -149,7 +149,8 @@ check_user(int validated, int mode) if (status != TS_CURRENT || ISSET(validated, FLAG_CHECK_USER)) { /* Bail out if we are non-interactive and a password is required */ if (ISSET(mode, MODE_NONINTERACTIVE)) { - warningx(_("sorry, a password is required to run %s"), getprogname()); + validated |= FLAG_NON_INTERACTIVE; + log_auth_failure(validated, 0); rval = -1; goto done; } @@ -161,7 +162,7 @@ check_user(int validated, int mode) prompt = expand_prompt(user_prompt ? user_prompt : def_passprompt, user_name, user_shost); - rval = verify_user(auth_pw, prompt); + rval = verify_user(auth_pw, prompt, validated); } /* Only update timestamp if user was validated. */ if (rval == true && ISSET(validated, VALIDATE_OK) && diff --git a/plugins/sudoers/logging.c b/plugins/sudoers/logging.c index 1f4fc6ac8..0f0b13c93 100644 --- a/plugins/sudoers/logging.c +++ b/plugins/sudoers/logging.c @@ -73,6 +73,8 @@ static char *new_logline(const char *, int); extern sigjmp_buf error_jmp; +extern char **NewArgv; /* XXX - for auditing */ + #define MAXSYSLOGTRIES 16 /* num of retries for broken syslogs */ /* @@ -247,8 +249,8 @@ do_logfile(char *msg) /* * Log and mail the denial message, optionally informing the user. */ -void -log_denial(int status, int inform_user) +static void +log_denial(int status, bool inform_user) { char *logline, *message; debug_decl(log_denial, SUDO_DEBUG_LOGGING) @@ -301,6 +303,81 @@ log_denial(int status, int inform_user) debug_return; } +/* + * Log and audit that user was not allowed to run the command. + */ +void +log_failure(int status, int flags) +{ + debug_decl(log_failure, SUDO_DEBUG_LOGGING) + bool inform_user; + + /* Handle auditing first. */ + if (ISSET(status, FLAG_NO_USER | FLAG_NO_HOST)) + audit_failure(NewArgv, _("No user or host")); + else + audit_failure(NewArgv, _("validation failure")); + + /* The user doesn't always get to see the log message (path info). */ + inform_user = def_path_info && (flags == NOT_FOUND_DOT || flags == NOT_FOUND); + log_denial(status, inform_user); + + if (!inform_user) { + /* + * We'd like to not leak path info at all here, but that can + * *really* confuse the users. To really close the leak we'd + * have to say "not allowed to run foo" even when the problem + * is just "no foo in path" since the user can trivially set + * their path to just contain a single dir. + */ + if (flags == NOT_FOUND) + warningx(_("%s: command not found"), user_cmnd); + else if (flags == NOT_FOUND_DOT) + warningx(_("ignoring `%s' found in '.'\nUse `sudo ./%s' if this is the `%s' you wish to run."), user_cmnd, user_cmnd, user_cmnd); + } +} + +/* + * Log and audit that user was not able to authenticate themselves. + */ +void +log_auth_failure(int status, int tries) +{ + int flags = NO_MAIL; + debug_decl(log_auth_failure, SUDO_DEBUG_LOGGING) + + /* Handle auditing first. */ + audit_failure(NewArgv, _("authentication failure")); + + /* + * Do we need to send mail? + * We want to avoid sending multiple messages for the same command + * so if we are going to send an email about the denial, that takes + * precedence. + */ + if (ISSET(status, VALIDATE_OK)) { + /* Command allowed, auth failed; do we need to send mail? */ + if (def_mail_badpass || def_mail_always) + flags = 0; + } else { + /* Command denied, auth failed; make sure we don't send mail twice. */ + if (def_mail_badpass && !should_mail(status)) + flags = 0; + /* Don't log the bad password message, we'll log a denial instead. */ + flags |= NO_LOG; + } + + /* + * If sudoers denied the command we'll log that separately. + */ + if (ISSET(status, FLAG_BAD_PASSWORD)) { + log_error(flags, ngettext("%d incorrect password attempt", + "%d incorrect password attempts", tries), tries); + } else if (ISSET(status, FLAG_NON_INTERACTIVE)) { + log_error(flags, _("password required")); + } +} + /* * Log and potentially mail the allowed command. */ @@ -369,10 +446,12 @@ vlog_error(int flags, const char *fmt, va_list ap) /* * Log to syslog and/or a file. */ - if (def_syslog) - do_syslog(def_syslog_badpri, logline); - if (def_logfile) - do_logfile(logline); + if (!ISSET(flags, NO_LOG)) { + if (def_syslog) + do_syslog(def_syslog_badpri, logline); + if (def_logfile) + do_logfile(logline); + } efree(logline); diff --git a/plugins/sudoers/logging.h b/plugins/sudoers/logging.h index d8611ec08..648c2f601 100644 --- a/plugins/sudoers/logging.h +++ b/plugins/sudoers/logging.h @@ -35,6 +35,7 @@ #define USE_ERRNO 0x02 #define NO_MAIL 0x04 #define NO_STDERR 0x08 +#define NO_LOG 0x10 /* * Maximum number of characters to log per entry. The syslogger @@ -51,13 +52,13 @@ */ #define LOG_INDENT " " -void audit_success(char *[]); -void audit_failure(char *[], char const * const, ...); -void log_allowed(int); -void log_denial(int, int); +void audit_success(char *exec_args[]); +void audit_failure(char *exec_args[], char const *const fmt, ...); +void log_allowed(int status); +void log_auth_failure(int status, int tries); +void log_failure(int status, int flags); void log_error(int flags, const char *fmt, ...) __printflike(2, 3); void log_fatal(int flags, const char *fmt, ...) __printflike(2, 3) __attribute__((__noreturn__)); -void reapchild(int); void writeln_wrap(FILE *fp, char *line, size_t len, size_t maxlen); #endif /* _LOGGING_H */ diff --git a/plugins/sudoers/sudoers.c b/plugins/sudoers/sudoers.c index c639e2080..b0a18e8dd 100644 --- a/plugins/sudoers/sudoers.c +++ b/plugins/sudoers/sudoers.c @@ -289,6 +289,7 @@ sudoers_policy_close(int exit_status, int error_code) /* * The init_session function is called before executing the command * and before uid/gid changes occur. + * Returns 1 on success, 0 on failure and -1 on error. */ static int sudoers_policy_init_session(struct passwd *pwd, char **user_env[]) @@ -301,7 +302,7 @@ sudoers_policy_init_session(struct passwd *pwd, char **user_env[]) if (sigsetjmp(error_jmp, 1)) { /* called via error(), errorx() or log_fatal() */ - return -1; + debug_return_bool(-1); } debug_return_bool(sudo_auth_begin_session(pwd, user_env)); @@ -371,10 +372,6 @@ sudoers_policy_main(int argc, char * const argv[], int pwflag, char *env_add[], /* Find command in path */ cmnd_status = set_cmnd(); - if (cmnd_status == -1) { - rval = -1; - goto done; - } #ifdef HAVE_SETLOCALE if (!setlocale(LC_ALL, def_sudoers_locale)) { @@ -463,8 +460,11 @@ sudoers_policy_main(int argc, char * const argv[], int pwflag, char *env_add[], /* Require a password if sudoers says so. */ rval = check_user(validated, sudo_mode); - if (rval != true) + if (rval != true) { + if (!ISSET(validated, VALIDATE_OK)) + log_failure(validated, cmnd_status); goto done; + } /* If run as root with SUDO_USER set, set sudo_user.pw to that user. */ /* XXX - causes confusion when root is not listed in sudoers */ @@ -482,30 +482,7 @@ sudoers_policy_main(int argc, char * const argv[], int pwflag, char *env_add[], /* If the user was not allowed to run the command we are done. */ if (!ISSET(validated, VALIDATE_OK)) { - if (ISSET(validated, FLAG_NO_USER | FLAG_NO_HOST)) { - audit_failure(NewArgv, _("No user or host")); - log_denial(validated, 1); - } else { - if (def_path_info) { - /* - * We'd like to not leak path info at all here, but that can - * *really* confuse the users. To really close the leak we'd - * have to say "not allowed to run foo" even when the problem - * is just "no foo in path" since the user can trivially set - * their path to just contain a single dir. - */ - log_denial(validated, - !(cmnd_status == NOT_FOUND_DOT || cmnd_status == NOT_FOUND)); - if (cmnd_status == NOT_FOUND) - warningx(_("%s: command not found"), user_cmnd); - else if (cmnd_status == NOT_FOUND_DOT) - warningx(_("ignoring `%s' found in '.'\nUse `sudo ./%s' if this is the `%s' you wish to run."), user_cmnd, user_cmnd, user_cmnd); - } else { - /* Just tell the user they are not allowed to run foo. */ - log_denial(validated, 1); - } - audit_failure(NewArgv, _("validation failure")); - } + log_failure(validated, cmnd_status); goto bad; } diff --git a/plugins/sudoers/sudoers.h b/plugins/sudoers/sudoers.h index bf07650d3..8a4bad766 100644 --- a/plugins/sudoers/sudoers.h +++ b/plugins/sudoers/sudoers.h @@ -104,6 +104,9 @@ struct sudo_user { #define FLAG_NO_USER 0x020 #define FLAG_NO_HOST 0x040 #define FLAG_NO_CHECK 0x080 +#define FLAG_NON_INTERACTIVE 0x100 +#define FLAG_BAD_PASSWORD 0x200 +#define FLAG_AUTH_ERROR 0x400 /* * find_path()/load_cmnd() return values @@ -219,7 +222,7 @@ void remove_timestamp(bool); bool user_is_exempt(void); /* sudo_auth.c */ -int verify_user(struct passwd *pw, char *prompt); +int verify_user(struct passwd *pw, char *prompt, int validated); int sudo_auth_begin_session(struct passwd *pw, char **user_env[]); int sudo_auth_end_session(struct passwd *pw); int sudo_auth_init(struct passwd *pw);