Skip to content

Commit f752ae5

Browse files
millertmillert
committedOct 10, 2019
Treat an ID of -1 as invalid since that means "no change".
Fixes CVE-2019-14287. Found by Joe Vennix from Apple Information Security.
·
TAGSUDO_1_8_28p1
1 parent fd5d0f5 commit f752ae5

File tree

2 files changed

+57
-47
lines changed

2 files changed

+57
-47
lines changed
 

‎NEWS

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,10 @@ What's new in Sudo 1.8.28
7676
* Fixed a bug where the terminal's file context was not restored
7777
when using SELinux RBAC. Bug #898.
7878

79+
* Fixed CVE-2019-14287, a bug where a sudo user may be able to
80+
run a command as root when the Runas specification explicitly
81+
disallows root access as long as the ALL keyword is listed first.
82+
7983
What's new in Sudo 1.8.27
8084

8185
* On HP-UX, sudo will now update the utmps file when running a command

‎lib/util/strtoid.c

Lines changed: 53 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
/*
22
* SPDX-License-Identifier: ISC
33
*
4-
* Copyright (c) 2013-2016 Todd C. Miller <Todd.Miller@sudo.ws>
4+
* Copyright (c) 2013-2019 Todd C. Miller <Todd.Miller@sudo.ws>
55
*
66
* Permission to use, copy, modify, and distribute this software for any
77
* purpose with or without fee is hereby granted, provided that the above
@@ -48,6 +48,27 @@
4848
#include "sudo_debug.h"
4949
#include "sudo_util.h"
5050

51+
/*
52+
* Make sure that the ID ends with a valid separator char.
53+
*/
54+
static bool
55+
valid_separator(const char *p, const char *ep, const char *sep)
56+
{
57+
bool valid = false;
58+
debug_decl(valid_separator, SUDO_DEBUG_UTIL)
59+
60+
if (ep != p) {
61+
/* check for valid separator (including '\0') */
62+
if (sep == NULL)
63+
sep = "";
64+
do {
65+
if (*ep == *sep)
66+
valid = true;
67+
} while (*sep++ != '\0');
68+
}
69+
debug_return_bool(valid);
70+
}
71+
5172
/*
5273
* Parse a uid/gid in string form.
5374
* If sep is non-NULL, it contains valid separator characters (e.g. comma, space)
@@ -62,36 +83,33 @@ sudo_strtoid_v1(const char *p, const char *sep, char **endp, const char **errstr
6283
char *ep;
6384
id_t ret = 0;
6485
long long llval;
65-
bool valid = false;
6686
debug_decl(sudo_strtoid, SUDO_DEBUG_UTIL)
6787

6888
/* skip leading space so we can pick up the sign, if any */
6989
while (isspace((unsigned char)*p))
7090
p++;
71-
if (sep == NULL)
72-
sep = "";
91+
92+
/* While id_t may be 64-bit signed, uid_t and gid_t are 32-bit unsigned. */
7393
errno = 0;
7494
llval = strtoll(p, &ep, 10);
75-
if (ep != p) {
76-
/* check for valid separator (including '\0') */
77-
do {
78-
if (*ep == *sep)
79-
valid = true;
80-
} while (*sep++ != '\0');
95+
if ((errno == ERANGE && llval == LLONG_MAX) || llval > (id_t)UINT_MAX) {
96+
errno = ERANGE;
97+
if (errstr != NULL)
98+
*errstr = N_("value too large");
99+
goto done;
81100
}
82-
if (!valid) {
101+
if ((errno == ERANGE && llval == LLONG_MIN) || llval < INT_MIN) {
102+
errno = ERANGE;
83103
if (errstr != NULL)
84-
*errstr = N_("invalid value");
85-
errno = EINVAL;
104+
*errstr = N_("value too small");
86105
goto done;
87106
}
88-
if (errno == ERANGE) {
89-
if (errstr != NULL) {
90-
if (llval == LLONG_MAX)
91-
*errstr = N_("value too large");
92-
else
93-
*errstr = N_("value too small");
94-
}
107+
108+
/* Disallow id -1, which means "no change". */
109+
if (!valid_separator(p, ep, sep) || llval == -1 || llval == (id_t)UINT_MAX) {
110+
if (errstr != NULL)
111+
*errstr = N_("invalid value");
112+
errno = EINVAL;
95113
goto done;
96114
}
97115
ret = (id_t)llval;
@@ -108,30 +126,15 @@ sudo_strtoid_v1(const char *p, const char *sep, char **endp, const char **errstr
108126
{
109127
char *ep;
110128
id_t ret = 0;
111-
bool valid = false;
112129
debug_decl(sudo_strtoid, SUDO_DEBUG_UTIL)
113130

114131
/* skip leading space so we can pick up the sign, if any */
115132
while (isspace((unsigned char)*p))
116133
p++;
117-
if (sep == NULL)
118-
sep = "";
134+
119135
errno = 0;
120136
if (*p == '-') {
121137
long lval = strtol(p, &ep, 10);
122-
if (ep != p) {
123-
/* check for valid separator (including '\0') */
124-
do {
125-
if (*ep == *sep)
126-
valid = true;
127-
} while (*sep++ != '\0');
128-
}
129-
if (!valid) {
130-
if (errstr != NULL)
131-
*errstr = N_("invalid value");
132-
errno = EINVAL;
133-
goto done;
134-
}
135138
if ((errno == ERANGE && lval == LONG_MAX) || lval > INT_MAX) {
136139
errno = ERANGE;
137140
if (errstr != NULL)
@@ -144,28 +147,31 @@ sudo_strtoid_v1(const char *p, const char *sep, char **endp, const char **errstr
144147
*errstr = N_("value too small");
145148
goto done;
146149
}
147-
ret = (id_t)lval;
148-
} else {
149-
unsigned long ulval = strtoul(p, &ep, 10);
150-
if (ep != p) {
151-
/* check for valid separator (including '\0') */
152-
do {
153-
if (*ep == *sep)
154-
valid = true;
155-
} while (*sep++ != '\0');
156-
}
157-
if (!valid) {
150+
151+
/* Disallow id -1, which means "no change". */
152+
if (!valid_separator(p, ep, sep) || lval == -1) {
158153
if (errstr != NULL)
159154
*errstr = N_("invalid value");
160155
errno = EINVAL;
161156
goto done;
162157
}
158+
ret = (id_t)lval;
159+
} else {
160+
unsigned long ulval = strtoul(p, &ep, 10);
163161
if ((errno == ERANGE && ulval == ULONG_MAX) || ulval > UINT_MAX) {
164162
errno = ERANGE;
165163
if (errstr != NULL)
166164
*errstr = N_("value too large");
167165
goto done;
168166
}
167+
168+
/* Disallow id -1, which means "no change". */
169+
if (!valid_separator(p, ep, sep) || ulval == UINT_MAX) {
170+
if (errstr != NULL)
171+
*errstr = N_("invalid value");
172+
errno = EINVAL;
173+
goto done;
174+
}
169175
ret = (id_t)ulval;
170176
}
171177
if (errstr != NULL)

0 commit comments

Comments
 (0)
Please sign in to comment.