From d5a695f198c57239116712e0f01e51ef3a19a8b3 Mon Sep 17 00:00:00 2001 From: David Shaw Date: Thu, 20 Dec 2001 05:02:30 +0000 Subject: [PATCH] New function to check the permissions of GNUPGHOME and the various files that live there for safe permission/ownership (--no-permission-warning to disable) The newer glibcs print scary warnings about using mktemp(). The use here was actually safe, but the warning was bound to confuse people, so here is an arguably better tempname creator that pulls random bits from the pool. --- g10/ChangeLog | 24 +++++++++++++++++++++ g10/g10.c | 34 ++++++++++++++++++++++++++++++ g10/keydb.c | 2 ++ g10/keyserver.c | 54 ++++++++++++++++++++++++++++++++++-------------- g10/main.h | 1 + g10/misc.c | 55 ++++++++++++++++++++++++++++++++++++++++++++----- g10/options.h | 2 ++ g10/tdbio.c | 2 ++ 8 files changed, 153 insertions(+), 21 deletions(-) diff --git a/g10/ChangeLog b/g10/ChangeLog index 5ff54e138..94d34c13e 100644 --- a/g10/ChangeLog +++ b/g10/ChangeLog @@ -1,3 +1,27 @@ +2001-12-19 David Shaw + + * misc.c (check_permissions): New function to stat() and ensure + the permissions of GNUPGHOME and the files have safe permissions. + + * keydb.c (keydb_add_resource): Check keyring permissions. + + * tdbio.c (tdbio_set_dbname): Check permissions of trustdb.gpg + + * keyserver.c (keyserver_spawn): Disable keyserver schemes that + involve running external programs if the options file has unsafe + permissions or ownership. + + * g10.c, options.h: New option --no-permission-warning to disable + the permission warning message(s). This also permits use of the + keyserver if it had been disabled (see above). Also check the + permissions/ownership of random_seed. + + * keyserver.c (keyserver_spawn): The new glibc prints a warning + when using mktemp() (the code was already secure, but the warning + was bound to cause confusion). Use a different implementation + based on get_random_bits() instead. Also try a few times to get + the temp dir before giving up. + 2001-12-19 Werner Koch * g10.c, passphrase.c [CYGWIN32]: Allow this as an alias for MINGW32. diff --git a/g10/g10.c b/g10/g10.c index e2ad269c7..d1a277cc8 100644 --- a/g10/g10.c +++ b/g10/g10.c @@ -166,6 +166,7 @@ enum cmd_and_opt_values { aNull = 0, oNoVerbose, oTrustDBName, oNoSecmemWarn, + oNoPermissionWarn, oNoArmor, oNoDefKeyring, oNoGreeting, @@ -408,6 +409,7 @@ static ARGPARSE_OPTS opts[] = { { oNoVerbose, "no-verbose", 0, "@"}, { oTrustDBName, "trustdb-name", 2, "@" }, { oNoSecmemWarn, "no-secmem-warning", 0, "@" }, /* used only by regression tests */ + { oNoPermissionWarn, "no-permission-warning", 0, "@" }, { oNoArmor, "no-armor", 0, "@"}, { oNoArmor, "no-armour", 0, "@"}, { oNoDefKeyring, "no-default-keyring", 0, "@" }, @@ -682,6 +684,7 @@ main( int argc, char **argv ) char **orig_argv; const char *fname; char *username; + STRLIST unsafe_files=NULL; int may_coredump; STRLIST sl, remusr= NULL, locusr=NULL; STRLIST nrings=NULL, sec_nrings=NULL; @@ -815,6 +818,20 @@ main( int argc, char **argv ) pargs.flags= 1; /* do not remove the args */ next_pass: if( configname ) { + + if(check_permissions(configname,1)) + { + add_to_strlist(&unsafe_files,configname); + + /* If any options file is unsafe, then disable the keyserver + code. Since the keyserver code can call an external + program, and the external program to call is set in the + options file, a unsafe options file can lead to an + arbitrary program being run. */ + + opt.keyserver_disable=1; + } + configlineno = 0; configfp = fopen( configname, "r" ); if( !configfp ) { @@ -988,6 +1005,8 @@ main( int argc, char **argv ) case oAlwaysTrust: opt.always_trust = 1; break; case oLoadExtension: #ifndef __riscos__ + if(check_permissions(pargs.r.ret_str,1)) + add_to_strlist(&unsafe_files,pargs.r.ret_str); register_cipher_extension(orig_argc? *orig_argv:NULL, pargs.r.ret_str); #else /* __riscos__ */ @@ -1089,6 +1108,7 @@ main( int argc, char **argv ) case oCipherAlgo: def_cipher_string = m_strdup(pargs.r.ret_str); break; case oDigestAlgo: def_digest_string = m_strdup(pargs.r.ret_str); break; case oNoSecmemWarn: secmem_set_flags( secmem_get_flags() | 1 ); break; + case oNoPermissionWarn: opt.no_perm_warn=1; break; case oCharset: if( set_native_charset( pargs.r.ret_str ) ) log_error(_("%s is not a valid character set\n"), @@ -1162,6 +1182,7 @@ main( int argc, char **argv ) default : pargs.err = configfp? 1:2; break; } } + if( configfp ) { fclose( configfp ); configfp = NULL; @@ -1187,6 +1208,18 @@ main( int argc, char **argv ) } #endif + check_permissions(opt.homedir,0); + + if(unsafe_files) + { + STRLIST tmp; + + for(tmp=unsafe_files;tmp;tmp=tmp->next) + check_permissions(tmp->d,0); + + free_strlist(unsafe_files); + } + if( may_coredump && !opt.quiet ) log_info(_("WARNING: program may create a core file!\n")); @@ -1334,6 +1367,7 @@ main( int argc, char **argv ) /* set the random seed file */ if( use_random_seed ) { char *p = make_filename(opt.homedir, "random_seed", NULL ); + check_permissions(p,0); set_random_seed_file(p); m_free(p); } diff --git a/g10/keydb.c b/g10/keydb.c index 8fb6b115a..f4888bf3a 100644 --- a/g10/keydb.c +++ b/g10/keydb.c @@ -114,6 +114,8 @@ keydb_add_resource (const char *url, int force, int secret) else filename = m_strdup (resname); + check_permissions(filename,0); + if (!force) force = secret? !any_secret : !any_public; diff --git a/g10/keyserver.c b/g10/keyserver.c index 1f9cf2100..908f510b5 100644 --- a/g10/keyserver.c +++ b/g10/keyserver.c @@ -34,6 +34,7 @@ #include "options.h" #include "memory.h" #include "keydb.h" +#include "cipher.h" #include "status.h" #include "i18n.h" #include "util.h" @@ -122,7 +123,7 @@ parse_keyserver_uri(char *uri) opt.keyserver_port="0"; else { - unsigned char *ch; + char *ch; /* Get the port */ opt.keyserver_port=strsep(&uri,"/"); @@ -278,6 +279,14 @@ keyserver_spawn(int action,STRLIST list,u32 (*kidlist)[2],int count) BUG (); #endif + if(opt.keyserver_disable && !opt.no_perm_warn) + { + log_info(_("keyserver scheme \"%s\" disabled due to unsafe " + "options file permissions\n"),opt.keyserver_scheme); + + return KEYSERVER_SCHEME_NOT_FOUND; + } + /* Build the filename for the helper to execute */ filename=m_alloc(strlen("gpgkeys_")+strlen(opt.keyserver_scheme)+1); @@ -287,16 +296,38 @@ keyserver_spawn(int action,STRLIST list,u32 (*kidlist)[2],int count) if(opt.keyserver_options.use_temp_files) { + int attempts; const char *tmp=get_temp_dir(); + byte *randombits; - tempdir=m_alloc(strlen(tmp)+1+8+11+1); - sprintf(tempdir,"%s" DIRSEP_S "gpg-XXXXXX",tmp); + tempdir=m_alloc(strlen(tmp)+1+12+1); - /* Yes, I'm using mktemp. No, this isn't automatically insecure - because of it. I am using it to make a temp dir, not a file, - and I happily fail if it already exists. */ + /* Try 4 times to make the temp directory */ + for(attempts=0;attempts<4;attempts++) + { + /* Using really random bits is probably overkill here. The + worst thing that can happen with a directory name collision + is that the user will get an error message. */ + randombits=get_random_bits(8*4,0,0); - mktemp(tempdir); + sprintf(tempdir,"%s" DIRSEP_S "gpg-%02X%02X%02X%02X",tmp, + randombits[0],randombits[1],randombits[2],randombits[3]); + + m_free(randombits); + + if(mkdir(tempdir,0700)==0) + { + madedir=1; + break; + } + } + + if(!madedir) + { + log_error(_("%s: can't create temp directory after %d tries: %s\n"), + tempdir,attempts,strerror(errno)); + goto fail; + } tempfile_in=m_alloc(strlen(tempdir)+1+10+1); sprintf(tempfile_in,"%s" DIRSEP_S "ksrvin" EXTSEP_S "txt",tempdir); @@ -304,15 +335,6 @@ keyserver_spawn(int action,STRLIST list,u32 (*kidlist)[2],int count) tempfile_out=m_alloc(strlen(tempdir)+1+11+1); sprintf(tempfile_out,"%s" DIRSEP_S "ksrvout" EXTSEP_S "txt",tempdir); - if(mkdir(tempdir,0700)==-1) - { - log_error(_("%s: can't create directory: %s\n"), - tempdir,strerror(errno)); - goto fail; - } - - madedir=1; - tochild=fopen(tempfile_in,"w"); if(tochild==NULL) { diff --git a/g10/main.h b/g10/main.h index 32ed0b3e8..4a94bdeb9 100644 --- a/g10/main.h +++ b/g10/main.h @@ -67,6 +67,7 @@ int openpgp_cipher_test_algo( int algo ); int openpgp_pk_test_algo( int algo, unsigned int usage_flags ); int openpgp_pk_algo_usage ( int algo ); int openpgp_md_test_algo( int algo ); +int check_permissions(const char *path,int checkonly); /*-- helptext.c --*/ void display_online_help( const char *keyword ); diff --git a/g10/misc.c b/g10/misc.c index e75c712da..d1ff14c4b 100644 --- a/g10/misc.c +++ b/g10/misc.c @@ -24,6 +24,9 @@ #include #include #include +#ifdef HAVE_STAT +#include +#endif #if defined(__linux__) && defined(__alpha__) && __GLIBC__ < 2 #include #include @@ -327,8 +330,6 @@ openpgp_pk_algo_usage ( int algo ) return use; } - - int openpgp_md_test_algo( int algo ) { @@ -337,16 +338,60 @@ openpgp_md_test_algo( int algo ) return check_digest_algo(algo); } +int +check_permissions(const char *path,int checkonly) +{ +#ifdef HAVE_STAT + struct stat statbuf; + int isdir=0; + if(opt.no_perm_warn) + return 0; + /* It's okay if the file doesn't exist */ + if(stat(path,&statbuf)!=0) + return 0; + isdir=S_ISDIR(statbuf.st_mode); + /* The user doesn't own the file */ + if(statbuf.st_uid != getuid()) + { + if(!checkonly) + log_info(_("Warning: unsafe ownership on %s \"%s\"\n"), + isdir?"directory":"file",path); + return 1; + } + /* This works for both directories and files - basically, we don't + care what the owner permissions are, so long as the group and + other permissions are 0. */ + if((statbuf.st_mode & (S_IRWXG|S_IRWXO)) != 0) + { + char *dir; + /* However, if the directory the directory/file is in is owned + by the user and is 700, then this is not a problem. + Theoretically, we could walk this test up to the root + directory /, but for the sake of sanity, I'm stopping at one + level down. */ + dir=make_dirname(path); + if(stat(dir,&statbuf)==0 && statbuf.st_uid==getuid() && + S_ISDIR(statbuf.st_mode) && (statbuf.st_mode & (S_IRWXG|S_IRWXO))==0) + { + m_free(dir); + return 0; + } + m_free(dir); + if(!checkonly) + log_info(_("Warning: unsafe permissions on %s \"%s\"\n"), + isdir?"directory":"file",path); + return 1; + } +#endif - - - + return 0; +} diff --git a/g10/options.h b/g10/options.h index 4f4eca4bf..6b54a8708 100644 --- a/g10/options.h +++ b/g10/options.h @@ -104,6 +104,8 @@ struct { int keep_temp_files:1; STRLIST other; } keyserver_options; + int keyserver_disable; + int no_perm_warn; char *temp_dir; int no_encrypt_to; int interactive; diff --git a/g10/tdbio.c b/g10/tdbio.c index 50064cf59..0c8f84246 100644 --- a/g10/tdbio.c +++ b/g10/tdbio.c @@ -444,6 +444,8 @@ tdbio_set_dbname( const char *new_dbname, int create ) : make_filename(opt.homedir, "trustdb" EXTSEP_S "gpg", NULL ); + check_permissions(fname,0); + if( access( fname, R_OK ) ) { if( errno != ENOENT ) { log_error( _("%s: can't access: %s\n"), fname, strerror(errno) );