isa/sudo
1
0
Fork 0
Code Issues Pull Requests Actions 1 Packages Projects Releases Wiki Activity

Try to make sudo less vulnerable to ROWHAMMER attacks.

Browse Source 
We now use ROWHAMMER-resistent values for ALLOW, DENY, AUTH_SUCCESS,
AUTH_FAILURE, AUTH_ERROR and AUTH_NONINTERACTIVE.  In addition, we
explicitly test for expected values instead of using a negated test
against an error value.  In the parser match functions this means
explicitly checking for ALLOW or DENY instead of accepting anything
that is not set to UNSPEC.

Thanks to Andrew J. Adiletta, M. Caner Tol, Yarkin Doroz, and Berk
Sunar, all affiliated with the Vernam Applied Cryptography and
Cybersecurity Lab at Worcester Polytechnic Institute, for the report.
Paper preprint: https://arxiv.org/abs/2309.02545
This commit is contained in:
Todd C. Miller
2023-09-09 14:07:04 -06:00
parent 525803db23
commit 7873f8334c
6 changed files with 96 additions and 54 deletions
Download Patch File Download Diff File Expand all files Collapse all files

27
plugins/sudoers/auth/passwd.c
View File

@@ -68,7 +68,7 @@ sudo_passwd_verify(const struct sudoers_context *ctx, struct passwd *pw,
char des_pass[9], *epass; char des_pass[9], *epass;
char *pw_epasswd = auth->data; char *pw_epasswd = auth->data;
size_t pw_len; size_t pw_len;
int matched = 0; int ret;
debug_decl(sudo_passwd_verify, SUDOERS_DEBUG_AUTH); debug_decl(sudo_passwd_verify, SUDOERS_DEBUG_AUTH);
/* An empty plain-text password must match an empty encrypted password. */ /* An empty plain-text password must match an empty encrypted password. */
@@ -80,7 +80,7 @@ sudo_passwd_verify(const struct sudoers_context *ctx, struct passwd *pw,
*/ */
pw_len = strlen(pw_epasswd); pw_len = strlen(pw_epasswd);
if (pw_len == DESLEN || HAS_AGEINFO(pw_epasswd, pw_len)) { if (pw_len == DESLEN || HAS_AGEINFO(pw_epasswd, pw_len)) {
strlcpy(des_pass, pass, sizeof(des_pass)); (void)strlcpy(des_pass, pass, sizeof(des_pass));
pass = des_pass; pass = des_pass;
} }
@@ -90,16 +90,20 @@ sudo_passwd_verify(const struct sudoers_context *ctx, struct passwd *pw,
* only compare the first DESLEN characters in that case. * only compare the first DESLEN characters in that case.
*/ */
epass = (char *) crypt(pass, pw_epasswd); epass = (char *) crypt(pass, pw_epasswd);
ret = AUTH_FAILURE;
if (epass != NULL) { if (epass != NULL) {
if (HAS_AGEINFO(pw_epasswd, pw_len) && strlen(epass) == DESLEN) if (HAS_AGEINFO(pw_epasswd, pw_len) && strlen(epass) == DESLEN) {
matched = !strncmp(pw_epasswd, epass, DESLEN); if (strncmp(pw_epasswd, epass, DESLEN) == 0)
else ret = AUTH_SUCCESS;
matched = !strcmp(pw_epasswd, epass); } else {
if (strcmp(pw_epasswd, epass) == 0)
ret = AUTH_SUCCESS;
}
} }
explicit_bzero(des_pass, sizeof(des_pass)); explicit_bzero(des_pass, sizeof(des_pass));
debug_return_int(matched ? AUTH_SUCCESS : AUTH_FAILURE); debug_return_int(ret);
} }
#else #else
int int
@@ -107,13 +111,16 @@ sudo_passwd_verify(const struct sudoers_context *ctx, struct passwd *pw,
const char *pass, sudo_auth *auth, struct sudo_conv_callback *callback) const char *pass, sudo_auth *auth, struct sudo_conv_callback *callback)
{ {
char *pw_passwd = auth->data; char *pw_passwd = auth->data;
int matched; int ret;
debug_decl(sudo_passwd_verify, SUDOERS_DEBUG_AUTH); debug_decl(sudo_passwd_verify, SUDOERS_DEBUG_AUTH);
/* Simple string compare for systems without crypt(). */ /* Simple string compare for systems without crypt(). */
matched = !strcmp(pass, pw_passwd); if (strcmp(pass, pw_passwd) == 0)
ret = AUTH_SUCCESS;
else
ret = AUTH_FAILURE;
debug_return_int(matched ? AUTH_SUCCESS : AUTH_FAILURE); debug_return_int(ret);
} }
#endif #endif

51
plugins/sudoers/auth/sudo_auth.c
View File

@@ -116,10 +116,16 @@ sudo_auth_init(const struct sudoers_context *ctx, struct passwd *pw,
if (auth->init && !IS_DISABLED(auth)) { if (auth->init && !IS_DISABLED(auth)) {
/* Disable if it failed to init unless there was a fatal error. */ /* Disable if it failed to init unless there was a fatal error. */
status = (auth->init)(ctx, pw, auth); status = (auth->init)(ctx, pw, auth);
if (status == AUTH_FAILURE) switch (status) {
case AUTH_SUCCESS:
break;
case AUTH_FAILURE:
SET(auth->flags, FLAG_DISABLED); SET(auth->flags, FLAG_DISABLED);
else if (status == AUTH_ERROR) break;
break; /* assume error msg already printed */ default:
/* Assume error msg already printed. */
debug_return_int(-1);
}
} }
} }
@@ -166,7 +172,7 @@ sudo_auth_init(const struct sudoers_context *ctx, struct passwd *pw,
} }
} }
debug_return_int(status == AUTH_ERROR ? -1 : 0); debug_return_int(0);
} }
/* /*
@@ -209,7 +215,7 @@ sudo_auth_cleanup(const struct sudoers_context *ctx, struct passwd *pw,
for (auth = auth_switch; auth->name; auth++) { for (auth = auth_switch; auth->name; auth++) {
if (auth->cleanup && !IS_DISABLED(auth)) { if (auth->cleanup && !IS_DISABLED(auth)) {
int status = (auth->cleanup)(ctx, pw, auth, force); int status = (auth->cleanup)(ctx, pw, auth, force);
if (status == AUTH_ERROR) { if (status != AUTH_SUCCESS) {
/* Assume error msg already printed. */ /* Assume error msg already printed. */
debug_return_int(-1); debug_return_int(-1);
} }
@@ -306,7 +312,7 @@ verify_user(const struct sudoers_context *ctx, struct passwd *pw, char *prompt,
SET(auth->flags, FLAG_DISABLED); SET(auth->flags, FLAG_DISABLED);
else if (status == AUTH_NONINTERACTIVE) else if (status == AUTH_NONINTERACTIVE)
goto done; goto done;
else if (status == AUTH_ERROR || user_interrupted()) else if (status != AUTH_SUCCESS || user_interrupted())
goto done; /* assume error msg already printed */ goto done; /* assume error msg already printed */
} }
} }
@@ -365,7 +371,6 @@ done:
case AUTH_NONINTERACTIVE: case AUTH_NONINTERACTIVE:
SET(validated, FLAG_NO_USER_INPUT); SET(validated, FLAG_NO_USER_INPUT);
FALLTHROUGH; FALLTHROUGH;
case AUTH_ERROR:
default: default:
log_auth_failure(ctx, validated, 0); log_auth_failure(ctx, validated, 0);
ret = -1; ret = -1;
@@ -377,25 +382,33 @@ done:
/* /*
* Call authentication method begin session hooks. * Call authentication method begin session hooks.
* Returns 1 on success and -1 on error. * Returns true on success, false on failure and -1 on error.
*/ */
int int
sudo_auth_begin_session(const struct sudoers_context *ctx, struct passwd *pw, sudo_auth_begin_session(const struct sudoers_context *ctx, struct passwd *pw,
char **user_env[]) char **user_env[])
{ {
sudo_auth *auth; sudo_auth *auth;
int ret = true;
debug_decl(sudo_auth_begin_session, SUDOERS_DEBUG_AUTH); debug_decl(sudo_auth_begin_session, SUDOERS_DEBUG_AUTH);
for (auth = auth_switch; auth->name; auth++) { for (auth = auth_switch; auth->name; auth++) {
if (auth->begin_session && !IS_DISABLED(auth)) { if (auth->begin_session && !IS_DISABLED(auth)) {
int status = (auth->begin_session)(ctx, pw, user_env, auth); int status = (auth->begin_session)(ctx, pw, user_env, auth);
if (status != AUTH_SUCCESS) { switch (status) {
case AUTH_SUCCESS:
break;
case AUTH_FAILURE:
ret = false;
break;
default:
/* Assume error msg already printed. */ /* Assume error msg already printed. */
debug_return_int(-1); ret = -1;
break;
} }
} }
} }
debug_return_int(1); debug_return_int(ret);
} }
bool bool
@@ -416,25 +429,33 @@ sudo_auth_needs_end_session(void)
/* /*
* Call authentication method end session hooks. * Call authentication method end session hooks.
* Returns 1 on success and -1 on error. * Returns true on success, false on failure and -1 on error.
*/ */
int int
sudo_auth_end_session(void) sudo_auth_end_session(void)
{ {
sudo_auth *auth; sudo_auth *auth;
int ret = true;
int status; int status;
debug_decl(sudo_auth_end_session, SUDOERS_DEBUG_AUTH); debug_decl(sudo_auth_end_session, SUDOERS_DEBUG_AUTH);
for (auth = auth_switch; auth->name; auth++) { for (auth = auth_switch; auth->name; auth++) {
if (auth->end_session && !IS_DISABLED(auth)) { if (auth->end_session && !IS_DISABLED(auth)) {
status = (auth->end_session)(auth); status = (auth->end_session)(auth);
if (status == AUTH_ERROR) { switch (status) {
case AUTH_SUCCESS:
break;
case AUTH_FAILURE:
ret = false;
break;
default:
/* Assume error msg already printed. */ /* Assume error msg already printed. */
debug_return_int(-1); ret = -1;
break;
} }
} }
} }
debug_return_int(1); debug_return_int(ret);
} }
/* /*

12
plugins/sudoers/auth/sudo_auth.h
View File

@@ -19,12 +19,12 @@
#ifndef SUDO_AUTH_H #ifndef SUDO_AUTH_H
#define SUDO_AUTH_H #define SUDO_AUTH_H
/* Auth function return values. */ /* Auth function return values (rowhammer resistent). */
#define AUTH_SUCCESS 0 #define AUTH_SUCCESS 0x52a2925 /* 0101001010100010100100100101 */
#define AUTH_FAILURE 1 #define AUTH_FAILURE 0xad5d6da /* 1010110101011101011011011010 */
#define AUTH_INTR 2 #define AUTH_INTR 0x69d61fc8 /* 1101001110101100001111111001000 */
#define AUTH_ERROR 3 #define AUTH_ERROR 0x1629e037 /* 0010110001010011110000000110111 */
#define AUTH_NONINTERACTIVE 4 #define AUTH_NONINTERACTIVE 0x1fc8d3ac /* 11111110010001101001110101100 */
typedef struct sudo_auth { typedef struct sudo_auth {
unsigned int flags; /* various flags, see below */ unsigned int flags; /* various flags, see below */

12
plugins/sudoers/lookup.c
View File

@@ -100,7 +100,7 @@ sudoers_lookup_pseudo(struct sudo_nss_list *snl, struct sudoers_context *ctx,
int user_match = userlist_matches(nss->parse_tree, ctx->user.pw, int user_match = userlist_matches(nss->parse_tree, ctx->user.pw,
&us->users); &us->users);
if (user_match != ALLOW) { if (user_match != ALLOW) {
if (callback != NULL && user_match != UNSPEC) { if (callback != NULL && user_match == DENY) {
callback(nss->parse_tree, us, user_match, NULL, UNSPEC, callback(nss->parse_tree, us, user_match, NULL, UNSPEC,
NULL, UNSPEC, UNSPEC, UNSPEC, cb_data); NULL, UNSPEC, UNSPEC, UNSPEC, cb_data);
} }
@@ -189,7 +189,7 @@ sudoers_lookup_pseudo(struct sudo_nss_list *snl, struct sudoers_context *ctx,
host_match, cs, date_match, runas_match, host_match, cs, date_match, runas_match,
cmnd_match, cb_data); cmnd_match, cb_data);
} }
if (cmnd_match != UNSPEC) { if (SPECIFIED(cmnd_match)) {
/* /*
* We take the last match but must process * We take the last match but must process
* the entire policy for pwcheck == all. * the entire policy for pwcheck == all.
@@ -245,7 +245,7 @@ sudoers_lookup_check(struct sudo_nss *nss, struct sudoers_context *ctx,
TAILQ_FOREACH_REVERSE(us, &nss->parse_tree->userspecs, userspec_list, entries) { TAILQ_FOREACH_REVERSE(us, &nss->parse_tree->userspecs, userspec_list, entries) {
int user_match = userlist_matches(nss->parse_tree, ctx->user.pw, &us->users); int user_match = userlist_matches(nss->parse_tree, ctx->user.pw, &us->users);
if (user_match != ALLOW) { if (user_match != ALLOW) {
if (callback != NULL && user_match != UNSPEC) { if (callback != NULL && user_match == DENY) {
callback(nss->parse_tree, us, user_match, NULL, UNSPEC, NULL, callback(nss->parse_tree, us, user_match, NULL, UNSPEC, NULL,
UNSPEC, UNSPEC, UNSPEC, cb_data); UNSPEC, UNSPEC, UNSPEC, cb_data);
} }
@@ -290,7 +290,7 @@ sudoers_lookup_check(struct sudo_nss *nss, struct sudoers_context *ctx,
cs, date_match, runas_match, cmnd_match, cb_data); cs, date_match, runas_match, cmnd_match, cb_data);
} }
if (cmnd_match != UNSPEC) { if (SPECIFIED(cmnd_match)) {
/* /*
* If user is running command as themselves, * If user is running command as themselves,
* set ctx->runas.pw = ctx->user.pw. * set ctx->runas.pw = ctx->user.pw.
@@ -542,7 +542,7 @@ sudoers_lookup(struct sudo_nss_list *snl, struct sudoers_context *ctx,
m = sudoers_lookup_check(nss, ctx, &validated, &info, now, callback, m = sudoers_lookup_check(nss, ctx, &validated, &info, now, callback,
cb_data, &cs, &defs); cb_data, &cs, &defs);
if (m != UNSPEC) { if (SPECIFIED(m)) {
match = m; match = m;
parse_tree = nss->parse_tree; parse_tree = nss->parse_tree;
} }
@@ -550,7 +550,7 @@ sudoers_lookup(struct sudo_nss_list *snl, struct sudoers_context *ctx,
if (!sudo_nss_can_continue(nss, m)) if (!sudo_nss_can_continue(nss, m))
break; break;
} }
if (match != UNSPEC) { if (SPECIFIED(match)) {
if (info.cmnd_path != NULL) { if (info.cmnd_path != NULL) {
/* Update cmnd, cmnd_stat, cmnd_status from matching entry. */ /* Update cmnd, cmnd_stat, cmnd_status from matching entry. */
free(ctx->user.cmnd); free(ctx->user.cmnd);

25
plugins/sudoers/match.c
View File

@@ -91,7 +91,7 @@ user_matches(const struct sudoers_parse_tree *parse_tree,
if ((a = alias_get(parse_tree, m->name, USERALIAS)) != NULL) { if ((a = alias_get(parse_tree, m->name, USERALIAS)) != NULL) {
/* XXX */ /* XXX */
const int rc = userlist_matches(parse_tree, pw, &a->members); const int rc = userlist_matches(parse_tree, pw, &a->members);
if (rc != UNSPEC) { if (SPECIFIED(rc)) {
if (m->negated) { if (m->negated) {
matched = rc == ALLOW ? DENY : ALLOW; matched = rc == ALLOW ? DENY : ALLOW;
} else { } else {
@@ -123,7 +123,8 @@ userlist_matches(const struct sudoers_parse_tree *parse_tree,
debug_decl(userlist_matches, SUDOERS_DEBUG_MATCH); debug_decl(userlist_matches, SUDOERS_DEBUG_MATCH);
TAILQ_FOREACH_REVERSE(m, list, member_list, entries) { TAILQ_FOREACH_REVERSE(m, list, member_list, entries) {
if ((matched = user_matches(parse_tree, pw, m)) != UNSPEC) matched = user_matches(parse_tree, pw, m);
if (SPECIFIED(matched))
break; break;
} }
debug_return_int(matched); debug_return_int(matched);
@@ -184,7 +185,7 @@ runas_userlist_matches(const struct sudoers_parse_tree *parse_tree,
if (a != NULL) { if (a != NULL) {
const int rc = runas_userlist_matches(parse_tree, const int rc = runas_userlist_matches(parse_tree,
&a->members, matching_user); &a->members, matching_user);
if (rc != UNSPEC) { if (SPECIFIED(rc)) {
if (m->negated) { if (m->negated) {
user_matched = rc == ALLOW ? DENY : ALLOW; user_matched = rc == ALLOW ? DENY : ALLOW;
} else { } else {
@@ -211,7 +212,7 @@ runas_userlist_matches(const struct sudoers_parse_tree *parse_tree,
user_matched = m->negated ? DENY : ALLOW; user_matched = m->negated ? DENY : ALLOW;
break; break;
} }
if (user_matched != UNSPEC) { if (SPECIFIED(user_matched)) {
if (matching_user != NULL && m->type != ALIAS) if (matching_user != NULL && m->type != ALIAS)
*matching_user = m; *matching_user = m;
break; break;
@@ -246,7 +247,7 @@ runas_grouplist_matches(const struct sudoers_parse_tree *parse_tree,
if (a != NULL) { if (a != NULL) {
const int rc = runas_grouplist_matches(parse_tree, const int rc = runas_grouplist_matches(parse_tree,
&a->members, matching_group); &a->members, matching_group);
if (rc != UNSPEC) { if (SPECIFIED(rc)) {
if (m->negated) { if (m->negated) {
group_matched = rc == ALLOW ? DENY : ALLOW; group_matched = rc == ALLOW ? DENY : ALLOW;
} else { } else {
@@ -262,14 +263,14 @@ runas_grouplist_matches(const struct sudoers_parse_tree *parse_tree,
group_matched = m->negated ? DENY : ALLOW; group_matched = m->negated ? DENY : ALLOW;
break; break;
} }
if (group_matched != UNSPEC) { if (SPECIFIED(group_matched)) {
if (matching_group != NULL && m->type != ALIAS) if (matching_group != NULL && m->type != ALIAS)
*matching_group = m; *matching_group = m;
break; break;
} }
} }
} }
if (group_matched == UNSPEC) { if (!SPECIFIED(group_matched)) {
struct gid_list *runas_groups; struct gid_list *runas_groups;
/* /*
* The runas group was not explicitly allowed by sudoers. * The runas group was not explicitly allowed by sudoers.
@@ -349,7 +350,7 @@ hostlist_matches_int(const struct sudoers_parse_tree *parse_tree,
TAILQ_FOREACH_REVERSE(m, list, member_list, entries) { TAILQ_FOREACH_REVERSE(m, list, member_list, entries) {
matched = host_matches(parse_tree, pw, lhost, shost, m); matched = host_matches(parse_tree, pw, lhost, shost, m);
if (matched != UNSPEC) if (SPECIFIED(matched))
break; break;
} }
debug_return_int(matched); debug_return_int(matched);
@@ -402,7 +403,7 @@ host_matches(const struct sudoers_parse_tree *parse_tree,
/* XXX */ /* XXX */
const int rc = hostlist_matches_int(parse_tree, pw, lhost, const int rc = hostlist_matches_int(parse_tree, pw, lhost,
shost, &a->members); shost, &a->members);
if (rc != UNSPEC) { if (SPECIFIED(rc)) {
if (m->negated) { if (m->negated) {
matched = rc == ALLOW ? DENY : ALLOW; matched = rc == ALLOW ? DENY : ALLOW;
} else { } else {
@@ -440,7 +441,7 @@ cmndlist_matches(const struct sudoers_parse_tree *parse_tree,
TAILQ_FOREACH_REVERSE(m, list, member_list, entries) { TAILQ_FOREACH_REVERSE(m, list, member_list, entries) {
matched = cmnd_matches(parse_tree, m, runchroot, info); matched = cmnd_matches(parse_tree, m, runchroot, info);
if (matched != UNSPEC) if (SPECIFIED(matched))
break; break;
} }
debug_return_int(matched); debug_return_int(matched);
@@ -471,7 +472,7 @@ cmnd_matches(const struct sudoers_parse_tree *parse_tree,
a = alias_get(parse_tree, m->name, CMNDALIAS); a = alias_get(parse_tree, m->name, CMNDALIAS);
if (a != NULL) { if (a != NULL) {
rc = cmndlist_matches(parse_tree, &a->members, runchroot, info); rc = cmndlist_matches(parse_tree, &a->members, runchroot, info);
if (rc != UNSPEC) { if (SPECIFIED(rc)) {
if (m->negated) { if (m->negated) {
matched = rc == ALLOW ? DENY : ALLOW; matched = rc == ALLOW ? DENY : ALLOW;
} else { } else {
@@ -511,7 +512,7 @@ cmnd_matches_all(const struct sudoers_parse_tree *parse_tree,
if (a != NULL) { if (a != NULL) {
TAILQ_FOREACH_REVERSE(m, &a->members, member_list, entries) { TAILQ_FOREACH_REVERSE(m, &a->members, member_list, entries) {
matched = cmnd_matches_all(parse_tree, m, runchroot, info); matched = cmnd_matches_all(parse_tree, m, runchroot, info);
if (matched != UNSPEC) { if (SPECIFIED(matched)) {
if (negated) if (negated)
matched = matched == ALLOW ? DENY : ALLOW; matched = matched == ALLOW ? DENY : ALLOW;
break; break;

23
plugins/sudoers/parse.h
View File

@@ -36,15 +36,28 @@
# define SUDOERS_NAME_MATCH # define SUDOERS_NAME_MATCH
#endif #endif
/* Allowed by policy (rowhammer resistent). */
#undef ALLOW
#define ALLOW 0x52a2925 /* 0101001010100010100100100101 */
/* Denied by policy (rowhammer resistent). */
#undef DENY
#define DENY 0xad5d6da /* 1010110101011101011011011010 */
/* Neither allowed, nor denied. */
#undef UNSPEC #undef UNSPEC
#define UNSPEC -1 #define UNSPEC -1
#undef DENY
#define DENY 0 /* Tag implied by root access (SETENV only). */
#undef ALLOW
#define ALLOW 1
#undef IMPLIED #undef IMPLIED
#define IMPLIED 2 #define IMPLIED 2
/*
* We must explicitly check against ALLOW and DENY instead testing
* that the value is not UNSPEC to avoid potential ROWHAMMER issues.
*/
#define SPECIFIED(_v) ((_v) == ALLOW || (_v) == DENY)
/* /*
* Initialize all tags to UNSPEC. * Initialize all tags to UNSPEC.
*/ */
@@ -94,7 +107,7 @@
* Returns true if the specified tag is not UNSPEC or IMPLIED, else false. * Returns true if the specified tag is not UNSPEC or IMPLIED, else false.
*/ */
#define TAG_SET(tt) \ #define TAG_SET(tt) \
((tt) != UNSPEC && (tt) != IMPLIED) ((tt) == true || (tt) == false)
/* /*
* Returns true if any tags set in nt differ between ot and nt, else false. * Returns true if any tags set in nt differ between ot and nt, else false.