From 26b3b366c961cfab170b6920c45a52642122687f Mon Sep 17 00:00:00 2001 From: Sebastian Messmer Date: Sat, 23 Sep 2017 20:17:05 +0100 Subject: [PATCH 01/26] Use local state file instead of myClientId file --- src/cryfs/CMakeLists.txt | 2 +- src/cryfs/config/CryConfigCreator.cpp | 5 +- src/cryfs/config/CryConfigLoader.cpp | 5 +- src/cryfs/filesystem/CryDevice.cpp | 1 - src/cryfs/localstate/LocalStateMetadata.cpp | 108 ++++++++++++++++++ src/cryfs/localstate/LocalStateMetadata.h | 36 ++++++ src/cryfs/localstate/MyClientId.cpp | 54 --------- src/cryfs/localstate/MyClientId.h | 30 ----- test/cryfs/CMakeLists.txt | 2 +- .../localstate/LocalStateMetadataTest.cpp | 38 ++++++ test/cryfs/localstate/MyClientIdTest.cpp | 23 ---- 11 files changed, 190 insertions(+), 114 deletions(-) create mode 100644 src/cryfs/localstate/LocalStateMetadata.cpp create mode 100644 src/cryfs/localstate/LocalStateMetadata.h delete mode 100644 src/cryfs/localstate/MyClientId.cpp delete mode 100644 src/cryfs/localstate/MyClientId.h create mode 100644 test/cryfs/localstate/LocalStateMetadataTest.cpp delete mode 100644 test/cryfs/localstate/MyClientIdTest.cpp diff --git a/src/cryfs/CMakeLists.txt b/src/cryfs/CMakeLists.txt index 6c40b491..10e12c5b 100644 --- a/src/cryfs/CMakeLists.txt +++ b/src/cryfs/CMakeLists.txt @@ -40,8 +40,8 @@ set(LIB_SOURCES filesystem/cachingfsblobstore/SymlinkBlobRef.cpp filesystem/CryFile.cpp filesystem/CryDevice.cpp - localstate/MyClientId.cpp localstate/LocalStateDir.cpp + localstate/LocalStateMetadata.cpp ) add_library(${PROJECT_NAME} STATIC ${LIB_SOURCES}) diff --git a/src/cryfs/config/CryConfigCreator.cpp b/src/cryfs/config/CryConfigCreator.cpp index 1eed1c53..75747e45 100644 --- a/src/cryfs/config/CryConfigCreator.cpp +++ b/src/cryfs/config/CryConfigCreator.cpp @@ -3,7 +3,7 @@ #include #include #include -#include +#include using cpputils::Console; using cpputils::unique_ref; @@ -30,7 +30,8 @@ namespace cryfs { config.SetRootBlob(_generateRootBlobId()); config.SetEncryptionKey(_generateEncKey(config.Cipher())); config.SetFilesystemId(_generateFilesystemID()); - uint32_t myClientId = MyClientId(LocalStateDir::forFilesystemId(config.FilesystemId())).loadOrGenerate(); + auto localState = LocalStateMetadata::loadOrGenerate(LocalStateDir::forFilesystemId(config.FilesystemId())); + uint32_t myClientId = localState.myClientId(); config.SetExclusiveClientId(_generateExclusiveClientId(missingBlockIsIntegrityViolationFromCommandLine, myClientId)); #ifndef CRYFS_NO_COMPATIBILITY config.SetHasVersionNumbers(true); diff --git a/src/cryfs/config/CryConfigLoader.cpp b/src/cryfs/config/CryConfigLoader.cpp index 6c39b06b..7fc25a1c 100644 --- a/src/cryfs/config/CryConfigLoader.cpp +++ b/src/cryfs/config/CryConfigLoader.cpp @@ -7,7 +7,7 @@ #include #include #include "../localstate/LocalStateDir.h" -#include "../localstate/MyClientId.h" +#include "../localstate/LocalStateMetadata.h" namespace bf = boost::filesystem; using cpputils::unique_ref; @@ -57,7 +57,8 @@ optional CryConfigLoader::_loadConfig(const b config->save(); } _checkCipher(*config->config()); - uint32_t myClientId = MyClientId(LocalStateDir::forFilesystemId(config->config()->FilesystemId())).loadOrGenerate(); + auto localState = LocalStateMetadata::loadOrGenerate(LocalStateDir::forFilesystemId(config->config()->FilesystemId())); + uint32_t myClientId = localState.myClientId(); _checkMissingBlocksAreIntegrityViolations(&*config, myClientId); return ConfigLoadResult {std::move(*config), myClientId}; } diff --git a/src/cryfs/filesystem/CryDevice.cpp b/src/cryfs/filesystem/CryDevice.cpp index 95e57fd0..5b32dc85 100644 --- a/src/cryfs/filesystem/CryDevice.cpp +++ b/src/cryfs/filesystem/CryDevice.cpp @@ -19,7 +19,6 @@ #include #include #include -#include "cryfs/localstate/MyClientId.h" #include "cryfs/localstate/LocalStateDir.h" using std::string; diff --git a/src/cryfs/localstate/LocalStateMetadata.cpp b/src/cryfs/localstate/LocalStateMetadata.cpp new file mode 100644 index 00000000..bcd76624 --- /dev/null +++ b/src/cryfs/localstate/LocalStateMetadata.cpp @@ -0,0 +1,108 @@ +#include "LocalStateMetadata.h" +#include +#include +#include +#include +#include + +using boost::optional; +using boost::none; +using boost::property_tree::ptree; +using boost::property_tree::write_json; +using boost::property_tree::read_json; +using std::ifstream; +using std::ofstream; +using std::istream; +using std::ostream; +using cpputils::Random; +using blockstore::integrity::KnownBlockVersions; +namespace bf = boost::filesystem; + +namespace cryfs { + +LocalStateMetadata::LocalStateMetadata(uint32_t myClientId) +: _myClientId(myClientId) {} + +LocalStateMetadata LocalStateMetadata::loadOrGenerate(const bf::path &statePath) { + auto metadataFile = statePath / "metadata"; + auto loaded = _load(metadataFile); + if (loaded != none) { + return *loaded; + } + // If it couldn't be loaded, generate a new client id. + return _generate(metadataFile); +} + +optional LocalStateMetadata::_load(const bf::path &metadataFilePath) { + ifstream file(metadataFilePath.native()); + if (!file.good()) { + // State file doesn't exist + return none; + } + return _deserialize(file); +} + +void LocalStateMetadata::_save(const bf::path &metadataFilePath) const { + ofstream file(metadataFilePath.native(), std::ios::trunc); + _serialize(file); +} + +namespace { +uint32_t _generateClientId() { + uint32_t result; + do { + result = *reinterpret_cast(Random::PseudoRandom().getFixedSize().data()); + } while(result == KnownBlockVersions::CLIENT_ID_FOR_DELETED_BLOCK); // Safety check - CLIENT_ID_FOR_DELETED_BLOCK shouldn't be used by any valid client. + return result; +} + +#ifndef CRYFS_NO_COMPATIBILITY +optional _tryLoadClientIdFromLegacyFile(const bf::path &metadataFilePath) { + auto myClientIdFile = metadataFilePath.parent_path() / "myClientId"; + ifstream file(myClientIdFile.native()); + if (!file.good()) { + return none; + } + + uint32_t value; + file >> value; + file.close(); + //bf::remove(myClientIdFile); + return value; +} +#endif +} + +LocalStateMetadata LocalStateMetadata::_generate(const bf::path &metadataFilePath) { + uint32_t myClientId = _generateClientId(); +#ifndef CRYFS_NO_COMPATIBILITY + // In the old format, this was stored in a "myClientId" file. If that file exists, load it from there. + optional legacy = _tryLoadClientIdFromLegacyFile(metadataFilePath); + if (legacy != none) { + myClientId = *legacy; + } +#endif + + LocalStateMetadata result(myClientId); + result._save(metadataFilePath); + return result; +} + +void LocalStateMetadata::_serialize(ostream& stream) const { + ptree pt; + pt.put("myClientId", myClientId()); + + write_json(stream, pt); +} + +LocalStateMetadata LocalStateMetadata::_deserialize(istream& stream) { + ptree pt; + read_json(stream, pt); + + uint32_t myClientId = pt.get("myClientId"); + + return LocalStateMetadata(myClientId); +} + + +} diff --git a/src/cryfs/localstate/LocalStateMetadata.h b/src/cryfs/localstate/LocalStateMetadata.h new file mode 100644 index 00000000..96671c21 --- /dev/null +++ b/src/cryfs/localstate/LocalStateMetadata.h @@ -0,0 +1,36 @@ +#pragma once +#ifndef MESSMER_CRYFS_LOCALSTATE_LOCALSTATEMETADATA_H_ +#define MESSMER_CRYFS_LOCALSTATE_LOCALSTATEMETADATA_H_ + +#include +#include +#include + +namespace cryfs { + +class LocalStateMetadata final { +public: + + static LocalStateMetadata loadOrGenerate(const boost::filesystem::path &statePath); + + uint32_t myClientId() const; + +private: + LocalStateMetadata(uint32_t myClientId); + + static boost::optional _load(const boost::filesystem::path &metadataFilePath); + static LocalStateMetadata _deserialize(std::istream& stream); + static LocalStateMetadata _generate(const boost::filesystem::path &metadataFilePath); + void _save(const boost::filesystem::path &metadataFilePath) const; + void _serialize(std::ostream& stream) const; + + const uint32_t _myClientId; +}; + +inline uint32_t LocalStateMetadata::myClientId() const { + return _myClientId; +} + +} + +#endif diff --git a/src/cryfs/localstate/MyClientId.cpp b/src/cryfs/localstate/MyClientId.cpp deleted file mode 100644 index 323bf043..00000000 --- a/src/cryfs/localstate/MyClientId.cpp +++ /dev/null @@ -1,54 +0,0 @@ -#include "MyClientId.h" -#include -#include -#include - -using boost::optional; -using boost::none; -using std::ifstream; -using std::ofstream; -using cpputils::Random; -using blockstore::integrity::KnownBlockVersions; -namespace bf = boost::filesystem; - -namespace cryfs { - - MyClientId::MyClientId(const bf::path &statePath) - :_stateFilePath(statePath / "myClientId") { - } - - uint32_t MyClientId::loadOrGenerate() const { - auto loaded = _load(); - if (loaded != none) { - return *loaded; - } - // If it couldn't be loaded, generate a new client id. - auto generated = _generate(); - _save(generated); - return generated; - } - - uint32_t MyClientId::_generate() { - uint32_t result; - do { - result = *reinterpret_cast(Random::PseudoRandom().getFixedSize().data()); - } while(result == KnownBlockVersions::CLIENT_ID_FOR_DELETED_BLOCK); // Safety check - CLIENT_ID_FOR_DELETED_BLOCK shouldn't be used by any valid client. - return result; - } - - optional MyClientId::_load() const { - ifstream file(_stateFilePath.native()); - if (!file.good()) { - // State file doesn't exist - return none; - } - uint32_t value; - file >> value; - return value; - } - - void MyClientId::_save(uint32_t clientId) const { - ofstream file(_stateFilePath.native()); - file << clientId; - } -} diff --git a/src/cryfs/localstate/MyClientId.h b/src/cryfs/localstate/MyClientId.h deleted file mode 100644 index 5667e4af..00000000 --- a/src/cryfs/localstate/MyClientId.h +++ /dev/null @@ -1,30 +0,0 @@ -#pragma once -#ifndef MESSMER_CRYFS_LOCALSTATE_MYCLIENTID_H_ -#define MESSMER_CRYFS_LOCALSTATE_MYCLIENTID_H_ - -#include -#include -#include - -namespace cryfs { - - class MyClientId final { - public: - MyClientId(const boost::filesystem::path &statePath); - - uint32_t loadOrGenerate() const; - - private: - const boost::filesystem::path _stateFilePath; - - static uint32_t _generate(); - boost::optional _load() const; - void _save(uint32_t clientId) const; - - DISALLOW_COPY_AND_ASSIGN(MyClientId); - }; - -} - - -#endif diff --git a/test/cryfs/CMakeLists.txt b/test/cryfs/CMakeLists.txt index e440aae6..4da693b6 100644 --- a/test/cryfs/CMakeLists.txt +++ b/test/cryfs/CMakeLists.txt @@ -17,7 +17,7 @@ set(SOURCES filesystem/CryFsTest.cpp filesystem/CryNodeTest.cpp filesystem/FileSystemTest.cpp - localstate/MyClientIdTest.cpp + localstate/LocalStateMetadataTest.cpp ) add_executable(${PROJECT_NAME} ${SOURCES}) diff --git a/test/cryfs/localstate/LocalStateMetadataTest.cpp b/test/cryfs/localstate/LocalStateMetadataTest.cpp new file mode 100644 index 00000000..16f17d91 --- /dev/null +++ b/test/cryfs/localstate/LocalStateMetadataTest.cpp @@ -0,0 +1,38 @@ +#include + +#include +#include +#include + +using cpputils::TempDir; +using cryfs::LocalStateMetadata; +using std::ofstream; + +class LocalStateMetadataTest : public ::testing::Test { +public: + TempDir stateDir; + TempDir stateDir2; +}; + +TEST_F(LocalStateMetadataTest, myClientId_ValueIsConsistent) { + LocalStateMetadata metadata1 = LocalStateMetadata::loadOrGenerate(stateDir.path()); + LocalStateMetadata metadata2 = LocalStateMetadata::loadOrGenerate(stateDir.path()); + EXPECT_EQ(metadata1.myClientId(), metadata2.myClientId()); +} + +TEST_F(LocalStateMetadataTest, myClientId_ValueIsRandomForNewClient) { + LocalStateMetadata metadata1 = LocalStateMetadata::loadOrGenerate(stateDir.path()); + LocalStateMetadata metadata2 = LocalStateMetadata::loadOrGenerate(stateDir2.path()); + EXPECT_NE(metadata1.myClientId(), metadata2.myClientId()); +} + +#ifndef CRYFS_NO_COMPATIBILITY +TEST_F(LocalStateMetadataTest, myClientId_TakesLegacyValueIfSpecified) { + ofstream file((stateDir.path() / "myClientId").native()); + file << 12345u; + file.close(); + + LocalStateMetadata metadata = LocalStateMetadata::loadOrGenerate(stateDir.path()); + EXPECT_EQ(12345u, metadata.myClientId()); +} +#endif diff --git a/test/cryfs/localstate/MyClientIdTest.cpp b/test/cryfs/localstate/MyClientIdTest.cpp deleted file mode 100644 index 89a680d6..00000000 --- a/test/cryfs/localstate/MyClientIdTest.cpp +++ /dev/null @@ -1,23 +0,0 @@ -#include - -#include "cryfs/localstate/MyClientId.h" -#include - -using cpputils::TempDir; -using cryfs::MyClientId; - -class MyClientIdTest : public ::testing::Test { -public: - TempDir stateDir; - TempDir stateDir2; -}; - -TEST_F(MyClientIdTest, ValueIsConsistent) { - uint32_t myClientId = MyClientId(stateDir.path()).loadOrGenerate(); - EXPECT_EQ(myClientId, MyClientId(stateDir.path()).loadOrGenerate()); -} - -TEST_F(MyClientIdTest, ValueIsRandomForNewClient) { - uint32_t myClientId = MyClientId(stateDir.path()).loadOrGenerate(); - EXPECT_NE(myClientId, MyClientId(stateDir2.path()).loadOrGenerate()); -} From aace4c2f1392e8bb77144433968e6310754368cf Mon Sep 17 00:00:00 2001 From: Sebastian Messmer Date: Thu, 28 Sep 2017 07:41:08 +0100 Subject: [PATCH 02/26] Check that filesystem id didn't change since we loaded the basedir the last time --- src/cryfs-cli/Cli.cpp | 14 ++++ src/cryfs-cli/Cli.h | 1 + src/cryfs/CMakeLists.txt | 1 + src/cryfs/localstate/BasedirMetadata.cpp | 82 +++++++++++++++++++ src/cryfs/localstate/BasedirMetadata.h | 18 ++++ src/cryfs/localstate/LocalStateDir.cpp | 25 +++++- src/cryfs/localstate/LocalStateDir.h | 5 ++ src/cryfs/localstate/LocalStateMetadata.cpp | 2 +- test/cryfs/CMakeLists.txt | 3 +- test/cryfs/localstate/BasedirMetadataTest.cpp | 67 +++++++++++++++ 10 files changed, 212 insertions(+), 6 deletions(-) create mode 100644 src/cryfs/localstate/BasedirMetadata.cpp create mode 100644 src/cryfs/localstate/BasedirMetadata.h create mode 100644 test/cryfs/localstate/BasedirMetadataTest.cpp diff --git a/src/cryfs-cli/Cli.cpp b/src/cryfs-cli/Cli.cpp index 7dc4be7f..d37e8ee9 100644 --- a/src/cryfs-cli/Cli.cpp +++ b/src/cryfs-cli/Cli.cpp @@ -21,6 +21,8 @@ #include "VersionChecker.h" #include #include +#include +#include #include "Environment.h" //TODO Many functions accessing the ProgramOptions object. Factor out into class that stores it as a member. @@ -195,6 +197,17 @@ namespace cryfs { return *configFile; } + void Cli::_checkConfigIntegrity(const bf::path& basedir, const CryConfigFile& config) { + if (!BasedirMetadata::filesystemIdForBasedirIsCorrect(basedir, config.config()->FilesystemId())) { + if (!_console->askYesNo("The filesystem id in the config file is different to the last time we loaded a filesystem from this basedir. This can be genuine if you replaced the filesystem with a different one. If you didn't do that, it is possible that an attacker did. Do you want to continue loading the file system?", false)) { + throw std::runtime_error( + "The filesystem id in the config file is different to the last time we loaded a filesystem from this basedir."); + } + } + // Update local state (or create it if it didn't exist yet) + BasedirMetadata::updateFilesystemIdForBasedir(basedir, config.config()->FilesystemId()); + } + CryConfigLoader::ConfigLoadResult Cli::_loadOrCreateConfig(const ProgramOptions &options) { try { auto configFile = _determineConfigFile(options); @@ -203,6 +216,7 @@ namespace cryfs { std::cerr << "Could not load config file. Did you enter the correct password?" << std::endl; exit(1); } + _checkConfigIntegrity(options.baseDir(), config->configFile); return std::move(*config); } catch (const std::exception &e) { std::cerr << "Error: " << e.what() << std::endl; diff --git a/src/cryfs-cli/Cli.h b/src/cryfs-cli/Cli.h index 84d5a20e..ed32b7c2 100644 --- a/src/cryfs-cli/Cli.h +++ b/src/cryfs-cli/Cli.h @@ -23,6 +23,7 @@ namespace cryfs { void _checkForUpdates(); void _runFilesystem(const program_options::ProgramOptions &options); CryConfigLoader::ConfigLoadResult _loadOrCreateConfig(const program_options::ProgramOptions &options); + void _checkConfigIntegrity(const boost::filesystem::path& basedir, const CryConfigFile& config); boost::optional _loadOrCreateConfigFile(const boost::filesystem::path &configFilePath, const boost::optional &cipher, const boost::optional &blocksizeBytes, const boost::optional &missingBlockIsIntegrityViolation); boost::filesystem::path _determineConfigFile(const program_options::ProgramOptions &options); static std::string _askPasswordForExistingFilesystem(); diff --git a/src/cryfs/CMakeLists.txt b/src/cryfs/CMakeLists.txt index 10e12c5b..b80877e2 100644 --- a/src/cryfs/CMakeLists.txt +++ b/src/cryfs/CMakeLists.txt @@ -42,6 +42,7 @@ set(LIB_SOURCES filesystem/CryDevice.cpp localstate/LocalStateDir.cpp localstate/LocalStateMetadata.cpp + localstate/BasedirMetadata.cpp ) add_library(${PROJECT_NAME} STATIC ${LIB_SOURCES}) diff --git a/src/cryfs/localstate/BasedirMetadata.cpp b/src/cryfs/localstate/BasedirMetadata.cpp new file mode 100644 index 00000000..854455f1 --- /dev/null +++ b/src/cryfs/localstate/BasedirMetadata.cpp @@ -0,0 +1,82 @@ +#include "BasedirMetadata.h" +#include +#include +#include +#include +#include "LocalStateDir.h" + +namespace bf = boost::filesystem; +using boost::property_tree::ptree; +using boost::property_tree::write_json; +using boost::property_tree::read_json; +using boost::optional; +using boost::none; +using std::ostream; +using std::istream; +using std::ifstream; +using std::ofstream; + +namespace cryfs { + +namespace { +bf::path _localStateConfigFile(const bf::path& basedir) { + std::string basedir_id; + CryptoPP::SHA512 hash; + CryptoPP::StringSource(bf::canonical(basedir).native(), true, + new CryptoPP::HashFilter(hash, + new CryptoPP::HexEncoder( + new CryptoPP::StringSink(basedir_id) + ) + ) + ); + return LocalStateDir::forMapFromBasedirToConfigFiles() / basedir_id; +} + +void _serialize(ostream& stream, const CryConfig::FilesystemID& filesystemId) { + ptree pt; + pt.put("filesystemId", filesystemId.ToString()); + + write_json(stream, pt); +} + +CryConfig::FilesystemID _deserialize(istream& stream) { + ptree pt; + read_json(stream, pt); + + std::string filesystemId = pt.get("filesystemId"); + + return CryConfig::FilesystemID::FromString(filesystemId); +} + +optional _load(const bf::path &metadataFilePath) { + ifstream file(metadataFilePath.native()); + if (!file.good()) { + // State file doesn't exist + return none; + } + return _deserialize(file); +} + +void _save(const bf::path &metadataFilePath, const CryConfig::FilesystemID& filesystemId) { + ofstream file(metadataFilePath.native(), std::ios::trunc); + _serialize(file, filesystemId); +} + +} + +bool BasedirMetadata::filesystemIdForBasedirIsCorrect(const bf::path &basedir, const CryConfig::FilesystemID &filesystemId) { + auto metadataFile = _localStateConfigFile(basedir); + auto loaded = _load(metadataFile); + if (loaded == none) { + // Local state not known. Possibly the file system is currently being created. + return true; + } + return loaded == filesystemId; +} + +void BasedirMetadata::updateFilesystemIdForBasedir(const bf::path &basedir, const CryConfig::FilesystemID &filesystemId) { + auto metadataFile = _localStateConfigFile(basedir); + _save(metadataFile, filesystemId); +} + +} diff --git a/src/cryfs/localstate/BasedirMetadata.h b/src/cryfs/localstate/BasedirMetadata.h new file mode 100644 index 00000000..f77b45f0 --- /dev/null +++ b/src/cryfs/localstate/BasedirMetadata.h @@ -0,0 +1,18 @@ +#pragma once +#ifndef MESSMER_CRYFS_LOCALSTATE_BASEDIRMETADATA_H_ +#define MESSMER_CRYFS_LOCALSTATE_BASEDIRMETADATA_H_ + +#include +#include "../config/CryConfig.h" + +namespace cryfs { + +class BasedirMetadata final { +public: + static bool filesystemIdForBasedirIsCorrect(const boost::filesystem::path &basedir, const CryConfig::FilesystemID &filesystemId); + static void updateFilesystemIdForBasedir(const boost::filesystem::path &basedir, const CryConfig::FilesystemID &filesystemId); +}; + +} + +#endif diff --git a/src/cryfs/localstate/LocalStateDir.cpp b/src/cryfs/localstate/LocalStateDir.cpp index 7bee2b97..fd26ed37 100644 --- a/src/cryfs/localstate/LocalStateDir.cpp +++ b/src/cryfs/localstate/LocalStateDir.cpp @@ -5,19 +5,36 @@ namespace bf = boost::filesystem; namespace cryfs { + namespace { + // TODO constexpr? + bf::path& appDir() { + static bf::path singleton = cpputils::system::HomeDirectory::get() / ".cryfs"; + return singleton; + } + } + bf::path LocalStateDir::forFilesystemId(const CryConfig::FilesystemID &filesystemId) { - bf::path app_dir = cpputils::system::HomeDirectory::get() / ".cryfs"; - _createDirIfNotExists(app_dir); - bf::path filesystems_dir = app_dir / "filesystems"; + _createDirIfNotExists(appDir()); + bf::path filesystems_dir = appDir() / "filesystems"; _createDirIfNotExists(filesystems_dir); bf::path this_filesystem_dir = filesystems_dir / filesystemId.ToString(); _createDirIfNotExists(this_filesystem_dir); return this_filesystem_dir; } + bf::path LocalStateDir::forMapFromBasedirToConfigFiles() { + bf::path result = appDir() / "map_basedir_initialconfigfile"; + _createDirIfNotExists(result); + return result; + } + void LocalStateDir::_createDirIfNotExists(const bf::path &path) { if (!bf::exists(path)) { - bf::create_directory(path); + bf::create_directories(path); } } + + void LocalStateDir::setAppDir(boost::filesystem::path path) { + appDir() = std::move(path); + } } diff --git a/src/cryfs/localstate/LocalStateDir.h b/src/cryfs/localstate/LocalStateDir.h index 481c1d9a..882a4cda 100644 --- a/src/cryfs/localstate/LocalStateDir.h +++ b/src/cryfs/localstate/LocalStateDir.h @@ -11,6 +11,11 @@ namespace cryfs { class LocalStateDir final { public: static boost::filesystem::path forFilesystemId(const CryConfig::FilesystemID &filesystemId); + static boost::filesystem::path forMapFromBasedirToConfigFiles(); + + // Use this from test cases to not pollute local config + // TODO Make test cases call this + static void setAppDir(boost::filesystem::path path); private: LocalStateDir(); // static functions only diff --git a/src/cryfs/localstate/LocalStateMetadata.cpp b/src/cryfs/localstate/LocalStateMetadata.cpp index bcd76624..de78da50 100644 --- a/src/cryfs/localstate/LocalStateMetadata.cpp +++ b/src/cryfs/localstate/LocalStateMetadata.cpp @@ -67,7 +67,7 @@ optional _tryLoadClientIdFromLegacyFile(const bf::path &metadataFilePa uint32_t value; file >> value; file.close(); - //bf::remove(myClientIdFile); + bf::remove(myClientIdFile); return value; } #endif diff --git a/test/cryfs/CMakeLists.txt b/test/cryfs/CMakeLists.txt index 4da693b6..eaf94232 100644 --- a/test/cryfs/CMakeLists.txt +++ b/test/cryfs/CMakeLists.txt @@ -17,7 +17,8 @@ set(SOURCES filesystem/CryFsTest.cpp filesystem/CryNodeTest.cpp filesystem/FileSystemTest.cpp - localstate/LocalStateMetadataTest.cpp + localstate/LocalStateMetadataTest.cpp + localstate/BasedirMetadataTest.cpp ) add_executable(${PROJECT_NAME} ${SOURCES}) diff --git a/test/cryfs/localstate/BasedirMetadataTest.cpp b/test/cryfs/localstate/BasedirMetadataTest.cpp new file mode 100644 index 00000000..cefb22e0 --- /dev/null +++ b/test/cryfs/localstate/BasedirMetadataTest.cpp @@ -0,0 +1,67 @@ +#include + +#include +#include +#include +#include + +using cpputils::TempDir; +using cryfs::BasedirMetadata; +using std::ofstream; +namespace bf = boost::filesystem; +using FilesystemID = cryfs::CryConfig::FilesystemID ; + +class BasedirMetadataTest : public ::testing::Test { +public: + TempDir tempdir; + bf::path basedir1; + bf::path basedir2; + const FilesystemID id1; + const FilesystemID id2; + + BasedirMetadataTest() + : tempdir() + , basedir1(tempdir.path() / "my/basedir") + , basedir2(tempdir.path() / "my/other/basedir") + , id1(FilesystemID::FromString("1491BB4932A389EE14BC7090AC772972")) + , id2(FilesystemID::FromString("A1491BB493214BC7090C772972A389EE")) + { + // Use temporary local state dir to not pollute local state + cryfs::LocalStateDir::setAppDir(tempdir.path() / "appdir"); + // Create basedirs so bf::canonical() works + bf::create_directories(basedir1); + bf::create_directories(basedir2); + } + +}; + +TEST_F(BasedirMetadataTest, givenEmptyState_whenCalled_thenSucceeds) { + EXPECT_TRUE(BasedirMetadata::filesystemIdForBasedirIsCorrect(basedir1, id1)); +} + +TEST_F(BasedirMetadataTest, givenStateWithBasedir_whenCalledForDifferentBasedir_thenSucceeds) { + BasedirMetadata::updateFilesystemIdForBasedir(basedir2, id1); + EXPECT_TRUE(BasedirMetadata::filesystemIdForBasedirIsCorrect(basedir1, id1)); +} + +TEST_F(BasedirMetadataTest, givenStateWithBasedir_whenCalledWithSameId_thenSucceeds) { + BasedirMetadata::updateFilesystemIdForBasedir(basedir1, id1); + EXPECT_TRUE(BasedirMetadata::filesystemIdForBasedirIsCorrect(basedir1, id1)); +} + +TEST_F(BasedirMetadataTest, givenStateWithBasedir_whenCalledWithDifferentId_thenFails) { + BasedirMetadata::updateFilesystemIdForBasedir(basedir1, id2); + EXPECT_FALSE(BasedirMetadata::filesystemIdForBasedirIsCorrect(basedir1, id1)); +} + +TEST_F(BasedirMetadataTest, givenStateWithUpdatedBasedir_whenCalledWithSameId_thenSucceeds) { + BasedirMetadata::updateFilesystemIdForBasedir(basedir1, id2); + BasedirMetadata::updateFilesystemIdForBasedir(basedir1, id1); + EXPECT_TRUE(BasedirMetadata::filesystemIdForBasedirIsCorrect(basedir1, id1)); +} + +TEST_F(BasedirMetadataTest, givenStateWithUpdatedBasedir_whenCalledWithDifferentId_thenFails) { + BasedirMetadata::updateFilesystemIdForBasedir(basedir1, id2); + BasedirMetadata::updateFilesystemIdForBasedir(basedir1, id1); + EXPECT_FALSE(BasedirMetadata::filesystemIdForBasedirIsCorrect(basedir1, id2)); +} From 49719e3e66f9054659b21a6092acd9af2115104a Mon Sep 17 00:00:00 2001 From: Sebastian Messmer Date: Thu, 28 Sep 2017 08:19:30 +0100 Subject: [PATCH 03/26] Use config file instead of dir for basedir metadata --- src/cryfs-cli/Cli.cpp | 6 +- src/cryfs/localstate/BasedirMetadata.cpp | 77 ++++++++----------- src/cryfs/localstate/BasedirMetadata.h | 18 ++++- src/cryfs/localstate/LocalStateDir.cpp | 19 +++-- src/cryfs/localstate/LocalStateDir.h | 2 +- test/cryfs/localstate/BasedirMetadataTest.cpp | 26 +++---- 6 files changed, 76 insertions(+), 72 deletions(-) diff --git a/src/cryfs-cli/Cli.cpp b/src/cryfs-cli/Cli.cpp index d37e8ee9..36610651 100644 --- a/src/cryfs-cli/Cli.cpp +++ b/src/cryfs-cli/Cli.cpp @@ -198,14 +198,16 @@ namespace cryfs { } void Cli::_checkConfigIntegrity(const bf::path& basedir, const CryConfigFile& config) { - if (!BasedirMetadata::filesystemIdForBasedirIsCorrect(basedir, config.config()->FilesystemId())) { + auto basedirMetadata = BasedirMetadata::load(); + if (!basedirMetadata.filesystemIdForBasedirIsCorrect(basedir, config.config()->FilesystemId())) { if (!_console->askYesNo("The filesystem id in the config file is different to the last time we loaded a filesystem from this basedir. This can be genuine if you replaced the filesystem with a different one. If you didn't do that, it is possible that an attacker did. Do you want to continue loading the file system?", false)) { throw std::runtime_error( "The filesystem id in the config file is different to the last time we loaded a filesystem from this basedir."); } } // Update local state (or create it if it didn't exist yet) - BasedirMetadata::updateFilesystemIdForBasedir(basedir, config.config()->FilesystemId()); + basedirMetadata.updateFilesystemIdForBasedir(basedir, config.config()->FilesystemId()); + basedirMetadata.save(); } CryConfigLoader::ConfigLoadResult Cli::_loadOrCreateConfig(const ProgramOptions &options) { diff --git a/src/cryfs/localstate/BasedirMetadata.cpp b/src/cryfs/localstate/BasedirMetadata.cpp index 854455f1..108bba61 100644 --- a/src/cryfs/localstate/BasedirMetadata.cpp +++ b/src/cryfs/localstate/BasedirMetadata.cpp @@ -15,68 +15,57 @@ using std::ostream; using std::istream; using std::ifstream; using std::ofstream; +using std::string; namespace cryfs { namespace { -bf::path _localStateConfigFile(const bf::path& basedir) { - std::string basedir_id; - CryptoPP::SHA512 hash; - CryptoPP::StringSource(bf::canonical(basedir).native(), true, - new CryptoPP::HashFilter(hash, - new CryptoPP::HexEncoder( - new CryptoPP::StringSink(basedir_id) - ) - ) - ); - return LocalStateDir::forMapFromBasedirToConfigFiles() / basedir_id; -} -void _serialize(ostream& stream, const CryConfig::FilesystemID& filesystemId) { - ptree pt; - pt.put("filesystemId", filesystemId.ToString()); +ptree _load(const bf::path &metadataFilePath) { + ptree result; - write_json(stream, pt); -} - -CryConfig::FilesystemID _deserialize(istream& stream) { - ptree pt; - read_json(stream, pt); - - std::string filesystemId = pt.get("filesystemId"); - - return CryConfig::FilesystemID::FromString(filesystemId); -} - -optional _load(const bf::path &metadataFilePath) { ifstream file(metadataFilePath.native()); - if (!file.good()) { - // State file doesn't exist - return none; + if (file.good()) { + read_json(file, result); } - return _deserialize(file); + + return result; } -void _save(const bf::path &metadataFilePath, const CryConfig::FilesystemID& filesystemId) { +void _save(const bf::path &metadataFilePath, const ptree& data) { ofstream file(metadataFilePath.native(), std::ios::trunc); - _serialize(file, filesystemId); + write_json(file, data); +} + +string jsonPathForBasedir(const bf::path &basedir) { + return bf::canonical(basedir).native() + ".filesystemId"; } } -bool BasedirMetadata::filesystemIdForBasedirIsCorrect(const bf::path &basedir, const CryConfig::FilesystemID &filesystemId) { - auto metadataFile = _localStateConfigFile(basedir); - auto loaded = _load(metadataFile); - if (loaded == none) { - // Local state not known. Possibly the file system is currently being created. - return true; +BasedirMetadata::BasedirMetadata(ptree data) + :_data(std::move(data)) {} + +BasedirMetadata BasedirMetadata::load() { + return BasedirMetadata(_load(LocalStateDir::forBasedirMetadata())); +} + +void BasedirMetadata::save() { + _save(LocalStateDir::forBasedirMetadata(), _data); +} + +bool BasedirMetadata::filesystemIdForBasedirIsCorrect(const bf::path &basedir, const CryConfig::FilesystemID &filesystemId) const { + auto entry = _data.get_optional(jsonPathForBasedir(basedir)); + if (entry == boost::none) { + return true; // Basedir not known in local state yet. } - return loaded == filesystemId; + auto filesystemIdFromState = CryConfig::FilesystemID::FromString(*entry); + return filesystemIdFromState == filesystemId; } -void BasedirMetadata::updateFilesystemIdForBasedir(const bf::path &basedir, const CryConfig::FilesystemID &filesystemId) { - auto metadataFile = _localStateConfigFile(basedir); - _save(metadataFile, filesystemId); +BasedirMetadata& BasedirMetadata::updateFilesystemIdForBasedir(const bf::path &basedir, const CryConfig::FilesystemID &filesystemId) { + _data.put(jsonPathForBasedir(basedir), filesystemId.ToString()); + return *this; } } diff --git a/src/cryfs/localstate/BasedirMetadata.h b/src/cryfs/localstate/BasedirMetadata.h index f77b45f0..9cad2feb 100644 --- a/src/cryfs/localstate/BasedirMetadata.h +++ b/src/cryfs/localstate/BasedirMetadata.h @@ -3,14 +3,28 @@ #define MESSMER_CRYFS_LOCALSTATE_BASEDIRMETADATA_H_ #include +#include #include "../config/CryConfig.h" namespace cryfs { class BasedirMetadata final { public: - static bool filesystemIdForBasedirIsCorrect(const boost::filesystem::path &basedir, const CryConfig::FilesystemID &filesystemId); - static void updateFilesystemIdForBasedir(const boost::filesystem::path &basedir, const CryConfig::FilesystemID &filesystemId); + static BasedirMetadata load(); + void save(); + + BasedirMetadata(const BasedirMetadata&) = delete; + BasedirMetadata& operator=(const BasedirMetadata&) = delete; + BasedirMetadata(BasedirMetadata&&) = default; + BasedirMetadata& operator=(BasedirMetadata&&) = default; + + bool filesystemIdForBasedirIsCorrect(const boost::filesystem::path &basedir, const CryConfig::FilesystemID &filesystemId) const; + BasedirMetadata& updateFilesystemIdForBasedir(const boost::filesystem::path &basedir, const CryConfig::FilesystemID &filesystemId); + +private: + BasedirMetadata(boost::property_tree::ptree data); + + boost::property_tree::ptree _data; }; } diff --git a/src/cryfs/localstate/LocalStateDir.cpp b/src/cryfs/localstate/LocalStateDir.cpp index fd26ed37..5bc2a2c7 100644 --- a/src/cryfs/localstate/LocalStateDir.cpp +++ b/src/cryfs/localstate/LocalStateDir.cpp @@ -14,18 +14,17 @@ namespace cryfs { } bf::path LocalStateDir::forFilesystemId(const CryConfig::FilesystemID &filesystemId) { - _createDirIfNotExists(appDir()); - bf::path filesystems_dir = appDir() / "filesystems"; - _createDirIfNotExists(filesystems_dir); - bf::path this_filesystem_dir = filesystems_dir / filesystemId.ToString(); - _createDirIfNotExists(this_filesystem_dir); - return this_filesystem_dir; + _createDirIfNotExists(appDir()); + bf::path filesystems_dir = appDir() / "filesystems"; + _createDirIfNotExists(filesystems_dir); + bf::path this_filesystem_dir = filesystems_dir / filesystemId.ToString(); + _createDirIfNotExists(this_filesystem_dir); + return this_filesystem_dir; } - bf::path LocalStateDir::forMapFromBasedirToConfigFiles() { - bf::path result = appDir() / "map_basedir_initialconfigfile"; - _createDirIfNotExists(result); - return result; + bf::path LocalStateDir::forBasedirMetadata() { + _createDirIfNotExists(appDir()); + return appDir() / "basedirs"; } void LocalStateDir::_createDirIfNotExists(const bf::path &path) { diff --git a/src/cryfs/localstate/LocalStateDir.h b/src/cryfs/localstate/LocalStateDir.h index 882a4cda..2a1ad481 100644 --- a/src/cryfs/localstate/LocalStateDir.h +++ b/src/cryfs/localstate/LocalStateDir.h @@ -11,7 +11,7 @@ namespace cryfs { class LocalStateDir final { public: static boost::filesystem::path forFilesystemId(const CryConfig::FilesystemID &filesystemId); - static boost::filesystem::path forMapFromBasedirToConfigFiles(); + static boost::filesystem::path forBasedirMetadata(); // Use this from test cases to not pollute local config // TODO Make test cases call this diff --git a/test/cryfs/localstate/BasedirMetadataTest.cpp b/test/cryfs/localstate/BasedirMetadataTest.cpp index cefb22e0..f5636b30 100644 --- a/test/cryfs/localstate/BasedirMetadataTest.cpp +++ b/test/cryfs/localstate/BasedirMetadataTest.cpp @@ -36,32 +36,32 @@ public: }; TEST_F(BasedirMetadataTest, givenEmptyState_whenCalled_thenSucceeds) { - EXPECT_TRUE(BasedirMetadata::filesystemIdForBasedirIsCorrect(basedir1, id1)); + EXPECT_TRUE(BasedirMetadata::load().filesystemIdForBasedirIsCorrect(basedir1, id1)); } TEST_F(BasedirMetadataTest, givenStateWithBasedir_whenCalledForDifferentBasedir_thenSucceeds) { - BasedirMetadata::updateFilesystemIdForBasedir(basedir2, id1); - EXPECT_TRUE(BasedirMetadata::filesystemIdForBasedirIsCorrect(basedir1, id1)); + BasedirMetadata::load().updateFilesystemIdForBasedir(basedir2, id1).save(); + EXPECT_TRUE(BasedirMetadata::load().filesystemIdForBasedirIsCorrect(basedir1, id1)); } TEST_F(BasedirMetadataTest, givenStateWithBasedir_whenCalledWithSameId_thenSucceeds) { - BasedirMetadata::updateFilesystemIdForBasedir(basedir1, id1); - EXPECT_TRUE(BasedirMetadata::filesystemIdForBasedirIsCorrect(basedir1, id1)); + BasedirMetadata::load().updateFilesystemIdForBasedir(basedir1, id1).save(); + EXPECT_TRUE(BasedirMetadata::load().filesystemIdForBasedirIsCorrect(basedir1, id1)); } TEST_F(BasedirMetadataTest, givenStateWithBasedir_whenCalledWithDifferentId_thenFails) { - BasedirMetadata::updateFilesystemIdForBasedir(basedir1, id2); - EXPECT_FALSE(BasedirMetadata::filesystemIdForBasedirIsCorrect(basedir1, id1)); + BasedirMetadata::load().updateFilesystemIdForBasedir(basedir1, id2).save(); + EXPECT_FALSE(BasedirMetadata::load().filesystemIdForBasedirIsCorrect(basedir1, id1)); } TEST_F(BasedirMetadataTest, givenStateWithUpdatedBasedir_whenCalledWithSameId_thenSucceeds) { - BasedirMetadata::updateFilesystemIdForBasedir(basedir1, id2); - BasedirMetadata::updateFilesystemIdForBasedir(basedir1, id1); - EXPECT_TRUE(BasedirMetadata::filesystemIdForBasedirIsCorrect(basedir1, id1)); + BasedirMetadata::load().updateFilesystemIdForBasedir(basedir1, id2).save(); + BasedirMetadata::load().updateFilesystemIdForBasedir(basedir1, id1).save(); + EXPECT_TRUE(BasedirMetadata::load().filesystemIdForBasedirIsCorrect(basedir1, id1)); } TEST_F(BasedirMetadataTest, givenStateWithUpdatedBasedir_whenCalledWithDifferentId_thenFails) { - BasedirMetadata::updateFilesystemIdForBasedir(basedir1, id2); - BasedirMetadata::updateFilesystemIdForBasedir(basedir1, id1); - EXPECT_FALSE(BasedirMetadata::filesystemIdForBasedirIsCorrect(basedir1, id2)); + BasedirMetadata::load().updateFilesystemIdForBasedir(basedir1, id2).save(); + BasedirMetadata::load().updateFilesystemIdForBasedir(basedir1, id1).save(); + EXPECT_FALSE(BasedirMetadata::load().filesystemIdForBasedirIsCorrect(basedir1, id2)); } From 9cc3697e1b4bf0a4986aaaba771dd71b26ea69c3 Mon Sep 17 00:00:00 2001 From: Sebastian Messmer Date: Thu, 28 Sep 2017 15:13:03 -0700 Subject: [PATCH 04/26] Fix CI build --- test/blockstore/interface/BlockStore2Test.cpp | 6 ++++++ test/blockstore/testutils/BlockStoreTest.h | 9 +++++---- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/test/blockstore/interface/BlockStore2Test.cpp b/test/blockstore/interface/BlockStore2Test.cpp index 9d8bf171..4dc36eb1 100644 --- a/test/blockstore/interface/BlockStore2Test.cpp +++ b/test/blockstore/interface/BlockStore2Test.cpp @@ -16,6 +16,12 @@ using cpputils::DataFixture; using cpputils::unique_ref; using boost::optional; +namespace boost { + inline void PrintTo(const optional &, ::std::ostream *os) { + *os << "optional"; + } +} + using namespace blockstore; class BlockStore2Mock: public BlockStore2 { diff --git a/test/blockstore/testutils/BlockStoreTest.h b/test/blockstore/testutils/BlockStoreTest.h index 4dcd8276..517da3ca 100644 --- a/test/blockstore/testutils/BlockStoreTest.h +++ b/test/blockstore/testutils/BlockStoreTest.h @@ -285,7 +285,7 @@ TYPED_TEST_P(BlockStoreTest, Resize_Smaller_ToZero_BlockIsStillUsable) { block->resize(0); this->TestBlockIsUsable(std::move(block), blockStore.get()); } - +/* TYPED_TEST_P(BlockStoreTest, TryCreateTwoBlocksWithSameBlockIdAndSameSize) { auto blockStore = this->fixture.createBlockStore(); blockstore::BlockId blockId = blockstore::BlockId::FromString("1491BB4932A389EE14BC7090AC772972"); @@ -295,7 +295,7 @@ TYPED_TEST_P(BlockStoreTest, TryCreateTwoBlocksWithSameBlockIdAndSameSize) { EXPECT_NE(boost::none, block); EXPECT_EQ(boost::none, block2); } -/* + TYPED_TEST_P(BlockStoreTest, TryCreateTwoBlocksWithSameBlockIdAndDifferentSize) { auto blockStore = this->fixture.createBlockStore(); blockstore::BlockId blockId = blockstore::BlockId::FromString("1491BB4932A389EE14BC7090AC772972"); @@ -383,6 +383,7 @@ REGISTER_TYPED_TEST_CASE_P(BlockStoreTest, ForEachBlock_twoblocks, ForEachBlock_threeblocks, ForEachBlock_doesntListRemovedBlocks_oneblock, + ForEachBlock_doesntListRemovedBlocks_twoblocks, Resize_Larger_FromZero, Resize_Larger_FromZero_BlockIsStillUsable, Resize_Larger, @@ -390,10 +391,10 @@ REGISTER_TYPED_TEST_CASE_P(BlockStoreTest, Resize_Smaller, Resize_Smaller_BlockIsStillUsable, Resize_Smaller_ToZero, - Resize_Smaller_ToZero_BlockIsStillUsable, - TryCreateTwoBlocksWithSameBlockIdAndSameSize + Resize_Smaller_ToZero_BlockIsStillUsable //TODO Just disabled because gtest doesn't allow more template parameters. Fix and reenable! // see https://github.com/google/googletest/issues/1267 + //TryCreateTwoBlocksWithSameBlockIdAndSameSize, //TryCreateTwoBlocksWithSameBlockIdAndDifferentSize, //TryCreateTwoBlocksWithSameBlockIdAndFirstNullSize, //TryCreateTwoBlocksWithSameBlockIdAndSecondNullSize, From 9fc8b257a05324df16f66442d43b20e7e21136f2 Mon Sep 17 00:00:00 2001 From: Sebastian Messmer Date: Thu, 28 Sep 2017 21:15:51 -0700 Subject: [PATCH 05/26] Fix CI --- test/blockstore/testutils/BlockStoreTest.h | 1 + 1 file changed, 1 insertion(+) diff --git a/test/blockstore/testutils/BlockStoreTest.h b/test/blockstore/testutils/BlockStoreTest.h index 517da3ca..7db86f3d 100644 --- a/test/blockstore/testutils/BlockStoreTest.h +++ b/test/blockstore/testutils/BlockStoreTest.h @@ -5,6 +5,7 @@ #include #include #include +#include #include "blockstore/interface/BlockStore.h" From 7a5b23db13a053340b6a6c731e234b96ac633624 Mon Sep 17 00:00:00 2001 From: Sebastian Messmer Date: Sat, 30 Sep 2017 08:49:24 +0100 Subject: [PATCH 06/26] Remember hashed filesystem key in local state so attacker can't replace it --- src/cpp-utils/CMakeLists.txt | 1 + src/cpp-utils/crypto/hash/Hash.cpp | 30 +++++++++++++ src/cpp-utils/crypto/hash/Hash.h | 28 ++++++++++++ src/cpp-utils/data/Data.cpp | 24 ++++++++++ src/cpp-utils/data/Data.h | 4 ++ src/cryfs/config/CryConfigCreator.cpp | 5 ++- src/cryfs/config/CryConfigLoader.cpp | 2 +- src/cryfs/localstate/LocalStateMetadata.cpp | 37 ++++++++++----- src/cryfs/localstate/LocalStateMetadata.h | 10 +++-- test/cpp-utils/CMakeLists.txt | 1 + test/cpp-utils/crypto/hash/HashTest.cpp | 45 +++++++++++++++++++ test/cpp-utils/data/DataTest.cpp | 15 +++++++ .../localstate/LocalStateMetadataTest.cpp | 26 ++++++++--- 13 files changed, 204 insertions(+), 24 deletions(-) create mode 100644 src/cpp-utils/crypto/hash/Hash.cpp create mode 100644 src/cpp-utils/crypto/hash/Hash.h create mode 100644 test/cpp-utils/crypto/hash/HashTest.cpp diff --git a/src/cpp-utils/CMakeLists.txt b/src/cpp-utils/CMakeLists.txt index a9c000be..0425774a 100644 --- a/src/cpp-utils/CMakeLists.txt +++ b/src/cpp-utils/CMakeLists.txt @@ -7,6 +7,7 @@ set(SOURCES crypto/kdf/PasswordBasedKDF.cpp crypto/RandomPadding.cpp crypto/symmetric/EncryptionKey.cpp + crypto/hash/Hash.cpp process/daemonize.cpp process/subprocess.cpp tempfile/TempFile.cpp diff --git a/src/cpp-utils/crypto/hash/Hash.cpp b/src/cpp-utils/crypto/hash/Hash.cpp new file mode 100644 index 00000000..7fb9ef22 --- /dev/null +++ b/src/cpp-utils/crypto/hash/Hash.cpp @@ -0,0 +1,30 @@ +#include "Hash.h" +#include +#include + +using cpputils::Random; +using CryptoPP::SHA512; + +namespace cpputils { +namespace hash { + +Hash hash(const Data& data, Salt salt) { + SHA512 hasher; + hasher.Update((CryptoPP::byte*)salt.data(), Salt::BINARY_LENGTH); + hasher.Update((CryptoPP::byte*)data.data(), data.size()); + + Digest digest = Digest::Null(); + hasher.Final((CryptoPP::byte*)digest.data()); + + return Hash{ + .digest = std::move(digest), + .salt = std::move(salt) + }; +} + +Salt generateSalt() { + return Random::PseudoRandom().getFixedSize<8>(); +} + +} +} diff --git a/src/cpp-utils/crypto/hash/Hash.h b/src/cpp-utils/crypto/hash/Hash.h new file mode 100644 index 00000000..3246d788 --- /dev/null +++ b/src/cpp-utils/crypto/hash/Hash.h @@ -0,0 +1,28 @@ +#pragma once +#ifndef MESSMER_CPPUTILS_CRYPTO_HASH_HASH_H +#define MESSMER_CPPUTILS_CRYPTO_HASH_HASH_H + +#include +#include + +namespace cpputils { +namespace hash { + +using Digest = FixedSizeData<64>; +using Salt = FixedSizeData<8>; + +struct Hash final { + Digest digest; + Salt salt; +}; + + +Salt generateSalt(); +Hash hash(const cpputils::Data& data, Salt salt); + + +} +} + + +#endif diff --git a/src/cpp-utils/data/Data.cpp b/src/cpp-utils/data/Data.cpp index b2ddb169..4db5f4da 100644 --- a/src/cpp-utils/data/Data.cpp +++ b/src/cpp-utils/data/Data.cpp @@ -1,5 +1,7 @@ #include "Data.h" #include +#include +#include using std::istream; using std::ofstream; @@ -42,4 +44,26 @@ Data Data::LoadFromStream(istream &stream, size_t size) { return result; } +Data Data::FromString(const std::string &data) { + ASSERT(data.size() % 2 == 0, "hex encoded data cannot have odd number of characters"); + Data result(data.size() / 2); + CryptoPP::StringSource(data, true, + new CryptoPP::HexDecoder( + new CryptoPP::ArraySink((CryptoPP::byte*)result._data, result.size()) + ) + ); + return result; +} + +std::string Data::ToString() const { + std::string result; + CryptoPP::ArraySource((CryptoPP::byte*)_data, _size, true, + new CryptoPP::HexEncoder( + new CryptoPP::StringSink(result) + ) + ); + ASSERT(result.size() == 2 * _size, "Created wrongly sized string"); + return result; +} + } diff --git a/src/cpp-utils/data/Data.h b/src/cpp-utils/data/Data.h index 62e9c448..bdcf2f7d 100644 --- a/src/cpp-utils/data/Data.h +++ b/src/cpp-utils/data/Data.h @@ -45,6 +45,10 @@ public: static Data LoadFromStream(std::istream &stream, size_t size); void StoreToStream(std::ostream &stream) const; + // TODO Unify ToString/FromString functions from Data/FixedSizeData using free functions + static Data FromString(const std::string &data); + std::string ToString() const; + private: size_t _size; void *_data; diff --git a/src/cryfs/config/CryConfigCreator.cpp b/src/cryfs/config/CryConfigCreator.cpp index 75747e45..4da4072a 100644 --- a/src/cryfs/config/CryConfigCreator.cpp +++ b/src/cryfs/config/CryConfigCreator.cpp @@ -28,10 +28,11 @@ namespace cryfs { config.SetCreatedWithVersion(gitversion::VersionString()); config.SetBlocksizeBytes(_generateBlocksizeBytes(blocksizeBytesFromCommandLine)); config.SetRootBlob(_generateRootBlobId()); - config.SetEncryptionKey(_generateEncKey(config.Cipher())); config.SetFilesystemId(_generateFilesystemID()); - auto localState = LocalStateMetadata::loadOrGenerate(LocalStateDir::forFilesystemId(config.FilesystemId())); + auto encryptionKey = _generateEncKey(config.Cipher()); + auto localState = LocalStateMetadata::loadOrGenerate(LocalStateDir::forFilesystemId(config.FilesystemId()), cpputils::Data::FromString(encryptionKey)); uint32_t myClientId = localState.myClientId(); + config.SetEncryptionKey(std::move(encryptionKey)); config.SetExclusiveClientId(_generateExclusiveClientId(missingBlockIsIntegrityViolationFromCommandLine, myClientId)); #ifndef CRYFS_NO_COMPATIBILITY config.SetHasVersionNumbers(true); diff --git a/src/cryfs/config/CryConfigLoader.cpp b/src/cryfs/config/CryConfigLoader.cpp index 7fc25a1c..9040466c 100644 --- a/src/cryfs/config/CryConfigLoader.cpp +++ b/src/cryfs/config/CryConfigLoader.cpp @@ -57,7 +57,7 @@ optional CryConfigLoader::_loadConfig(const b config->save(); } _checkCipher(*config->config()); - auto localState = LocalStateMetadata::loadOrGenerate(LocalStateDir::forFilesystemId(config->config()->FilesystemId())); + auto localState = LocalStateMetadata::loadOrGenerate(LocalStateDir::forFilesystemId(config->config()->FilesystemId()), cpputils::Data::FromString(config->config()->EncryptionKey())); uint32_t myClientId = localState.myClientId(); _checkMissingBlocksAreIntegrityViolations(&*config, myClientId); return ConfigLoadResult {std::move(*config), myClientId}; diff --git a/src/cryfs/localstate/LocalStateMetadata.cpp b/src/cryfs/localstate/LocalStateMetadata.cpp index de78da50..e2f4d219 100644 --- a/src/cryfs/localstate/LocalStateMetadata.cpp +++ b/src/cryfs/localstate/LocalStateMetadata.cpp @@ -14,23 +14,30 @@ using std::ifstream; using std::ofstream; using std::istream; using std::ostream; -using cpputils::Random; +using std::string; using blockstore::integrity::KnownBlockVersions; +using cpputils::hash::Hash; +using cpputils::Data; +using cpputils::Random; namespace bf = boost::filesystem; namespace cryfs { -LocalStateMetadata::LocalStateMetadata(uint32_t myClientId) -: _myClientId(myClientId) {} +LocalStateMetadata::LocalStateMetadata(uint32_t myClientId, Hash encryptionKeyHash) +: _myClientId(myClientId), _encryptionKeyHash(std::move(encryptionKeyHash)) {} -LocalStateMetadata LocalStateMetadata::loadOrGenerate(const bf::path &statePath) { +LocalStateMetadata LocalStateMetadata::loadOrGenerate(const bf::path &statePath, const Data& encryptionKey) { auto metadataFile = statePath / "metadata"; auto loaded = _load(metadataFile); - if (loaded != none) { - return *loaded; + if (loaded == none) { + // If it couldn't be loaded, generate a new client id. + return _generate(metadataFile, encryptionKey); } - // If it couldn't be loaded, generate a new client id. - return _generate(metadataFile); + + if (loaded->_encryptionKeyHash.digest != cpputils::hash::hash(encryptionKey, loaded->_encryptionKeyHash.salt).digest) { + throw std::runtime_error("The filesystem encryption key differs from the last time we loaded this filesystem. Did an attacker replace the file system?"); + } + return *loaded; } optional LocalStateMetadata::_load(const bf::path &metadataFilePath) { @@ -73,7 +80,7 @@ optional _tryLoadClientIdFromLegacyFile(const bf::path &metadataFilePa #endif } -LocalStateMetadata LocalStateMetadata::_generate(const bf::path &metadataFilePath) { +LocalStateMetadata LocalStateMetadata::_generate(const bf::path &metadataFilePath, const Data& encryptionKey) { uint32_t myClientId = _generateClientId(); #ifndef CRYFS_NO_COMPATIBILITY // In the old format, this was stored in a "myClientId" file. If that file exists, load it from there. @@ -83,7 +90,7 @@ LocalStateMetadata LocalStateMetadata::_generate(const bf::path &metadataFilePat } #endif - LocalStateMetadata result(myClientId); + LocalStateMetadata result(myClientId, cpputils::hash::hash(encryptionKey, cpputils::hash::generateSalt())); result._save(metadataFilePath); return result; } @@ -91,6 +98,8 @@ LocalStateMetadata LocalStateMetadata::_generate(const bf::path &metadataFilePat void LocalStateMetadata::_serialize(ostream& stream) const { ptree pt; pt.put("myClientId", myClientId()); + pt.put("encryptionKey.salt", _encryptionKeyHash.salt.ToString()); + pt.put("encryptionKey.hash", _encryptionKeyHash.digest.ToString()); write_json(stream, pt); } @@ -100,9 +109,13 @@ LocalStateMetadata LocalStateMetadata::_deserialize(istream& stream) { read_json(stream, pt); uint32_t myClientId = pt.get("myClientId"); + string encryptionKeySalt = pt.get("encryptionKey.salt"); + string encryptionKeyDigest = pt.get("encryptionKey.hash"); - return LocalStateMetadata(myClientId); + return LocalStateMetadata(myClientId, Hash{ + .digest = cpputils::hash::Digest::FromString(std::move(encryptionKeyDigest)), + .salt = cpputils::hash::Salt::FromString(std::move(encryptionKeySalt)) + }); } - } diff --git a/src/cryfs/localstate/LocalStateMetadata.h b/src/cryfs/localstate/LocalStateMetadata.h index 96671c21..63cf2513 100644 --- a/src/cryfs/localstate/LocalStateMetadata.h +++ b/src/cryfs/localstate/LocalStateMetadata.h @@ -5,26 +5,28 @@ #include #include #include +#include namespace cryfs { class LocalStateMetadata final { public: - static LocalStateMetadata loadOrGenerate(const boost::filesystem::path &statePath); + static LocalStateMetadata loadOrGenerate(const boost::filesystem::path &statePath, const cpputils::Data& encryptionKey); uint32_t myClientId() const; private: - LocalStateMetadata(uint32_t myClientId); + const uint32_t _myClientId; + const cpputils::hash::Hash _encryptionKeyHash; static boost::optional _load(const boost::filesystem::path &metadataFilePath); static LocalStateMetadata _deserialize(std::istream& stream); - static LocalStateMetadata _generate(const boost::filesystem::path &metadataFilePath); + static LocalStateMetadata _generate(const boost::filesystem::path &metadataFilePath, const cpputils::Data& encryptionKey); void _save(const boost::filesystem::path &metadataFilePath) const; void _serialize(std::ostream& stream) const; - const uint32_t _myClientId; + LocalStateMetadata(uint32_t myClientId, cpputils::hash::Hash encryptionKey); }; inline uint32_t LocalStateMetadata::myClientId() const { diff --git a/test/cpp-utils/CMakeLists.txt b/test/cpp-utils/CMakeLists.txt index 1a751855..3c9d0f11 100644 --- a/test/cpp-utils/CMakeLists.txt +++ b/test/cpp-utils/CMakeLists.txt @@ -6,6 +6,7 @@ set(SOURCES crypto/symmetric/testutils/FakeAuthenticatedCipher.cpp crypto/kdf/SCryptTest.cpp crypto/kdf/SCryptParametersTest.cpp + crypto/hash/HashTest.cpp MacrosIncludeTest.cpp pointer/unique_ref_test.cpp pointer/cast_include_test.cpp diff --git a/test/cpp-utils/crypto/hash/HashTest.cpp b/test/cpp-utils/crypto/hash/HashTest.cpp new file mode 100644 index 00000000..20dcee7a --- /dev/null +++ b/test/cpp-utils/crypto/hash/HashTest.cpp @@ -0,0 +1,45 @@ +#include +#include +#include + +using namespace cpputils::hash; +using cpputils::DataFixture; +using cpputils::Data; + +TEST(HashTest, generateSalt_isIndeterministic) { + EXPECT_NE(generateSalt(), generateSalt()); +} + +TEST(HashTest, hash_setsSaltCorrectly) { + Salt salt = generateSalt(); + Data data = DataFixture::generate(1024); + EXPECT_EQ(salt, hash(data, salt).salt); +} + +TEST(HashTest, hash_isDeterministicWithSameDataSameSalt) { + Salt salt = generateSalt(); + Data data = DataFixture::generate(1024); + EXPECT_EQ(hash(data, salt).digest, hash(data, salt).digest); +} + +TEST(HashTest, hash_isIndeterministicWithSameDataDifferentSalt) { + Salt salt1 = generateSalt(); + Salt salt2 = generateSalt(); + Data data = DataFixture::generate(1024); + EXPECT_NE(hash(data, salt1).digest, hash(data, salt2).digest); +} + +TEST(HashTest, hash_isIndeterministicWithDifferentDataSameSalt) { + Salt salt = generateSalt(); + Data data1 = DataFixture::generate(1024, 1); + Data data2 = DataFixture::generate(1024, 2); + EXPECT_NE(hash(data1, salt).digest, hash(data2, salt).digest); +} + +TEST(HashTest, hash_isIndeterministicWithDifferentDataDifferentSalt) { + Salt salt1 = generateSalt(); + Salt salt2 = generateSalt(); + Data data1 = DataFixture::generate(1024, 1); + Data data2 = DataFixture::generate(1024, 2); + EXPECT_NE(hash(data1, salt1).digest, hash(data2, salt2).digest); +} diff --git a/test/cpp-utils/data/DataTest.cpp b/test/cpp-utils/data/DataTest.cpp index 6f9df070..937bbe04 100644 --- a/test/cpp-utils/data/DataTest.cpp +++ b/test/cpp-utils/data/DataTest.cpp @@ -14,6 +14,7 @@ using cpputils::TempFile; using std::ifstream; using std::ofstream; +using std::string; namespace bf = boost::filesystem; @@ -206,3 +207,17 @@ TEST_F(DataTest, LoadingNonexistingFile) { TempFile file(false); // Pass false to constructor, so the tempfile is not created EXPECT_FALSE(Data::LoadFromFile(file.path())); } + +class DataTestWithStringParam: public DataTest, public WithParamInterface {}; +INSTANTIATE_TEST_CASE_P(DataTestWithStringParam, DataTestWithStringParam, Values("", "2898B4B8A13C0F0278CCE465DB", "6FFEBAD90C0DAA2B79628F0627CE9841")); + +TEST_P(DataTestWithStringParam, FromAndToString) { + Data data = Data::FromString(GetParam()); + EXPECT_EQ(GetParam(), data.ToString()); +} + +TEST_P(DataTestWithStringParam, ToAndFromString) { + Data data = Data::FromString(GetParam()); + Data data2 = Data::FromString(data.ToString()); + EXPECT_EQ(data, data2); +} diff --git a/test/cryfs/localstate/LocalStateMetadataTest.cpp b/test/cryfs/localstate/LocalStateMetadataTest.cpp index 16f17d91..da72c06e 100644 --- a/test/cryfs/localstate/LocalStateMetadataTest.cpp +++ b/test/cryfs/localstate/LocalStateMetadataTest.cpp @@ -3,9 +3,12 @@ #include #include #include +#include using cpputils::TempDir; +using cpputils::Data; using cryfs::LocalStateMetadata; +using cpputils::DataFixture; using std::ofstream; class LocalStateMetadataTest : public ::testing::Test { @@ -15,14 +18,14 @@ public: }; TEST_F(LocalStateMetadataTest, myClientId_ValueIsConsistent) { - LocalStateMetadata metadata1 = LocalStateMetadata::loadOrGenerate(stateDir.path()); - LocalStateMetadata metadata2 = LocalStateMetadata::loadOrGenerate(stateDir.path()); + LocalStateMetadata metadata1 = LocalStateMetadata::loadOrGenerate(stateDir.path(), Data(0)); + LocalStateMetadata metadata2 = LocalStateMetadata::loadOrGenerate(stateDir.path(), Data(0)); EXPECT_EQ(metadata1.myClientId(), metadata2.myClientId()); } TEST_F(LocalStateMetadataTest, myClientId_ValueIsRandomForNewClient) { - LocalStateMetadata metadata1 = LocalStateMetadata::loadOrGenerate(stateDir.path()); - LocalStateMetadata metadata2 = LocalStateMetadata::loadOrGenerate(stateDir2.path()); + LocalStateMetadata metadata1 = LocalStateMetadata::loadOrGenerate(stateDir.path(), Data(0)); + LocalStateMetadata metadata2 = LocalStateMetadata::loadOrGenerate(stateDir2.path(), Data(0)); EXPECT_NE(metadata1.myClientId(), metadata2.myClientId()); } @@ -32,7 +35,20 @@ TEST_F(LocalStateMetadataTest, myClientId_TakesLegacyValueIfSpecified) { file << 12345u; file.close(); - LocalStateMetadata metadata = LocalStateMetadata::loadOrGenerate(stateDir.path()); + LocalStateMetadata metadata = LocalStateMetadata::loadOrGenerate(stateDir.path(), Data(0)); EXPECT_EQ(12345u, metadata.myClientId()); } #endif + +TEST_F(LocalStateMetadataTest, encryptionKeyHash_whenLoadingWithSameKey_thenDoesntCrash) { + LocalStateMetadata::loadOrGenerate(stateDir.path(), DataFixture::generate(1024)); + LocalStateMetadata::loadOrGenerate(stateDir.path(), DataFixture::generate(1024)); +} + +TEST_F(LocalStateMetadataTest, encryptionKeyHash_whenLoadingWithDifferentKey_thenCrashes) { + LocalStateMetadata::loadOrGenerate(stateDir.path(), DataFixture::generate(1024, 1)); + EXPECT_THROW( + LocalStateMetadata::loadOrGenerate(stateDir.path(), DataFixture::generate(1024, 2)), + std::runtime_error + ); +} From 011c6d26ce7c68ff55676cb9dbabb0aecc0e7c65 Mon Sep 17 00:00:00 2001 From: Sebastian Messmer Date: Sat, 30 Sep 2017 09:03:19 +0100 Subject: [PATCH 07/26] Don't pollute users local state dir when running test cases --- src/cryfs/localstate/LocalStateDir.cpp | 10 ++-------- src/cryfs/localstate/LocalStateDir.h | 4 ---- test/cryfs/config/CryConfigCreatorTest.cpp | 3 ++- test/cryfs/config/CryConfigLoaderTest.cpp | 3 ++- test/cryfs/localstate/BasedirMetadataTest.cpp | 5 ++--- 5 files changed, 8 insertions(+), 17 deletions(-) diff --git a/src/cryfs/localstate/LocalStateDir.cpp b/src/cryfs/localstate/LocalStateDir.cpp index 5bc2a2c7..83c7cca4 100644 --- a/src/cryfs/localstate/LocalStateDir.cpp +++ b/src/cryfs/localstate/LocalStateDir.cpp @@ -6,10 +6,8 @@ namespace bf = boost::filesystem; namespace cryfs { namespace { - // TODO constexpr? - bf::path& appDir() { - static bf::path singleton = cpputils::system::HomeDirectory::get() / ".cryfs"; - return singleton; + bf::path appDir() { + return cpputils::system::HomeDirectory::get() / ".cryfs"; } } @@ -32,8 +30,4 @@ namespace cryfs { bf::create_directories(path); } } - - void LocalStateDir::setAppDir(boost::filesystem::path path) { - appDir() = std::move(path); - } } diff --git a/src/cryfs/localstate/LocalStateDir.h b/src/cryfs/localstate/LocalStateDir.h index 2a1ad481..3ccf84a8 100644 --- a/src/cryfs/localstate/LocalStateDir.h +++ b/src/cryfs/localstate/LocalStateDir.h @@ -13,10 +13,6 @@ namespace cryfs { static boost::filesystem::path forFilesystemId(const CryConfig::FilesystemID &filesystemId); static boost::filesystem::path forBasedirMetadata(); - // Use this from test cases to not pollute local config - // TODO Make test cases call this - static void setAppDir(boost::filesystem::path path); - private: LocalStateDir(); // static functions only diff --git a/test/cryfs/config/CryConfigCreatorTest.cpp b/test/cryfs/config/CryConfigCreatorTest.cpp index ef849d97..53d28cb6 100644 --- a/test/cryfs/config/CryConfigCreatorTest.cpp +++ b/test/cryfs/config/CryConfigCreatorTest.cpp @@ -4,6 +4,7 @@ #include #include #include "../testutils/MockConsole.h" +#include "../testutils/TestWithFakeHomeDirectory.h" #include #include @@ -46,7 +47,7 @@ using ::testing::WithParamInterface; #define IGNORE_ASK_FOR_MISSINGBLOCKISINTEGRITYVIOLATION() \ EXPECT_CALL(*console, askYesNo(HasSubstr("missing block"), false)) -class CryConfigCreatorTest: public ::testing::Test { +class CryConfigCreatorTest: public ::testing::Test, TestWithFakeHomeDirectory { public: CryConfigCreatorTest() : console(make_shared()), diff --git a/test/cryfs/config/CryConfigLoaderTest.cpp b/test/cryfs/config/CryConfigLoaderTest.cpp index 77527755..4df10ef4 100644 --- a/test/cryfs/config/CryConfigLoaderTest.cpp +++ b/test/cryfs/config/CryConfigLoaderTest.cpp @@ -1,6 +1,7 @@ #include #include #include "../testutils/MockConsole.h" +#include "../testutils/TestWithFakeHomeDirectory.h" #include #include #include @@ -34,7 +35,7 @@ namespace boost { } #include -class CryConfigLoaderTest: public ::testing::Test, public TestWithMockConsole { +class CryConfigLoaderTest: public ::testing::Test, public TestWithMockConsole, TestWithFakeHomeDirectory { public: CryConfigLoaderTest(): file(false) { console = mockConsole(); diff --git a/test/cryfs/localstate/BasedirMetadataTest.cpp b/test/cryfs/localstate/BasedirMetadataTest.cpp index f5636b30..153f703d 100644 --- a/test/cryfs/localstate/BasedirMetadataTest.cpp +++ b/test/cryfs/localstate/BasedirMetadataTest.cpp @@ -4,6 +4,7 @@ #include #include #include +#include "../testutils/TestWithFakeHomeDirectory.h" using cpputils::TempDir; using cryfs::BasedirMetadata; @@ -11,7 +12,7 @@ using std::ofstream; namespace bf = boost::filesystem; using FilesystemID = cryfs::CryConfig::FilesystemID ; -class BasedirMetadataTest : public ::testing::Test { +class BasedirMetadataTest : public ::testing::Test, TestWithFakeHomeDirectory { public: TempDir tempdir; bf::path basedir1; @@ -26,8 +27,6 @@ public: , id1(FilesystemID::FromString("1491BB4932A389EE14BC7090AC772972")) , id2(FilesystemID::FromString("A1491BB493214BC7090C772972A389EE")) { - // Use temporary local state dir to not pollute local state - cryfs::LocalStateDir::setAppDir(tempdir.path() / "appdir"); // Create basedirs so bf::canonical() works bf::create_directories(basedir1); bf::create_directories(basedir2); From e5d6875523df68b8198aa38b21cf532ccc2d4074 Mon Sep 17 00:00:00 2001 From: Sebastian Messmer Date: Sat, 30 Sep 2017 09:06:50 +0100 Subject: [PATCH 08/26] Don't install old boost on CI because we're installing the new one afterwards anyhow --- .circleci/config.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index a765b2e7..76d74c26 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -49,7 +49,7 @@ references: sudo chmod o-w /etc/apt/sources.list.d/clang.list DEBIAN_FRONTEND=noninteractive sudo apt-get update -qq - DEBIAN_FRONTEND=noninteractive sudo apt-get install -y git ccache $APT_COMPILER_PACKAGE cmake make libcurl4-openssl-dev libboost-filesystem-dev libboost-system-dev libboost-chrono-dev libboost-program-options-dev libboost-thread-dev libcrypto++-dev libssl-dev libfuse-dev python + DEBIAN_FRONTEND=noninteractive sudo apt-get install -y git ccache $APT_COMPILER_PACKAGE cmake make libcurl4-openssl-dev libcrypto++-dev libssl-dev libfuse-dev python # Use /dev/urandom when /dev/random is accessed to use less entropy sudo cp -a /dev/urandom /dev/random From be9f7a4c3deebc1acc37a4a3ec3b936939d12664 Mon Sep 17 00:00:00 2001 From: Sebastian Messmer Date: Sat, 30 Sep 2017 09:18:46 +0100 Subject: [PATCH 09/26] Don't pollute users local state dir when running test cases --- test/cryfs-cli/testutils/CliTest.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/cryfs-cli/testutils/CliTest.h b/test/cryfs-cli/testutils/CliTest.h index c9825c6f..207c014e 100644 --- a/test/cryfs-cli/testutils/CliTest.h +++ b/test/cryfs-cli/testutils/CliTest.h @@ -12,8 +12,9 @@ #include #include #include "../../cryfs/testutils/MockConsole.h" +#include "../../cryfs/testutils/TestWithFakeHomeDirectory.h" -class CliTest : public ::testing::Test { +class CliTest : public ::testing::Test, TestWithFakeHomeDirectory { public: CliTest(): _basedir(), _mountdir(), basedir(_basedir.path()), mountdir(_mountdir.path()), logfile(), configfile(false), console(std::make_shared()) {} From bd34a04d0cf95074c7eaaabd2749a0d72650b904 Mon Sep 17 00:00:00 2001 From: Sebastian Messmer Date: Sat, 30 Sep 2017 09:30:31 +0100 Subject: [PATCH 10/26] Fix test cases --- test/cryfs/config/CryConfigLoaderTest.cpp | 25 ++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) diff --git a/test/cryfs/config/CryConfigLoaderTest.cpp b/test/cryfs/config/CryConfigLoaderTest.cpp index 4df10ef4..375da043 100644 --- a/test/cryfs/config/CryConfigLoaderTest.cpp +++ b/test/cryfs/config/CryConfigLoaderTest.cpp @@ -15,6 +15,7 @@ using cpputils::make_unique_ref; using cpputils::TempFile; using cpputils::SCrypt; using cpputils::DataFixture; +using cpputils::Data; using cpputils::NoninteractiveConsole; using boost::optional; using boost::none; @@ -35,6 +36,20 @@ namespace boost { } #include +class FakeRandomGenerator final : public cpputils::RandomGenerator { +public: + FakeRandomGenerator(Data output) + : _output(std::move(output)) {} + + void _get(void *target, size_t bytes) override { + ASSERT_EQ(_output.size(), bytes); + std::memcpy(target, _output.data(), bytes); + } + +private: + Data _output; +}; + class CryConfigLoaderTest: public ::testing::Test, public TestWithMockConsole, TestWithFakeHomeDirectory { public: CryConfigLoaderTest(): file(false) { @@ -79,7 +94,11 @@ public: } void CreateWithEncryptionKey(const string &encKey, const string &password = "mypassword") { - auto cfg = loader(password, false).loadOrCreate(file.path()).value().configFile; + auto askPassword = [password] { return password;}; + FakeRandomGenerator generator(Data::FromString(encKey)); + auto loader = CryConfigLoader(console, generator, SCrypt::TestSettings, askPassword, + askPassword, none, none, none); + auto cfg = loader.loadOrCreate(file.path()).value().configFile; cfg.config()->SetEncryptionKey(encKey); cfg.save(); } @@ -177,9 +196,9 @@ TEST_F(CryConfigLoaderTest, RootBlob_Create) { } TEST_F(CryConfigLoaderTest, EncryptionKey_Load) { - CreateWithEncryptionKey("encryptionkey"); + CreateWithEncryptionKey("3B4682CF22F3CA199E385729B9F3CA19D325229E385729B9443CA19D325229E3"); auto loaded = Load().value(); - EXPECT_EQ("encryptionkey", loaded.config()->EncryptionKey()); + EXPECT_EQ("3B4682CF22F3CA199E385729B9F3CA19D325229E385729B9443CA19D325229E3", loaded.config()->EncryptionKey()); } TEST_F(CryConfigLoaderTest, EncryptionKey_Create) { From 85759961ef9bd6416f7961529f93c2beebe2af9d Mon Sep 17 00:00:00 2001 From: Sebastian Messmer Date: Sat, 30 Sep 2017 18:53:03 +0100 Subject: [PATCH 11/26] Don't keep update check connection open after update check is finished --- src/cryfs-cli/Cli.cpp | 16 +++++++------- src/cryfs-cli/Cli.h | 9 ++++---- src/cryfs-cli/VersionChecker.cpp | 7 +++--- src/cryfs-cli/VersionChecker.h | 5 +++-- src/cryfs-cli/main.cpp | 5 +++-- test/cryfs/config/CryConfigLoaderTest.cpp | 27 ++++++++++++++--------- 6 files changed, 37 insertions(+), 32 deletions(-) diff --git a/src/cryfs-cli/Cli.cpp b/src/cryfs-cli/Cli.cpp index 36610651..bf398830 100644 --- a/src/cryfs-cli/Cli.cpp +++ b/src/cryfs-cli/Cli.cpp @@ -72,8 +72,8 @@ using gitversion::VersionCompare; namespace cryfs { - Cli::Cli(RandomGenerator &keyGenerator, const SCryptSettings &scryptSettings, shared_ptr console, shared_ptr httpClient): - _keyGenerator(keyGenerator), _scryptSettings(scryptSettings), _console(), _httpClient(httpClient), _noninteractive(false) { + Cli::Cli(RandomGenerator &keyGenerator, const SCryptSettings &scryptSettings, shared_ptr console): + _keyGenerator(keyGenerator), _scryptSettings(scryptSettings), _console(), _noninteractive(false) { _noninteractive = Environment::isNoninteractive(); if (_noninteractive) { _console = make_shared(console); @@ -82,7 +82,7 @@ namespace cryfs { } } - void Cli::_showVersion() { + void Cli::_showVersion(unique_ref httpClient) { cout << "CryFS Version " << gitversion::VersionString() << endl; if (gitversion::IsDevVersion()) { cout << "WARNING! This is a development version based on git commit " << gitversion::GitCommitId() << @@ -99,7 +99,7 @@ namespace cryfs { } else if (Environment::isNoninteractive()) { cout << "Automatic checking for security vulnerabilities and updates is disabled in noninteractive mode." << endl; } else { - _checkForUpdates(); + _checkForUpdates(std::move(httpClient)); } #else # warning Update checks are disabled. The resulting executable will not go online to check for newer versions or known security vulnerabilities. @@ -107,8 +107,8 @@ namespace cryfs { cout << endl; } - void Cli::_checkForUpdates() { - VersionChecker versionChecker(_httpClient); + void Cli::_checkForUpdates(unique_ref httpClient) { + VersionChecker versionChecker(std::move(httpClient)); optional newestVersion = versionChecker.newestVersion(); if (newestVersion == none) { cout << "Could not check for updates." << endl; @@ -378,9 +378,9 @@ namespace cryfs { return false; } - int Cli::main(int argc, const char *argv[]) { + int Cli::main(int argc, const char *argv[], unique_ref httpClient) { cpputils::showBacktraceOnSigSegv(); - _showVersion(); + _showVersion(std::move(httpClient)); ProgramOptions options = program_options::Parser(argc, argv).parse(CryCiphers::supportedCipherNames()); diff --git a/src/cryfs-cli/Cli.h b/src/cryfs-cli/Cli.h index ed32b7c2..96c9e1d3 100644 --- a/src/cryfs-cli/Cli.h +++ b/src/cryfs-cli/Cli.h @@ -16,11 +16,11 @@ namespace cryfs { class Cli final { public: - Cli(cpputils::RandomGenerator &keyGenerator, const cpputils::SCryptSettings &scryptSettings, std::shared_ptr console, std::shared_ptr httpClient); - int main(int argc, const char *argv[]); + Cli(cpputils::RandomGenerator &keyGenerator, const cpputils::SCryptSettings &scryptSettings, std::shared_ptr console); + int main(int argc, const char *argv[], cpputils::unique_ref httpClient); private: - void _checkForUpdates(); + void _checkForUpdates(cpputils::unique_ref httpClient); void _runFilesystem(const program_options::ProgramOptions &options); CryConfigLoader::ConfigLoadResult _loadOrCreateConfig(const program_options::ProgramOptions &options); void _checkConfigIntegrity(const boost::filesystem::path& basedir, const CryConfigFile& config); @@ -32,7 +32,7 @@ namespace cryfs { static std::string _askPasswordFromStdin(const std::string &prompt); static bool _confirmPassword(const std::string &password); static bool _checkPassword(const std::string &password); - void _showVersion(); + void _showVersion(cpputils::unique_ref httpClient); void _initLogfile(const program_options::ProgramOptions &options); void _sanityChecks(const program_options::ProgramOptions &options); void _checkMountdirDoesntContainBasedir(const program_options::ProgramOptions &options); @@ -47,7 +47,6 @@ namespace cryfs { cpputils::RandomGenerator &_keyGenerator; cpputils::SCryptSettings _scryptSettings; std::shared_ptr _console; - std::shared_ptr _httpClient; bool _noninteractive; DISALLOW_COPY_AND_ASSIGN(Cli); diff --git a/src/cryfs-cli/VersionChecker.cpp b/src/cryfs-cli/VersionChecker.cpp index ec5ba989..7d17b75c 100644 --- a/src/cryfs-cli/VersionChecker.cpp +++ b/src/cryfs-cli/VersionChecker.cpp @@ -8,17 +8,16 @@ using boost::optional; using boost::none; using std::string; -using std::shared_ptr; -using std::make_shared; using cpputils::HttpClient; using cpputils::CurlHttpClient; using boost::property_tree::ptree; using boost::property_tree::json_parser_error; +using cpputils::unique_ref; using namespace cpputils::logging; namespace cryfs { - VersionChecker::VersionChecker(shared_ptr httpClient) + VersionChecker::VersionChecker(unique_ref httpClient) : _versionInfo(_getVersionInfo(std::move(httpClient))) { } @@ -49,7 +48,7 @@ namespace cryfs { return none; } - optional VersionChecker::_getVersionInfo(shared_ptr httpClient) { + optional VersionChecker::_getVersionInfo(unique_ref httpClient) { long timeoutMsec = 2000; optional response = httpClient->get("https://www.cryfs.org/version_info.json", timeoutMsec); if (response == none) { diff --git a/src/cryfs-cli/VersionChecker.h b/src/cryfs-cli/VersionChecker.h index e3ea2d7f..220570c1 100644 --- a/src/cryfs-cli/VersionChecker.h +++ b/src/cryfs-cli/VersionChecker.h @@ -6,17 +6,18 @@ #include #include #include +#include namespace cryfs { class VersionChecker final { public: //TODO Write a cpputils::shared_ref and use it - VersionChecker(std::shared_ptr httpClient); + VersionChecker(cpputils::unique_ref httpClient); boost::optional newestVersion() const; boost::optional securityWarningFor(const std::string &version) const; private: - static boost::optional _getVersionInfo(std::shared_ptr httpClient); + static boost::optional _getVersionInfo(cpputils::unique_ref httpClient); static boost::optional _parseJson(const std::string &json); boost::optional _versionInfo; diff --git a/src/cryfs-cli/main.cpp b/src/cryfs-cli/main.cpp index 162b8c82..c5981475 100644 --- a/src/cryfs-cli/main.cpp +++ b/src/cryfs-cli/main.cpp @@ -9,14 +9,15 @@ using cpputils::Random; using cpputils::SCrypt; using cpputils::CurlHttpClient; using cpputils::IOStreamConsole; +using cpputils::make_unique_ref; using std::make_shared; using std::cerr; int main(int argc, const char *argv[]) { try { auto &keyGenerator = Random::OSRandom(); - return Cli(keyGenerator, SCrypt::DefaultSettings, make_shared(), - make_shared()).main(argc, argv); + return Cli(keyGenerator, SCrypt::DefaultSettings, make_shared()) + .main(argc, argv, make_unique_ref()); } catch (const std::exception &e) { cerr << "Error: " << e.what(); return EXIT_FAILURE; diff --git a/test/cryfs/config/CryConfigLoaderTest.cpp b/test/cryfs/config/CryConfigLoaderTest.cpp index 375da043..364ce7d4 100644 --- a/test/cryfs/config/CryConfigLoaderTest.cpp +++ b/test/cryfs/config/CryConfigLoaderTest.cpp @@ -281,18 +281,7 @@ TEST_F(CryConfigLoaderTest, AsksWhenMigratingOlderFilesystem) { EXPECT_NE(boost::none, Load()); } -TEST_F(CryConfigLoaderTest, DoesNotAskForMigrationWhenCorrectVersion) { - EXPECT_CALL(*console, askYesNo(HasSubstr("Do you want to migrate it?"), false)).Times(0); - CreateWithVersion(gitversion::VersionString()); - EXPECT_NE(boost::none, Load()); -} - -TEST_F(CryConfigLoaderTest, DontMigrateWhenAnsweredNo) { - EXPECT_CALL(*console, askYesNo(HasSubstr("Do you want to migrate it?"), false)).Times(1).WillOnce(Return(false)); - - string version = olderVersion(); - CreateWithVersion(version); try { Load(); EXPECT_TRUE(false); // expect throw @@ -307,9 +296,25 @@ TEST_F(CryConfigLoaderTest, MyClientIdIsIndeterministic) { uint32_t myClientId = loader("mypassword", true).loadOrCreate(file1.path()).value().myClientId; EXPECT_NE(myClientId, loader("mypassword", true).loadOrCreate(file2.path()).value().myClientId); } +TEST_F(CryConfigLoaderTest, DoesNotAskForMigrationWhenCorrectVersion) { + EXPECT_CALL(*console, askYesNo(HasSubstr("Do you want to migrate it?"), false)).Times(0); + CreateWithVersion(gitversion::VersionString()); + EXPECT_NE(boost::none, Load()); +} + +TEST_F(CryConfigLoaderTest, DontMigrateWhenAnsweredNo) { + EXPECT_CALL(*console, askYesNo(HasSubstr("Do you want to migrate it?"), false)).Times(1).WillOnce(Return(false)); + + string version = olderVersion(); + CreateWithVersion(version); TEST_F(CryConfigLoaderTest, MyClientIdIsLoadedCorrectly) { TempFile file(false); uint32_t myClientId = loader("mypassword", true).loadOrCreate(file.path()).value().myClientId; EXPECT_EQ(myClientId, loader("mypassword", true).loadOrCreate(file.path()).value().myClientId); } + +// TODO Test behavior if +// - filesystem id changed +// - filesystem id correct but encryption key changed +// TODO Add cryfs-cli tests for the same thing From 7e01e84d351cb64c7ecd2c880d70d6c5b0a1de2e Mon Sep 17 00:00:00 2001 From: Sebastian Messmer Date: Sat, 30 Sep 2017 21:35:02 +0100 Subject: [PATCH 12/26] Fix accidental change from last commit --- test/cryfs/config/CryConfigLoaderTest.cpp | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/test/cryfs/config/CryConfigLoaderTest.cpp b/test/cryfs/config/CryConfigLoaderTest.cpp index 364ce7d4..71a57ce1 100644 --- a/test/cryfs/config/CryConfigLoaderTest.cpp +++ b/test/cryfs/config/CryConfigLoaderTest.cpp @@ -281,7 +281,18 @@ TEST_F(CryConfigLoaderTest, AsksWhenMigratingOlderFilesystem) { EXPECT_NE(boost::none, Load()); } +TEST_F(CryConfigLoaderTest, DoesNotAskForMigrationWhenCorrectVersion) { + EXPECT_CALL(*console, askYesNo(HasSubstr("Do you want to migrate it?"), false)).Times(0); + CreateWithVersion(gitversion::VersionString()); + EXPECT_NE(boost::none, Load()); +} + +TEST_F(CryConfigLoaderTest, DontMigrateWhenAnsweredNo) { + EXPECT_CALL(*console, askYesNo(HasSubstr("Do you want to migrate it?"), false)).Times(1).WillOnce(Return(false)); + + string version = olderVersion(); + CreateWithVersion(version); try { Load(); EXPECT_TRUE(false); // expect throw @@ -296,18 +307,7 @@ TEST_F(CryConfigLoaderTest, MyClientIdIsIndeterministic) { uint32_t myClientId = loader("mypassword", true).loadOrCreate(file1.path()).value().myClientId; EXPECT_NE(myClientId, loader("mypassword", true).loadOrCreate(file2.path()).value().myClientId); } -TEST_F(CryConfigLoaderTest, DoesNotAskForMigrationWhenCorrectVersion) { - EXPECT_CALL(*console, askYesNo(HasSubstr("Do you want to migrate it?"), false)).Times(0); - CreateWithVersion(gitversion::VersionString()); - EXPECT_NE(boost::none, Load()); -} - -TEST_F(CryConfigLoaderTest, DontMigrateWhenAnsweredNo) { - EXPECT_CALL(*console, askYesNo(HasSubstr("Do you want to migrate it?"), false)).Times(1).WillOnce(Return(false)); - - string version = olderVersion(); - CreateWithVersion(version); TEST_F(CryConfigLoaderTest, MyClientIdIsLoadedCorrectly) { TempFile file(false); uint32_t myClientId = loader("mypassword", true).loadOrCreate(file.path()).value().myClientId; From be8a1efd3561bad2d79d77c9fa8cbde0d5fff3cf Mon Sep 17 00:00:00 2001 From: Sebastian Messmer Date: Sat, 30 Sep 2017 22:24:33 +0100 Subject: [PATCH 13/26] Improve test cases --- src/cryfs-cli/Cli.cpp | 2 +- src/cryfs-cli/VersionChecker.cpp | 6 +++--- src/cryfs-cli/VersionChecker.h | 4 ++-- test/cryfs-cli/CMakeLists.txt | 1 + test/cryfs-cli/IntegrityCheckTest.cpp | 3 +++ test/cryfs-cli/VersionCheckerTest.cpp | 6 +++--- test/cryfs-cli/testutils/CliTest.h | 6 +++--- test/cryfs/config/CryConfigLoaderTest.cpp | 26 +++++++++++++++++------ 8 files changed, 36 insertions(+), 18 deletions(-) create mode 100644 test/cryfs-cli/IntegrityCheckTest.cpp diff --git a/src/cryfs-cli/Cli.cpp b/src/cryfs-cli/Cli.cpp index bf398830..f0e474bf 100644 --- a/src/cryfs-cli/Cli.cpp +++ b/src/cryfs-cli/Cli.cpp @@ -108,7 +108,7 @@ namespace cryfs { } void Cli::_checkForUpdates(unique_ref httpClient) { - VersionChecker versionChecker(std::move(httpClient)); + VersionChecker versionChecker(httpClient.get()); optional newestVersion = versionChecker.newestVersion(); if (newestVersion == none) { cout << "Could not check for updates." << endl; diff --git a/src/cryfs-cli/VersionChecker.cpp b/src/cryfs-cli/VersionChecker.cpp index 7d17b75c..89a1b94b 100644 --- a/src/cryfs-cli/VersionChecker.cpp +++ b/src/cryfs-cli/VersionChecker.cpp @@ -17,8 +17,8 @@ using namespace cpputils::logging; namespace cryfs { - VersionChecker::VersionChecker(unique_ref httpClient) - : _versionInfo(_getVersionInfo(std::move(httpClient))) { + VersionChecker::VersionChecker(HttpClient* httpClient) + : _versionInfo(_getVersionInfo(httpClient)) { } optional VersionChecker::newestVersion() const { @@ -48,7 +48,7 @@ namespace cryfs { return none; } - optional VersionChecker::_getVersionInfo(unique_ref httpClient) { + optional VersionChecker::_getVersionInfo(HttpClient* httpClient) { long timeoutMsec = 2000; optional response = httpClient->get("https://www.cryfs.org/version_info.json", timeoutMsec); if (response == none) { diff --git a/src/cryfs-cli/VersionChecker.h b/src/cryfs-cli/VersionChecker.h index 220570c1..bc1e54a9 100644 --- a/src/cryfs-cli/VersionChecker.h +++ b/src/cryfs-cli/VersionChecker.h @@ -12,12 +12,12 @@ namespace cryfs { class VersionChecker final { public: //TODO Write a cpputils::shared_ref and use it - VersionChecker(cpputils::unique_ref httpClient); + VersionChecker(cpputils::HttpClient* httpClient); boost::optional newestVersion() const; boost::optional securityWarningFor(const std::string &version) const; private: - static boost::optional _getVersionInfo(cpputils::unique_ref httpClient); + static boost::optional _getVersionInfo(cpputils::HttpClient* httpClient); static boost::optional _parseJson(const std::string &json); boost::optional _versionInfo; diff --git a/test/cryfs-cli/CMakeLists.txt b/test/cryfs-cli/CMakeLists.txt index e9e62ee6..decbbfa4 100644 --- a/test/cryfs-cli/CMakeLists.txt +++ b/test/cryfs-cli/CMakeLists.txt @@ -11,6 +11,7 @@ set(SOURCES CliTest_ShowingHelp.cpp EnvironmentTest.cpp VersionCheckerTest.cpp + IntegrityCheckTest.cpp ) add_executable(${PROJECT_NAME} ${SOURCES}) diff --git a/test/cryfs-cli/IntegrityCheckTest.cpp b/test/cryfs-cli/IntegrityCheckTest.cpp new file mode 100644 index 00000000..45a16cb0 --- /dev/null +++ b/test/cryfs-cli/IntegrityCheckTest.cpp @@ -0,0 +1,3 @@ +//TODO Add cryfs-cli tests for +// - filesystem id changed +// - filesystem id correct but encryption key changed diff --git a/test/cryfs-cli/VersionCheckerTest.cpp b/test/cryfs-cli/VersionCheckerTest.cpp index 66950038..8f2bdea6 100644 --- a/test/cryfs-cli/VersionCheckerTest.cpp +++ b/test/cryfs-cli/VersionCheckerTest.cpp @@ -15,15 +15,15 @@ using namespace cryfs; class VersionCheckerTest: public ::testing::Test { public: unique_ref versionChecker() { - return make_unique_ref(http); + return make_unique_ref(_http.get()); } void setVersionInfo(const string &versionInfo) { - http->addWebsite("https://www.cryfs.org/version_info.json", versionInfo); + _http->addWebsite("https://www.cryfs.org/version_info.json", versionInfo); } private: - shared_ptr http = make_shared(); + unique_ref _http = make_unique_ref(); }; TEST_F(VersionCheckerTest, NewestVersion_NoInternet) { diff --git a/test/cryfs-cli/testutils/CliTest.h b/test/cryfs-cli/testutils/CliTest.h index 207c014e..4a65b440 100644 --- a/test/cryfs-cli/testutils/CliTest.h +++ b/test/cryfs-cli/testutils/CliTest.h @@ -26,8 +26,8 @@ public: cpputils::TempFile configfile; std::shared_ptr console; - std::shared_ptr _httpClient() { - std::shared_ptr httpClient = std::make_shared(); + cpputils::unique_ref _httpClient() { + cpputils::unique_ref httpClient = cpputils::make_unique_ref(); httpClient->addWebsite("https://www.cryfs.org/version_info.json", "{\"version_info\":{\"current\":\"0.8.5\"}}"); return httpClient; } @@ -44,7 +44,7 @@ public: std::cin.putback('\n'); std::cin.putback('s'); std::cin.putback('s'); std::cin.putback('a'); std::cin.putback('p'); std::cin.putback('\n'); std::cin.putback('s'); std::cin.putback('s'); std::cin.putback('a'); std::cin.putback('p'); // Run Cryfs - cryfs::Cli(keyGenerator, cpputils::SCrypt::TestSettings, console, _httpClient()).main(_args.size(), _args.data()); + cryfs::Cli(keyGenerator, cpputils::SCrypt::TestSettings, console).main(_args.size(), _args.data(), _httpClient()); } void EXPECT_EXIT_WITH_HELP_MESSAGE(std::vector args, const std::string &message = "") { diff --git a/test/cryfs/config/CryConfigLoaderTest.cpp b/test/cryfs/config/CryConfigLoaderTest.cpp index 71a57ce1..9eb373d1 100644 --- a/test/cryfs/config/CryConfigLoaderTest.cpp +++ b/test/cryfs/config/CryConfigLoaderTest.cpp @@ -98,7 +98,11 @@ public: FakeRandomGenerator generator(Data::FromString(encKey)); auto loader = CryConfigLoader(console, generator, SCrypt::TestSettings, askPassword, askPassword, none, none, none); - auto cfg = loader.loadOrCreate(file.path()).value().configFile; + loader.loadOrCreate(file.path()).value().configFile; + } + + void ChangeEncryptionKey(const string &encKey, const string& password = "mypassword") { + auto cfg = loader(password, false).loadOrCreate(file.path()).value().configFile; cfg.config()->SetEncryptionKey(encKey); cfg.save(); } @@ -116,6 +120,12 @@ public: cfg.save(); } + void ChangeFilesystemID(const CryConfig::FilesystemID &filesystemId, const string& password = "mypassword") { + auto cfg = loader(password, false).loadOrCreate(file.path()).value().configFile; + cfg.config()->SetFilesystemId(filesystemId); + cfg.save(); + } + string olderVersion() { string olderVersion; if (std::stol(gitversion::MinorVersion()) > 0) { @@ -201,6 +211,15 @@ TEST_F(CryConfigLoaderTest, EncryptionKey_Load) { EXPECT_EQ("3B4682CF22F3CA199E385729B9F3CA19D325229E385729B9443CA19D325229E3", loaded.config()->EncryptionKey()); } +TEST_F(CryConfigLoaderTest, EncryptionKey_Load_whenKeyChanged_thenFails) { + CreateWithEncryptionKey("3B4682CF22F3CA199E385729B9F3CA19D325229E385729B9443CA19D325229E3"); + ChangeEncryptionKey("3B4682CF22F3CA199E385729B9F3CA19D325229E385729B9443CA19D325229E4"); + EXPECT_THROW( + Load(), + std::runtime_error + ); +} + TEST_F(CryConfigLoaderTest, EncryptionKey_Create) { auto created = Create(); //aes-256-gcm is the default cipher chosen by mockConsole() @@ -313,8 +332,3 @@ TEST_F(CryConfigLoaderTest, MyClientIdIsLoadedCorrectly) { uint32_t myClientId = loader("mypassword", true).loadOrCreate(file.path()).value().myClientId; EXPECT_EQ(myClientId, loader("mypassword", true).loadOrCreate(file.path()).value().myClientId); } - -// TODO Test behavior if -// - filesystem id changed -// - filesystem id correct but encryption key changed -// TODO Add cryfs-cli tests for the same thing From 04341f3f7dd06efca51536e7c8567786a80ae699 Mon Sep 17 00:00:00 2001 From: Sebastian Messmer Date: Sat, 30 Sep 2017 22:42:34 +0100 Subject: [PATCH 14/26] Add test cases that make sure that CryFS notices if an attacker replaces the whole file system --- test/cryfs-cli/CMakeLists.txt | 2 +- test/cryfs-cli/CliTest_IntegrityCheck.cpp | 44 +++++++++++++++++++++++ test/cryfs-cli/IntegrityCheckTest.cpp | 3 -- 3 files changed, 45 insertions(+), 4 deletions(-) create mode 100644 test/cryfs-cli/CliTest_IntegrityCheck.cpp delete mode 100644 test/cryfs-cli/IntegrityCheckTest.cpp diff --git a/test/cryfs-cli/CMakeLists.txt b/test/cryfs-cli/CMakeLists.txt index decbbfa4..a122ae97 100644 --- a/test/cryfs-cli/CMakeLists.txt +++ b/test/cryfs-cli/CMakeLists.txt @@ -11,7 +11,7 @@ set(SOURCES CliTest_ShowingHelp.cpp EnvironmentTest.cpp VersionCheckerTest.cpp - IntegrityCheckTest.cpp + CliTest_IntegrityCheck.cpp ) add_executable(${PROJECT_NAME} ${SOURCES}) diff --git a/test/cryfs-cli/CliTest_IntegrityCheck.cpp b/test/cryfs-cli/CliTest_IntegrityCheck.cpp new file mode 100644 index 00000000..24c2641a --- /dev/null +++ b/test/cryfs-cli/CliTest_IntegrityCheck.cpp @@ -0,0 +1,44 @@ +#include "testutils/CliTest.h" +#include + +using std::vector; +using std::string; +using cryfs::CryConfig; +using cryfs::CryConfigFile; + +class CliTest_IntegrityCheck: public CliTest { +public: + void modifyFilesystemId() { + auto configFile = CryConfigFile::load(basedir / "cryfs.config", "pass").value(); + configFile.config()->SetFilesystemId(CryConfig::FilesystemID::FromString("0123456789ABCDEF0123456789ABCDEF")); + configFile.save(); + } + + void modifyFilesystemKey() { + auto configFile = CryConfigFile::load(basedir / "cryfs.config", "pass").value(); + configFile.config()->SetEncryptionKey("0123456789ABCDEF0123456789ABCDEF"); + configFile.save(); + } +}; + +TEST_F(CliTest_IntegrityCheck, givenIncorrectFilesystemId_thenFails) { + vector args {basedir.c_str(), mountdir.c_str(), "--cipher", "aes-256-gcm", "-f"}; + //TODO Remove "-f" parameter, once EXPECT_RUN_SUCCESS can handle that + EXPECT_RUN_SUCCESS(args, mountdir); + modifyFilesystemId(); + EXPECT_RUN_ERROR( + args, + "Error: The filesystem id in the config file is different to the last time we loaded a filesystem from this basedir." + ); +} + +TEST_F(CliTest_IntegrityCheck, givenIncorrectFilesystemKey_thenFails) { + vector args {basedir.c_str(), mountdir.c_str(), "--cipher", "aes-256-gcm", "-f"}; + //TODO Remove "-f" parameter, once EXPECT_RUN_SUCCESS can handle that + EXPECT_RUN_SUCCESS(args, mountdir); + modifyFilesystemKey(); + EXPECT_RUN_ERROR( + args, + "Error: The filesystem encryption key differs from the last time we loaded this filesystem. Did an attacker replace the file system?" + ); +} diff --git a/test/cryfs-cli/IntegrityCheckTest.cpp b/test/cryfs-cli/IntegrityCheckTest.cpp deleted file mode 100644 index 45a16cb0..00000000 --- a/test/cryfs-cli/IntegrityCheckTest.cpp +++ /dev/null @@ -1,3 +0,0 @@ -//TODO Add cryfs-cli tests for -// - filesystem id changed -// - filesystem id correct but encryption key changed From 0a7fce670195f05944dd2b3ff3bb3f2b07efa6b1 Mon Sep 17 00:00:00 2001 From: Sebastian Messmer Date: Sat, 30 Sep 2017 22:44:24 +0100 Subject: [PATCH 15/26] Improve CryConfigLoaderTest --- test/cryfs/config/CryConfigLoaderTest.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/cryfs/config/CryConfigLoaderTest.cpp b/test/cryfs/config/CryConfigLoaderTest.cpp index 9eb373d1..ac109fee 100644 --- a/test/cryfs/config/CryConfigLoaderTest.cpp +++ b/test/cryfs/config/CryConfigLoaderTest.cpp @@ -102,7 +102,7 @@ public: } void ChangeEncryptionKey(const string &encKey, const string& password = "mypassword") { - auto cfg = loader(password, false).loadOrCreate(file.path()).value().configFile; + auto cfg = CryConfigFile::load(file.path(), password).value(); cfg.config()->SetEncryptionKey(encKey); cfg.save(); } @@ -121,7 +121,7 @@ public: } void ChangeFilesystemID(const CryConfig::FilesystemID &filesystemId, const string& password = "mypassword") { - auto cfg = loader(password, false).loadOrCreate(file.path()).value().configFile; + auto cfg = CryConfigFile::load(file.path(), password).value(); cfg.config()->SetFilesystemId(filesystemId); cfg.save(); } From 378777796704b61133b42eb1553a4e838605bb87 Mon Sep 17 00:00:00 2001 From: Sebastian Messmer Date: Sun, 1 Oct 2017 09:04:29 +0100 Subject: [PATCH 16/26] Optimize std::move use --- .../compressing/CompressedBlock.h | 2 +- .../low2highlevel/LowToHighLevelBlock.cpp | 2 +- .../implementations/testfake/FakeBlock.cpp | 2 +- src/cpp-utils/assert/AssertFailed.h | 2 +- .../crypto/symmetric/EncryptionKey.h | 2 +- src/cpp-utils/data/DataUtils.cpp | 2 +- src/cpp-utils/data/DataUtils.h | 2 +- src/cpp-utils/random/RandomDataBuffer.h | 4 ++-- .../random/RandomGeneratorThread.cpp | 2 +- .../random/ThreadsafeRandomDataBuffer.h | 6 ++--- src/cpp-utils/thread/LoopThread.cpp | 2 +- src/cpp-utils/thread/ThreadSystem.cpp | 6 +++-- src/cryfs-cli/CallAfterTimeout.h | 2 +- src/cryfs-cli/Cli.cpp | 8 +++---- src/cryfs-cli/Cli.h | 2 +- src/cryfs-cli/program_options/Parser.cpp | 2 +- .../program_options/ProgramOptions.cpp | 18 +++++++------- .../program_options/ProgramOptions.h | 16 ++++++------- src/cryfs/config/CryConfig.cpp | 24 +++++++++---------- src/cryfs/config/CryConfig.h | 12 +++++----- src/cryfs/config/CryConfigFile.cpp | 12 +++++----- src/cryfs/config/CryConfigFile.h | 6 ++--- src/cryfs/config/CryConfigLoader.cpp | 16 ++++++------- src/cryfs/config/CryConfigLoader.h | 6 ++--- .../config/crypto/CryConfigEncryptor.cpp | 4 ++-- src/cryfs/config/crypto/CryConfigEncryptor.h | 2 +- .../crypto/inner/ConcreteInnerEncryptor.h | 6 ++--- .../config/crypto/outer/OuterEncryptor.cpp | 4 ++-- .../config/crypto/outer/OuterEncryptor.h | 2 +- src/fspp/fuse/Fuse.cpp | 4 ++-- src/fspp/fuse/Fuse.h | 2 +- 31 files changed, 92 insertions(+), 90 deletions(-) diff --git a/src/blockstore/implementations/compressing/CompressedBlock.h b/src/blockstore/implementations/compressing/CompressedBlock.h index 84951d8d..77637d66 100644 --- a/src/blockstore/implementations/compressing/CompressedBlock.h +++ b/src/blockstore/implementations/compressing/CompressedBlock.h @@ -111,7 +111,7 @@ size_t CompressedBlock::size() const { template void CompressedBlock::resize(size_t newSize) { - _decompressedData = cpputils::DataUtils::resize(std::move(_decompressedData), newSize); + _decompressedData = cpputils::DataUtils::resize(_decompressedData, newSize); _dataChanged = true; } diff --git a/src/blockstore/implementations/low2highlevel/LowToHighLevelBlock.cpp b/src/blockstore/implementations/low2highlevel/LowToHighLevelBlock.cpp index 2e2abda8..b20cc502 100644 --- a/src/blockstore/implementations/low2highlevel/LowToHighLevelBlock.cpp +++ b/src/blockstore/implementations/low2highlevel/LowToHighLevelBlock.cpp @@ -67,7 +67,7 @@ size_t LowToHighLevelBlock::size() const { } void LowToHighLevelBlock::resize(size_t newSize) { - _data = DataUtils::resize(std::move(_data), newSize); + _data = DataUtils::resize(_data, newSize); _dataChanged = true; } diff --git a/src/blockstore/implementations/testfake/FakeBlock.cpp b/src/blockstore/implementations/testfake/FakeBlock.cpp index 57a2e241..1517d5d2 100644 --- a/src/blockstore/implementations/testfake/FakeBlock.cpp +++ b/src/blockstore/implementations/testfake/FakeBlock.cpp @@ -39,7 +39,7 @@ size_t FakeBlock::size() const { } void FakeBlock::resize(size_t newSize) { - *_data = cpputils::DataUtils::resize(std::move(*_data), newSize); + *_data = cpputils::DataUtils::resize(*_data, newSize); _dataChanged = true; } diff --git a/src/cpp-utils/assert/AssertFailed.h b/src/cpp-utils/assert/AssertFailed.h index 3d9b8978..db1c5ae3 100644 --- a/src/cpp-utils/assert/AssertFailed.h +++ b/src/cpp-utils/assert/AssertFailed.h @@ -10,7 +10,7 @@ namespace cpputils { class AssertFailed final: public std::exception { public: - AssertFailed(const std::string &message) : _message(message) { } + AssertFailed(std::string message) : _message(std::move(message)) { } const char *what() const throw() override { return _message.c_str(); diff --git a/src/cpp-utils/crypto/symmetric/EncryptionKey.h b/src/cpp-utils/crypto/symmetric/EncryptionKey.h index ceff107f..534d4a8a 100644 --- a/src/cpp-utils/crypto/symmetric/EncryptionKey.h +++ b/src/cpp-utils/crypto/symmetric/EncryptionKey.h @@ -27,7 +27,7 @@ private: constexpr static size_t BINARY_LENGTH = FixedSizeData::BINARY_LENGTH; constexpr static size_t STRING_LENGTH = FixedSizeData::STRING_LENGTH; - EncryptionKeyData(const FixedSizeData& keyData); + EncryptionKeyData(const FixedSizeData& keyData); ~EncryptionKeyData(); // Disallow copying and moving diff --git a/src/cpp-utils/data/DataUtils.cpp b/src/cpp-utils/data/DataUtils.cpp index 66a1819a..83c2be92 100644 --- a/src/cpp-utils/data/DataUtils.cpp +++ b/src/cpp-utils/data/DataUtils.cpp @@ -2,7 +2,7 @@ namespace cpputils { namespace DataUtils { - Data resize(Data data, size_t newSize) { + Data resize(const Data& data, size_t newSize) { Data newData(newSize); newData.FillWithZeroes(); // TODO Only fill region after copied old data with zeroes std::memcpy(newData.data(), data.data(), std::min(newData.size(), data.size())); diff --git a/src/cpp-utils/data/DataUtils.h b/src/cpp-utils/data/DataUtils.h index e61abc6f..938d9a89 100644 --- a/src/cpp-utils/data/DataUtils.h +++ b/src/cpp-utils/data/DataUtils.h @@ -10,7 +10,7 @@ namespace cpputils { //Return a new data object with the given size and initialize as much as possible with the given input data. //If the new data object is larger, then the remaining bytes will be zero filled. - Data resize(Data data, size_t newSize); + Data resize(const Data& data, size_t newSize); } } diff --git a/src/cpp-utils/random/RandomDataBuffer.h b/src/cpp-utils/random/RandomDataBuffer.h index 89c5fbe9..026a955c 100644 --- a/src/cpp-utils/random/RandomDataBuffer.h +++ b/src/cpp-utils/random/RandomDataBuffer.h @@ -15,7 +15,7 @@ namespace cpputils { void get(void *target, size_t bytes); - void add(Data data); + void add(const Data& data); private: size_t _usedUntil; @@ -37,7 +37,7 @@ namespace cpputils { _usedUntil += numBytes; } - inline void RandomDataBuffer::add(Data newData) { + inline void RandomDataBuffer::add(const Data& newData) { // Concatenate old and new random data size_t oldSize = size(); Data combined(oldSize + newData.size()); diff --git a/src/cpp-utils/random/RandomGeneratorThread.cpp b/src/cpp-utils/random/RandomGeneratorThread.cpp index 3bbe4872..418f7124 100644 --- a/src/cpp-utils/random/RandomGeneratorThread.cpp +++ b/src/cpp-utils/random/RandomGeneratorThread.cpp @@ -21,7 +21,7 @@ namespace cpputils { size_t neededRandomDataSize = _maxSize - _buffer->size(); ASSERT(_maxSize > _buffer->size(), "This could theoretically fail if another thread refilled the buffer. But we should be the only refilling thread."); Data randomData = _generateRandomData(neededRandomDataSize); - _buffer->add(std::move(randomData)); + _buffer->add(randomData); return true; // Run another iteration (don't terminate thread) } diff --git a/src/cpp-utils/random/ThreadsafeRandomDataBuffer.h b/src/cpp-utils/random/ThreadsafeRandomDataBuffer.h index c9ee6938..adbec7ac 100644 --- a/src/cpp-utils/random/ThreadsafeRandomDataBuffer.h +++ b/src/cpp-utils/random/ThreadsafeRandomDataBuffer.h @@ -17,7 +17,7 @@ namespace cpputils { void get(void *target, size_t numBytes); - void add(Data data); + void add(const Data& data); void waitUntilSizeIsLessThan(size_t numBytes); @@ -63,9 +63,9 @@ namespace cpputils { return gettableBytes; } - inline void ThreadsafeRandomDataBuffer::add(Data data) { + inline void ThreadsafeRandomDataBuffer::add(const Data& data) { boost::unique_lock lock(_mutex); - _buffer.add(std::move(data)); + _buffer.add(data); _dataAddedCv.notify_all(); } diff --git a/src/cpp-utils/thread/LoopThread.cpp b/src/cpp-utils/thread/LoopThread.cpp index 6573d902..12489433 100644 --- a/src/cpp-utils/thread/LoopThread.cpp +++ b/src/cpp-utils/thread/LoopThread.cpp @@ -6,7 +6,7 @@ using boost::none; namespace cpputils { - LoopThread::LoopThread(function loopIteration): _loopIteration(loopIteration), _runningHandle(none) { + LoopThread::LoopThread(function loopIteration): _loopIteration(std::move(loopIteration)), _runningHandle(none) { } LoopThread::~LoopThread() { diff --git a/src/cpp-utils/thread/ThreadSystem.cpp b/src/cpp-utils/thread/ThreadSystem.cpp index 81522704..6cce5866 100644 --- a/src/cpp-utils/thread/ThreadSystem.cpp +++ b/src/cpp-utils/thread/ThreadSystem.cpp @@ -19,7 +19,7 @@ namespace cpputils { ThreadSystem::Handle ThreadSystem::start(function loopIteration) { boost::unique_lock lock(_mutex); - auto thread = _startThread(loopIteration); + auto thread = _startThread(std::move(loopIteration)); _runningThreads.push_back(RunningThread{loopIteration, std::move(thread)}); return std::prev(_runningThreads.end()); } @@ -61,7 +61,9 @@ namespace cpputils { } boost::thread ThreadSystem::_startThread(function loopIteration) { - return boost::thread(std::bind(&ThreadSystem::_runThread, loopIteration)); + return boost::thread([loopIteration = std::move(loopIteration)] { + ThreadSystem::_runThread(std::move(loopIteration)); + }); } void ThreadSystem::_runThread(function loopIteration) { diff --git a/src/cryfs-cli/CallAfterTimeout.h b/src/cryfs-cli/CallAfterTimeout.h index 09db6195..afab392e 100644 --- a/src/cryfs-cli/CallAfterTimeout.h +++ b/src/cryfs-cli/CallAfterTimeout.h @@ -26,7 +26,7 @@ namespace cryfs { }; inline CallAfterTimeout::CallAfterTimeout(boost::chrono::milliseconds timeout, std::function callback) - :_callback(callback), _timeout(timeout), _start(), _checkTimeoutThread(std::bind(&CallAfterTimeout::_checkTimeoutThreadIteration, this)) { + :_callback(std::move(callback)), _timeout(timeout), _start(), _checkTimeoutThread(std::bind(&CallAfterTimeout::_checkTimeoutThreadIteration, this)) { resetTimer(); _checkTimeoutThread.start(); } diff --git a/src/cryfs-cli/Cli.cpp b/src/cryfs-cli/Cli.cpp index f0e474bf..f81d5278 100644 --- a/src/cryfs-cli/Cli.cpp +++ b/src/cryfs-cli/Cli.cpp @@ -213,7 +213,7 @@ namespace cryfs { CryConfigLoader::ConfigLoadResult Cli::_loadOrCreateConfig(const ProgramOptions &options) { try { auto configFile = _determineConfigFile(options); - auto config = _loadOrCreateConfigFile(configFile, options.cipher(), options.blocksizeBytes(), options.missingBlockIsIntegrityViolation()); + auto config = _loadOrCreateConfigFile(std::move(configFile), options.cipher(), options.blocksizeBytes(), options.missingBlockIsIntegrityViolation()); if (config == none) { std::cerr << "Could not load config file. Did you enter the correct password?" << std::endl; exit(1); @@ -226,17 +226,17 @@ namespace cryfs { } } - optional Cli::_loadOrCreateConfigFile(const bf::path &configFilePath, const optional &cipher, const optional &blocksizeBytes, const optional &missingBlockIsIntegrityViolation) { + optional Cli::_loadOrCreateConfigFile(bf::path configFilePath, const optional &cipher, const optional &blocksizeBytes, const optional &missingBlockIsIntegrityViolation) { if (_noninteractive) { return CryConfigLoader(_console, _keyGenerator, _scryptSettings, &Cli::_askPasswordNoninteractive, &Cli::_askPasswordNoninteractive, - cipher, blocksizeBytes, missingBlockIsIntegrityViolation).loadOrCreate(configFilePath); + cipher, blocksizeBytes, missingBlockIsIntegrityViolation).loadOrCreate(std::move(configFilePath)); } else { return CryConfigLoader(_console, _keyGenerator, _scryptSettings, &Cli::_askPasswordForExistingFilesystem, &Cli::_askPasswordForNewFilesystem, - cipher, blocksizeBytes, missingBlockIsIntegrityViolation).loadOrCreate(configFilePath); + cipher, blocksizeBytes, missingBlockIsIntegrityViolation).loadOrCreate(std::move(configFilePath)); } } diff --git a/src/cryfs-cli/Cli.h b/src/cryfs-cli/Cli.h index 96c9e1d3..f1398948 100644 --- a/src/cryfs-cli/Cli.h +++ b/src/cryfs-cli/Cli.h @@ -24,7 +24,7 @@ namespace cryfs { void _runFilesystem(const program_options::ProgramOptions &options); CryConfigLoader::ConfigLoadResult _loadOrCreateConfig(const program_options::ProgramOptions &options); void _checkConfigIntegrity(const boost::filesystem::path& basedir, const CryConfigFile& config); - boost::optional _loadOrCreateConfigFile(const boost::filesystem::path &configFilePath, const boost::optional &cipher, const boost::optional &blocksizeBytes, const boost::optional &missingBlockIsIntegrityViolation); + boost::optional _loadOrCreateConfigFile(boost::filesystem::path configFilePath, const boost::optional &cipher, const boost::optional &blocksizeBytes, const boost::optional &missingBlockIsIntegrityViolation); boost::filesystem::path _determineConfigFile(const program_options::ProgramOptions &options); static std::string _askPasswordForExistingFilesystem(); static std::string _askPasswordForNewFilesystem(); diff --git a/src/cryfs-cli/program_options/Parser.cpp b/src/cryfs-cli/program_options/Parser.cpp index c25b58e5..42c13880 100644 --- a/src/cryfs-cli/program_options/Parser.cpp +++ b/src/cryfs-cli/program_options/Parser.cpp @@ -92,7 +92,7 @@ ProgramOptions Parser::parse(const vector &supportedCiphers) const { } } - return ProgramOptions(baseDir, mountDir, configfile, foreground, unmountAfterIdleMinutes, logfile, cipher, blocksizeBytes, noIntegrityChecks, missingBlockIsIntegrityViolation, fuseOptions); + return ProgramOptions(std::move(baseDir), std::move(mountDir), std::move(configfile), foreground, std::move(unmountAfterIdleMinutes), std::move(logfile), std::move(cipher), blocksizeBytes, noIntegrityChecks, std::move(missingBlockIsIntegrityViolation), std::move(fuseOptions)); } void Parser::_checkValidCipher(const string &cipher, const vector &supportedCiphers) { diff --git a/src/cryfs-cli/program_options/ProgramOptions.cpp b/src/cryfs-cli/program_options/ProgramOptions.cpp index 36ab01f7..a39a37d1 100644 --- a/src/cryfs-cli/program_options/ProgramOptions.cpp +++ b/src/cryfs-cli/program_options/ProgramOptions.cpp @@ -8,16 +8,16 @@ using std::vector; using boost::optional; namespace bf = boost::filesystem; -ProgramOptions::ProgramOptions(const bf::path &baseDir, const bf::path &mountDir, const optional &configFile, - bool foreground, const optional &unmountAfterIdleMinutes, - const optional &logFile, const optional &cipher, - const optional &blocksizeBytes, +ProgramOptions::ProgramOptions(bf::path baseDir, bf::path mountDir, optional configFile, + bool foreground, optional unmountAfterIdleMinutes, + optional logFile, optional cipher, + optional blocksizeBytes, bool noIntegrityChecks, - const boost::optional &missingBlockIsIntegrityViolation, - const vector &fuseOptions) - :_baseDir(baseDir), _mountDir(mountDir), _configFile(configFile), _foreground(foreground), _noIntegrityChecks(noIntegrityChecks), - _cipher(cipher), _blocksizeBytes(blocksizeBytes), _unmountAfterIdleMinutes(unmountAfterIdleMinutes), - _missingBlockIsIntegrityViolation(missingBlockIsIntegrityViolation), _logFile(logFile), _fuseOptions(fuseOptions) { + boost::optional missingBlockIsIntegrityViolation, + vector fuseOptions) + :_baseDir(std::move(baseDir)), _mountDir(std::move(mountDir)), _configFile(std::move(configFile)), _foreground(foreground), _noIntegrityChecks(noIntegrityChecks), + _cipher(std::move(cipher)), _blocksizeBytes(std::move(blocksizeBytes)), _unmountAfterIdleMinutes(std::move(unmountAfterIdleMinutes)), + _missingBlockIsIntegrityViolation(std::move(missingBlockIsIntegrityViolation)), _logFile(std::move(logFile)), _fuseOptions(std::move(fuseOptions)) { } const bf::path &ProgramOptions::baseDir() const { diff --git a/src/cryfs-cli/program_options/ProgramOptions.h b/src/cryfs-cli/program_options/ProgramOptions.h index a4582b80..561b380e 100644 --- a/src/cryfs-cli/program_options/ProgramOptions.h +++ b/src/cryfs-cli/program_options/ProgramOptions.h @@ -12,15 +12,15 @@ namespace cryfs { namespace program_options { class ProgramOptions final { public: - ProgramOptions(const boost::filesystem::path &baseDir, const boost::filesystem::path &mountDir, - const boost::optional &configFile, - bool foreground, const boost::optional &unmountAfterIdleMinutes, - const boost::optional &logFile, - const boost::optional &cipher, - const boost::optional &blocksizeBytes, + ProgramOptions(boost::filesystem::path baseDir, boost::filesystem::path mountDir, + boost::optional configFile, + bool foreground, boost::optional unmountAfterIdleMinutes, + boost::optional logFile, + boost::optional cipher, + boost::optional blocksizeBytes, bool noIntegrityChecks, - const boost::optional &missingBlockIsIntegrityViolation, - const std::vector &fuseOptions); + boost::optional missingBlockIsIntegrityViolation, + std::vector fuseOptions); ProgramOptions(ProgramOptions &&rhs) = default; const boost::filesystem::path &baseDir() const; diff --git a/src/cryfs/config/CryConfig.cpp b/src/cryfs/config/CryConfig.cpp index 148c4a80..d216f6a8 100644 --- a/src/cryfs/config/CryConfig.cpp +++ b/src/cryfs/config/CryConfig.cpp @@ -89,40 +89,40 @@ const std::string &CryConfig::RootBlob() const { return _rootBlob; } -void CryConfig::SetRootBlob(const std::string &value) { - _rootBlob = value; +void CryConfig::SetRootBlob(std::string value) { + _rootBlob = std::move(value); } const string &CryConfig::EncryptionKey() const { return _encKey; } -void CryConfig::SetEncryptionKey(const std::string &value) { - _encKey = value; +void CryConfig::SetEncryptionKey(std::string value) { + _encKey = std::move(value); } const std::string &CryConfig::Cipher() const { return _cipher; }; -void CryConfig::SetCipher(const std::string &value) { - _cipher = value; +void CryConfig::SetCipher(std::string value) { + _cipher = std::move(value); } const std::string &CryConfig::Version() const { return _version; } -void CryConfig::SetVersion(const std::string &value) { - _version = value; +void CryConfig::SetVersion(std::string value) { + _version = std::move(value); } const std::string &CryConfig::CreatedWithVersion() const { return _createdWithVersion; } -void CryConfig::SetCreatedWithVersion(const std::string &value) { - _createdWithVersion = value; +void CryConfig::SetCreatedWithVersion(std::string value) { + _createdWithVersion = std::move(value); } uint64_t CryConfig::BlocksizeBytes() const { @@ -137,8 +137,8 @@ const CryConfig::FilesystemID &CryConfig::FilesystemId() const { return _filesystemId; } -void CryConfig::SetFilesystemId(const FilesystemID &value) { - _filesystemId = value; +void CryConfig::SetFilesystemId(FilesystemID value) { + _filesystemId = std::move(value); } optional CryConfig::ExclusiveClientId() const { diff --git a/src/cryfs/config/CryConfig.h b/src/cryfs/config/CryConfig.h index 39a336b1..645db7c7 100644 --- a/src/cryfs/config/CryConfig.h +++ b/src/cryfs/config/CryConfig.h @@ -18,26 +18,26 @@ public: CryConfig(const CryConfig &rhs) = default; const std::string &RootBlob() const; - void SetRootBlob(const std::string &value); + void SetRootBlob(std::string value); const std::string &EncryptionKey() const; - void SetEncryptionKey(const std::string &value); + void SetEncryptionKey(std::string value); const std::string &Cipher() const; - void SetCipher(const std::string &value); + void SetCipher(std::string value); const std::string &Version() const; - void SetVersion(const std::string &value); + void SetVersion(std::string value); const std::string &CreatedWithVersion() const; - void SetCreatedWithVersion(const std::string &value); + void SetCreatedWithVersion(std::string value); uint64_t BlocksizeBytes() const; void SetBlocksizeBytes(uint64_t value); using FilesystemID = cpputils::FixedSizeData<16>; const FilesystemID &FilesystemId() const; - void SetFilesystemId(const FilesystemID &value); + void SetFilesystemId(FilesystemID value); // If the exclusive client Id is set, then additional integrity measures (i.e. treating missing blocks as integrity violations) are enabled. // Because this only works in a single-client setting, only this one client Id is allowed to access the file system. diff --git a/src/cryfs/config/CryConfigFile.cpp b/src/cryfs/config/CryConfigFile.cpp index b204f090..b34fcc43 100644 --- a/src/cryfs/config/CryConfigFile.cpp +++ b/src/cryfs/config/CryConfigFile.cpp @@ -25,7 +25,7 @@ CryConfigFile::~CryConfigFile() { //We do not call save() here, because we do not want the config file to be re-encrypted on each filesystem run } -optional CryConfigFile::load(const bf::path &path, const string &password) { +optional CryConfigFile::load(bf::path path, const string &password) { auto encryptedConfigData = Data::LoadFromFile(path); if (encryptedConfigData == none) { LOG(ERROR, "Config file not found"); @@ -44,7 +44,7 @@ optional CryConfigFile::load(const bf::path &path, const string & LOG(ERROR, "Inner cipher algorithm used to encrypt config file doesn't match config value"); return none; } - auto configFile = CryConfigFile(path, std::move(config), std::move(*encryptor)); + auto configFile = CryConfigFile(std::move(path), std::move(config), std::move(*encryptor)); if (decrypted->wasInDeprecatedConfigFormat) { // Migrate it to new format configFile.save(); @@ -53,17 +53,17 @@ optional CryConfigFile::load(const bf::path &path, const string & return std::move(configFile); } -CryConfigFile CryConfigFile::create(const bf::path &path, const CryConfig &config, const string &password, const SCryptSettings &scryptSettings) { +CryConfigFile CryConfigFile::create(bf::path path, CryConfig config, const string &password, const SCryptSettings &scryptSettings) { if (bf::exists(path)) { throw std::runtime_error("Config file exists already."); } - auto result = CryConfigFile(path, config, CryConfigEncryptorFactory::deriveKey(password, scryptSettings)); + auto result = CryConfigFile(std::move(path), std::move(config), CryConfigEncryptorFactory::deriveKey(password, scryptSettings)); result.save(); return result; } -CryConfigFile::CryConfigFile(const bf::path &path, const CryConfig &config, unique_ref encryptor) - : _path (path), _config(config), _encryptor(std::move(encryptor)) { +CryConfigFile::CryConfigFile(bf::path path, CryConfig config, unique_ref encryptor) + : _path(std::move(path)), _config(std::move(config)), _encryptor(std::move(encryptor)) { } void CryConfigFile::save() const { diff --git a/src/cryfs/config/CryConfigFile.h b/src/cryfs/config/CryConfigFile.h index 52204c61..7cfcc110 100644 --- a/src/cryfs/config/CryConfigFile.h +++ b/src/cryfs/config/CryConfigFile.h @@ -14,15 +14,15 @@ namespace cryfs { CryConfigFile(CryConfigFile &&rhs) = default; ~CryConfigFile(); - static CryConfigFile create(const boost::filesystem::path &path, const CryConfig &config, const std::string &password, const cpputils::SCryptSettings &scryptSettings); - static boost::optional load(const boost::filesystem::path &path, const std::string &password); + static CryConfigFile create(boost::filesystem::path path, CryConfig config, const std::string &password, const cpputils::SCryptSettings &scryptSettings); + static boost::optional load(boost::filesystem::path path, const std::string &password); void save() const; CryConfig *config(); const CryConfig *config() const; private: - CryConfigFile(const boost::filesystem::path &path, const CryConfig &config, cpputils::unique_ref encryptor); + CryConfigFile(boost::filesystem::path path, CryConfig config, cpputils::unique_ref encryptor); boost::filesystem::path _path; CryConfig _config; diff --git a/src/cryfs/config/CryConfigLoader.cpp b/src/cryfs/config/CryConfigLoader.cpp index 9040466c..ea7ec05c 100644 --- a/src/cryfs/config/CryConfigLoader.cpp +++ b/src/cryfs/config/CryConfigLoader.cpp @@ -31,16 +31,16 @@ using namespace cpputils::logging; namespace cryfs { CryConfigLoader::CryConfigLoader(shared_ptr console, RandomGenerator &keyGenerator, const SCryptSettings &scryptSettings, function askPasswordForExistingFilesystem, function askPasswordForNewFilesystem, const optional &cipherFromCommandLine, const boost::optional &blocksizeBytesFromCommandLine, const boost::optional &missingBlockIsIntegrityViolationFromCommandLine) - : _console(console), _creator(console, keyGenerator), _scryptSettings(scryptSettings), + : _console(std::move(console)), _creator(console, keyGenerator), _scryptSettings(scryptSettings), _askPasswordForExistingFilesystem(askPasswordForExistingFilesystem), _askPasswordForNewFilesystem(askPasswordForNewFilesystem), _cipherFromCommandLine(cipherFromCommandLine), _blocksizeBytesFromCommandLine(blocksizeBytesFromCommandLine), _missingBlockIsIntegrityViolationFromCommandLine(missingBlockIsIntegrityViolationFromCommandLine) { } -optional CryConfigLoader::_loadConfig(const bf::path &filename) { +optional CryConfigLoader::_loadConfig(bf::path filename) { string password = _askPasswordForExistingFilesystem(); std::cout << "Loading config file (this can take some time)..." << std::flush; - auto config = CryConfigFile::load(filename, password); + auto config = CryConfigFile::load(std::move(filename), password); if (config == none) { return none; } @@ -101,20 +101,20 @@ void CryConfigLoader::_checkMissingBlocksAreIntegrityViolations(CryConfigFile *c } } -optional CryConfigLoader::loadOrCreate(const bf::path &filename) { +optional CryConfigLoader::loadOrCreate(bf::path filename) { if (bf::exists(filename)) { - return _loadConfig(filename); + return _loadConfig(std::move(filename)); } else { - return _createConfig(filename); + return _createConfig(std::move(filename)); } } -CryConfigLoader::ConfigLoadResult CryConfigLoader::_createConfig(const bf::path &filename) { +CryConfigLoader::ConfigLoadResult CryConfigLoader::_createConfig(bf::path filename) { auto config = _creator.create(_cipherFromCommandLine, _blocksizeBytesFromCommandLine, _missingBlockIsIntegrityViolationFromCommandLine); //TODO Ask confirmation if using insecure password (<8 characters) string password = _askPasswordForNewFilesystem(); std::cout << "Creating config file (this can take some time)..." << std::flush; - auto result = CryConfigFile::create(filename, std::move(config.config), password, _scryptSettings); + auto result = CryConfigFile::create(std::move(filename), std::move(config.config), password, _scryptSettings); std::cout << "done" << std::endl; return ConfigLoadResult {std::move(result), config.myClientId}; } diff --git a/src/cryfs/config/CryConfigLoader.h b/src/cryfs/config/CryConfigLoader.h index cb1f892b..5cfdc93d 100644 --- a/src/cryfs/config/CryConfigLoader.h +++ b/src/cryfs/config/CryConfigLoader.h @@ -21,11 +21,11 @@ public: uint32_t myClientId; }; - boost::optional loadOrCreate(const boost::filesystem::path &filename); + boost::optional loadOrCreate(boost::filesystem::path filename); private: - boost::optional _loadConfig(const boost::filesystem::path &filename); - ConfigLoadResult _createConfig(const boost::filesystem::path &filename); + boost::optional _loadConfig(boost::filesystem::path filename); + ConfigLoadResult _createConfig(boost::filesystem::path filename); void _checkVersion(const CryConfig &config); void _checkCipher(const CryConfig &config) const; void _checkMissingBlocksAreIntegrityViolations(CryConfigFile *configFile, uint32_t myClientId); diff --git a/src/cryfs/config/crypto/CryConfigEncryptor.cpp b/src/cryfs/config/crypto/CryConfigEncryptor.cpp index 5c97982f..e6f04927 100644 --- a/src/cryfs/config/crypto/CryConfigEncryptor.cpp +++ b/src/cryfs/config/crypto/CryConfigEncryptor.cpp @@ -15,8 +15,8 @@ namespace cryfs { constexpr size_t CryConfigEncryptor::OuterKeySize; constexpr size_t CryConfigEncryptor::MaxTotalKeySize; - CryConfigEncryptor::CryConfigEncryptor(FixedSizeData derivedKey, cpputils::Data kdfParameters) - : _derivedKey(std::move(derivedKey)), _kdfParameters(std::move(kdfParameters)) { + CryConfigEncryptor::CryConfigEncryptor(const FixedSizeData& derivedKey, cpputils::Data kdfParameters) + : _derivedKey(derivedKey), _kdfParameters(std::move(kdfParameters)) { } Data CryConfigEncryptor::encrypt(const Data &plaintext, const string &cipherName) const { diff --git a/src/cryfs/config/crypto/CryConfigEncryptor.h b/src/cryfs/config/crypto/CryConfigEncryptor.h index a206c0da..8954dad0 100644 --- a/src/cryfs/config/crypto/CryConfigEncryptor.h +++ b/src/cryfs/config/crypto/CryConfigEncryptor.h @@ -23,7 +23,7 @@ namespace cryfs { bool wasInDeprecatedConfigFormat; }; - CryConfigEncryptor(cpputils::FixedSizeData derivedKey, cpputils::Data _kdfParameters); + CryConfigEncryptor(const cpputils::FixedSizeData& derivedKey, cpputils::Data _kdfParameters); cpputils::Data encrypt(const cpputils::Data &plaintext, const std::string &cipherName) const; boost::optional decrypt(const cpputils::Data &data) const; diff --git a/src/cryfs/config/crypto/inner/ConcreteInnerEncryptor.h b/src/cryfs/config/crypto/inner/ConcreteInnerEncryptor.h index 03b26311..8f8c406f 100644 --- a/src/cryfs/config/crypto/inner/ConcreteInnerEncryptor.h +++ b/src/cryfs/config/crypto/inner/ConcreteInnerEncryptor.h @@ -13,7 +13,7 @@ namespace cryfs { public: static constexpr size_t CONFIG_SIZE = 900; // Inner config data is grown to this size before encryption to hide its actual size - ConcreteInnerEncryptor(typename Cipher::EncryptionKey key); + ConcreteInnerEncryptor(const typename Cipher::EncryptionKey& key); InnerConfig encrypt(const cpputils::Data &config) const override; boost::optional decrypt(const InnerConfig &innerConfig) const override; @@ -26,8 +26,8 @@ namespace cryfs { }; template - ConcreteInnerEncryptor::ConcreteInnerEncryptor(typename Cipher::EncryptionKey key) - : _key(std::move(key)) { + ConcreteInnerEncryptor::ConcreteInnerEncryptor(const typename Cipher::EncryptionKey& key) + : _key(key) { } template diff --git a/src/cryfs/config/crypto/outer/OuterEncryptor.cpp b/src/cryfs/config/crypto/outer/OuterEncryptor.cpp index 4bfb2508..056133aa 100644 --- a/src/cryfs/config/crypto/outer/OuterEncryptor.cpp +++ b/src/cryfs/config/crypto/outer/OuterEncryptor.cpp @@ -11,8 +11,8 @@ using boost::none; using namespace cpputils::logging; namespace cryfs { - OuterEncryptor::OuterEncryptor(Cipher::EncryptionKey key, cpputils::Data kdfParameters) - : _key(std::move(key)), _kdfParameters(std::move(kdfParameters)) { + OuterEncryptor::OuterEncryptor(const Cipher::EncryptionKey& key, cpputils::Data kdfParameters) + : _key(key), _kdfParameters(std::move(kdfParameters)) { } OuterConfig OuterEncryptor::encrypt(const Data &plaintext) const { diff --git a/src/cryfs/config/crypto/outer/OuterEncryptor.h b/src/cryfs/config/crypto/outer/OuterEncryptor.h index ece3f7e2..344d8c8b 100644 --- a/src/cryfs/config/crypto/outer/OuterEncryptor.h +++ b/src/cryfs/config/crypto/outer/OuterEncryptor.h @@ -14,7 +14,7 @@ namespace cryfs { using Cipher = cpputils::AES256_GCM; static constexpr size_t CONFIG_SIZE = 1024; // Config data is grown to this size before encryption to hide its actual size - OuterEncryptor(Cipher::EncryptionKey key, cpputils::Data kdfParameters); + OuterEncryptor(const Cipher::EncryptionKey& key, cpputils::Data kdfParameters); OuterConfig encrypt(const cpputils::Data &encryptedInnerConfig) const; boost::optional decrypt(const OuterConfig &outerConfig) const; diff --git a/src/fspp/fuse/Fuse.cpp b/src/fspp/fuse/Fuse.cpp index 1e518396..677826cf 100644 --- a/src/fspp/fuse/Fuse.cpp +++ b/src/fspp/fuse/Fuse.cpp @@ -217,8 +217,8 @@ Fuse::~Fuse() { _argv.clear(); } -Fuse::Fuse(Filesystem *fs, const std::string &fstype, const boost::optional &fsname) - :_fs(fs), _mountdir(), _running(false), _fstype(fstype), _fsname(fsname) { +Fuse::Fuse(Filesystem *fs, std::string fstype, boost::optional fsname) + :_fs(fs), _mountdir(), _running(false), _fstype(std::move(fstype)), _fsname(std::move(fsname)) { } void Fuse::_logException(const std::exception &e) { diff --git a/src/fspp/fuse/Fuse.h b/src/fspp/fuse/Fuse.h index 91470121..0b52d96c 100644 --- a/src/fspp/fuse/Fuse.h +++ b/src/fspp/fuse/Fuse.h @@ -19,7 +19,7 @@ class Filesystem; class Fuse final { public: - explicit Fuse(Filesystem *fs, const std::string &fstype, const boost::optional &fsname); + explicit Fuse(Filesystem *fs, std::string fstype, boost::optional fsname); ~Fuse(); void run(const boost::filesystem::path &mountdir, const std::vector &fuseOptions); From ba4308398f1dc071b476f8e01e16b133db53584d Mon Sep 17 00:00:00 2001 From: Sebastian Messmer Date: Sun, 1 Oct 2017 22:14:01 +0100 Subject: [PATCH 17/26] Add CXXFLAGS to cache key --- .circleci/config.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 76d74c26..fabbbd1b 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -7,7 +7,7 @@ references: run: name: Initialize Cache command: | - echo "${APT_COMPILER_PACKAGE}_${BUILD_TOOLSET}_${CXX}_${CC}_${BUILD_TYPE}" > /tmp/_build_env_vars + echo "${APT_COMPILER_PACKAGE}_${BUILD_TOOLSET}_${CXX}_${CC}_${BUILD_TYPE}_${CXXFLAGS}" > /tmp/_build_env_vars echo Build env vars used for cache keys: cat /tmp/_build_env_vars container_setup_pre: &container_setup_pre From d5075ec09ecb74306c2c95f81d1a8548c7b57d68 Mon Sep 17 00:00:00 2001 From: Sebastian Messmer Date: Mon, 2 Oct 2017 04:14:19 +0100 Subject: [PATCH 18/26] Fix --- src/cryfs/config/CryConfigLoader.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cryfs/config/CryConfigLoader.cpp b/src/cryfs/config/CryConfigLoader.cpp index ea7ec05c..f52ab77a 100644 --- a/src/cryfs/config/CryConfigLoader.cpp +++ b/src/cryfs/config/CryConfigLoader.cpp @@ -31,7 +31,7 @@ using namespace cpputils::logging; namespace cryfs { CryConfigLoader::CryConfigLoader(shared_ptr console, RandomGenerator &keyGenerator, const SCryptSettings &scryptSettings, function askPasswordForExistingFilesystem, function askPasswordForNewFilesystem, const optional &cipherFromCommandLine, const boost::optional &blocksizeBytesFromCommandLine, const boost::optional &missingBlockIsIntegrityViolationFromCommandLine) - : _console(std::move(console)), _creator(console, keyGenerator), _scryptSettings(scryptSettings), + : _console(console), _creator(std::move(console), keyGenerator), _scryptSettings(scryptSettings), _askPasswordForExistingFilesystem(askPasswordForExistingFilesystem), _askPasswordForNewFilesystem(askPasswordForNewFilesystem), _cipherFromCommandLine(cipherFromCommandLine), _blocksizeBytesFromCommandLine(blocksizeBytesFromCommandLine), _missingBlockIsIntegrityViolationFromCommandLine(missingBlockIsIntegrityViolationFromCommandLine) { From 180170e2509f0a1b30da8168cca9fb1be68da1d8 Mon Sep 17 00:00:00 2001 From: Sebastian Messmer Date: Mon, 2 Oct 2017 07:56:31 +0100 Subject: [PATCH 19/26] Fix memory leak reported by asan. Not a bad one since it only happens on program exit when the memory is freed anyhow, but better be clean ;) --- src/fspp/fuse/Fuse.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/fspp/fuse/Fuse.cpp b/src/fspp/fuse/Fuse.cpp index 677826cf..8ed14dbb 100644 --- a/src/fspp/fuse/Fuse.cpp +++ b/src/fspp/fuse/Fuse.cpp @@ -211,7 +211,7 @@ fuse_operations *operations() { Fuse::~Fuse() { for(char *arg : _argv) { - delete arg; + delete[] arg; arg = nullptr; } _argv.clear(); From 0af087c120e0b978659e64468b89a7214d4b978b Mon Sep 17 00:00:00 2001 From: Sebastian Messmer Date: Mon, 2 Oct 2017 08:01:38 +0100 Subject: [PATCH 20/26] Fix clang warning --- test/cryfs/config/CryConfigLoaderTest.cpp | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/test/cryfs/config/CryConfigLoaderTest.cpp b/test/cryfs/config/CryConfigLoaderTest.cpp index ac109fee..401688d6 100644 --- a/test/cryfs/config/CryConfigLoaderTest.cpp +++ b/test/cryfs/config/CryConfigLoaderTest.cpp @@ -34,6 +34,11 @@ namespace boost { return stream << "CryConfigFile()"; } } +namespace cryfs { + inline ostream &operator<<(ostream &stream, const CryConfigLoader::ConfigLoadResult &) { + return stream << "ConfigLoadResult()"; + } +} #include class FakeRandomGenerator final : public cpputils::RandomGenerator { @@ -98,7 +103,7 @@ public: FakeRandomGenerator generator(Data::FromString(encKey)); auto loader = CryConfigLoader(console, generator, SCrypt::TestSettings, askPassword, askPassword, none, none, none); - loader.loadOrCreate(file.path()).value().configFile; + ASSERT_NE(boost::none, loader.loadOrCreate(file.path())); } void ChangeEncryptionKey(const string &encKey, const string& password = "mypassword") { From ea3a7cf0e296df61d3ccc12a2dfa2bf363aa8404 Mon Sep 17 00:00:00 2001 From: Sebastian Messmer Date: Mon, 2 Oct 2017 08:02:55 +0100 Subject: [PATCH 21/26] Add AddressSanitizer to CI --- .circleci/config.yml | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/.circleci/config.yml b/.circleci/config.yml index fabbbd1b..32546c8e 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -344,6 +344,15 @@ jobs: APT_COMPILER_PACKAGE: clang-5.0 CXXFLAGS: "-DCRYFS_NO_COMPATIBILITY" BUILD_TYPE: "Debug" + address_sanitizer: + <<: *job_definition + environment: + CC: clang-5.0 + CXX: clang++-5.0 + BUILD_TOOLSET: clang + APT_COMPILER_PACKAGE: clang-5.0 + CXXFLAGS: "-O2 -fsanitize=address -fno-omit-frame-pointer" + BUILD_TYPE: "Debug" workflows: version: 2 @@ -384,4 +393,6 @@ workflows: <<: *enable_for_tags - no_compatibility: <<: *enable_for_tags + - address_sanitizer: + <<: *enable_for_tags From f0e92b88190515bb37d6b1421827f6825365bc7c Mon Sep 17 00:00:00 2001 From: Sebastian Messmer Date: Mon, 2 Oct 2017 08:08:34 +0100 Subject: [PATCH 22/26] Enable additional asan checks --- .circleci/config.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.circleci/config.yml b/.circleci/config.yml index 32546c8e..4d698d8b 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -353,6 +353,7 @@ jobs: APT_COMPILER_PACKAGE: clang-5.0 CXXFLAGS: "-O2 -fsanitize=address -fno-omit-frame-pointer" BUILD_TYPE: "Debug" + ASAN_OPTIONS: "check_initialization_order=1 detect_leaks=1" workflows: version: 2 From 5b4a814640523dc318a5e0e4508a5fa1716d23b6 Mon Sep 17 00:00:00 2001 From: Sebastian Messmer Date: Mon, 2 Oct 2017 08:35:19 +0100 Subject: [PATCH 23/26] Stricter asan settings --- .circleci/config.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 4d698d8b..f1cfb8ce 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -351,9 +351,9 @@ jobs: CXX: clang++-5.0 BUILD_TOOLSET: clang APT_COMPILER_PACKAGE: clang-5.0 - CXXFLAGS: "-O2 -fsanitize=address -fno-omit-frame-pointer" + CXXFLAGS: "-O2 -fsanitize=address -fno-omit-frame-pointer -fsanitize-address-use-after-scope" BUILD_TYPE: "Debug" - ASAN_OPTIONS: "check_initialization_order=1 detect_leaks=1" + ASAN_OPTIONS: "check_initialization_order=1 detect_leaks=1 detect_stack_use_after_return=1" workflows: version: 2 From 600854572cfd1ad82ee08c72e7ddaedcea8e5c91 Mon Sep 17 00:00:00 2001 From: Sebastian Messmer Date: Sun, 8 Oct 2017 06:54:31 +0100 Subject: [PATCH 24/26] Disable network tests by default, because they can fail depending on the network setup --- test/cpp-utils/network/CurlHttpClientTest.cpp | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/test/cpp-utils/network/CurlHttpClientTest.cpp b/test/cpp-utils/network/CurlHttpClientTest.cpp index cccac537..d23fb219 100644 --- a/test/cpp-utils/network/CurlHttpClientTest.cpp +++ b/test/cpp-utils/network/CurlHttpClientTest.cpp @@ -9,6 +9,12 @@ using testing::MatchesRegex; using namespace cpputils; +// Disable these by default because they depend on network +// and - even if network is available - can fail depending +// on the concrete network setup (e.g. if invalid domains are +// answered with an ISP page instead of HTTP error) +#ifdef CRYFS_ENABLE_NETWORK_TESTS + TEST(CurlHttpClientTest, InvalidProtocol) { EXPECT_EQ(none, CurlHttpClient().get("invalid://example.com")); } @@ -30,3 +36,5 @@ TEST(CurlHttpClientTest, ValidHttps) { string content = CurlHttpClient().get("https://example.com").value(); EXPECT_THAT(content, MatchesRegex(".*Example Domain.*")); } + +#endif From 5d07ce6e1284b9ec15be3af866b419ffe1567af8 Mon Sep 17 00:00:00 2001 From: Sebastian Messmer Date: Sun, 8 Oct 2017 07:10:13 +0100 Subject: [PATCH 25/26] Improve ASAN settings --- .circleci/config.yml | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index f1cfb8ce..990c5572 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -351,9 +351,10 @@ jobs: CXX: clang++-5.0 BUILD_TOOLSET: clang APT_COMPILER_PACKAGE: clang-5.0 - CXXFLAGS: "-O2 -fsanitize=address -fno-omit-frame-pointer -fsanitize-address-use-after-scope" + CXXFLAGS: "-O2 -fsanitize=address -fno-omit-frame-pointer -fno-common -fsanitize-address-use-after-scope" BUILD_TYPE: "Debug" - ASAN_OPTIONS: "check_initialization_order=1 detect_leaks=1 detect_stack_use_after_return=1" + # Note: Leak detection is disabled because libfuse itself is leaky... + ASAN_OPTIONS: "detect_leaks=0 check_initialization_order=1 detect_stack_use_after_return=1 detect_invalid_pointer_pairs=1 atexit=1" workflows: version: 2 From 8eda3bfcd5da937da0befefd5a4e2e367a993cda Mon Sep 17 00:00:00 2001 From: Sebastian Messmer Date: Sun, 8 Oct 2017 13:15:11 +0100 Subject: [PATCH 26/26] Fix std::move misuse --- src/cpp-utils/thread/ThreadSystem.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/cpp-utils/thread/ThreadSystem.cpp b/src/cpp-utils/thread/ThreadSystem.cpp index 6cce5866..3d09cf93 100644 --- a/src/cpp-utils/thread/ThreadSystem.cpp +++ b/src/cpp-utils/thread/ThreadSystem.cpp @@ -19,8 +19,8 @@ namespace cpputils { ThreadSystem::Handle ThreadSystem::start(function loopIteration) { boost::unique_lock lock(_mutex); - auto thread = _startThread(std::move(loopIteration)); - _runningThreads.push_back(RunningThread{loopIteration, std::move(thread)}); + auto thread = _startThread(loopIteration); + _runningThreads.push_back(RunningThread{std::move(loopIteration), std::move(thread)}); return std::prev(_runningThreads.end()); }