From d68247070fd8b7a288f45989d1c33490be620fde Mon Sep 17 00:00:00 2001 From: Sebastian Messmer Date: Mon, 10 Dec 2018 21:20:18 -0800 Subject: [PATCH] When an integrity violation occurs, gracefully unmount the file system and refuse to mount on future attempts --- TODO-0.10.txt | 1 - src/blockstore/CMakeLists.txt | 1 - .../caching/CachingBlockStore2.cpp | 2 + .../caching/CachingBlockStore2.h | 5 + .../integrity/IntegrityBlockStore2.cpp | 48 ++-- .../integrity/IntegrityBlockStore2.h | 29 ++- .../integrity/IntegrityViolationError.cpp | 1 - .../integrity/IntegrityViolationError.h | 27 -- .../integrity/KnownBlockVersions.cpp | 73 ++++-- .../integrity/KnownBlockVersions.h | 13 +- src/cpp-utils/data/Deserializer.h | 12 + src/cpp-utils/data/Serializer.h | 11 + src/cryfs-cli/Cli.cpp | 22 +- src/cryfs-cli/main.cpp | 2 +- src/cryfs/ErrorCodes.h | 3 + src/cryfs/filesystem/CryDevice.cpp | 29 ++- src/cryfs/filesystem/CryDevice.h | 8 +- .../cachingfsblobstore/CachingFsBlobStore.cpp | 2 + .../cachingfsblobstore/CachingFsBlobStore.h | 5 + src/stats/main.cpp | 5 +- .../IntegrityBlockStoreTest_Generic.cpp | 4 +- .../IntegrityBlockStoreTest_Specific.cpp | 244 +++++++++++++----- test/cryfs-cli/CliTest_IntegrityCheck.cpp | 118 ++++++++- test/cryfs-cli/testutils/CliTest.h | 32 ++- test/cryfs/filesystem/CryFsTest.cpp | 18 +- test/cryfs/filesystem/FileSystemTest.cpp | 11 +- test/cryfs/filesystem/testutils/CryTestBase.h | 8 +- 27 files changed, 531 insertions(+), 203 deletions(-) delete mode 100644 src/blockstore/implementations/integrity/IntegrityViolationError.cpp delete mode 100644 src/blockstore/implementations/integrity/IntegrityViolationError.h diff --git a/TODO-0.10.txt b/TODO-0.10.txt index f802222e..db2db4e0 100644 --- a/TODO-0.10.txt +++ b/TODO-0.10.txt @@ -3,5 +3,4 @@ Change AppVeyor Badge to master branch Add coverity and PVR Studio? -- Fix integrity violation reporting. Just crashing is not a good UX. - Max time for items in cache to reduce conflicts? diff --git a/src/blockstore/CMakeLists.txt b/src/blockstore/CMakeLists.txt index e20c64b8..80857186 100644 --- a/src/blockstore/CMakeLists.txt +++ b/src/blockstore/CMakeLists.txt @@ -27,7 +27,6 @@ set(SOURCES implementations/integrity/IntegrityBlockStore2.cpp implementations/integrity/KnownBlockVersions.cpp implementations/integrity/ClientIdAndBlockId.cpp - implementations/integrity/IntegrityViolationError.cpp implementations/mock/MockBlockStore.cpp implementations/mock/MockBlock.cpp ) diff --git a/src/blockstore/implementations/caching/CachingBlockStore2.cpp b/src/blockstore/implementations/caching/CachingBlockStore2.cpp index 5402c88e..e9e9733c 100644 --- a/src/blockstore/implementations/caching/CachingBlockStore2.cpp +++ b/src/blockstore/implementations/caching/CachingBlockStore2.cpp @@ -16,6 +16,8 @@ using std::mutex; namespace blockstore { namespace caching { +constexpr double CachingBlockStore2::MAX_LIFETIME_SEC; + CachingBlockStore2::CachedBlock::CachedBlock(const CachingBlockStore2* blockStore, const BlockId &blockId, cpputils::Data data, bool isDirty) : _blockStore(blockStore), _blockId(blockId), _data(std::move(data)), _dirty(isDirty) { } diff --git a/src/blockstore/implementations/caching/CachingBlockStore2.h b/src/blockstore/implementations/caching/CachingBlockStore2.h index 5ccf8664..1d0c3314 100644 --- a/src/blockstore/implementations/caching/CachingBlockStore2.h +++ b/src/blockstore/implementations/caching/CachingBlockStore2.h @@ -54,6 +54,11 @@ private: mutable std::unordered_set _cachedBlocksNotInBaseStore; mutable Cache, 1000> _cache; +public: + static constexpr double MAX_LIFETIME_SEC = decltype(_cache)::MAX_LIFETIME_SEC; + +private: + DISALLOW_COPY_AND_ASSIGN(CachingBlockStore2); }; diff --git a/src/blockstore/implementations/integrity/IntegrityBlockStore2.cpp b/src/blockstore/implementations/integrity/IntegrityBlockStore2.cpp index 92894b74..62ae8f43 100644 --- a/src/blockstore/implementations/integrity/IntegrityBlockStore2.cpp +++ b/src/blockstore/implementations/integrity/IntegrityBlockStore2.cpp @@ -36,10 +36,9 @@ Data IntegrityBlockStore2::_prependHeaderToData(const BlockId& blockId, uint32_t return result; } -void IntegrityBlockStore2::_checkHeader(const BlockId &blockId, const Data &data) const { +bool IntegrityBlockStore2::_checkHeader(const BlockId &blockId, const Data &data) const { _checkFormatHeader(data); - _checkIdHeader(blockId, data); - _checkVersionHeader(blockId, data); + return _checkIdHeader(blockId, data) && _checkVersionHeader(blockId, data); } void IntegrityBlockStore2::_checkFormatHeader(const Data &data) const { @@ -48,20 +47,26 @@ void IntegrityBlockStore2::_checkFormatHeader(const Data &data) const { } } -void IntegrityBlockStore2::_checkVersionHeader(const BlockId &blockId, const Data &data) const { +bool IntegrityBlockStore2::_checkVersionHeader(const BlockId &blockId, const Data &data) const { uint32_t clientId = _readClientId(data); uint64_t version = _readVersion(data); if(!_knownBlockVersions.checkAndUpdateVersion(clientId, blockId, version)) { integrityViolationDetected("The block version number is too low. Did an attacker try to roll back the block or to re-introduce a deleted block?"); + return false; } + + return true; } -void IntegrityBlockStore2::_checkIdHeader(const BlockId &expectedBlockId, const Data &data) const { +bool IntegrityBlockStore2::_checkIdHeader(const BlockId &expectedBlockId, const Data &data) const { BlockId actualBlockId = _readBlockId(data); if (expectedBlockId != actualBlockId) { integrityViolationDetected("The block id is wrong. Did an attacker try to rename some blocks?"); + return false; } + + return true; } uint16_t IntegrityBlockStore2::_readFormatHeader(const Data &data) { @@ -84,44 +89,34 @@ Data IntegrityBlockStore2::_removeHeader(const Data &data) { return data.copyAndRemovePrefix(HEADER_LENGTH); } -void IntegrityBlockStore2::_checkNoPastIntegrityViolations() const { - if (_integrityViolationDetected) { - throw std::runtime_error(string() + - "There was an integrity violation detected. Preventing any further access to the file system. " + - "If you want to reset the integrity data (i.e. accept changes made by a potential attacker), " + - "please unmount the file system and delete the following file before re-mounting it: " + - _knownBlockVersions.path().string()); - } -} - void IntegrityBlockStore2::integrityViolationDetected(const string &reason) const { if (_allowIntegrityViolations) { LOG(WARN, "Integrity violation (but integrity checks are disabled): {}", reason); return; } - _integrityViolationDetected = true; - throw IntegrityViolationError(reason); + _knownBlockVersions.setIntegrityViolationOnPreviousRun(true); + _onIntegrityViolation(); } -IntegrityBlockStore2::IntegrityBlockStore2(unique_ref baseBlockStore, const boost::filesystem::path &integrityFilePath, uint32_t myClientId, bool allowIntegrityViolations, bool missingBlockIsIntegrityViolation) -: _baseBlockStore(std::move(baseBlockStore)), _knownBlockVersions(integrityFilePath, myClientId), _allowIntegrityViolations(allowIntegrityViolations), _missingBlockIsIntegrityViolation(missingBlockIsIntegrityViolation), _integrityViolationDetected(false) { +IntegrityBlockStore2::IntegrityBlockStore2(unique_ref baseBlockStore, const boost::filesystem::path &integrityFilePath, uint32_t myClientId, bool allowIntegrityViolations, bool missingBlockIsIntegrityViolation, std::function onIntegrityViolation) +: _baseBlockStore(std::move(baseBlockStore)), _knownBlockVersions(integrityFilePath, myClientId), _allowIntegrityViolations(allowIntegrityViolations), _missingBlockIsIntegrityViolation(missingBlockIsIntegrityViolation), _onIntegrityViolation(std::move(onIntegrityViolation)) { + if (_knownBlockVersions.integrityViolationOnPreviousRun()) { + throw IntegrityViolationOnPreviousRun(_knownBlockVersions.path()); + } } bool IntegrityBlockStore2::tryCreate(const BlockId &blockId, const Data &data) { - _checkNoPastIntegrityViolations(); uint64_t version = _knownBlockVersions.incrementVersion(blockId); Data dataWithHeader = _prependHeaderToData(blockId, _knownBlockVersions.myClientId(), version, data); return _baseBlockStore->tryCreate(blockId, dataWithHeader); } bool IntegrityBlockStore2::remove(const BlockId &blockId) { - _checkNoPastIntegrityViolations(); _knownBlockVersions.markBlockAsDeleted(blockId); return _baseBlockStore->remove(blockId); } optional IntegrityBlockStore2::load(const BlockId &blockId) const { - _checkNoPastIntegrityViolations(); auto loaded = _baseBlockStore->load(blockId); if (none == loaded) { if (_missingBlockIsIntegrityViolation && _knownBlockVersions.blockShouldExist(blockId)) { @@ -132,13 +127,17 @@ optional IntegrityBlockStore2::load(const BlockId &blockId) const { #ifndef CRYFS_NO_COMPATIBILITY if (FORMAT_VERSION_HEADER_OLD == _readFormatHeader(*loaded)) { Data migrated = _migrateBlock(blockId, *loaded); - _checkHeader(blockId, migrated); + if (!_checkHeader(blockId, migrated) && !_allowIntegrityViolations) { + return optional(none); + } Data content = _removeHeader(migrated); const_cast(this)->store(blockId, content); return optional(_removeHeader(migrated)); } #endif - _checkHeader(blockId, *loaded); + if (!_checkHeader(blockId, *loaded) && !_allowIntegrityViolations) { + return optional(none); + } return optional(_removeHeader(*loaded)); } @@ -154,7 +153,6 @@ Data IntegrityBlockStore2::_migrateBlock(const BlockId &blockId, const Data &dat #endif void IntegrityBlockStore2::store(const BlockId &blockId, const Data &data) { - _checkNoPastIntegrityViolations(); uint64_t version = _knownBlockVersions.incrementVersion(blockId); Data dataWithHeader = _prependHeaderToData(blockId, _knownBlockVersions.myClientId(), version, data); return _baseBlockStore->store(blockId, dataWithHeader); diff --git a/src/blockstore/implementations/integrity/IntegrityBlockStore2.h b/src/blockstore/implementations/integrity/IntegrityBlockStore2.h index 136a0b93..6cc3f1e9 100644 --- a/src/blockstore/implementations/integrity/IntegrityBlockStore2.h +++ b/src/blockstore/implementations/integrity/IntegrityBlockStore2.h @@ -5,18 +5,34 @@ #include "../../interface/BlockStore2.h" #include #include "KnownBlockVersions.h" -#include "IntegrityViolationError.h" namespace blockstore { namespace integrity { +// This exception is thrown if the filesystem can't be loaded because an integrity violation happened +// in one of its earlier runs. +// TODO Use block store factory with expected<> result instead of exception throwing. +class IntegrityViolationOnPreviousRun final : public std::exception { +public: + IntegrityViolationOnPreviousRun(boost::filesystem::path stateFile) + : _stateFile(std::move(stateFile)) {} + + const boost::filesystem::path& stateFile() const { + return _stateFile; + } + +private: + // The state file/directory that has to be deleted so the file system works again + boost::filesystem::path _stateFile; +}; + //TODO Format version headers // This blockstore implements integrity measures. // It depends on being used on top of an encrypted block store that protects integrity of the block contents (i.e. uses an authenticated cipher). class IntegrityBlockStore2 final: public BlockStore2 { public: - IntegrityBlockStore2(cpputils::unique_ref baseBlockStore, const boost::filesystem::path &integrityFilePath, uint32_t myClientId, bool allowIntegrityViolations, bool missingBlockIsIntegrityViolation); + IntegrityBlockStore2(cpputils::unique_ref baseBlockStore, const boost::filesystem::path &integrityFilePath, uint32_t myClientId, bool allowIntegrityViolations, bool missingBlockIsIntegrityViolation, std::function onIntegrityViolation); bool tryCreate(const BlockId &blockId, const cpputils::Data &data) override; bool remove(const BlockId &blockId) override; @@ -49,10 +65,10 @@ public: private: static cpputils::Data _prependHeaderToData(const BlockId &blockId, uint32_t myClientId, uint64_t version, const cpputils::Data &data); - void _checkHeader(const BlockId &blockId, const cpputils::Data &data) const; + WARN_UNUSED_RESULT bool _checkHeader(const BlockId &blockId, const cpputils::Data &data) const; void _checkFormatHeader(const cpputils::Data &data) const; - void _checkIdHeader(const BlockId &expectedBlockId, const cpputils::Data &data) const; - void _checkVersionHeader(const BlockId &blockId, const cpputils::Data &data) const; + WARN_UNUSED_RESULT bool _checkIdHeader(const BlockId &expectedBlockId, const cpputils::Data &data) const; + WARN_UNUSED_RESULT bool _checkVersionHeader(const BlockId &blockId, const cpputils::Data &data) const; static uint16_t _readFormatHeader(const cpputils::Data &data); static uint32_t _readClientId(const cpputils::Data &data); static BlockId _readBlockId(const cpputils::Data &data); @@ -61,14 +77,13 @@ private: static cpputils::Data _migrateBlock(const BlockId &blockId, const cpputils::Data &data); #endif static cpputils::Data _removeHeader(const cpputils::Data &data); - void _checkNoPastIntegrityViolations() const; void integrityViolationDetected(const std::string &reason) const; cpputils::unique_ref _baseBlockStore; mutable KnownBlockVersions _knownBlockVersions; const bool _allowIntegrityViolations; const bool _missingBlockIsIntegrityViolation; - mutable bool _integrityViolationDetected; + std::function _onIntegrityViolation; DISALLOW_COPY_AND_ASSIGN(IntegrityBlockStore2); }; diff --git a/src/blockstore/implementations/integrity/IntegrityViolationError.cpp b/src/blockstore/implementations/integrity/IntegrityViolationError.cpp deleted file mode 100644 index adab6dd9..00000000 --- a/src/blockstore/implementations/integrity/IntegrityViolationError.cpp +++ /dev/null @@ -1 +0,0 @@ -#include "IntegrityViolationError.h" diff --git a/src/blockstore/implementations/integrity/IntegrityViolationError.h b/src/blockstore/implementations/integrity/IntegrityViolationError.h deleted file mode 100644 index d377f85b..00000000 --- a/src/blockstore/implementations/integrity/IntegrityViolationError.h +++ /dev/null @@ -1,27 +0,0 @@ -#pragma once -#ifndef MESSMER_BLOCKSTORE_IMPLEMENTATIONS_INTEGRITY_INTEGRITYVIOLATIONERROR_H_ -#define MESSMER_BLOCKSTORE_IMPLEMENTATIONS_INTEGRITY_INTEGRITYVIOLATIONERROR_H_ - -#include -#include -#include - -namespace blockstore { - namespace integrity { - - class IntegrityViolationError final : public std::runtime_error { - private: - // Constructor is private to make sure that only IntegrityBlockStore can throw this exception. - // This is because IntegrityBlockStore wants to know about integrity violations and - // block all further file system access if it happens. - IntegrityViolationError(const std::string &reason) - : std::runtime_error("Integrity violation: " + reason) { - } - friend class IntegrityBlockStore2; - }; - - - } -} - -#endif diff --git a/src/blockstore/implementations/integrity/KnownBlockVersions.cpp b/src/blockstore/implementations/integrity/KnownBlockVersions.cpp index 3ce1904f..5f7a35e4 100644 --- a/src/blockstore/implementations/integrity/KnownBlockVersions.cpp +++ b/src/blockstore/implementations/integrity/KnownBlockVersions.cpp @@ -17,20 +17,22 @@ using cpputils::Deserializer; namespace blockstore { namespace integrity { -const string KnownBlockVersions::HEADER = "cryfs.integritydata.knownblockversions;0"; +const string KnownBlockVersions::OLD_HEADER = "cryfs.integritydata.knownblockversions;0"; +const string KnownBlockVersions::HEADER = "cryfs.integritydata.knownblockversions;1"; constexpr uint32_t KnownBlockVersions::CLIENT_ID_FOR_DELETED_BLOCK; KnownBlockVersions::KnownBlockVersions(const bf::path &stateFilePath, uint32_t myClientId) - :_knownVersions(), _lastUpdateClientId(), _stateFilePath(stateFilePath), _myClientId(myClientId), _mutex(), _valid(true) { + :_integrityViolationOnPreviousRun(false), _knownVersions(), _lastUpdateClientId(), _stateFilePath(stateFilePath), _myClientId(myClientId), _mutex(), _valid(true) { unique_lock lock(_mutex); ASSERT(_myClientId != CLIENT_ID_FOR_DELETED_BLOCK, "This is not a valid client id"); _loadStateFile(); } KnownBlockVersions::KnownBlockVersions(KnownBlockVersions &&rhs) // NOLINT (intentionally not noexcept) - : _knownVersions(), _lastUpdateClientId(), _stateFilePath(), _myClientId(0), _mutex(), _valid(true) { + : _integrityViolationOnPreviousRun(false), _knownVersions(), _lastUpdateClientId(), _stateFilePath(), _myClientId(0), _mutex(), _valid(true) { unique_lock rhsLock(rhs._mutex); unique_lock lock(_mutex); + _integrityViolationOnPreviousRun = rhs._integrityViolationOnPreviousRun; _knownVersions = std::move(rhs._knownVersions); _lastUpdateClientId = std::move(rhs._lastUpdateClientId); _stateFilePath = std::move(rhs._stateFilePath); @@ -45,6 +47,14 @@ KnownBlockVersions::~KnownBlockVersions() { } } +void KnownBlockVersions::setIntegrityViolationOnPreviousRun(bool value) { + _integrityViolationOnPreviousRun = value; +} + +bool KnownBlockVersions::integrityViolationOnPreviousRun() const { + return _integrityViolationOnPreviousRun; +} + bool KnownBlockVersions::checkAndUpdateVersion(uint32_t clientId, const BlockId &blockId, uint64_t version) { unique_lock lock(_mutex); ASSERT(clientId != CLIENT_ID_FOR_DELETED_BLOCK, "This is not a valid client id"); @@ -89,13 +99,25 @@ void KnownBlockVersions::_loadStateFile() { // File doesn't exist means we loaded empty state. return; } - Deserializer deserializer(&*file); - if (HEADER != deserializer.readString()) { + const string loaded_header = deserializer.readString(); + +#ifndef CRYFS_NO_COMPATIBILITY + if (OLD_HEADER == loaded_header) { + _knownVersions = _deserializeKnownVersions(&deserializer); + _lastUpdateClientId = _deserializeLastUpdateClientIds(&deserializer); + + deserializer.finished(); + _saveStateFile(); + return; + } +#endif + if (HEADER != loaded_header) { throw std::runtime_error("Invalid local state: Invalid integrity file header."); } - _deserializeKnownVersions(&deserializer); - _deserializeLastUpdateClientIds(&deserializer); + _integrityViolationOnPreviousRun = deserializer.readBool(); + _knownVersions = _deserializeKnownVersions(&deserializer); + _lastUpdateClientId = _deserializeLastUpdateClientIds(&deserializer); deserializer.finished(); }; @@ -104,30 +126,34 @@ void KnownBlockVersions::_loadStateFile() { void KnownBlockVersions::_saveStateFile() const { Serializer serializer( Serializer::StringSize(HEADER) + + Serializer::BoolSize() + sizeof(uint64_t) + _knownVersions.size() * (sizeof(uint32_t) + BlockId::BINARY_LENGTH + sizeof(uint64_t)) + sizeof(uint64_t) + _lastUpdateClientId.size() * (BlockId::BINARY_LENGTH + sizeof(uint32_t))); serializer.writeString(HEADER); - _serializeKnownVersions(&serializer); - _serializeLastUpdateClientIds(&serializer); + serializer.writeBool(_integrityViolationOnPreviousRun); + _serializeKnownVersions(&serializer, _knownVersions); + _serializeLastUpdateClientIds(&serializer, _lastUpdateClientId); serializer.finished().StoreToFile(_stateFilePath); } -void KnownBlockVersions::_deserializeKnownVersions(Deserializer *deserializer) { +std::unordered_map KnownBlockVersions::_deserializeKnownVersions(Deserializer *deserializer) { uint64_t numEntries = deserializer->readUint64(); - _knownVersions.clear(); - _knownVersions.reserve(static_cast(1.2 * numEntries)); // Reserve for factor 1.2 more, so the file system doesn't immediately have to resize it on the first new block. + std::unordered_map result; + result.reserve(static_cast(1.2 * numEntries)); // Reserve for factor 1.2 more, so the file system doesn't immediately have to resize it on the first new block. for (uint64_t i = 0 ; i < numEntries; ++i) { auto entry = _deserializeKnownVersionsEntry(deserializer); - _knownVersions.insert(entry); + result.insert(entry); } + + return result; } -void KnownBlockVersions::_serializeKnownVersions(Serializer *serializer) const { - uint64_t numEntries = _knownVersions.size(); +void KnownBlockVersions::_serializeKnownVersions(Serializer *serializer, const std::unordered_map& knownVersions) { + uint64_t numEntries = knownVersions.size(); serializer->writeUint64(numEntries); - for (const auto &entry : _knownVersions) { + for (const auto &entry : knownVersions) { _serializeKnownVersionsEntry(serializer, entry); } } @@ -146,21 +172,22 @@ void KnownBlockVersions::_serializeKnownVersionsEntry(Serializer *serializer, co serializer->writeUint64(entry.second); } -void KnownBlockVersions::_deserializeLastUpdateClientIds(Deserializer *deserializer) { +std::unordered_map KnownBlockVersions::_deserializeLastUpdateClientIds(Deserializer *deserializer) { uint64_t numEntries = deserializer->readUint64(); - _lastUpdateClientId.clear(); - _lastUpdateClientId.reserve(static_cast(1.2 * numEntries)); // Reserve for factor 1.2 more, so the file system doesn't immediately have to resize it on the first new block. + std::unordered_map result; + result.reserve(static_cast(1.2 * numEntries)); // Reserve for factor 1.2 more, so the file system doesn't immediately have to resize it on the first new block. for (uint64_t i = 0 ; i < numEntries; ++i) { auto entry = _deserializeLastUpdateClientIdEntry(deserializer); - _lastUpdateClientId.insert(entry); + result.insert(entry); } + return result; } -void KnownBlockVersions::_serializeLastUpdateClientIds(Serializer *serializer) const { - uint64_t numEntries = _lastUpdateClientId.size(); +void KnownBlockVersions::_serializeLastUpdateClientIds(Serializer *serializer, const std::unordered_map& lastUpdateClientId) { + uint64_t numEntries = lastUpdateClientId.size(); serializer->writeUint64(numEntries); - for (const auto &entry : _lastUpdateClientId) { + for (const auto &entry : lastUpdateClientId) { _serializeLastUpdateClientIdEntry(serializer, entry); } } diff --git a/src/blockstore/implementations/integrity/KnownBlockVersions.h b/src/blockstore/implementations/integrity/KnownBlockVersions.h index 32c4034e..c03de2ce 100644 --- a/src/blockstore/implementations/integrity/KnownBlockVersions.h +++ b/src/blockstore/implementations/integrity/KnownBlockVersions.h @@ -36,9 +36,13 @@ namespace blockstore { uint32_t myClientId() const; const boost::filesystem::path &path() const; + bool integrityViolationOnPreviousRun() const; + void setIntegrityViolationOnPreviousRun(bool value); + static constexpr uint32_t CLIENT_ID_FOR_DELETED_BLOCK = 0; private: + bool _integrityViolationOnPreviousRun; std::unordered_map _knownVersions; std::unordered_map _lastUpdateClientId; // The client who last updated the block @@ -47,19 +51,20 @@ namespace blockstore { mutable std::mutex _mutex; bool _valid; + static const std::string OLD_HEADER; static const std::string HEADER; void _loadStateFile(); void _saveStateFile() const; - void _deserializeKnownVersions(cpputils::Deserializer *deserializer); - void _serializeKnownVersions(cpputils::Serializer *serializer) const; + static std::unordered_map _deserializeKnownVersions(cpputils::Deserializer *deserializer); + static void _serializeKnownVersions(cpputils::Serializer *serializer, const std::unordered_map& knownVersions); static std::pair _deserializeKnownVersionsEntry(cpputils::Deserializer *deserializer); static void _serializeKnownVersionsEntry(cpputils::Serializer *serializer, const std::pair &entry); - void _deserializeLastUpdateClientIds(cpputils::Deserializer *deserializer); - void _serializeLastUpdateClientIds(cpputils::Serializer *serializer) const; + static std::unordered_map _deserializeLastUpdateClientIds(cpputils::Deserializer *deserializer); + static void _serializeLastUpdateClientIds(cpputils::Serializer *serializer, const std::unordered_map& lastUpdateClientIds); static std::pair _deserializeLastUpdateClientIdEntry(cpputils::Deserializer *deserializer); static void _serializeLastUpdateClientIdEntry(cpputils::Serializer *serializer, const std::pair &entry); diff --git a/src/cpp-utils/data/Deserializer.h b/src/cpp-utils/data/Deserializer.h index 0c75f172..cc2c7392 100644 --- a/src/cpp-utils/data/Deserializer.h +++ b/src/cpp-utils/data/Deserializer.h @@ -13,6 +13,7 @@ namespace cpputils { public: Deserializer(const Data *source); + bool readBool(); uint8_t readUint8(); int8_t readInt8(); uint16_t readUint16(); @@ -42,6 +43,17 @@ namespace cpputils { inline Deserializer::Deserializer(const Data *source): _pos(0), _source(source) { } + inline bool Deserializer::readBool() { + uint8_t read = readUint8(); + if (read == 1) { + return true; + } else if (read == 0) { + return false; + } else { + throw std::runtime_error("Read invalid bool value"); + } + } + inline uint8_t Deserializer::readUint8() { return _read(); } diff --git a/src/cpp-utils/data/Serializer.h b/src/cpp-utils/data/Serializer.h index 3cafbdc3..45bd81bc 100644 --- a/src/cpp-utils/data/Serializer.h +++ b/src/cpp-utils/data/Serializer.h @@ -16,6 +16,7 @@ namespace cpputils { public: Serializer(size_t size); + void writeBool(bool value); void writeUint8(uint8_t value); void writeInt8(int8_t value); void writeUint16(uint16_t value); @@ -32,6 +33,7 @@ namespace cpputils { // It does not store a data size but limits the size by the size of the serialization result void writeTailData(const Data &value); + static size_t BoolSize(); static size_t DataSize(const Data &value); static size_t StringSize(const std::string &value); @@ -50,6 +52,14 @@ namespace cpputils { inline Serializer::Serializer(size_t size): _pos(0), _result(size) { } + inline size_t Serializer::BoolSize() { + return sizeof(uint8_t); + } + + inline void Serializer::writeBool(bool value) { + writeUint8(value ? 1 : 0); + } + inline void Serializer::writeUint8(uint8_t value) { _write(value); } @@ -118,6 +128,7 @@ namespace cpputils { _pos += count; } + inline void Serializer::writeString(const std::string &value) { _writeData(value.c_str(), value.size() + 1); // +1 for the nullbyte } diff --git a/src/cryfs-cli/Cli.cpp b/src/cryfs-cli/Cli.cpp index b0646bec..66e5a51d 100644 --- a/src/cryfs-cli/Cli.cpp +++ b/src/cryfs-cli/Cli.cpp @@ -51,6 +51,7 @@ using std::string; using std::endl; using std::shared_ptr; using std::make_shared; +using std::unique_ptr; using std::make_unique; using std::function; using boost::optional; @@ -224,9 +225,24 @@ namespace cryfs { LocalStateDir localStateDir(Environment::localStateDir()); auto blockStore = make_unique_ref(options.baseDir()); auto config = _loadOrCreateConfig(options, localStateDir); + unique_ptr fuse = nullptr; + auto onIntegrityViolation = [&fuse] () { + if (fuse.get() != nullptr) { + LOG(ERR, "Integrity violation detected. Unmounting."); + fuse->stop(); + } else { + // the file system isn't initialized yet, i.e. we failed in the initial steps when + // setting up _device before running initFilesystem. + // We can't unmount a not-mounted file system, but we can make sure it doesn't get mounted. + // Error code is "success" because that's also what is returned if this happens while + // the file system is already mounted - it gets unmounted cleanly. + // TODO Should we reconsider this? + throw CryfsException("Integrity violation detected. Unmounting.", ErrorCode::Success); + } + }; const bool missingBlockIsIntegrityViolation = config.configFile.config()->missingBlockIsIntegrityViolation(); _device = optional>(make_unique_ref(std::move(config.configFile), std::move(blockStore), std::move(localStateDir), config.myClientId, - options.allowIntegrityViolations(), missingBlockIsIntegrityViolation)); + options.allowIntegrityViolations(), missingBlockIsIntegrityViolation, std::move(onIntegrityViolation))); _sanityCheckFilesystem(_device->get()); auto initFilesystem = [&] (fspp::fuse::Fuse *fs){ @@ -241,7 +257,7 @@ namespace cryfs { return make_shared(std::move(*_device)); }; - auto fuse = make_unique(initFilesystem, std::move(onMounted), "cryfs", "cryfs@" + options.baseDir().string()); + fuse = make_unique(initFilesystem, std::move(onMounted), "cryfs", "cryfs@" + options.baseDir().string()); _initLogfile(options); @@ -378,7 +394,7 @@ namespace cryfs { _sanityChecks(options); _runFilesystem(options, std::move(onMounted)); } catch (const CryfsException &e) { - if (e.errorCode() != ErrorCode::Success) { + if (e.what() != string()) { std::cerr << "Error: " << e.what() << std::endl; } return exitCode(e.errorCode()); diff --git a/src/cryfs-cli/main.cpp b/src/cryfs-cli/main.cpp index 6dcc1c0d..c3fe0a72 100644 --- a/src/cryfs-cli/main.cpp +++ b/src/cryfs-cli/main.cpp @@ -28,7 +28,7 @@ int main(int argc, const char *argv[]) { return Cli(keyGenerator, SCrypt::DefaultSettings, make_shared()) .main(argc, argv, std::move(httpClient), []{}); } catch (const CryfsException &e) { - if (e.errorCode() != ErrorCode::Success) { + if (e.what() != string()) { std::cerr << "Error: " << e.what() << std::endl; } return exitCode(e.errorCode()); diff --git a/src/cryfs/ErrorCodes.h b/src/cryfs/ErrorCodes.h index 60c5bef3..060263e8 100644 --- a/src/cryfs/ErrorCodes.h +++ b/src/cryfs/ErrorCodes.h @@ -51,6 +51,9 @@ enum class ErrorCode : int { // File system is in single-client mode and can only be used from the client that created it. SingleClientFileSystem = 23, + + // A previous run of the file system detected an integrity violation. Preventing access to make sure the user notices. The file system will be accessible again after the user deletes the integrity state file. + IntegrityViolationOnPreviousRun = 24, }; inline int exitCode(ErrorCode code) { diff --git a/src/cryfs/filesystem/CryDevice.cpp b/src/cryfs/filesystem/CryDevice.cpp index 358853c7..c6d9ee7e 100644 --- a/src/cryfs/filesystem/CryDevice.cpp +++ b/src/cryfs/filesystem/CryDevice.cpp @@ -20,6 +20,7 @@ #include #include #include "cryfs/localstate/LocalStateDir.h" +#include using std::string; @@ -51,14 +52,14 @@ namespace bf = boost::filesystem; namespace cryfs { -CryDevice::CryDevice(CryConfigFile configFile, unique_ref blockStore, const LocalStateDir& localStateDir, uint32_t myClientId, bool allowIntegrityViolations, bool missingBlockIsIntegrityViolation) -: _fsBlobStore(CreateFsBlobStore(std::move(blockStore), &configFile, localStateDir, myClientId, allowIntegrityViolations, missingBlockIsIntegrityViolation)), +CryDevice::CryDevice(CryConfigFile configFile, unique_ref blockStore, const LocalStateDir& localStateDir, uint32_t myClientId, bool allowIntegrityViolations, bool missingBlockIsIntegrityViolation, std::function onIntegrityViolation) +: _fsBlobStore(CreateFsBlobStore(std::move(blockStore), &configFile, localStateDir, myClientId, allowIntegrityViolations, missingBlockIsIntegrityViolation, std::move(onIntegrityViolation))), _rootBlobId(GetOrCreateRootBlobId(&configFile)), _onFsAction() { } -unique_ref CryDevice::CreateFsBlobStore(unique_ref blockStore, CryConfigFile *configFile, const LocalStateDir& localStateDir, uint32_t myClientId, bool allowIntegrityViolations, bool missingBlockIsIntegrityViolation) { - auto blobStore = CreateBlobStore(std::move(blockStore), localStateDir, configFile, myClientId, allowIntegrityViolations, missingBlockIsIntegrityViolation); +unique_ref CryDevice::CreateFsBlobStore(unique_ref blockStore, CryConfigFile *configFile, const LocalStateDir& localStateDir, uint32_t myClientId, bool allowIntegrityViolations, bool missingBlockIsIntegrityViolation, std::function onIntegrityViolation) { + auto blobStore = CreateBlobStore(std::move(blockStore), localStateDir, configFile, myClientId, allowIntegrityViolations, missingBlockIsIntegrityViolation, std::move(onIntegrityViolation)); #ifndef CRYFS_NO_COMPATIBILITY auto fsBlobStore = MigrateOrCreateFsBlobStore(std::move(blobStore), configFile); @@ -83,8 +84,8 @@ unique_ref CryDevice::MigrateOrCreateFsBlobStore(uniqu } #endif -unique_ref CryDevice::CreateBlobStore(unique_ref blockStore, const LocalStateDir& localStateDir, CryConfigFile *configFile, uint32_t myClientId, bool allowIntegrityViolations, bool missingBlockIsIntegrityViolation) { - auto integrityEncryptedBlockStore = CreateIntegrityEncryptedBlockStore(std::move(blockStore), localStateDir, configFile, myClientId, allowIntegrityViolations, missingBlockIsIntegrityViolation); +unique_ref CryDevice::CreateBlobStore(unique_ref blockStore, const LocalStateDir& localStateDir, CryConfigFile *configFile, uint32_t myClientId, bool allowIntegrityViolations, bool missingBlockIsIntegrityViolation, std::function onIntegrityViolation) { + auto integrityEncryptedBlockStore = CreateIntegrityEncryptedBlockStore(std::move(blockStore), localStateDir, configFile, myClientId, allowIntegrityViolations, missingBlockIsIntegrityViolation, std::move(onIntegrityViolation)); // Create integrityEncryptedBlockStore not in the same line as BlobStoreOnBlocks, because it can modify BlocksizeBytes // in the configFile and therefore has to be run before the second parameter to the BlobStoreOnBlocks parameter is evaluated. return make_unique_ref( @@ -96,7 +97,7 @@ unique_ref CryDevice::CreateBlobStore(unique_refconfig()->BlocksizeBytes()); } -unique_ref CryDevice::CreateIntegrityEncryptedBlockStore(unique_ref blockStore, const LocalStateDir& localStateDir, CryConfigFile *configFile, uint32_t myClientId, bool allowIntegrityViolations, bool missingBlockIsIntegrityViolation) { +unique_ref CryDevice::CreateIntegrityEncryptedBlockStore(unique_ref blockStore, const LocalStateDir& localStateDir, CryConfigFile *configFile, uint32_t myClientId, bool allowIntegrityViolations, bool missingBlockIsIntegrityViolation, std::function onIntegrityViolation) { auto encryptedBlockStore = CreateEncryptedBlockStore(*configFile->config(), std::move(blockStore)); auto statePath = localStateDir.forFilesystemId(configFile->config()->FilesystemId()); auto integrityFilePath = statePath / "integritydata"; @@ -110,7 +111,19 @@ unique_ref CryDevice::CreateIntegrityEncryptedBlockStore(unique_ref } #endif - return make_unique_ref(std::move(encryptedBlockStore), integrityFilePath, myClientId, allowIntegrityViolations, missingBlockIsIntegrityViolation); + try { + return make_unique_ref(std::move(encryptedBlockStore), integrityFilePath, myClientId, + allowIntegrityViolations, missingBlockIsIntegrityViolation, + std::move(onIntegrityViolation)); + } catch (const blockstore::integrity::IntegrityViolationOnPreviousRun& e) { + throw CryfsException(string() + + "There was an integrity violation detected. Preventing any further access to the file system. " + + "This can either happen if an attacker changed your files or rolled back the file system to a previous state, " + + "but it can also happen if you rolled back the file system yourself, for example restored a backup. " + + "If you want to reset the integrity data (i.e. accept changes made by a potential attacker), " + + "please delete the following file before re-mounting it: " + + e.stateFile().string(), ErrorCode::IntegrityViolationOnPreviousRun); + } } BlockId CryDevice::CreateRootBlobAndReturnId() { diff --git a/src/cryfs/filesystem/CryDevice.h b/src/cryfs/filesystem/CryDevice.h index 521fa3d1..9f083acc 100644 --- a/src/cryfs/filesystem/CryDevice.h +++ b/src/cryfs/filesystem/CryDevice.h @@ -19,7 +19,7 @@ namespace cryfs { class CryDevice final: public fspp::Device { public: - CryDevice(CryConfigFile config, cpputils::unique_ref blockStore, const LocalStateDir& localStateDir, uint32_t myClientId, bool allowIntegrityViolations, bool missingBlockIsIntegrityViolation); + CryDevice(CryConfigFile config, cpputils::unique_ref blockStore, const LocalStateDir& localStateDir, uint32_t myClientId, bool allowIntegrityViolations, bool missingBlockIsIntegrityViolation, std::function onIntegrityViolation); statvfs statfs() override; @@ -54,12 +54,12 @@ private: blockstore::BlockId GetOrCreateRootBlobId(CryConfigFile *config); blockstore::BlockId CreateRootBlobAndReturnId(); - static cpputils::unique_ref CreateFsBlobStore(cpputils::unique_ref blockStore, CryConfigFile *configFile, const LocalStateDir& localStateDir, uint32_t myClientId, bool allowIntegrityViolations, bool missingBlockIsIntegrityViolation); + static cpputils::unique_ref CreateFsBlobStore(cpputils::unique_ref blockStore, CryConfigFile *configFile, const LocalStateDir& localStateDir, uint32_t myClientId, bool allowIntegrityViolations, bool missingBlockIsIntegrityViolation, std::function onIntegrityViolation); #ifndef CRYFS_NO_COMPATIBILITY static cpputils::unique_ref MigrateOrCreateFsBlobStore(cpputils::unique_ref blobStore, CryConfigFile *configFile); #endif - static cpputils::unique_ref CreateBlobStore(cpputils::unique_ref blockStore, const LocalStateDir& localStateDir, CryConfigFile *configFile, uint32_t myClientId, bool allowIntegrityViolations, bool missingBlockIsIntegrityViolation); - static cpputils::unique_ref CreateIntegrityEncryptedBlockStore(cpputils::unique_ref blockStore, const LocalStateDir& localStateDir, CryConfigFile *configFile, uint32_t myClientId, bool allowIntegrityViolations, bool missingBlockIsIntegrityViolation); + static cpputils::unique_ref CreateBlobStore(cpputils::unique_ref blockStore, const LocalStateDir& localStateDir, CryConfigFile *configFile, uint32_t myClientId, bool allowIntegrityViolations, bool missingBlockIsIntegrityViolation, std::function onIntegrityViolation); + static cpputils::unique_ref CreateIntegrityEncryptedBlockStore(cpputils::unique_ref blockStore, const LocalStateDir& localStateDir, CryConfigFile *configFile, uint32_t myClientId, bool allowIntegrityViolations, bool missingBlockIsIntegrityViolation, std::function onIntegrityViolation); static cpputils::unique_ref CreateEncryptedBlockStore(const CryConfig &config, cpputils::unique_ref baseBlockStore); struct BlobWithParent { diff --git a/src/cryfs/filesystem/cachingfsblobstore/CachingFsBlobStore.cpp b/src/cryfs/filesystem/cachingfsblobstore/CachingFsBlobStore.cpp index a067f582..fa3c7b17 100644 --- a/src/cryfs/filesystem/cachingfsblobstore/CachingFsBlobStore.cpp +++ b/src/cryfs/filesystem/cachingfsblobstore/CachingFsBlobStore.cpp @@ -15,6 +15,8 @@ using cryfs::fsblobstore::SymlinkBlob; namespace cryfs { namespace cachingfsblobstore { + constexpr double CachingFsBlobStore::MAX_LIFETIME_SEC; + optional> CachingFsBlobStore::load(const BlockId &blockId) { auto fromCache = _cache.pop(blockId); if (fromCache != none) { diff --git a/src/cryfs/filesystem/cachingfsblobstore/CachingFsBlobStore.h b/src/cryfs/filesystem/cachingfsblobstore/CachingFsBlobStore.h index 2e881d40..6bba337a 100644 --- a/src/cryfs/filesystem/cachingfsblobstore/CachingFsBlobStore.h +++ b/src/cryfs/filesystem/cachingfsblobstore/CachingFsBlobStore.h @@ -40,6 +40,11 @@ namespace cryfs { //TODO Use other cache config (i.e. smaller max number of entries) here than in blockstore blockstore::caching::Cache, 50> _cache; + public: + static constexpr double MAX_LIFETIME_SEC = decltype(_cache)::MAX_LIFETIME_SEC; + + private: + DISALLOW_COPY_AND_ASSIGN(CachingFsBlobStore); }; diff --git a/src/stats/main.cpp b/src/stats/main.cpp index 3d59a13c..6012d97f 100644 --- a/src/stats/main.cpp +++ b/src/stats/main.cpp @@ -83,7 +83,10 @@ unique_ref makeBlockStore(const path& basedir, const CryConfigLoader auto encryptedBlockStore = CryCiphers::find(config.configFile.config()->Cipher()).createEncryptedBlockstore(std::move(onDiskBlockStore), config.configFile.config()->EncryptionKey()); auto statePath = localStateDir.forFilesystemId(config.configFile.config()->FilesystemId()); auto integrityFilePath = statePath / "integritydata"; - auto integrityBlockStore = make_unique_ref(std::move(encryptedBlockStore), integrityFilePath, config.myClientId, false, true); + auto onIntegrityViolation = [] () { + std::cerr << "Warning: Integrity violation encountered" << std::endl; + }; + auto integrityBlockStore = make_unique_ref(std::move(encryptedBlockStore), integrityFilePath, config.myClientId, false, true, onIntegrityViolation); return make_unique_ref(std::move(integrityBlockStore)); } diff --git a/test/blockstore/implementations/integrity/IntegrityBlockStoreTest_Generic.cpp b/test/blockstore/implementations/integrity/IntegrityBlockStoreTest_Generic.cpp index 0c46d909..bc1c8ed2 100644 --- a/test/blockstore/implementations/integrity/IntegrityBlockStoreTest_Generic.cpp +++ b/test/blockstore/implementations/integrity/IntegrityBlockStoreTest_Generic.cpp @@ -25,7 +25,7 @@ public: TempFile stateFile; unique_ref createBlockStore() override { return make_unique_ref( - make_unique_ref(make_unique_ref(), stateFile.path(), 0x12345678, AllowIntegrityViolations, MissingBlockIsIntegrityViolation) + make_unique_ref(make_unique_ref(), stateFile.path(), 0x12345678, AllowIntegrityViolations, MissingBlockIsIntegrityViolation, [] {}) ); } }; @@ -47,7 +47,7 @@ public: TempFile stateFile; unique_ref createBlockStore() override { - return make_unique_ref(make_unique_ref(), stateFile.path(), 0x12345678, AllowIntegrityViolations, MissingBlockIsIntegrityViolation); + return make_unique_ref(make_unique_ref(), stateFile.path(), 0x12345678, AllowIntegrityViolations, MissingBlockIsIntegrityViolation, [] {}); } }; diff --git a/test/blockstore/implementations/integrity/IntegrityBlockStoreTest_Specific.cpp b/test/blockstore/implementations/integrity/IntegrityBlockStoreTest_Specific.cpp index c816fd8e..12aa5cf6 100644 --- a/test/blockstore/implementations/integrity/IntegrityBlockStoreTest_Specific.cpp +++ b/test/blockstore/implementations/integrity/IntegrityBlockStoreTest_Specific.cpp @@ -16,40 +16,50 @@ using cpputils::TempFile; using cpputils::serialize; using cpputils::deserialize; using boost::none; -using std::make_unique; using std::unique_ptr; using blockstore::inmemory::InMemoryBlockStore2; using namespace blockstore::integrity; +namespace { +class FakeCallback final { +public: + FakeCallback(): wasCalled_(false) {} + + bool wasCalled() const { + return wasCalled_; + } + + std::function callback() { + return [this] () { + wasCalled_ = true; + }; + } + +private: + bool wasCalled_; +}; +} + +template class IntegrityBlockStoreTest: public Test { public: static constexpr unsigned int BLOCKSIZE = 1024; IntegrityBlockStoreTest(): stateFile(false), + onIntegrityViolation(), baseBlockStore(new InMemoryBlockStore2), - blockStore(make_unique_ref(std::move(cpputils::nullcheck(std::unique_ptr(baseBlockStore)).value()), stateFile.path(), myClientId, false, false)), + blockStore(make_unique_ref(std::move(cpputils::nullcheck(std::unique_ptr(baseBlockStore)).value()), stateFile.path(), myClientId, AllowIntegrityViolations, MissingBlockIsIntegrityViolation, onIntegrityViolation.callback())), data(DataFixture::generate(BLOCKSIZE)) { } static constexpr uint32_t myClientId = 0x12345678; TempFile stateFile; + FakeCallback onIntegrityViolation; InMemoryBlockStore2 *baseBlockStore; unique_ref blockStore; Data data; - std::pair> makeBlockStoreWithDeletionPrevention() { - InMemoryBlockStore2 *baseBlockStore = new InMemoryBlockStore2; - auto blockStore = make_unique(std::move(cpputils::nullcheck(std::unique_ptr(baseBlockStore)).value()), stateFile.path(), myClientId, false, true); - return std::make_pair(baseBlockStore, std::move(blockStore)); - } - - std::pair> makeBlockStoreWithoutDeletionPrevention() { - InMemoryBlockStore2 *baseBlockStore = new InMemoryBlockStore2; - auto blockStore = make_unique(std::move(cpputils::nullcheck(std::unique_ptr(baseBlockStore)).value()), stateFile.path(), myClientId, false, false); - return std::make_pair(baseBlockStore, std::move(blockStore)); - } - blockstore::BlockId CreateBlockReturnKey() { return CreateBlockReturnKey(data); } @@ -114,34 +124,58 @@ private: DISALLOW_COPY_AND_ASSIGN(IntegrityBlockStoreTest); }; -constexpr uint32_t IntegrityBlockStoreTest::myClientId; +using IntegrityBlockStoreTest_Default = IntegrityBlockStoreTest; +using IntegrityBlockStoreTest_MissingBlockIsIntegrityViolation = IntegrityBlockStoreTest; +using IntegrityBlockStoreTest_AllowIntegrityViolations = IntegrityBlockStoreTest; +using IntegrityBlockStoreTest_AllowIntegrityViolations_MissingBlockIsIntegrityViolation = IntegrityBlockStoreTest; + +template +constexpr uint32_t IntegrityBlockStoreTest::myClientId; // Test that a decreasing version number is not allowed -TEST_F(IntegrityBlockStoreTest, RollbackPrevention_DoesntAllowDecreasingVersionNumberForSameClient_1) { +TEST_F(IntegrityBlockStoreTest_Default, RollbackPrevention_DoesntAllowDecreasingVersionNumberForSameClient_1) { auto blockId = CreateBlockReturnKey(); Data oldBaseBlock = loadBaseBlock(blockId); modifyBlock(blockId); rollbackBaseBlock(blockId, oldBaseBlock); - EXPECT_THROW( - blockStore->load(blockId), - IntegrityViolationError - ); + EXPECT_EQ(boost::none, blockStore->load(blockId)); + EXPECT_TRUE(onIntegrityViolation.wasCalled()); } -TEST_F(IntegrityBlockStoreTest, RollbackPrevention_DoesntAllowDecreasingVersionNumberForSameClient_2) { +// Test that a decreasing version number is allowed if allowIntegrityViolations is set. +TEST_F(IntegrityBlockStoreTest_AllowIntegrityViolations, RollbackPrevention_AllowsDecreasingVersionNumberForSameClient_1) { + auto blockId = CreateBlockReturnKey(); + Data oldBaseBlock = loadBaseBlock(blockId); + modifyBlock(blockId); + rollbackBaseBlock(blockId, oldBaseBlock); + EXPECT_NE(boost::none, blockStore->load(blockId)); + EXPECT_FALSE(onIntegrityViolation.wasCalled()); +} + +TEST_F(IntegrityBlockStoreTest_Default, RollbackPrevention_DoesntAllowDecreasingVersionNumberForSameClient_2) { auto blockId = CreateBlockReturnKey(); // Increase the version number modifyBlock(blockId); // Decrease the version number again decreaseVersionNumber(blockId); - EXPECT_THROW( - blockStore->load(blockId), - IntegrityViolationError - ); + + EXPECT_EQ(boost::none, blockStore->load(blockId)); + EXPECT_TRUE(onIntegrityViolation.wasCalled()); +} + +TEST_F(IntegrityBlockStoreTest_AllowIntegrityViolations, RollbackPrevention_AllowsDecreasingVersionNumberForSameClient_2) { + auto blockId = CreateBlockReturnKey(); + // Increase the version number + modifyBlock(blockId); + // Decrease the version number again + decreaseVersionNumber(blockId); + + EXPECT_NE(boost::none, blockStore->load(blockId)); + EXPECT_FALSE(onIntegrityViolation.wasCalled()); } // Test that a different client doesn't need to have a higher version number (i.e. version numbers are per client). -TEST_F(IntegrityBlockStoreTest, RollbackPrevention_DoesAllowDecreasingVersionNumberForDifferentClient) { +TEST_F(IntegrityBlockStoreTest_Default, RollbackPrevention_DoesAllowDecreasingVersionNumberForDifferentClient) { auto blockId = CreateBlockReturnKey(); // Increase the version number modifyBlock(blockId); @@ -149,10 +183,22 @@ TEST_F(IntegrityBlockStoreTest, RollbackPrevention_DoesAllowDecreasingVersionNum changeClientId(blockId); decreaseVersionNumber(blockId); EXPECT_NE(boost::none, blockStore->load(blockId)); + EXPECT_FALSE(onIntegrityViolation.wasCalled()); +} + +TEST_F(IntegrityBlockStoreTest_AllowIntegrityViolations, RollbackPrevention_DoesAllowDecreasingVersionNumberForDifferentClient) { + auto blockId = CreateBlockReturnKey(); + // Increase the version number + modifyBlock(blockId); + // Fake a modification by a different client with lower version numbers + changeClientId(blockId); + decreaseVersionNumber(blockId); + EXPECT_NE(boost::none, blockStore->load(blockId)); + EXPECT_FALSE(onIntegrityViolation.wasCalled()); } // Test that it doesn't allow a rollback to the "newest" block of a client, when this block was superseded by a version of a different client -TEST_F(IntegrityBlockStoreTest, RollbackPrevention_DoesntAllowSameVersionNumberForOldClient) { +TEST_F(IntegrityBlockStoreTest_Default, RollbackPrevention_DoesntAllowSameVersionNumberForOldClient) { auto blockId = CreateBlockReturnKey(); // Increase the version number modifyBlock(blockId); @@ -162,62 +208,99 @@ TEST_F(IntegrityBlockStoreTest, RollbackPrevention_DoesntAllowSameVersionNumberF loadBlock(blockId); // make the block store know about this other client's modification // Rollback to old client rollbackBaseBlock(blockId, oldBaseBlock); - EXPECT_THROW( - blockStore->load(blockId), - IntegrityViolationError - ); + EXPECT_EQ(boost::none, blockStore->load(blockId)); + EXPECT_TRUE(onIntegrityViolation.wasCalled()); +} + +// Test that it does allow a rollback to the "newest" block of a client, when this block was superseded by a version of a different client, but integrity violations are allowed +TEST_F(IntegrityBlockStoreTest_AllowIntegrityViolations, RollbackPrevention_AllowsSameVersionNumberForOldClient) { + auto blockId = CreateBlockReturnKey(); + // Increase the version number + modifyBlock(blockId); + Data oldBaseBlock = loadBaseBlock(blockId); + // Fake a modification by a different client with lower version numbers + changeClientId(blockId); + loadBlock(blockId); // make the block store know about this other client's modification + // Rollback to old client + rollbackBaseBlock(blockId, oldBaseBlock); + EXPECT_NE(boost::none, blockStore->load(blockId)); + EXPECT_FALSE(onIntegrityViolation.wasCalled()); } // Test that deleted blocks cannot be re-introduced -TEST_F(IntegrityBlockStoreTest, RollbackPrevention_DoesntAllowReintroducingDeletedBlocks) { +TEST_F(IntegrityBlockStoreTest_Default, RollbackPrevention_DoesntAllowReintroducingDeletedBlocks) { auto blockId = CreateBlockReturnKey(); Data oldBaseBlock = loadBaseBlock(blockId); deleteBlock(blockId); insertBaseBlock(blockId, std::move(oldBaseBlock)); - EXPECT_THROW( - blockStore->load(blockId), - IntegrityViolationError - ); + EXPECT_EQ(boost::none, blockStore->load(blockId)); + EXPECT_TRUE(onIntegrityViolation.wasCalled()); +} + +// Test that deleted blocks can be re-introduced if integrity violations are allowed +TEST_F(IntegrityBlockStoreTest_AllowIntegrityViolations, RollbackPrevention_AllowsReintroducingDeletedBlocks) { + auto blockId = CreateBlockReturnKey(); + Data oldBaseBlock = loadBaseBlock(blockId); + deleteBlock(blockId); + insertBaseBlock(blockId, std::move(oldBaseBlock)); + EXPECT_NE(boost::none, blockStore->load(blockId)); + EXPECT_FALSE(onIntegrityViolation.wasCalled()); } // This can happen if a client synchronization is delayed. Another client might have won the conflict and pushed a new version for the deleted block. -TEST_F(IntegrityBlockStoreTest, RollbackPrevention_AllowsReintroducingDeletedBlocksWithNewVersionNumber) { +TEST_F(IntegrityBlockStoreTest_Default, RollbackPrevention_AllowsReintroducingDeletedBlocksWithNewVersionNumber) { auto blockId = CreateBlockReturnKey(); Data oldBaseBlock = loadBaseBlock(blockId); deleteBlock(blockId); insertBaseBlock(blockId, std::move(oldBaseBlock)); increaseVersionNumber(blockId); EXPECT_NE(boost::none, blockStore->load(blockId)); + EXPECT_FALSE(onIntegrityViolation.wasCalled()); +} + +TEST_F(IntegrityBlockStoreTest_AllowIntegrityViolations, RollbackPrevention_AllowsReintroducingDeletedBlocksWithNewVersionNumber) { + auto blockId = CreateBlockReturnKey(); + Data oldBaseBlock = loadBaseBlock(blockId); + deleteBlock(blockId); + insertBaseBlock(blockId, std::move(oldBaseBlock)); + increaseVersionNumber(blockId); + EXPECT_NE(boost::none, blockStore->load(blockId)); + EXPECT_FALSE(onIntegrityViolation.wasCalled()); } // Check that in a multi-client scenario, missing blocks are not integrity errors, because another client might have deleted them. -TEST_F(IntegrityBlockStoreTest, DeletionPrevention_AllowsDeletingBlocksWhenDeactivated) { - InMemoryBlockStore2 *baseBlockStore; - unique_ptr blockStore; - std::tie(baseBlockStore, blockStore) = makeBlockStoreWithoutDeletionPrevention(); +TEST_F(IntegrityBlockStoreTest_Default, DeletionPrevention_AllowsDeletingBlocksWhenDeactivated) { auto blockId = blockStore->create(Data(0)); baseBlockStore->remove(blockId); EXPECT_EQ(boost::none, blockStore->load(blockId)); + EXPECT_FALSE(onIntegrityViolation.wasCalled()); +} + +TEST_F(IntegrityBlockStoreTest_AllowIntegrityViolations, DeletionPrevention_AllowsDeletingBlocksWhenDeactivated) { + auto blockId = blockStore->create(Data(0)); + baseBlockStore->remove(blockId); + EXPECT_EQ(boost::none, blockStore->load(blockId)); + EXPECT_FALSE(onIntegrityViolation.wasCalled()); } // Check that in a single-client scenario, missing blocks are integrity errors. -TEST_F(IntegrityBlockStoreTest, DeletionPrevention_DoesntAllowDeletingBlocksWhenActivated) { - InMemoryBlockStore2 *baseBlockStore; - unique_ptr blockStore; - std::tie(baseBlockStore, blockStore) = makeBlockStoreWithDeletionPrevention(); +TEST_F(IntegrityBlockStoreTest_MissingBlockIsIntegrityViolation, DeletionPrevention_DoesntAllowDeletingBlocksWhenActivated) { auto blockId = blockStore->create(Data(0)); baseBlockStore->remove(blockId); - EXPECT_THROW( - blockStore->load(blockId), - IntegrityViolationError - ); + EXPECT_EQ(boost::none, blockStore->load(blockId)); + EXPECT_TRUE(onIntegrityViolation.wasCalled()); +} + +// Check that in a single-client scenario, missing blocks don't throw if integrity violations are allowed. +TEST_F(IntegrityBlockStoreTest_AllowIntegrityViolations_MissingBlockIsIntegrityViolation, DeletionPrevention_AllowsDeletingBlocksWhenActivated) { + auto blockId = blockStore->create(Data(0)); + baseBlockStore->remove(blockId); + EXPECT_EQ(boost::none, blockStore->load(blockId)); + EXPECT_FALSE(onIntegrityViolation.wasCalled()); } // Check that in a multi-client scenario, missing blocks are not integrity errors, because another client might have deleted them. -TEST_F(IntegrityBlockStoreTest, DeletionPrevention_InForEachBlock_AllowsDeletingBlocksWhenDeactivated) { - InMemoryBlockStore2 *baseBlockStore; - unique_ptr blockStore; - std::tie(baseBlockStore, blockStore) = makeBlockStoreWithoutDeletionPrevention(); +TEST_F(IntegrityBlockStoreTest_Default, DeletionPrevention_InForEachBlock_AllowsDeletingBlocksWhenDeactivated) { auto blockId = blockStore->create(Data(0)); baseBlockStore->remove(blockId); int count = 0; @@ -225,29 +308,50 @@ TEST_F(IntegrityBlockStoreTest, DeletionPrevention_InForEachBlock_AllowsDeleting ++count; }); EXPECT_EQ(0, count); + EXPECT_FALSE(onIntegrityViolation.wasCalled()); +} + +TEST_F(IntegrityBlockStoreTest_AllowIntegrityViolations, DeletionPrevention_InForEachBlock_AllowsDeletingBlocksWhenDeactivated) { + auto blockId = blockStore->create(Data(0)); + baseBlockStore->remove(blockId); + int count = 0; + blockStore->forEachBlock([&count] (const blockstore::BlockId &) { + ++count; + }); + EXPECT_EQ(0, count); + EXPECT_FALSE(onIntegrityViolation.wasCalled()); } // Check that in a single-client scenario, missing blocks are integrity errors. -TEST_F(IntegrityBlockStoreTest, DeletionPrevention_InForEachBlock_DoesntAllowDeletingBlocksWhenActivated) { - InMemoryBlockStore2 *baseBlockStore; - unique_ptr blockStore; - std::tie(baseBlockStore, blockStore) = makeBlockStoreWithDeletionPrevention(); +TEST_F(IntegrityBlockStoreTest_MissingBlockIsIntegrityViolation, DeletionPrevention_InForEachBlock_DoesntAllowDeletingBlocksWhenActivated) { auto blockId = blockStore->create(Data(0)); baseBlockStore->remove(blockId); - EXPECT_THROW( - blockStore->forEachBlock([] (const blockstore::BlockId &) {}), - IntegrityViolationError - ); + blockStore->forEachBlock([] (const blockstore::BlockId &) {}); + EXPECT_TRUE(onIntegrityViolation.wasCalled()); } -TEST_F(IntegrityBlockStoreTest, LoadingWithDifferentBlockIdFails) { +// Check that in a single-client scenario, missing blocks don't throw if integrity violations are allowed. +TEST_F(IntegrityBlockStoreTest_AllowIntegrityViolations_MissingBlockIsIntegrityViolation, DeletionPrevention_InForEachBlock_AllowsDeletingBlocksWhenActivated) { + auto blockId = blockStore->create(Data(0)); + baseBlockStore->remove(blockId); + blockStore->forEachBlock([] (const blockstore::BlockId &) {}); + EXPECT_FALSE(onIntegrityViolation.wasCalled()); +} + +TEST_F(IntegrityBlockStoreTest_Default, LoadingWithDifferentBlockIdFails) { auto blockId = CreateBlockReturnKey(); blockstore::BlockId key2 = blockstore::BlockId::FromString("1491BB4932A389EE14BC7090AC772972"); baseBlockStore->store(key2, baseBlockStore->load(blockId).value()); - EXPECT_THROW( - blockStore->load(key2), - IntegrityViolationError - ); + EXPECT_EQ(boost::none, blockStore->load(key2)); + EXPECT_TRUE(onIntegrityViolation.wasCalled()); +} + +TEST_F(IntegrityBlockStoreTest_AllowIntegrityViolations, LoadingWithDifferentBlockIdDoesntFail) { + auto blockId = CreateBlockReturnKey(); + blockstore::BlockId key2 = blockstore::BlockId::FromString("1491BB4932A389EE14BC7090AC772972"); + baseBlockStore->store(key2, baseBlockStore->load(blockId).value()); + EXPECT_NE(boost::none, blockStore->load(key2)); + EXPECT_FALSE(onIntegrityViolation.wasCalled()); } // TODO Test more integrity cases: @@ -256,17 +360,17 @@ TEST_F(IntegrityBlockStoreTest, LoadingWithDifferentBlockIdFails) { // - Think about more... // TODO Test that disabling integrity checks allows all these cases -TEST_F(IntegrityBlockStoreTest, PhysicalBlockSize_zerophysical) { +TEST_F(IntegrityBlockStoreTest_Default, PhysicalBlockSize_zerophysical) { EXPECT_EQ(0u, blockStore->blockSizeFromPhysicalBlockSize(0)); } -TEST_F(IntegrityBlockStoreTest, PhysicalBlockSize_zerovirtual) { +TEST_F(IntegrityBlockStoreTest_Default, PhysicalBlockSize_zerovirtual) { auto blockId = CreateBlockReturnKey(Data(0)); auto base = baseBlockStore->load(blockId).value(); EXPECT_EQ(0u, blockStore->blockSizeFromPhysicalBlockSize(base.size())); } -TEST_F(IntegrityBlockStoreTest, PhysicalBlockSize_negativeboundaries) { +TEST_F(IntegrityBlockStoreTest_Default, PhysicalBlockSize_negativeboundaries) { // This tests that a potential if/else in blockSizeFromPhysicalBlockSize that catches negative values has the // correct boundary set. We test the highest value that is negative and the smallest value that is positive. auto physicalSizeForVirtualSizeZero = baseBlockStore->load(CreateBlockReturnKey(Data(0))).value().size(); @@ -277,7 +381,7 @@ TEST_F(IntegrityBlockStoreTest, PhysicalBlockSize_negativeboundaries) { EXPECT_EQ(1u, blockStore->blockSizeFromPhysicalBlockSize(physicalSizeForVirtualSizeZero + 1)); } -TEST_F(IntegrityBlockStoreTest, PhysicalBlockSize_positive) { +TEST_F(IntegrityBlockStoreTest_Default, PhysicalBlockSize_positive) { auto blockId = CreateBlockReturnKey(Data(10*1024)); auto base = baseBlockStore->load(blockId).value(); EXPECT_EQ(10*1024u, blockStore->blockSizeFromPhysicalBlockSize(base.size())); diff --git a/test/cryfs-cli/CliTest_IntegrityCheck.cpp b/test/cryfs-cli/CliTest_IntegrityCheck.cpp index 2f601b73..8a41d18f 100644 --- a/test/cryfs-cli/CliTest_IntegrityCheck.cpp +++ b/test/cryfs-cli/CliTest_IntegrityCheck.cpp @@ -3,6 +3,9 @@ #include #include #include +#include +#include +#include using std::vector; using std::string; @@ -13,22 +16,56 @@ using cryfs::CryKeyProvider; using cpputils::Data; using cpputils::EncryptionKey; using cpputils::SCrypt; +using cpputils::TempDir; +namespace bf = boost::filesystem; + +namespace { + +void writeFile(const bf::path& filename, const string& content) { + std::ofstream file(filename.c_str(), std::ios::trunc); + file << content; + ASSERT(file.good(), "Failed writing file to file system"); +} + +bool readingFileIsSuccessful(const bf::path& filename) { + std::ifstream file(filename.c_str()); + std::string content; + file >> content; // just read a little bit so we have a file access + return file.good(); +} + +void recursive_copy(const bf::path &src, const bf::path &dst) { + if (bf::exists(dst)) { + throw std::runtime_error(dst.generic_string() + " already exists"); + } + + if (bf::is_directory(src)) { + bf::create_directories(dst); + for (auto& item : bf::directory_iterator(src)) { + recursive_copy(item.path(), dst / item.path().filename()); + } + } else if (bf::is_regular_file(src)) { + bf::copy_file(src, dst); + } else { + throw std::runtime_error(dst.generic_string() + " neither dir nor file"); + } +} class FakeCryKeyProvider final : public CryKeyProvider { - EncryptionKey requestKeyForExistingFilesystem(size_t keySize, const Data& kdfParameters) override { + EncryptionKey requestKeyForExistingFilesystem(size_t keySize, const Data &kdfParameters) override { return SCrypt(SCrypt::TestSettings).deriveExistingKey(keySize, "pass", kdfParameters); } KeyResult requestKeyForNewFilesystem(size_t keySize) override { auto derived = SCrypt(SCrypt::TestSettings).deriveNewKey(keySize, "pass"); return { - std::move(derived.key), - std::move(derived.kdfParameters) + std::move(derived.key), + std::move(derived.kdfParameters) }; } }; -class CliTest_IntegrityCheck: public CliTest { +class CliTest_IntegrityCheck : public CliTest { public: void modifyFilesystemId() { FakeCryKeyProvider keyProvider; @@ -46,7 +83,7 @@ public: }; TEST_F(CliTest_IntegrityCheck, givenIncorrectFilesystemId_thenFails) { - vector args {basedir.string().c_str(), mountdir.string().c_str(), "--cipher", "aes-256-gcm", "-f"}; + vector args{basedir.string().c_str(), mountdir.string().c_str(), "--cipher", "aes-256-gcm", "-f"}; //TODO Remove "-f" parameter, once EXPECT_RUN_SUCCESS can handle that EXPECT_RUN_SUCCESS(args, mountdir); modifyFilesystemId(); @@ -58,7 +95,7 @@ TEST_F(CliTest_IntegrityCheck, givenIncorrectFilesystemId_thenFails) { } TEST_F(CliTest_IntegrityCheck, givenIncorrectFilesystemKey_thenFails) { - vector args {basedir.string().c_str(), mountdir.string().c_str(), "--cipher", "aes-256-gcm", "-f"}; + vector args{basedir.string().c_str(), mountdir.string().c_str(), "--cipher", "aes-256-gcm", "-f"}; //TODO Remove "-f" parameter, once EXPECT_RUN_SUCCESS can handle that EXPECT_RUN_SUCCESS(args, mountdir); modifyFilesystemKey(); @@ -68,3 +105,72 @@ TEST_F(CliTest_IntegrityCheck, givenIncorrectFilesystemKey_thenFails) { ErrorCode::EncryptionKeyChanged ); } + +// TODO Also enable this +TEST_F(CliTest_IntegrityCheck, givenFilesystemWithRolledBackBasedir_whenMounting_thenFails) { + vector args{basedir.string().c_str(), mountdir.string().c_str(), "--cipher", "aes-256-gcm", "-f"}; + //TODO Remove "-f" parameter, once EXPECT_RUN_SUCCESS/EXPECT_RUN_ERROR can handle that + + // create a filesystem with one file + EXPECT_RUN_SUCCESS(args, mountdir, [&] { + writeFile(mountdir / "myfile", "hello world"); + }); + + // backup the base directory + TempDir backup; + recursive_copy(basedir, backup.path() / "basedir"); + + // modify the file system contents + EXPECT_RUN_SUCCESS(args, mountdir, [&] { + writeFile(mountdir / "myfile", "hello world 2"); + }); + + // roll back base directory + bf::remove_all(basedir); + recursive_copy(backup.path() / "basedir", basedir); + + // error code is success because it unmounts normally + EXPECT_RUN_ERROR(args, "Integrity violation detected. Unmounting.", ErrorCode::Success, [&] { + EXPECT_FALSE(readingFileIsSuccessful(mountdir / "myfile")); + }); + + // Test it doesn't mount anymore now because it's marked with an integrity violation + EXPECT_RUN_ERROR(args, "There was an integrity violation detected. Preventing any further access to the file system.", ErrorCode::IntegrityViolationOnPreviousRun); +} + +TEST_F(CliTest_IntegrityCheck, whenRollingBackBasedirWhileMounted_thenUnmounts) { + vector args{basedir.string().c_str(), mountdir.string().c_str(), "--cipher", "aes-256-gcm", "-f"}; + //TODO Remove "-f" parameter, once EXPECT_RUN_SUCCESS/EXPECT_RUN_ERROR can handle that + + // create a filesystem with one file + EXPECT_RUN_SUCCESS(args, mountdir, [&] { + writeFile(mountdir / "myfile", "hello world"); + }); + + // backup the base directory + TempDir backup; + recursive_copy(basedir, backup.path() / "basedir"); + + // error code is success because it unmounts normally + EXPECT_RUN_ERROR(args, "Integrity violation detected. Unmounting.", ErrorCode::Success, [&] { + // modify the file system contents + writeFile(mountdir / "myfile", "hello world 2"); + ASSERT(readingFileIsSuccessful(mountdir / "myfile"), ""); // just to make sure reading usually works + + // wait for cache timeout (i.e. flush file system to disk) + constexpr auto cache_timeout = blockstore::caching::CachingBlockStore2::MAX_LIFETIME_SEC + cryfs::cachingfsblobstore::CachingFsBlobStore::MAX_LIFETIME_SEC; + boost::this_thread::sleep_for(boost::chrono::seconds(static_cast(std::ceil(cache_timeout * 3)))); + + // roll back base directory + bf::remove_all(basedir); + recursive_copy(backup.path() / "basedir", basedir); + + // expect reading now fails + EXPECT_FALSE(readingFileIsSuccessful(mountdir / "myfile")); + }); + + // Test it doesn't mount anymore now because it's marked with an integrity violation + EXPECT_RUN_ERROR(args, "There was an integrity violation detected. Preventing any further access to the file system.", ErrorCode::IntegrityViolationOnPreviousRun); +} + +} diff --git a/test/cryfs-cli/testutils/CliTest.h b/test/cryfs-cli/testutils/CliTest.h index 0214d444..dcfb1cff 100644 --- a/test/cryfs-cli/testutils/CliTest.h +++ b/test/cryfs-cli/testutils/CliTest.h @@ -60,21 +60,27 @@ public: EXPECT_RUN_ERROR(args, "Usage:[^\\x00]*"+message, errorCode); } - void EXPECT_RUN_ERROR(const std::vector& args, const std::string& message, cryfs::ErrorCode errorCode) { - FilesystemOutput filesystem_output = _run_filesystem(args, boost::none, []{}); + void EXPECT_RUN_ERROR(const std::vector& args, const std::string& message, cryfs::ErrorCode errorCode, std::function onMounted = [] {}) { + FilesystemOutput filesystem_output = run_filesystem(args, boost::none, std::move(onMounted)); EXPECT_EQ(exitCode(errorCode), filesystem_output.exit_code); - EXPECT_TRUE(std::regex_search(filesystem_output.stderr_, std::regex(message))); + if (!std::regex_search(filesystem_output.stderr_, std::regex(message))) { + std::cerr << filesystem_output.stderr_ << std::endl; + EXPECT_TRUE(false); + } } - void EXPECT_RUN_SUCCESS(const std::vector& args, const boost::filesystem::path &mountDir) { + void EXPECT_RUN_SUCCESS(const std::vector& args, const boost::optional &mountDir, std::function onMounted = [] {}) { //TODO Make this work when run in background ASSERT(std::find(args.begin(), args.end(), string("-f")) != args.end(), "Currently only works if run in foreground"); - FilesystemOutput filesystem_output = _run_filesystem(args, mountDir, []{}); + FilesystemOutput filesystem_output = run_filesystem(args, mountDir, std::move(onMounted)); EXPECT_EQ(0, filesystem_output.exit_code); - EXPECT_TRUE(std::regex_search(filesystem_output.stdout_, std::regex("Mounting filesystem"))); + if (!std::regex_search(filesystem_output.stdout_, std::regex("Mounting filesystem"))) { + std::cerr << filesystem_output.stdout_ << std::endl; + EXPECT_TRUE(false); + } } struct FilesystemOutput final { @@ -95,10 +101,10 @@ public: returncode = cpputils::Subprocess::call( std::string("fusermount -u ") + mountDir.string().c_str() + " 2>/dev/null").exitcode; #endif - ASSERT(returncode == 0, "Unmount failed"); + EXPECT_EQ(0, returncode); } - FilesystemOutput _run_filesystem(const std::vector& args, boost::optional mountDirForUnmounting, std::function onMounted) { + FilesystemOutput run_filesystem(const std::vector& args, boost::optional mountDirForUnmounting, std::function onMounted) { testing::internal::CaptureStdout(); testing::internal::CaptureStderr(); @@ -131,29 +137,29 @@ public: return true; }); - if(std::future_status::ready != on_mounted_success.wait_for(std::chrono::seconds(10))) { + if(std::future_status::ready != on_mounted_success.wait_for(std::chrono::seconds(1000))) { testing::internal::GetCapturedStdout(); // stop capturing stdout testing::internal::GetCapturedStderr(); // stop capturing stderr - std::cerr << "onMounted thread (e.g. used for unmount) didn't finish"; + std::cerr << "onMounted thread (e.g. used for unmount) didn't finish" << std::endl; // The std::future destructor of a future created with std::async blocks until the future is ready. // so, instead of causing a deadlock, rather abort exit(EXIT_FAILURE); } EXPECT_TRUE(on_mounted_success.get()); // this also re-throws any potential exceptions - if(std::future_status::ready != exit_code.wait_for(std::chrono::seconds(10))) { + if(std::future_status::ready != exit_code.wait_for(std::chrono::seconds(1000))) { testing::internal::GetCapturedStdout(); // stop capturing stdout testing::internal::GetCapturedStderr(); // stop capturing stderr - std::cerr << "Filesystem thread didn't finish"; + std::cerr << "Filesystem thread didn't finish" << std::endl; // The std::future destructor of a future created with std::async blocks until the future is ready. // so, instead of causing a deadlock, rather abort exit(EXIT_FAILURE); } return { - exit_code.get(), + exit_code.get(), // this also re-throws any potential exceptions testing::internal::GetCapturedStdout(), testing::internal::GetCapturedStderr() }; diff --git a/test/cryfs/filesystem/CryFsTest.cpp b/test/cryfs/filesystem/CryFsTest.cpp index 61854bdf..5b466dac 100644 --- a/test/cryfs/filesystem/CryFsTest.cpp +++ b/test/cryfs/filesystem/CryFsTest.cpp @@ -33,6 +33,8 @@ using cryfs::CryPresetPasswordBasedKeyProvider; namespace bf = boost::filesystem; using namespace cryfs; +namespace { + class CryFsTest: public Test, public TestWithMockConsole, public TestWithFakeHomeDirectory { public: CryFsTest(): tempLocalStateDir(), localStateDir(tempLocalStateDir.path()), rootdir(), config(false) { @@ -53,23 +55,31 @@ public: TempFile config; }; +auto failOnIntegrityViolation() { + return [] { + EXPECT_TRUE(false); + }; +} + TEST_F(CryFsTest, CreatedRootdirIsLoadableAfterClosing) { { - CryDevice dev(loadOrCreateConfig(), blockStore(), localStateDir, 0x12345678, false, false); + CryDevice dev(loadOrCreateConfig(), blockStore(), localStateDir, 0x12345678, false, false, failOnIntegrityViolation()); } - CryDevice dev(loadOrCreateConfig(), blockStore(), localStateDir, 0x12345678, false, false); + CryDevice dev(loadOrCreateConfig(), blockStore(), localStateDir, 0x12345678, false, false, failOnIntegrityViolation()); auto rootDir = dev.LoadDir(bf::path("/")); rootDir.value()->children(); } TEST_F(CryFsTest, LoadingFilesystemDoesntModifyConfigFile) { { - CryDevice dev(loadOrCreateConfig(), blockStore(), localStateDir, 0x12345678, false, false); + CryDevice dev(loadOrCreateConfig(), blockStore(), localStateDir, 0x12345678, false, false, failOnIntegrityViolation()); } Data configAfterCreating = Data::LoadFromFile(config.path()).value(); { - CryDevice dev(loadOrCreateConfig(), blockStore(), localStateDir, 0x12345678, false, false); + CryDevice dev(loadOrCreateConfig(), blockStore(), localStateDir, 0x12345678, false, false, failOnIntegrityViolation()); } Data configAfterLoading = Data::LoadFromFile(config.path()).value(); EXPECT_EQ(configAfterCreating, configAfterLoading); } + +} diff --git a/test/cryfs/filesystem/FileSystemTest.cpp b/test/cryfs/filesystem/FileSystemTest.cpp index dd796195..22193195 100644 --- a/test/cryfs/filesystem/FileSystemTest.cpp +++ b/test/cryfs/filesystem/FileSystemTest.cpp @@ -21,6 +21,14 @@ using cryfs::CryPresetPasswordBasedKeyProvider; using namespace cryfs; +namespace { + +auto failOnIntegrityViolation() { + return [] { + EXPECT_TRUE(false); + }; +} + class CryFsTestFixture: public FileSystemTestFixture, public TestWithMockConsole, public TestWithFakeHomeDirectory { public: CryFsTestFixture() @@ -33,7 +41,7 @@ public: auto keyProvider = make_unique_ref("mypassword", make_unique_ref(SCrypt::TestSettings)); auto config = CryConfigLoader(_console, Random::PseudoRandom(), std::move(keyProvider), localStateDir, none, none, none) .loadOrCreate(configFile.path(), false, false).value(); - return make_unique_ref(std::move(config.configFile), std::move(blockStore), localStateDir, config.myClientId, false, false); + return make_unique_ref(std::move(config.configFile), std::move(blockStore), localStateDir, config.myClientId, false, false, failOnIntegrityViolation()); } cpputils::TempDir tempLocalStateDir; @@ -42,3 +50,4 @@ public: }; FSPP_ADD_FILESYTEM_TESTS(CryFS, CryFsTestFixture); +} diff --git a/test/cryfs/filesystem/testutils/CryTestBase.h b/test/cryfs/filesystem/testutils/CryTestBase.h index e01f631f..e8510cab 100644 --- a/test/cryfs/filesystem/testutils/CryTestBase.h +++ b/test/cryfs/filesystem/testutils/CryTestBase.h @@ -9,11 +9,17 @@ #include "../../testutils/TestWithFakeHomeDirectory.h" #include "../../testutils/MockConsole.h" +inline auto failOnIntegrityViolation() { + return [] { + EXPECT_TRUE(false); + }; +} + class CryTestBase : public TestWithFakeHomeDirectory { public: CryTestBase(): _tempLocalStateDir(), _localStateDir(_tempLocalStateDir.path()), _configFile(false), _device(nullptr) { auto fakeBlockStore = cpputils::make_unique_ref(); - _device = std::make_unique(configFile(), std::move(fakeBlockStore), _localStateDir, 0x12345678, false, false); + _device = std::make_unique(configFile(), std::move(fakeBlockStore), _localStateDir, 0x12345678, false, false, failOnIntegrityViolation()); } cryfs::CryConfigFile configFile() {