Skip to content

Commit f09ab43

Browse files
author
git-core
committed
Use fork/exec instead of system(3)
By doing so we may - move change of identity to the child, so no need to change it back - drop saved set-user/group-ID safely An attacker will gain no more than a process with its ids. As a side effect, the code that unsets environment variables becomes useless. - implement asynchronous wait for child later
1 parent 0fa617a commit f09ab43

File tree

3 files changed

+71
-74
lines changed

3 files changed

+71
-74
lines changed

‎activity.c

Lines changed: 44 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -15,72 +15,61 @@
1515
** limitations under the License.
1616
*/
1717

18+
#include <sys/types.h>
1819
#include <unistd.h>
1920
#include <stdlib.h>
21+
#include <fcntl.h>
22+
#include <paths.h>
23+
#include <sys/wait.h>
2024

2125
#include "su.h"
2226

2327
int send_intent(const struct su_context *ctx,
2428
const char *socket_path, int allow, const char *action)
2529
{
26-
char command[PATH_MAX];
27-
pid_t euid;
28-
gid_t egid;
2930
int rc;
3031

31-
sprintf(command, "/system/bin/am broadcast -a '%s' --es socket '%s' --ei caller_uid '%d' --ei allow '%d' --ei version_code '%d' > /dev/null",
32-
action, socket_path, ctx->from.uid, allow, VERSION_CODE);
32+
pid_t pid = fork();
33+
/* Child */
34+
if (!pid) {
35+
char command[ARG_MAX];
3336

34-
// before sending the intent, make sure the (uid and euid) and (gid and egid) match,
35-
// otherwise LD_LIBRARY_PATH is wiped in Android 4.0+.
36-
// Also, sanitize all secure environment variables (from linker_environ.c in linker).
37+
snprintf(command, sizeof(command),
38+
"exec /system/bin/am broadcast -a %s --es socket '%s' "
39+
"--ei caller_uid %d --ei allow %d "
40+
"--ei version_code %d",
41+
action, socket_path, ctx->from.uid, allow, VERSION_CODE);
42+
char *args[] = { "sh", "-c", command, NULL, };
3743

38-
/* The same list than GLibc at this point */
39-
static const char* const unsec_vars[] = {
40-
"GCONV_PATH",
41-
"GETCONF_DIR",
42-
"HOSTALIASES",
43-
"LD_AUDIT",
44-
"LD_DEBUG",
45-
"LD_DEBUG_OUTPUT",
46-
"LD_DYNAMIC_WEAK",
47-
"LD_LIBRARY_PATH",
48-
"LD_ORIGIN_PATH",
49-
"LD_PRELOAD",
50-
"LD_PROFILE",
51-
"LD_SHOW_AUXV",
52-
"LD_USE_LOAD_BIAS",
53-
"LOCALDOMAIN",
54-
"LOCPATH",
55-
"MALLOC_TRACE",
56-
"MALLOC_CHECK_",
57-
"NIS_PATH",
58-
"NLSPATH",
59-
"RESOLV_HOST_CONF",
60-
"RES_OPTIONS",
61-
"TMPDIR",
62-
"TZDIR",
63-
"LD_AOUT_LIBRARY_PATH",
64-
"LD_AOUT_PRELOAD",
65-
// not listed in linker, used due to system() call
66-
"IFS",
67-
};
68-
const char* const* cp = unsec_vars;
69-
const char* const* endp = cp + sizeof(unsec_vars)/sizeof(unsec_vars[0]);
70-
while (cp < endp) {
71-
unsetenv(*cp);
72-
cp++;
44+
/*
45+
* before sending the intent, make sure the effective uid/gid match
46+
* the real uid/gid, otherwise LD_LIBRARY_PATH is wiped
47+
* in Android 4.0+.
48+
*/
49+
set_identity(ctx->from.uid);
50+
int zero = open("/dev/zero", O_RDONLY | O_CLOEXEC);
51+
dup2(zero, 0);
52+
int null = open("/dev/null", O_WRONLY | O_CLOEXEC);
53+
dup2(null, 1);
54+
dup2(null, 2);
55+
LOGD("Executing %s\n", command);
56+
execv(_PATH_BSHELL, args);
57+
PLOGE("exec am");
58+
_exit(EXIT_FAILURE);
7359
}
74-
75-
// sane value so "am" works
76-
setenv("LD_LIBRARY_PATH", "/vendor/lib:/system/lib", 1);
77-
euid = geteuid();
78-
egid = getegid();
79-
setegid(getgid());
80-
seteuid(getuid());
81-
rc = system(command);
82-
seteuid(0);
83-
setegid(egid);
84-
seteuid(euid);
85-
return rc;
60+
/* Parent */
61+
if (pid < 0) {
62+
PLOGE("fork");
63+
return -1;
64+
}
65+
pid = waitpid(pid, &rc, 0);
66+
if (pid < 0) {
67+
PLOGE("waitpid");
68+
return -1;
69+
}
70+
if (!WIFEXITED(rc) || WEXITSTATUS(rc)) {
71+
LOGE("am returned error %d\n", rc);
72+
return -1;
73+
}
74+
return 0;
8675
}

‎su.c

Lines changed: 26 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -123,6 +123,26 @@ static void populate_environment(const struct su_context *ctx)
123123
}
124124
}
125125

126+
void set_identity(unsigned int uid)
127+
{
128+
/*
129+
* Set effective uid back to root, otherwise setres[ug]id will fail
130+
* if uid isn't root.
131+
*/
132+
if (seteuid(0)) {
133+
PLOGE("seteuid (root)");
134+
exit(EXIT_FAILURE);
135+
}
136+
if (setresgid(uid, uid, uid)) {
137+
PLOGE("setresgid (%u)", uid);
138+
exit(EXIT_FAILURE);
139+
}
140+
if (setresuid(uid, uid, uid)) {
141+
PLOGE("setresuid (%u)", uid);
142+
exit(EXIT_FAILURE);
143+
}
144+
}
145+
126146
static void socket_cleanup(void)
127147
{
128148
unlink(socket_path);
@@ -308,25 +328,8 @@ static void allow(const struct su_context *ctx)
308328
arg0 = p;
309329
}
310330

311-
/*
312-
* Set effective uid back to root, otherwise setres[ug]id will fail
313-
* if ctx->to.uid isn't root.
314-
*/
315-
if (seteuid(0)) {
316-
PLOGE("seteuid (root)");
317-
exit(EXIT_FAILURE);
318-
}
319-
320331
populate_environment(ctx);
321-
322-
if (setresgid(ctx->to.uid, ctx->to.uid, ctx->to.uid)) {
323-
PLOGE("setresgid (%u)", ctx->to.uid);
324-
exit(EXIT_FAILURE);
325-
}
326-
if (setresuid(ctx->to.uid, ctx->to.uid, ctx->to.uid)) {
327-
PLOGE("setresuid (%u)", ctx->to.uid);
328-
exit(EXIT_FAILURE);
329-
}
332+
set_identity(ctx->to.uid);
330333

331334
#define PARG(arg) \
332335
(ctx->to.optind + (arg) < ctx->to.argc) ? " " : "", \
@@ -498,6 +501,11 @@ int main(int argc, char *argv[])
498501
}
499502
}
500503

504+
/*
505+
* set LD_LIBRARY_PATH if the linker has wiped out it due to we're suid.
506+
* This occurs on Android 4.0+
507+
*/
508+
setenv("LD_LIBRARY_PATH", "/vendor/lib:/system/lib", 0);
501509
if (ctx.from.uid == AID_ROOT || ctx.from.uid == AID_SHELL)
502510
allow(&ctx);
503511

‎su.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,7 @@ enum {
7979
};
8080

8181
extern int database_check(const struct su_context *ctx);
82-
82+
extern void set_identity(unsigned int uid);
8383
extern int send_intent(const struct su_context *ctx,
8484
const char *socket_path, int allow, const char *action);
8585

0 commit comments

Comments
 (0)