diff --git a/src/blockstore/implementations/versioncounting/KnownBlockVersions.cpp b/src/blockstore/implementations/versioncounting/KnownBlockVersions.cpp index 18686995..5f62e021 100644 --- a/src/blockstore/implementations/versioncounting/KnownBlockVersions.cpp +++ b/src/blockstore/implementations/versioncounting/KnownBlockVersions.cpp @@ -195,5 +195,15 @@ void KnownBlockVersions::markBlockAsDeleted(const Key &key) { _lastUpdateClientId[key] = CLIENT_ID_FOR_DELETED_BLOCK; } +bool KnownBlockVersions::blockShouldExist(const Key &key) const { + auto found = _lastUpdateClientId.find(key); + if (found == _lastUpdateClientId.end()) { + // We've never seen (i.e. loaded) this block. So we can't say it has to exist. + return false; + } + // We've seen the block before. If we didn't delete it, it should exist (only works for single-client scenario). + return found->second != CLIENT_ID_FOR_DELETED_BLOCK; +} + } } diff --git a/src/blockstore/implementations/versioncounting/KnownBlockVersions.h b/src/blockstore/implementations/versioncounting/KnownBlockVersions.h index f4b24b60..b3edbbf2 100644 --- a/src/blockstore/implementations/versioncounting/KnownBlockVersions.h +++ b/src/blockstore/implementations/versioncounting/KnownBlockVersions.h @@ -27,6 +27,8 @@ namespace blockstore { void markBlockAsDeleted(const Key &key); + bool blockShouldExist(const Key &key) const; + uint64_t getBlockVersion(uint32_t clientId, const Key &key) const; uint32_t myClientId() const; diff --git a/src/blockstore/implementations/versioncounting/VersionCountingBlockStore.h b/src/blockstore/implementations/versioncounting/VersionCountingBlockStore.h index b1617372..17e9392e 100644 --- a/src/blockstore/implementations/versioncounting/VersionCountingBlockStore.h +++ b/src/blockstore/implementations/versioncounting/VersionCountingBlockStore.h @@ -14,7 +14,7 @@ namespace versioncounting { class VersionCountingBlockStore final: public BlockStore { public: - VersionCountingBlockStore(cpputils::unique_ref baseBlockStore, const boost::filesystem::path &integrityFilePath); + VersionCountingBlockStore(cpputils::unique_ref baseBlockStore, const boost::filesystem::path &integrityFilePath, bool missingBlockIsIntegrityViolation); Key createKey() override; boost::optional> tryCreate(const Key &key, cpputils::Data data) override; @@ -32,13 +32,14 @@ public: private: cpputils::unique_ref _baseBlockStore; KnownBlockVersions _knownBlockVersions; + const bool _missingBlockIsIntegrityViolation; DISALLOW_COPY_AND_ASSIGN(VersionCountingBlockStore); }; -inline VersionCountingBlockStore::VersionCountingBlockStore(cpputils::unique_ref baseBlockStore, const boost::filesystem::path &integrityFilePath) - : _baseBlockStore(std::move(baseBlockStore)), _knownBlockVersions(integrityFilePath) { +inline VersionCountingBlockStore::VersionCountingBlockStore(cpputils::unique_ref baseBlockStore, const boost::filesystem::path &integrityFilePath, bool missingBlockIsIntegrityViolation) + : _baseBlockStore(std::move(baseBlockStore)), _knownBlockVersions(integrityFilePath), _missingBlockIsIntegrityViolation(missingBlockIsIntegrityViolation) { } inline Key VersionCountingBlockStore::createKey() { @@ -57,6 +58,9 @@ inline boost::optional> VersionCountingBlockStore::t inline boost::optional> VersionCountingBlockStore::load(const Key &key) { auto block = _baseBlockStore->load(key); if (block == boost::none) { + if (_missingBlockIsIntegrityViolation && _knownBlockVersions.blockShouldExist(key)) { + throw IntegrityViolationError("A block that should exist wasn't found. Did an attacker delete it?"); + } return boost::none; } return boost::optional>(VersionCountingBlock::Load(std::move(*block), &_knownBlockVersions)); diff --git a/src/cryfs/filesystem/CryDevice.cpp b/src/cryfs/filesystem/CryDevice.cpp index fdb916ef..41aae374 100644 --- a/src/cryfs/filesystem/CryDevice.cpp +++ b/src/cryfs/filesystem/CryDevice.cpp @@ -89,7 +89,7 @@ unique_ref CryDevice::CreateVersionCountingEncryptedBlockStore(uniqu } #endif - return make_unique_ref(std::move(encryptedBlockStore), integrityFilePath); + return make_unique_ref(std::move(encryptedBlockStore), integrityFilePath, false); } Key CryDevice::CreateRootBlobAndReturnKey() { diff --git a/test/blockstore/implementations/versioncounting/VersionCountingBlockStoreTest_Generic.cpp b/test/blockstore/implementations/versioncounting/VersionCountingBlockStoreTest_Generic.cpp index e59a00f0..83e40e2e 100644 --- a/test/blockstore/implementations/versioncounting/VersionCountingBlockStoreTest_Generic.cpp +++ b/test/blockstore/implementations/versioncounting/VersionCountingBlockStoreTest_Generic.cpp @@ -17,14 +17,16 @@ using cpputils::make_unique_ref; using cpputils::unique_ref; using cpputils::TempFile; +template class VersionCountingBlockStoreTestFixture: public BlockStoreTestFixture { public: VersionCountingBlockStoreTestFixture() :stateFile(false) {} TempFile stateFile; unique_ref createBlockStore() override { - return make_unique_ref(make_unique_ref(), stateFile.path()); + return make_unique_ref(make_unique_ref(), stateFile.path(), SINGLECLIENT); } }; -INSTANTIATE_TYPED_TEST_CASE_P(VersionCounting, BlockStoreTest, VersionCountingBlockStoreTestFixture); +INSTANTIATE_TYPED_TEST_CASE_P(VersionCounting_multiclient, BlockStoreTest, VersionCountingBlockStoreTestFixture); +INSTANTIATE_TYPED_TEST_CASE_P(VersionCounting_singleclient, BlockStoreTest, VersionCountingBlockStoreTestFixture); diff --git a/test/blockstore/implementations/versioncounting/VersionCountingBlockStoreTest_Specific.cpp b/test/blockstore/implementations/versioncounting/VersionCountingBlockStoreTest_Specific.cpp index b6375c98..612e7661 100644 --- a/test/blockstore/implementations/versioncounting/VersionCountingBlockStoreTest_Specific.cpp +++ b/test/blockstore/implementations/versioncounting/VersionCountingBlockStoreTest_Specific.cpp @@ -13,6 +13,8 @@ using cpputils::unique_ref; using cpputils::make_unique_ref; using cpputils::TempFile; using boost::none; +using std::make_unique; +using std::unique_ptr; using blockstore::testfake::FakeBlockStore; @@ -24,7 +26,7 @@ public: VersionCountingBlockStoreTest(): stateFile(false), baseBlockStore(new FakeBlockStore), - blockStore(make_unique_ref(std::move(cpputils::nullcheck(std::unique_ptr(baseBlockStore)).value()), stateFile.path())), + blockStore(make_unique_ref(std::move(cpputils::nullcheck(std::unique_ptr(baseBlockStore)).value()), stateFile.path(), false)), data(DataFixture::generate(BLOCKSIZE)) { } TempFile stateFile; @@ -32,6 +34,18 @@ public: unique_ref blockStore; Data data; + std::pair> makeBlockStoreWithDeletionPrevention() { + FakeBlockStore *baseBlockStore = new FakeBlockStore; + auto blockStore = make_unique(std::move(cpputils::nullcheck(std::unique_ptr(baseBlockStore)).value()), stateFile.path(), true); + return std::make_pair(baseBlockStore, std::move(blockStore)); + } + + std::pair> makeBlockStoreWithoutDeletionPrevention() { + FakeBlockStore *baseBlockStore = new FakeBlockStore; + auto blockStore = make_unique(std::move(cpputils::nullcheck(std::unique_ptr(baseBlockStore)).value()), stateFile.path(), false); + return std::make_pair(baseBlockStore, std::move(blockStore)); + } + blockstore::Key CreateBlockReturnKey() { return CreateBlockReturnKey(data); } @@ -174,9 +188,33 @@ TEST_F(VersionCountingBlockStoreTest, RollbackPrevention_AllowsReintroducingDele EXPECT_NE(boost::none, blockStore->load(key)); } +// Check that in a multi-client scenario, missing blocks are not integrity errors, because another client might have deleted them. +TEST_F(VersionCountingBlockStoreTest, DeletionPrevention_AllowsDeletingBlocksWhenDeactivated) { + FakeBlockStore *baseBlockStore; + unique_ptr blockStore; + std::tie(baseBlockStore, blockStore) = makeBlockStoreWithoutDeletionPrevention(); + auto key = blockStore->create(Data(0))->key(); + baseBlockStore->remove(baseBlockStore->load(key).value()); + EXPECT_EQ(boost::none, blockStore->load(key)); +} + +// Check that in a single-client scenario, missing blocks are integrity errors. +TEST_F(VersionCountingBlockStoreTest, DeletionPrevention_DoesntAllowDeletingBlocksWhenActivated) { + FakeBlockStore *baseBlockStore; + unique_ptr blockStore; + std::tie(baseBlockStore, blockStore) = makeBlockStoreWithDeletionPrevention(); + auto key = blockStore->create(Data(0))->key(); + baseBlockStore->remove(baseBlockStore->load(key).value()); + EXPECT_THROW( + blockStore->load(key), + IntegrityViolationError + ); +} + // TODO Test more integrity cases: // - RollbackPrevention_DoesntAllowReintroducingDeletedBlocks with different client id (i.e. trying to re-introduce the newest block of a different client) // - RollbackPrevention_AllowsReintroducingDeletedBlocksWithNewVersionNumber with different client id +// - Think about more... TEST_F(VersionCountingBlockStoreTest, PhysicalBlockSize_zerophysical) { EXPECT_EQ(0u, blockStore->blockSizeFromPhysicalBlockSize(0));