From b84930b157bcebe50ae8de142213b2192ad1b7bb Mon Sep 17 00:00:00 2001 From: csoler Date: Thu, 25 Jul 2013 20:05:31 +0000 Subject: [PATCH] Added multiple security tests to pgp keyring against disk full and exceeded quota. - copy files to tmp before appending new keys, then rename back to original. - always check for disk full before syncing keyrings - use tmp file to sync keyring - new RsDirUtil::fileExists() method - check system for disk space in pgp directory - added RsPGPDirectory() method to RsInit git-svn-id: http://svn.code.sf.net/p/retroshare/code/trunk@6541 b45a01b8-16f6-495d-af2f-9b41ad6348cc --- libretroshare/src/pgp/pgphandler.cc | 78 +++++++++++++++++++++++--- libretroshare/src/retroshare/rsinit.h | 1 + libretroshare/src/retroshare/rstypes.h | 1 + libretroshare/src/rsserver/rsinit.cc | 10 +++- libretroshare/src/util/rsdir.cc | 5 ++ libretroshare/src/util/rsdir.h | 1 + libretroshare/src/util/rsdiscspace.cc | 8 ++- 7 files changed, 94 insertions(+), 10 deletions(-) diff --git a/libretroshare/src/pgp/pgphandler.cc b/libretroshare/src/pgp/pgphandler.cc index 84fa57a17..fdb65a142 100644 --- a/libretroshare/src/pgp/pgphandler.cc +++ b/libretroshare/src/pgp/pgphandler.cc @@ -25,6 +25,7 @@ extern "C" { #include "retroshare/rsiface.h" // For rsicontrol. #include "retroshare/rspeers.h" // For rsicontrol. #include "util/rsdir.h" +#include "util/rsdiscspace.h" #include "pgp/pgpkeyutil.h" static const uint32_t PGP_CERTIFICATE_LIMIT_MAX_NAME_SIZE = 64 ; @@ -363,6 +364,11 @@ bool PGPHandler::GeneratePGPCertificate(const std::string& name, const std::stri { // Some basic checks + if(!RsDiscSpace::checkForDiscSpace(RS_PGP_DIRECTORY)) + { + errString = std::string("(EE) low disc space in pgp directory. Can't write safely to keyring.") ; + return false ; + } if(name.length() > PGP_CERTIFICATE_LIMIT_MAX_NAME_SIZE) { errString = std::string("(EE) name in certificate exceeds the maximum allowed name size") ; @@ -437,7 +443,14 @@ bool PGPHandler::GeneratePGPCertificate(const std::string& name, const std::stri // 5 - add key to secret keyring on disk. cinfo = NULL ; - int fd=ops_setup_file_append(&cinfo, _secring_path.c_str()); + std::string secring_path_tmp = _secring_path + ".tmp" ; + + if(RsDirUtil::fileExists(_secring_path) && !RsDirUtil::copyFile(_secring_path,secring_path_tmp)) + { + errString= std::string("Cannot copy secret keyring !! Disk full? Out of disk quota?") ; + return false ; + } + int fd=ops_setup_file_append(&cinfo, secring_path_tmp.c_str()); if(!ops_write_transferable_secret_key(key,(unsigned char *)passphrase.c_str(),passphrase.length(),ops_false,cinfo)) { @@ -446,10 +459,23 @@ bool PGPHandler::GeneratePGPCertificate(const std::string& name, const std::stri } ops_teardown_file_write(cinfo,fd) ; + if(!RsDirUtil::renameFile(secring_path_tmp,_secring_path)) + { + errString= std::string("Cannot rename tmp secret key file ") + secring_path_tmp + " into " + _secring_path +". Disk error?" ; + return false ; + } + // 6 - copy the public key to the public keyring on disk cinfo = NULL ; - fd=ops_setup_file_append(&cinfo, _pubring_path.c_str()); + std::string pubring_path_tmp = _pubring_path + ".tmp" ; + + if(RsDirUtil::fileExists(_pubring_path) && !RsDirUtil::copyFile(_pubring_path,pubring_path_tmp)) + { + errString= std::string("Cannot encode secret key to disk!! Disk full? Out of disk quota?") ; + return false ; + } + fd=ops_setup_file_append(&cinfo, pubring_path_tmp.c_str()); if(!ops_write_transferable_public_key(key, ops_false, cinfo)) { @@ -458,6 +484,11 @@ bool PGPHandler::GeneratePGPCertificate(const std::string& name, const std::stri } ops_teardown_file_write(cinfo,fd) ; + if(!RsDirUtil::renameFile(pubring_path_tmp,_pubring_path)) + { + errString= std::string("Cannot rename tmp public key file ") + pubring_path_tmp + " into " + _pubring_path +". Disk error?" ; + return false ; + } // 7 - clean ops_keydata_free(key) ; @@ -806,6 +837,11 @@ bool PGPHandler::importGPGKeyPair(const std::string& filename,PGPIdType& importe } ops_validate_result_free(result); + if(!RsDiscSpace::checkForDiscSpace(RS_PGP_DIRECTORY)) + { + import_error = std::string("(EE) low disc space in pgp directory. Can't write safely to keyring.") ; + return false ; + } // 5 - All test passed. Adding key to keyring. // { @@ -818,7 +854,19 @@ bool PGPHandler::importGPGKeyPair(const std::string& filename,PGPIdType& importe RsStackFileLock flck(_pgp_lock_filename) ; // lock access to PGP directory. ops_create_info_t *cinfo = NULL ; - int fd=ops_setup_file_append(&cinfo, _secring_path.c_str()); + + // Make a copy of the secret keyring + // + std::string secring_path_tmp = _secring_path + ".tmp" ; + if(RsDirUtil::fileExists(_secring_path) && !RsDirUtil::copyFile(_secring_path,secring_path_tmp)) + { + import_error = "(EE) Cannot write secret key to disk!! Disk full? Out of disk quota. Keyring will be left untouched." ; + return false ; + } + + // Append the new key + + int fd=ops_setup_file_append(&cinfo, secring_path_tmp.c_str()); if(!ops_write_transferable_secret_key_from_packet_data(seckey,ops_false,cinfo)) { @@ -827,6 +875,14 @@ bool PGPHandler::importGPGKeyPair(const std::string& filename,PGPIdType& importe } ops_teardown_file_write(cinfo,fd) ; + // Rename the new keyring to overwrite the old one. + // + if(!RsDirUtil::renameFile(secring_path_tmp,_secring_path)) + { + import_error = " (EE) Cannot move temp file " + secring_path_tmp + ". Bad write permissions?" ; + return false ; + } + addNewKeyToOPSKeyring(_secring,*seckey) ; initCertificateInfo(_secret_keyring_map[ imported_key_id.toStdString() ],seckey,_secring->nkeys-1) ; } @@ -1003,8 +1059,10 @@ bool PGPHandler::encryptTextToFile(const PGPIdType& key_id,const std::string& te { RsStackMutex mtx(pgphandlerMtx) ; // lock access to PGP memory structures. + std::string outfile_tmp = outfile + ".tmp" ; + ops_create_info_t *info; - int fd = ops_setup_file_write(&info, outfile.c_str(), ops_true); + int fd = ops_setup_file_write(&info, outfile_tmp.c_str(), ops_true); const ops_keydata_t *public_key = locked_getPublicKey(key_id,true) ; @@ -1016,13 +1074,13 @@ bool PGPHandler::encryptTextToFile(const PGPIdType& key_id,const std::string& te if(public_key->type != OPS_PTAG_CT_PUBLIC_KEY) { - std::cerr << "PGPHandler::encryptTextToFile(): ERROR: supplied id did not return a public key!" << outfile << std::endl; + std::cerr << "PGPHandler::encryptTextToFile(): ERROR: supplied id did not return a public key!" << std::endl; return false ; } if (fd < 0) { - std::cerr << "PGPHandler::encryptTextToFile(): ERROR: Cannot write to " << outfile << std::endl; + std::cerr << "PGPHandler::encryptTextToFile(): ERROR: Cannot write to " << outfile_tmp << std::endl; return false ; } ops_encrypt_stream(info, public_key, NULL, ops_false, ops_true); @@ -1030,6 +1088,12 @@ bool PGPHandler::encryptTextToFile(const PGPIdType& key_id,const std::string& te ops_writer_close(info); ops_create_info_delete(info); + if(!RsDirUtil::renameFile(outfile_tmp,outfile)) + { + std::cerr << "PGPHandler::encryptTextToFile(): ERROR: Cannot rename " + outfile_tmp + " to " + outfile + ". Disk error?" << std::endl; + return false ; + } + return true ; } @@ -1631,7 +1695,7 @@ bool PGPHandler::locked_syncPublicKeyring() } // Now check if the pubring was locally modified, which needs saving it again - if(_pubring_changed) + if(_pubring_changed && RsDiscSpace::checkForDiscSpace(RS_PGP_DIRECTORY)) { std::string tmp_keyring_file = _pubring_path + ".tmp" ; diff --git a/libretroshare/src/retroshare/rsinit.h b/libretroshare/src/retroshare/rsinit.h index 1400e2bf2..9e7001e4f 100644 --- a/libretroshare/src/retroshare/rsinit.h +++ b/libretroshare/src/retroshare/rsinit.h @@ -126,6 +126,7 @@ class RsInit */ static std::string RsConfigDirectory(); static std::string RsConfigKeysDirectory(); + static std::string RsPGPDirectory(); static std::string RsProfileConfigDirectory(); static bool getStartMinimised() ; diff --git a/libretroshare/src/retroshare/rstypes.h b/libretroshare/src/retroshare/rstypes.h index ec7e8f875..af3254eff 100644 --- a/libretroshare/src/retroshare/rstypes.h +++ b/libretroshare/src/retroshare/rstypes.h @@ -57,6 +57,7 @@ const uint32_t FT_STATE_CHECKING_HASH = 0x0007 ; const uint32_t RS_PARTIALS_DIRECTORY = 0x0000 ; const uint32_t RS_DOWNLOAD_DIRECTORY = 0x0001 ; const uint32_t RS_CONFIG_DIRECTORY = 0x0002 ; +const uint32_t RS_PGP_DIRECTORY = 0x0003 ; class Sha1CheckSum { diff --git a/libretroshare/src/rsserver/rsinit.cc b/libretroshare/src/rsserver/rsinit.cc index 689f98d8f..c009de129 100644 --- a/libretroshare/src/rsserver/rsinit.cc +++ b/libretroshare/src/rsserver/rsinit.cc @@ -609,7 +609,8 @@ int RsInit::InitRetroShare(int argcIgnored, char **argvIgnored, bool strictCheck get_configinit(RsInitConfig::basedir, RsInitConfig::preferedId); - std::string pgp_dir = RsInitConfig::basedir + "/pgp" ; + std::string pgp_dir = RsPGPDirectory() ; + if(!RsDirUtil::checkCreateDirectory(pgp_dir)) throw std::runtime_error("Cannot create pgp directory " + pgp_dir) ; @@ -719,7 +720,8 @@ bool RsInit::importIdentity(const std::string& fname,std::string& id,std::string bool RsInit::copyGnuPGKeyrings() { - std::string pgp_dir = RsInitConfig::basedir + "/pgp" ; + std::string pgp_dir = RsPGPDirectory() ; + if(!RsDirUtil::checkCreateDirectory(pgp_dir)) throw std::runtime_error("Cannot create pgp directory " + pgp_dir) ; @@ -1720,6 +1722,10 @@ std::string RsInit::RsConfigDirectory() { return RsInitConfig::basedir; } +std::string RsInit::RsPGPDirectory() +{ + return RsInitConfig::basedir + "/pgp" ; +} std::string RsInit::RsProfileConfigDirectory() { diff --git a/libretroshare/src/util/rsdir.cc b/libretroshare/src/util/rsdir.cc index 85fd5bc4e..367a9d24f 100644 --- a/libretroshare/src/util/rsdir.cc +++ b/libretroshare/src/util/rsdir.cc @@ -291,6 +291,11 @@ int RsDirUtil::breakupDirList(const std::string& path, return 1; } +/**** Copied and Tweaked from ftcontroller ***/ +bool RsDirUtil::fileExists(const std::string& filename) +{ + return ( access( filename.c_str(), F_OK ) != -1 ); +} /**** Copied and Tweaked from ftcontroller ***/ bool RsDirUtil::copyFile(const std::string& source,const std::string& dest) diff --git a/libretroshare/src/util/rsdir.h b/libretroshare/src/util/rsdir.h index 543973637..41736dc21 100644 --- a/libretroshare/src/util/rsdir.h +++ b/libretroshare/src/util/rsdir.h @@ -81,6 +81,7 @@ bool crc32File(FILE *f,uint64_t file_size,uint32_t chunk_size,CRC32Map& map) ; int breakupDirList(const std::string& path, std::list &subdirs); bool copyFile(const std::string& source,const std::string& dest); +bool fileExists(const std::string& file); bool checkFile(const std::string& filename,bool disallow_empty_file = false); bool checkDirectory(const std::string& dir); bool checkCreateDirectory(const std::string& dir); diff --git a/libretroshare/src/util/rsdiscspace.cc b/libretroshare/src/util/rsdiscspace.cc index dc4f51adc..b0e299964 100644 --- a/libretroshare/src/util/rsdiscspace.cc +++ b/libretroshare/src/util/rsdiscspace.cc @@ -130,7 +130,7 @@ bool RsDiscSpace::checkForDiscSpace(RsDiscSpace::DiscLocation loc) { RsStackMutex m(_mtx) ; // Locked - if(_partials_path == "" || _download_path == "") + if(_partials_path == "" && loc == RS_PARTIALS_DIRECTORY || _download_path == "" && loc == RS_DOWNLOAD_DIRECTORY) throw std::runtime_error("Download path and partial path not properly set in RsDiscSpace. Please call RsDiscSpace::setPartialsPath() and RsDiscSpace::setDownloadPath()") ; time_t now = time(NULL) ; @@ -160,6 +160,12 @@ bool RsDiscSpace::checkForDiscSpace(RsDiscSpace::DiscLocation loc) case RS_CONFIG_DIRECTORY: rs = crossSystemDiskStats(RsInit::RsConfigDirectory().c_str(),free_blocks,block_size) ; #ifdef DEBUG_RSDISCSPACE std::cerr << " path = " << RsInit::RsConfigDirectory() << std::endl ; +#endif + break ; + + case RS_PGP_DIRECTORY: rs = crossSystemDiskStats(RsInit::RsPGPDirectory().c_str(),free_blocks,block_size) ; +#ifdef DEBUG_RSDISCSPACE + std::cerr << " path = " << RsInit::RsPGPDirectory() << std::endl ; #endif break ; }