From: Thomas Walker Lynch Date: Thu, 21 Feb 2019 21:55:46 +0000 (+0100) Subject: checkpoint X-Git-Url: https://git.reasoningtechnology.com/style/static/gitweb.js?a=commitdiff_plain;h=cb448e0438a5dc26c7d991306b32b1b7d3935f16;p=subu checkpoint --- diff --git a/3_doc/subu-mk-0.txt b/3_doc/subu-mk-0.txt new file mode 100644 index 0000000..a817a3a --- /dev/null +++ b/3_doc/subu-mk-0.txt @@ -0,0 +1,50 @@ + If we allowed the subu_home directory to be put anywhere, this script could be + used by masteru gain access to any directory on the system. Hence, we have + the following constraints: + + constraint 1) the subu_home directory must not already exist, not as a + directory or as any other type of object. + + constraint 2) masteru must have 'x' privledges to reach $(masteru_home)/subuland, and have + permissions to create the subu_home subdirectory. + + constraint 3) subu_home may only be placed under the directory $(masteru_home)/subuland. + + convention 4) only subu_home directories may be placed in $(masteru_home)/subuland. + + -> without constraint 1), exploit 1 + + Suppose that a wily masteru was able to move an inaccessible directory that he + or she wanted access to and place it under $(masteru_home)/subuland. + Typically such directories are not moveable to wiley users in the first place, + but suppose the wily masteru found such a directory. + + Alternatively suppose the wily masteru made a directory under $(masteru_home)/subuland + of his or her own, and placed an inaccessible file or subdirectory inside of it. + + Alternatively suppose that the wily masteru made $(masteru_home)/subuland a hard link + or a symlink to a directory that contained a subdirectory that masteru did not have + access to. + + The the wiley masteru could create a subu by the same name as the directory he desired + access to, and trick the foolish subu-mk-0 into giving him or her access. + + -> without constraint 2) exploit 2 + + Then the masteru could place subu directories in places he can not access. + The foolish subu-mk-0 program would then add 'x' acls to as to reach this place. + The masteru could then change identity to the subu, (i.e. enter the container), + and then reach that place he or she could not reach before. + + -> without constraint 3) exploit 3 + + There is still a wrinkle. If masteru looses x privlege to a place, subu might keep + it, and then exploit 2 would work despite the existence of constraint 2. It would + be unusual that masteru not have x privledges to masteru_home. Furthmore, we + degree that masteru has x privige to subuland. Hence, exploit 3 can be prevented + by following simple accounting rules that are already normal. + + -> without convention 4) exploit 4 + + Constraint 4 is a convention that masteru + diff --git a/src/.deps/subu-mk-0.lib.Po b/src/.deps/subu-mk-0.lib.Po index 2e62d51..5682566 100644 --- a/src/.deps/subu-mk-0.lib.Po +++ b/src/.deps/subu-mk-0.lib.Po @@ -33,10 +33,10 @@ subu-mk-0.lib.o: subu-mk-0.lib.c /usr/include/stdc-predef.h \ /usr/include/alloca.h /usr/include/bits/stdlib-bsearch.h \ /usr/include/bits/stdlib-float.h /usr/include/pwd.h \ /usr/include/string.h /usr/include/strings.h /usr/include/sys/stat.h \ - /usr/include/bits/stat.h /usr/include/bits/statx.h dispatch.lib.h \ - local_common.h ../config.h \ - /usr/lib/gcc/x86_64-redhat-linux/8/include/stdbool.h dbprintf.aux.h \ - dispatch_useradd.lib.h subu-mk-0.lib.h + /usr/include/bits/stat.h /usr/include/bits/statx.h \ + /usr/lib/gcc/x86_64-redhat-linux/8/include/stdbool.h dispatch.lib.h \ + local_common.h ../config.h dbprintf.aux.h dispatch_useradd.lib.h \ + subu-mk-0.lib.h /usr/include/stdc-predef.h: @@ -166,14 +166,14 @@ subu-mk-0.lib.o: subu-mk-0.lib.c /usr/include/stdc-predef.h \ /usr/include/bits/statx.h: +/usr/lib/gcc/x86_64-redhat-linux/8/include/stdbool.h: + dispatch.lib.h: local_common.h: ../config.h: -/usr/lib/gcc/x86_64-redhat-linux/8/include/stdbool.h: - dbprintf.aux.h: dispatch_useradd.lib.h: diff --git a/src/dbprintf.aux.o b/src/dbprintf.aux.o deleted file mode 100644 index 1dc581d..0000000 Binary files a/src/dbprintf.aux.o and /dev/null differ diff --git a/src/dispatch.lib.c b/src/dispatch.lib.c deleted file mode 100644 index d4b9c4a..0000000 --- a/src/dispatch.lib.c +++ /dev/null @@ -1,68 +0,0 @@ -/* -fork/exec/wait a command - -if the error values returned by the exec'd program -are less than 1 << 16, then - -*/ - -// without this #define execvpe is undefined -#define _GNU_SOURCE -#include -#include - -#include -#include -#include -#include -#include "local_common.h" -#include "dispatch.lib.h" - - - -/* - Execs the command passed in argv[0]; - Returns -1 upon failure. - - The wstatus returned from wait() might be either the error we returned when - exec failed, or the return value from the command. An arbitary command is - passed in, so we don't know what its return values might be. Consquently, we - have no way of multiplexing a unique exec error code with the command return - value within wstatus. If the prorgrammer knows the return values of the command - passed in, and wants better behavior, he or she can spin a special version of - dispatch for that command. -*/ -int dispatch(char **argv, char **envp){ - if( !argv || !argv[0] ){ - fprintf(stderr, "argv[0] null. Null command passed into dispatch().\n"); - return -1; - } - #ifdef DEBUG - dbprintf("dispatching:"); - char **apt = argv; - while( apt ){ - dbprintf(" %s",*apt); - apt++; - } - dbprintf("\n"); - #endif - char *command = argv[0]; - pid_t pid = fork(); - if( pid == -1 ){ - fprintf(stderr, "fork() failed in dispatch().\n"); - return -1; - } - if( pid == 0 ){ // we are the child - execvpe(command, argv, envp); - // exec will only return if it has an error .. - perror(command); - return -1; - }else{ // we are the parent - int wstatus; - waitpid(pid, &wstatus, 0); - if(wstatus) - return -1; - else - return 0; - } -} diff --git a/src/dispatch.lib.h b/src/dispatch.lib.h deleted file mode 100644 index 620c6f9..0000000 --- a/src/dispatch.lib.h +++ /dev/null @@ -1,9 +0,0 @@ -#ifndef DISPATCH_LIB_H -#define DISPATCH_LIB_H -#include "local_common.h" - -int dispatch(char **argv, char **envp); - -#endif - - diff --git a/src/dispatch.lib.o b/src/dispatch.lib.o deleted file mode 100644 index 548fd25..0000000 Binary files a/src/dispatch.lib.o and /dev/null differ diff --git a/src/dispatch_exec.lib.c b/src/dispatch_exec.lib.c new file mode 100644 index 0000000..166fa28 --- /dev/null +++ b/src/dispatch_exec.lib.c @@ -0,0 +1,59 @@ +/* + fork/execs/wait the command passed in argv[0]; + Returns -1 upon failure. + + The wstatus returned from wait() might be either the error returned by exec + when it failed, or the return value from the command. An arbitary command is + passed in, so we don't know what its return values might be. Consquently, we + have no way of multiplexing a unique exec error code with the command return + value within wstatus. If the prorgrammer knows the return values of the + command passed in, and wants better behavior, he or she can spin a special + version of dispatch for that command. +*/ + +// without this #define execvpe is undefined +#define _GNU_SOURCE +#include +#include + +#include +#include +#include +#include +#include "local_common.h" +#include "dispatch_exec.lib.h" + +int dispatch_exec(char **argv, char **envp){ + if( !argv || !argv[0] ){ + fprintf(stderr, "argv[0] null. Null command passed into dispatch().\n"); + return -1; + } + #ifdef DEBUG + dbprintf("dispatching exec:"); + char **apt = argv; + while( apt ){ + dbprintf(" %s",*apt); + apt++; + } + dbprintf("\n"); + #endif + char *command = argv[0]; + pid_t pid = fork(); + if( pid == -1 ){ + fprintf(stderr, "fork() failed in dispatch().\n"); + return -1; + } + if( pid == 0 ){ // we are the child + execvpe(command, argv, envp); + // exec will only return if it has an error .. + perror(command); + return -1; + }else{ // we are the parent + int wstatus; + waitpid(pid, &wstatus, 0); + if(wstatus) + return -1; + else + return 0; + } +} diff --git a/src/dispatch_exec.lib.h b/src/dispatch_exec.lib.h new file mode 100644 index 0000000..29e66f5 --- /dev/null +++ b/src/dispatch_exec.lib.h @@ -0,0 +1,9 @@ +#ifndef DISPATCH_LIB_H +#define DISPATCH_LIB_H +#include "local_common.h" + +int dispatch_exec(char **argv, char **envp); + +#endif + + diff --git a/src/dispatch_f.lib.c b/src/dispatch_f.lib.c new file mode 100644 index 0000000..8baa18f --- /dev/null +++ b/src/dispatch_f.lib.c @@ -0,0 +1,79 @@ +/* + changes the uid, gid, and forks and calls the function + Returns -1 upon failure. + + The wstatus returned from wait() might be either the error returned by exec + when it failed, or the return value from the command. An arbitary command is + passed in, so we don't know what its return values might be. Consquently, we + have no way of multiplexing a unique exec error code with the command return + value within wstatus. If the prorgrammer knows the return values of the + command passed in, and wants better behavior, he or she can spin a special + version of dispatch for that command. +*/ + +// without this #define execvpe is undefined +#define _GNU_SOURCE + +#include +#include + +#include +#include +#include +#include +#include "local_common.h" +#include "dispatch_f.lib.h" + +int dispatch_f(char *fname, int (*f)()){ + char *perror_src = "displatch_f_as"; + #ifdef DEBUG + dbprintf("%s %s\n", perror_src, fname); + #endif + pid_t pid = fork(); + if( pid == -1 ){ + perror(perror_src); + fprintf(stderr, "%s %s\n", perror_src, fname); + return ERR_FORK; + } + if( pid == 0 ){ // we are the child + int ret = (*f)(); + return ret; + }else{ // we are the parent + int wstatus; + waitpid(pid, &wstatus, 0); + return wstatus; + } +} + +int dispatch_f_euid_egid(char *fname, int (*f)(), uid_t euid, gid_t egid){ + char *perror_src = "displatch_f_as"; + #ifdef DEBUG + dbprintf("%s %s %u %u\n", perror_src, fname, euid, egid); + #endif + pid_t pid = fork(); + if( pid == -1 ){ + perror(perror_src); + fprintf(stderr, "%s %s %u %u\n", perror_src, fname, euid, egid); + return ERR_FORK; + } + if( pid == 0 ){ // we are the child + if( seteuid(euid) == -1 ){ + perror(perror_src); + fprintf(stderr, "%s %s %u %u\n", perror_src, fname, euid, egid); + return ERR_SETEUID; + } + if( setegid(egid) == -1 ){ + perror(perror_src); + fprintf(stderr, "%s %s %u %u\n", perror_src, fname, euid, egid); + return ERR_SETEGID; + } + int ret = (*f)(); + return ret; + }else{ // we are the parent + int wstatus; + waitpid(pid, &wstatus, 0); + return wstatus; + } +} + + diff --git a/src/dispatch_f.lib.h b/src/dispatch_f.lib.h new file mode 100644 index 0000000..551d51a --- /dev/null +++ b/src/dispatch_f.lib.h @@ -0,0 +1,14 @@ +#ifndef DISPATCH_LIB_H +#define DISPATCH_LIB_H +#include "local_common.h" + +#define ERR_FORK -1; +#define ERR_SETEUID -2; +#define ERR_SETEGID -3; + +int dispatch_f(char *fname, int (*f)()); +int dispatch_f_euid_egid(char *fname, int (*f)(), uid_t euid, gid_t egid); + +#endif + + diff --git a/src/dispatch_useradd.lib.o b/src/dispatch_useradd.lib.o deleted file mode 100644 index a3b50a1..0000000 Binary files a/src/dispatch_useradd.lib.o and /dev/null differ diff --git a/src/subu-mk-0 b/src/subu-mk-0 deleted file mode 100755 index 0cc6234..0000000 Binary files a/src/subu-mk-0 and /dev/null differ diff --git a/src/subu-mk-0.cli.o b/src/subu-mk-0.cli.o deleted file mode 100644 index 0cb09fa..0000000 Binary files a/src/subu-mk-0.cli.o and /dev/null differ diff --git a/src/subu-mk-0.lib.c b/src/subu-mk-0.lib.c index caf5535..6e0d541 100644 --- a/src/subu-mk-0.lib.c +++ b/src/subu-mk-0.lib.c @@ -1,48 +1,68 @@ /* Makes a new subu user. - According to the man page, we are not alloed to free the memory allocated by getpwid(). + According to the man page, we are not allowed to free the memory allocated by getpwid(). + masteru is the user who ran this script. subu is the user being created. -setfacl -m d:u:masteru:rwX,u:masteru:rwX subuname + subu-mk-0 is a setuid root script. + + see also 3_doc/subu-mk-0.txt */ + // without this #define we get the warning: implicit declaration of function ‘seteuid’/‘setegid’ #define _GNU_SOURCE + #include #include - #include +#include #include #include #include #include #include +#include #include "dispatch.lib.h" #include "dispatch_useradd.lib.h" +#include "dispatch_f.lib.h" #include "subu-mk-0.lib.h" typedef unsigned int uint; +/* + Fedora 29's sss_cache is checking the inherited uid instead of the effective + uid, so setuid root scripts will fail when calling sss_cache. Fedora 29's + 'useradd' calls sss_cache. + +*/ +#define BUG_SSS_CACHE_RUID 1 + int subu_mk_0(char *subuname){ + char *perror_src = "subu_mk_0"; + //-------------------------------------------------------------------------------- #ifdef DEBUG - dbprintf("Checking we are running from a user and are setuid root.\n"); + dbprintf("Checking that we are running from a user and are setuid root.\n"); #endif - uid_t uid = getuid(); - uid_t euid = geteuid(); - gid_t gid = getgid(); - gid_t egid = getegid(); + uid_t master_uid = getuid(); + gid_t master_gid = getgid(); + uid_t set_euid = geteuid(); + gid_t set_egid = getegid(); #ifdef DEBUG - dbprintf("uid %u, gid %u, euid %u egid %u\n", uid, gid, euid, egid); + dbprintf("master_uid %u, master_gid %u, set_euid %u set_egid %u\n", master_uid, master_gid, set_euid, set_egid); #endif - if( uid == 0 || euid != 0 ){ + if( master_uid == 0 || set_euid != 0 ){ fprintf(stderr, "this program must be run setuid root from a user account\n"); return ERR_SETUID_ROOT; } //-------------------------------------------------------------------------------- + // recover the masteru user name from /etc/passwd + + // char *subuname was passed in as an argument size_t subuname_len; char *masteru_name; size_t masteru_name_len; @@ -90,20 +110,31 @@ int subu_mk_0(char *subuname){ #endif //-------------------------------------------------------------------------------- - // Just because masteru_home is referenced in /etc/passwd does not mean it exists. + // Just because masteru_home is referenced in /etc/passwd does not mean it exists, + // and does not mean that masteru owns it or has 'x' privileges. // We also require that the subuland sub directory exists. { #ifdef DEBUG dbprintf("checking that masteru_home and subuland exist\n"); #endif struct stat st; - if( stat(masteru_home, &st) == -1) { - fprintf(stderr, "Strange, masteru home does not exist, \"%s\".", masteru_home); + int stat_ret; + + stat_ret = stat(masteru_home, &st); + if( stat_ret == -1 ){ + fprintf(stderr, "masteru home directory does not exist, \"%s\".", masteru_home); + free(subuland); + return ERR_NOT_EXIST_MASTERU_HOME; + }else if( !S_ISDIR(st.st_mode) ) { + fprintf(stderr, "strange masteru home directory is not a directory, \"%s\".", masteru_home); free(subuland); return ERR_NOT_EXIST_MASTERU_HOME; + }else if( ){ } - if( stat(subuland, &st) == -1) { - fprintf(stderr, "$masteru_home/subuland/ does not exist"); + + stat(subuland, &st); + if( !S_ISDIR(st.st_mode) ) { + fprintf(stderr, "$masteru_home/subuland/ directory does not exist"); free(subuland); return ERR_NOT_EXIST_SUBULAND; } @@ -120,7 +151,7 @@ int subu_mk_0(char *subuname){ subuhome_len = subuland_len + subuname_len; subuhome = (char *)malloc(subuhome_len + 1); if( !subuhome ){ - perror("subu_mk_0"); + perror(perror_src); free(subuland); return ERR_MALLOC; } @@ -128,7 +159,7 @@ int subu_mk_0(char *subuname){ strcpy (subuhome + subuland_len, subuname); } #ifdef DEBUG - dbprintf("subuhome %s\n", subuhome); + dbprintf("subuhome: \"%s\"\n", subuhome); #endif /*-------------------------------------------------------------------------------- @@ -141,20 +172,42 @@ int subu_mk_0(char *subuname){ -d, --home-dir HOME_DIR The new user will be created using HOME_DIR as the value for the user's login directory. ... The directory HOME_DIR does not have to exist but will not be created if it is missing. + + Actually Fedora 29's 'useradd' is making the directory even when -d is specified. + Adding the -M option supresses it. */ + uid_t subuuid; gid_t subugid; + bool subuhome_already_exists = false; { #ifdef DEBUG - dbprintf("making subu\n"); + dbprintf("making subu\n"); #endif + + #if BUG_SSS_CACHE_RUID + if( setuid(0) == -1 ){ + perror(perror_src); + return ERR_BUG_SSS; + } + #endif + struct stat st; + if( stat(subuhome, &st) != -1 ){ + if( !S_ISDIR(st.st_mode) ) { + subuhome_already_exists = true; + }else{ + fprintf(stderr, "Home directory would clobber non-directory object already at %s\n", subuhome); + return ERR_MK_SUBUHOME; + }} + char *command = "/usr/sbin/useradd"; - char *argv[5]; + char *argv[6]; argv[0] = command; argv[1] = subuname; argv[2] = "-d"; argv[3] = subuhome; - argv[4] = (char *) NULL; + argv[4] = -M + argv[5] = (char *) NULL; char *envp[1]; envp[0] = (char *) NULL; struct dispatch_useradd_ret_t ret; @@ -167,8 +220,32 @@ int subu_mk_0(char *subuname){ } subuuid = ret.pw_record->pw_uid; subugid = ret.pw_record->pw_gid; + bool err_mk_subuhome = false; + if( !subuhome_already_exists && stat(subuhome, &st) != -1 ){ + if( S_ISDIR(st.st_mode) ){ + #if !BUG_USERADD_ALWAYS_MKHOME + err_mk_subuhome = true; + fprintf(stderr, "useradd -d unexpectedly created the subuhome, will delete that now\n"); + #endif + if( rmdir(subuhome) == -1 ){ + err_mk_subuhome = true; + fprintf(stderr, "could not delete the subuhome created by useradd, bailing\n"); + return ERR_MK_SUBUHOME; + } + }else{ + err_mk_subuhome = true; + fprintf(stderr, "useradd, or a parallel running process, has created a non-directory object at subuhome\n"); + return ERR_MK_SUBUHOME; + }} + + if( err_mk_subuhome ) + fprintf(stderr, "encountered some difficulties when attempging to make subu, you better have a look\n"); + #ifdef DEBUG - dbprintf("subu made without errors\n", command, subuname, subuhome); + if( err_mk_subuhome ) + dbprintf("useradd finished"); + else + dbprintf("useradd finished with no errors\n"); #endif } @@ -213,14 +290,14 @@ int subu_mk_0(char *subuname){ #endif int ret = mkdir(subuhome, 0x0700); if( ret == -1 ){ - perror("subu_mk_0"); + perror(perror_src); free(subuland); free(subuhome); return ERR_MK_SUBUHOME; } ret = chown(subuhome, subuuid, subugid); if( ret == -1 ){ - perror("subu_mk_0"); + perror(perror_src); free(subuland); free(subuhome); return ERR_MK_SUBUHOME; diff --git a/src/subu-mk-0.lib.h b/src/subu-mk-0.lib.h index a93701f..018289c 100644 --- a/src/subu-mk-0.lib.h +++ b/src/subu-mk-0.lib.h @@ -11,7 +11,8 @@ #define ERR_FAILED_USERADD 8 #define ERR_SETFACL 9 #define ERR_MALLOC 10 -#define ERR_MK_SUBUHOME 11 +#define ERR_BUG_SSS 11 +#define ERR_MK_SUBUHOME 12 int subu_mk_0(char *subuname); diff --git a/src/subu-mk-0.lib.o b/src/subu-mk-0.lib.o deleted file mode 100644 index 045b335..0000000 Binary files a/src/subu-mk-0.lib.o and /dev/null differ diff --git a/src/try_rmdir b/src/try_rmdir new file mode 100755 index 0000000..2c4c607 Binary files /dev/null and b/src/try_rmdir differ diff --git a/src/try_rmdir.c b/src/try_rmdir.c new file mode 100644 index 0000000..775c02d --- /dev/null +++ b/src/try_rmdir.c @@ -0,0 +1,8 @@ + +#include +#include +int main(){ + int retval = rmdir("/home/morpheus/subuland/ttemp0"); + printf("retval %d\n",retval); + return 0; +}