Skip to content

Commit

Permalink
Try to make sudo less vulnerable to ROWHAMMER attacks.
Browse files Browse the repository at this point in the history
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
  • Loading branch information
millert committed Sep 9, 2023
1 parent 525803d commit 7873f83
Show file tree
Hide file tree
Showing 6 changed files with 96 additions and 54 deletions.
27 changes: 17 additions & 10 deletions plugins/sudoers/auth/passwd.c
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ sudo_passwd_verify(const struct sudoers_context *ctx, struct passwd *pw,
char des_pass[9], *epass;
char *pw_epasswd = auth->data;
size_t pw_len;
int matched = 0;
int ret;
debug_decl(sudo_passwd_verify, SUDOERS_DEBUG_AUTH);

/* An empty plain-text password must match an empty encrypted password. */
Expand All @@ -80,7 +80,7 @@ sudo_passwd_verify(const struct sudoers_context *ctx, struct passwd *pw,
*/
pw_len = strlen(pw_epasswd);
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;
}

Expand All @@ -90,30 +90,37 @@ sudo_passwd_verify(const struct sudoers_context *ctx, struct passwd *pw,
* only compare the first DESLEN characters in that case.
*/
epass = (char *) crypt(pass, pw_epasswd);
ret = AUTH_FAILURE;
if (epass != NULL) {
if (HAS_AGEINFO(pw_epasswd, pw_len) && strlen(epass) == DESLEN)
matched = !strncmp(pw_epasswd, epass, DESLEN);
else
matched = !strcmp(pw_epasswd, epass);
if (HAS_AGEINFO(pw_epasswd, pw_len) && strlen(epass) == DESLEN) {
if (strncmp(pw_epasswd, epass, DESLEN) == 0)
ret = AUTH_SUCCESS;
} else {
if (strcmp(pw_epasswd, epass) == 0)
ret = AUTH_SUCCESS;
}
}

explicit_bzero(des_pass, sizeof(des_pass));

debug_return_int(matched ? AUTH_SUCCESS : AUTH_FAILURE);
debug_return_int(ret);
}
#else
int
sudo_passwd_verify(const struct sudoers_context *ctx, struct passwd *pw,
const char *pass, sudo_auth *auth, struct sudo_conv_callback *callback)
{
char *pw_passwd = auth->data;
int matched;
int ret;
debug_decl(sudo_passwd_verify, SUDOERS_DEBUG_AUTH);

/* 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

Expand Down
51 changes: 36 additions & 15 deletions plugins/sudoers/auth/sudo_auth.c
Original file line number Diff line number Diff line change
Expand Up @@ -116,10 +116,16 @@ sudo_auth_init(const struct sudoers_context *ctx, struct passwd *pw,
if (auth->init && !IS_DISABLED(auth)) {
/* Disable if it failed to init unless there was a fatal error. */
status = (auth->init)(ctx, pw, auth);
if (status == AUTH_FAILURE)
switch (status) {
case AUTH_SUCCESS:
break;
case AUTH_FAILURE:
SET(auth->flags, FLAG_DISABLED);
else if (status == AUTH_ERROR)
break; /* assume error msg already printed */
break;
default:
/* Assume error msg already printed. */
debug_return_int(-1);
}
}
}

Expand Down Expand Up @@ -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);
}

/*
Expand Down Expand Up @@ -209,7 +215,7 @@ sudo_auth_cleanup(const struct sudoers_context *ctx, struct passwd *pw,
for (auth = auth_switch; auth->name; auth++) {
if (auth->cleanup && !IS_DISABLED(auth)) {
int status = (auth->cleanup)(ctx, pw, auth, force);
if (status == AUTH_ERROR) {
if (status != AUTH_SUCCESS) {
/* Assume error msg already printed. */
debug_return_int(-1);
}
Expand Down Expand Up @@ -306,7 +312,7 @@ verify_user(const struct sudoers_context *ctx, struct passwd *pw, char *prompt,
SET(auth->flags, FLAG_DISABLED);
else if (status == AUTH_NONINTERACTIVE)
goto done;
else if (status == AUTH_ERROR || user_interrupted())
else if (status != AUTH_SUCCESS || user_interrupted())
goto done; /* assume error msg already printed */
}
}
Expand Down Expand Up @@ -365,7 +371,6 @@ verify_user(const struct sudoers_context *ctx, struct passwd *pw, char *prompt,
case AUTH_NONINTERACTIVE:
SET(validated, FLAG_NO_USER_INPUT);
FALLTHROUGH;
case AUTH_ERROR:
default:
log_auth_failure(ctx, validated, 0);
ret = -1;
Expand All @@ -377,25 +382,33 @@ verify_user(const struct sudoers_context *ctx, struct passwd *pw, char *prompt,

/*
* 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
sudo_auth_begin_session(const struct sudoers_context *ctx, struct passwd *pw,
char **user_env[])
{
sudo_auth *auth;
int ret = true;
debug_decl(sudo_auth_begin_session, SUDOERS_DEBUG_AUTH);

for (auth = auth_switch; auth->name; auth++) {
if (auth->begin_session && !IS_DISABLED(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. */
debug_return_int(-1);
ret = -1;
break;
}
}
}
debug_return_int(1);
debug_return_int(ret);
}

bool
Expand All @@ -416,25 +429,33 @@ sudo_auth_needs_end_session(void)

/*
* 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
sudo_auth_end_session(void)
{
sudo_auth *auth;
int ret = true;
int status;
debug_decl(sudo_auth_end_session, SUDOERS_DEBUG_AUTH);

for (auth = auth_switch; auth->name; auth++) {
if (auth->end_session && !IS_DISABLED(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. */
debug_return_int(-1);
ret = -1;
break;
}
}
}
debug_return_int(1);
debug_return_int(ret);
}

/*
Expand Down
12 changes: 6 additions & 6 deletions plugins/sudoers/auth/sudo_auth.h
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,12 @@
#ifndef SUDO_AUTH_H
#define SUDO_AUTH_H

/* Auth function return values. */
#define AUTH_SUCCESS 0
#define AUTH_FAILURE 1
#define AUTH_INTR 2
#define AUTH_ERROR 3
#define AUTH_NONINTERACTIVE 4
/* Auth function return values (rowhammer resistent). */

This comment has been minimized.

Copy link
@pavlinux

pavlinux Dec 25, 2023

Makefile:

gcc -DRND1=0x$(openssl rand -hex 4) \
       -DRND2=0x$(openssl rand -hex 4) \
       -DRND3=0x$(openssl rand -hex 4) \
       -DRND4=0x$(openssl rand -hex 4) \
       -DRND5=0x$(openssl rand -hex 4) 
...

sudo_auth.h

#define AUTH_SUCCESS RND1 /* х.з. */
#define AUTH_FAILURE RND2 /* х.з. */
#define AUTH_INTR RND3 /* х.з. */
#define AUTH_ERROR RND4 /* х.з. */
#define AUTH_NONINTERACTIVE RND5 /* х.з. */

This comment has been minimized.

Copy link
@millert

millert Dec 26, 2023

Author Collaborator

The values used were chosen such that it takes a large number of bit flips to change from allowed to denied. Using random values doesn't really protect against this attack.

#define AUTH_SUCCESS 0x52a2925 /* 0101001010100010100100100101 */
#define AUTH_FAILURE 0xad5d6da /* 1010110101011101011011011010 */
#define AUTH_INTR 0x69d61fc8 /* 1101001110101100001111111001000 */
#define AUTH_ERROR 0x1629e037 /* 0010110001010011110000000110111 */
#define AUTH_NONINTERACTIVE 0x1fc8d3ac /* 11111110010001101001110101100 */

typedef struct sudo_auth {
unsigned int flags; /* various flags, see below */
Expand Down
12 changes: 6 additions & 6 deletions plugins/sudoers/lookup.c
Original file line number Diff line number Diff line change
Expand Up @@ -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,
&us->users);
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, UNSPEC, UNSPEC, UNSPEC, cb_data);
}
Expand Down Expand Up @@ -189,7 +189,7 @@ sudoers_lookup_pseudo(struct sudo_nss_list *snl, struct sudoers_context *ctx,
host_match, cs, date_match, runas_match,
cmnd_match, cb_data);
}
if (cmnd_match != UNSPEC) {
if (SPECIFIED(cmnd_match)) {
/*
* We take the last match but must process
* the entire policy for pwcheck == all.
Expand Down Expand Up @@ -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) {
int user_match = userlist_matches(nss->parse_tree, ctx->user.pw, &us->users);
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,
UNSPEC, UNSPEC, UNSPEC, cb_data);
}
Expand Down Expand Up @@ -290,7 +290,7 @@ sudoers_lookup_check(struct sudo_nss *nss, struct sudoers_context *ctx,
cs, date_match, runas_match, cmnd_match, cb_data);
}

if (cmnd_match != UNSPEC) {
if (SPECIFIED(cmnd_match)) {
/*
* If user is running command as themselves,
* set ctx->runas.pw = ctx->user.pw.
Expand Down Expand Up @@ -542,15 +542,15 @@ sudoers_lookup(struct sudo_nss_list *snl, struct sudoers_context *ctx,

m = sudoers_lookup_check(nss, ctx, &validated, &info, now, callback,
cb_data, &cs, &defs);
if (m != UNSPEC) {
if (SPECIFIED(m)) {
match = m;
parse_tree = nss->parse_tree;
}

if (!sudo_nss_can_continue(nss, m))
break;
}
if (match != UNSPEC) {
if (SPECIFIED(match)) {
if (info.cmnd_path != NULL) {
/* Update cmnd, cmnd_stat, cmnd_status from matching entry. */
free(ctx->user.cmnd);
Expand Down
25 changes: 13 additions & 12 deletions plugins/sudoers/match.c
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ user_matches(const struct sudoers_parse_tree *parse_tree,
if ((a = alias_get(parse_tree, m->name, USERALIAS)) != NULL) {
/* XXX */
const int rc = userlist_matches(parse_tree, pw, &a->members);
if (rc != UNSPEC) {
if (SPECIFIED(rc)) {
if (m->negated) {
matched = rc == ALLOW ? DENY : ALLOW;
} else {
Expand Down Expand Up @@ -123,7 +123,8 @@ userlist_matches(const struct sudoers_parse_tree *parse_tree,
debug_decl(userlist_matches, SUDOERS_DEBUG_MATCH);

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;
}
debug_return_int(matched);
Expand Down Expand Up @@ -184,7 +185,7 @@ runas_userlist_matches(const struct sudoers_parse_tree *parse_tree,
if (a != NULL) {
const int rc = runas_userlist_matches(parse_tree,
&a->members, matching_user);
if (rc != UNSPEC) {
if (SPECIFIED(rc)) {
if (m->negated) {
user_matched = rc == ALLOW ? DENY : ALLOW;
} else {
Expand All @@ -211,7 +212,7 @@ runas_userlist_matches(const struct sudoers_parse_tree *parse_tree,
user_matched = m->negated ? DENY : ALLOW;
break;
}
if (user_matched != UNSPEC) {
if (SPECIFIED(user_matched)) {
if (matching_user != NULL && m->type != ALIAS)
*matching_user = m;
break;
Expand Down Expand Up @@ -246,7 +247,7 @@ runas_grouplist_matches(const struct sudoers_parse_tree *parse_tree,
if (a != NULL) {
const int rc = runas_grouplist_matches(parse_tree,
&a->members, matching_group);
if (rc != UNSPEC) {
if (SPECIFIED(rc)) {
if (m->negated) {
group_matched = rc == ALLOW ? DENY : ALLOW;
} else {
Expand All @@ -262,14 +263,14 @@ runas_grouplist_matches(const struct sudoers_parse_tree *parse_tree,
group_matched = m->negated ? DENY : ALLOW;
break;
}
if (group_matched != UNSPEC) {
if (SPECIFIED(group_matched)) {
if (matching_group != NULL && m->type != ALIAS)
*matching_group = m;
break;
}
}
}
if (group_matched == UNSPEC) {
if (!SPECIFIED(group_matched)) {
struct gid_list *runas_groups;
/*
* The runas group was not explicitly allowed by sudoers.
Expand Down Expand Up @@ -349,7 +350,7 @@ hostlist_matches_int(const struct sudoers_parse_tree *parse_tree,

TAILQ_FOREACH_REVERSE(m, list, member_list, entries) {
matched = host_matches(parse_tree, pw, lhost, shost, m);
if (matched != UNSPEC)
if (SPECIFIED(matched))
break;
}
debug_return_int(matched);
Expand Down Expand Up @@ -402,7 +403,7 @@ host_matches(const struct sudoers_parse_tree *parse_tree,
/* XXX */
const int rc = hostlist_matches_int(parse_tree, pw, lhost,
shost, &a->members);
if (rc != UNSPEC) {
if (SPECIFIED(rc)) {
if (m->negated) {
matched = rc == ALLOW ? DENY : ALLOW;
} else {
Expand Down Expand Up @@ -440,7 +441,7 @@ cmndlist_matches(const struct sudoers_parse_tree *parse_tree,

TAILQ_FOREACH_REVERSE(m, list, member_list, entries) {
matched = cmnd_matches(parse_tree, m, runchroot, info);
if (matched != UNSPEC)
if (SPECIFIED(matched))
break;
}
debug_return_int(matched);
Expand Down Expand Up @@ -471,7 +472,7 @@ cmnd_matches(const struct sudoers_parse_tree *parse_tree,
a = alias_get(parse_tree, m->name, CMNDALIAS);
if (a != NULL) {
rc = cmndlist_matches(parse_tree, &a->members, runchroot, info);
if (rc != UNSPEC) {
if (SPECIFIED(rc)) {
if (m->negated) {
matched = rc == ALLOW ? DENY : ALLOW;
} else {
Expand Down Expand Up @@ -511,7 +512,7 @@ cmnd_matches_all(const struct sudoers_parse_tree *parse_tree,
if (a != NULL) {
TAILQ_FOREACH_REVERSE(m, &a->members, member_list, entries) {
matched = cmnd_matches_all(parse_tree, m, runchroot, info);
if (matched != UNSPEC) {
if (SPECIFIED(matched)) {
if (negated)
matched = matched == ALLOW ? DENY : ALLOW;
break;
Expand Down
Loading

0 comments on commit 7873f83

Please sign in to comment.