From 636445cd82860b1214962b55af8a95440aa26af6 Mon Sep 17 00:00:00 2001 From: Sebastian Messmer Date: Sat, 13 Feb 2016 15:06:28 +0100 Subject: [PATCH] Introduce version flags for file system entities to allow future CryFS versions to be backwards-compatible even if the format changes. --- .../onblocks/datanodestore/DataInnerNode.cpp | 4 + .../onblocks/datanodestore/DataLeafNode.cpp | 4 + .../onblocks/datanodestore/DataNode.cpp | 2 + .../onblocks/datanodestore/DataNode.h | 3 + .../onblocks/datanodestore/DataNodeView.h | 14 ++- .../encrypted/EncryptedBlock.h | 33 +++++- .../implementations/ondisk/OnDiskBlock.cpp | 75 ++++++++++--- .../implementations/ondisk/OnDiskBlock.h | 11 +- src/cryfs/CMakeLists.txt | 1 + src/cryfs/filesystem/fsblobstore/DirBlob.cpp | 11 +- src/cryfs/filesystem/fsblobstore/FileBlob.cpp | 12 +- src/cryfs/filesystem/fsblobstore/FsBlob.h | 39 +++---- .../filesystem/fsblobstore/FsBlobStore.cpp | 9 +- .../filesystem/fsblobstore/FsBlobView.cpp | 5 + src/cryfs/filesystem/fsblobstore/FsBlobView.h | 105 ++++++++++++++++++ .../filesystem/fsblobstore/MagicNumbers.h | 20 ---- .../filesystem/fsblobstore/SymlinkBlob.cpp | 28 ++--- .../filesystem/fsblobstore/SymlinkBlob.h | 4 +- .../OnDiskBlockTest/OnDiskBlockCreateTest.cpp | 6 +- .../OnDiskBlockTest/OnDiskBlockFlushTest.cpp | 6 +- .../OnDiskBlockTest/OnDiskBlockLoadTest.cpp | 16 +-- test/cryfs/cli/CliTest_Setup.cpp | 20 ++-- test/cryfs/cli/CliTest_WrongEnvironment.cpp | 4 +- test/cryfs/cli/testutils/CliTest.h | 4 + 24 files changed, 305 insertions(+), 131 deletions(-) create mode 100644 src/cryfs/filesystem/fsblobstore/FsBlobView.cpp create mode 100644 src/cryfs/filesystem/fsblobstore/FsBlobView.h delete mode 100644 src/cryfs/filesystem/fsblobstore/MagicNumbers.h diff --git a/src/blobstore/implementations/onblocks/datanodestore/DataInnerNode.cpp b/src/blobstore/implementations/onblocks/datanodestore/DataInnerNode.cpp index dda86ece..64ca9ebc 100644 --- a/src/blobstore/implementations/onblocks/datanodestore/DataInnerNode.cpp +++ b/src/blobstore/implementations/onblocks/datanodestore/DataInnerNode.cpp @@ -15,6 +15,9 @@ namespace datanodestore { DataInnerNode::DataInnerNode(DataNodeView view) : DataNode(std::move(view)) { ASSERT(depth() > 0, "Inner node can't have depth 0. Is this a leaf maybe?"); + if (node().FormatVersion() != FORMAT_VERSION_HEADER) { + throw std::runtime_error("This node format is not supported. Was it created with a newer version of CryFS?"); + } } DataInnerNode::~DataInnerNode() { @@ -22,6 +25,7 @@ DataInnerNode::~DataInnerNode() { unique_ref DataInnerNode::InitializeNewNode(unique_ref block, const DataNode &first_child) { DataNodeView node(std::move(block)); + node.setFormatVersion(DataNode::FORMAT_VERSION_HEADER); node.setDepth(first_child.depth() + 1); node.setSize(1); auto result = make_unique_ref(std::move(node)); diff --git a/src/blobstore/implementations/onblocks/datanodestore/DataLeafNode.cpp b/src/blobstore/implementations/onblocks/datanodestore/DataLeafNode.cpp index 7e3511ea..d0681950 100644 --- a/src/blobstore/implementations/onblocks/datanodestore/DataLeafNode.cpp +++ b/src/blobstore/implementations/onblocks/datanodestore/DataLeafNode.cpp @@ -16,6 +16,9 @@ DataLeafNode::DataLeafNode(DataNodeView view) : DataNode(std::move(view)) { ASSERT(node().Depth() == 0, "Leaf node must have depth 0. Is it an inner node instead?"); ASSERT(numBytes() <= maxStoreableBytes(), "Leaf says it stores more bytes than it has space for"); + if (node().FormatVersion() != FORMAT_VERSION_HEADER) { + throw std::runtime_error("This node format is not supported. Was it created with a newer version of CryFS?"); + } } DataLeafNode::~DataLeafNode() { @@ -23,6 +26,7 @@ DataLeafNode::~DataLeafNode() { unique_ref DataLeafNode::InitializeNewNode(unique_ref block) { DataNodeView node(std::move(block)); + node.setFormatVersion(DataNode::FORMAT_VERSION_HEADER); node.setDepth(0); node.setSize(0); //fillDataWithZeroes(); not needed, because a newly created block will be zeroed out. DataLeafNodeTest.SpaceIsZeroFilledWhenGrowing ensures this. diff --git a/src/blobstore/implementations/onblocks/datanodestore/DataNode.cpp b/src/blobstore/implementations/onblocks/datanodestore/DataNode.cpp index 3b409e4f..4d9442a5 100644 --- a/src/blobstore/implementations/onblocks/datanodestore/DataNode.cpp +++ b/src/blobstore/implementations/onblocks/datanodestore/DataNode.cpp @@ -14,6 +14,8 @@ namespace blobstore { namespace onblocks { namespace datanodestore { +constexpr uint16_t DataNode::FORMAT_VERSION_HEADER; + DataNode::DataNode(DataNodeView node) : _node(std::move(node)) { } diff --git a/src/blobstore/implementations/onblocks/datanodestore/DataNode.h b/src/blobstore/implementations/onblocks/datanodestore/DataNode.h index e37c5415..75de60b2 100644 --- a/src/blobstore/implementations/onblocks/datanodestore/DataNode.h +++ b/src/blobstore/implementations/onblocks/datanodestore/DataNode.h @@ -24,6 +24,9 @@ public: void flush() const; protected: + // The FORMAT_VERSION_HEADER is used to allow future versions to have compatibility. + static constexpr uint16_t FORMAT_VERSION_HEADER = 0; + DataNode(DataNodeView block); DataNodeView &node(); diff --git a/src/blobstore/implementations/onblocks/datanodestore/DataNodeView.h b/src/blobstore/implementations/onblocks/datanodestore/DataNodeView.h index 7dfcb1c4..834061b3 100644 --- a/src/blobstore/implementations/onblocks/datanodestore/DataNodeView.h +++ b/src/blobstore/implementations/onblocks/datanodestore/DataNodeView.h @@ -28,10 +28,12 @@ public: //Total size of the header static constexpr uint32_t HEADERSIZE_BYTES = 8; + //Where in the header is the format version field (used to allow compatibility with future versions of CryFS) + static constexpr uint32_t FORMAT_VERSION_OFFSET_BYTES = 0; //format version uses 2 bytes //Where in the header is the depth field - static constexpr uint32_t DEPTH_OFFSET_BYTES = 0; + static constexpr uint32_t DEPTH_OFFSET_BYTES = 3; // depth uses 1 byte //Where in the header is the size field (for inner nodes: number of children, for leafs: content data size) - static constexpr uint32_t SIZE_OFFSET_BYTES = 4; + static constexpr uint32_t SIZE_OFFSET_BYTES = 4; // size uses 4 bytes //Size of a block (header + data region) @@ -66,6 +68,14 @@ public: DataNodeView(DataNodeView &&rhs) = default; + uint16_t FormatVersion() const { + return *((uint8_t*)_block->data()+DataNodeLayout::FORMAT_VERSION_OFFSET_BYTES); + } + + void setFormatVersion(uint16_t value) { + _block->write(&value, DataNodeLayout::FORMAT_VERSION_OFFSET_BYTES, sizeof(value)); + } + uint8_t Depth() const { return *((uint8_t*)_block->data()+DataNodeLayout::DEPTH_OFFSET_BYTES); } diff --git a/src/blockstore/implementations/encrypted/EncryptedBlock.h b/src/blockstore/implementations/encrypted/EncryptedBlock.h index 1cb40110..e72a3e0a 100644 --- a/src/blockstore/implementations/encrypted/EncryptedBlock.h +++ b/src/blockstore/implementations/encrypted/EncryptedBlock.h @@ -55,6 +55,11 @@ private: void _encryptToBaseBlock(); static cpputils::Data _prependKeyHeaderToData(const Key &key, cpputils::Data data); static bool _keyHeaderIsCorrect(const Key &key, const cpputils::Data &data); + static cpputils::Data _prependFormatHeader(const cpputils::Data &data); + static void _checkFormatHeader(const void *data); + + // This header is prepended to blocks to allow future versions to have compatibility. + static constexpr uint16_t FORMAT_VERSION_HEADER = 0; std::mutex _mutex; @@ -64,12 +69,16 @@ private: template constexpr unsigned int EncryptedBlock::HEADER_LENGTH; +template +constexpr uint16_t EncryptedBlock::FORMAT_VERSION_HEADER; + template boost::optional>> EncryptedBlock::TryCreateNew(BlockStore *baseBlockStore, const Key &key, cpputils::Data data, const typename Cipher::EncryptionKey &encKey) { cpputils::Data plaintextWithHeader = _prependKeyHeaderToData(key, std::move(data)); cpputils::Data encrypted = Cipher::encrypt((byte*)plaintextWithHeader.data(), plaintextWithHeader.size(), encKey); - auto baseBlock = baseBlockStore->tryCreate(key, std::move(encrypted)); + cpputils::Data encryptedWithFormatHeader = _prependFormatHeader(std::move(encrypted)); + auto baseBlock = baseBlockStore->tryCreate(key, std::move(encryptedWithFormatHeader)); if (baseBlock == boost::none) { //TODO Test this code branch return boost::none; @@ -78,10 +87,18 @@ boost::optional>> EncryptedBlock(std::move(*baseBlock), encKey, std::move(plaintextWithHeader)); } +template +cpputils::Data EncryptedBlock::_prependFormatHeader(const cpputils::Data &data) { + cpputils::Data dataWithHeader(sizeof(FORMAT_VERSION_HEADER) + data.size()); + std::memcpy(dataWithHeader.dataOffset(0), &FORMAT_VERSION_HEADER, sizeof(FORMAT_VERSION_HEADER)); + std::memcpy(dataWithHeader.dataOffset(sizeof(FORMAT_VERSION_HEADER)), data.data(), data.size()); + return dataWithHeader; +} + template boost::optional>> EncryptedBlock::TryDecrypt(cpputils::unique_ref baseBlock, const typename Cipher::EncryptionKey &encKey) { - //TODO Change BlockStore so we can read their "class Data" objects instead of "void *data()", and then we can change the Cipher interface to take Data objects instead of "byte *" + size - boost::optional plaintextWithHeader = Cipher::decrypt((byte*)baseBlock->data(), baseBlock->size(), encKey); + _checkFormatHeader(baseBlock->data()); + boost::optional plaintextWithHeader = Cipher::decrypt((byte*)baseBlock->data() + sizeof(FORMAT_VERSION_HEADER), baseBlock->size() - sizeof(FORMAT_VERSION_HEADER), encKey); if(plaintextWithHeader == boost::none) { //Decryption failed (e.g. an authenticated cipher detected modifications to the ciphertext) cpputils::logging::LOG(cpputils::logging::WARN) << "Decrypting block " << baseBlock->key().ToString() << " failed. Was the block modified by an attacker?"; @@ -95,6 +112,13 @@ boost::optional>> EncryptedBlock>(std::move(baseBlock), encKey, std::move(*plaintextWithHeader)); } +template +void EncryptedBlock::_checkFormatHeader(const void *data) { + if (*reinterpret_cast(data) != FORMAT_VERSION_HEADER) { + throw std::runtime_error("The encrypted block has the wrong format. Was it created with a newer version of CryFS?"); + } +} + template cpputils::Data EncryptedBlock::_prependKeyHeaderToData(const Key &key, cpputils::Data data) { static_assert(HEADER_LENGTH >= Key::BINARY_LENGTH, "Key doesn't fit into the header"); @@ -159,7 +183,8 @@ template void EncryptedBlock::_encryptToBaseBlock() { if (_dataChanged) { cpputils::Data encrypted = Cipher::encrypt((byte*)_plaintextWithHeader.data(), _plaintextWithHeader.size(), _encKey); - _baseBlock->write(encrypted.data(), 0, encrypted.size()); + _baseBlock->write(&FORMAT_VERSION_HEADER, 0, sizeof(FORMAT_VERSION_HEADER)); + _baseBlock->write(encrypted.data(), sizeof(FORMAT_VERSION_HEADER), encrypted.size()); _dataChanged = false; } } diff --git a/src/blockstore/implementations/ondisk/OnDiskBlock.cpp b/src/blockstore/implementations/ondisk/OnDiskBlock.cpp index ffb789bb..cb78e4b3 100644 --- a/src/blockstore/implementations/ondisk/OnDiskBlock.cpp +++ b/src/blockstore/implementations/ondisk/OnDiskBlock.cpp @@ -12,6 +12,7 @@ using std::ostream; using std::ifstream; using std::ofstream; using std::ios; +using std::string; using cpputils::Data; using cpputils::make_unique_ref; using cpputils::unique_ref; @@ -23,6 +24,9 @@ namespace bf = boost::filesystem; namespace blockstore { namespace ondisk { +const string OnDiskBlock::FORMAT_VERSION_HEADER_PREFIX = "cryfs;block;"; +const string OnDiskBlock::FORMAT_VERSION_HEADER = OnDiskBlock::FORMAT_VERSION_HEADER_PREFIX + "0"; + OnDiskBlock::OnDiskBlock(const Key &key, const bf::path &filepath, Data data) : Block(key), _filepath(filepath), _data(std::move(data)), _dataChanged(false), _mutex() { } @@ -37,7 +41,7 @@ const void *OnDiskBlock::data() const { void OnDiskBlock::write(const void *source, uint64_t offset, uint64_t size) { ASSERT(offset <= _data.size() && offset + size <= _data.size(), "Write outside of valid area"); //Also check offset < _data->size() because of possible overflow in the addition - std::memcpy((uint8_t*)_data.data()+offset, source, size); + std::memcpy(_data.dataOffset(offset), source, size); _dataChanged = true; } @@ -53,15 +57,8 @@ void OnDiskBlock::resize(size_t newSize) { optional> OnDiskBlock::LoadFromDisk(const bf::path &rootdir, const Key &key) { auto filepath = rootdir / key.ToString(); try { - //If it isn't a file, Data::LoadFromFile() would usually also crash. We still need this extra check - //upfront, because Data::LoadFromFile() doesn't crash if we give it the path of a directory - //instead the path of a file. - //TODO Data::LoadFromFile now returns boost::optional. Do we then still need this? - if(!bf::is_regular_file(filepath)) { - return none; - } - boost::optional data = Data::LoadFromFile(filepath); - if (!data) { + boost::optional data = _loadFromDisk(filepath); + if (data == none) { return none; } return make_unique_ref(key, filepath, std::move(*data)); @@ -87,13 +84,61 @@ void OnDiskBlock::RemoveFromDisk(const bf::path &rootdir, const Key &key) { bf::remove(filepath); } -void OnDiskBlock::_fillDataWithZeroes() { - _data.FillWithZeroes(); - _dataChanged = true; +void OnDiskBlock::_storeToDisk() const { + std::ofstream file(_filepath.c_str(), std::ios::binary | std::ios::trunc); + if (!file.good()) { + throw std::runtime_error("Could not open file for writing"); + } + file.write(FORMAT_VERSION_HEADER.c_str(), formatVersionHeaderSize()); + if (!file.good()) { + throw std::runtime_error("Error writing block header"); + } + _data.StoreToStream(file); + if (!file.good()) { + throw std::runtime_error("Error writing block data"); + } } -void OnDiskBlock::_storeToDisk() const { - _data.StoreToFile(_filepath); +optional OnDiskBlock::_loadFromDisk(const bf::path &filepath) { + //If it isn't a file, ifstream::good() would return false. We still need this extra check + //upfront, because ifstream::good() doesn't crash if we give it the path of a directory + //instead the path of a file. + if(!bf::is_regular_file(filepath)) { + return none; + } + ifstream file(filepath.c_str(), ios::binary); + if (!file.good()) { + return none; + } + _checkHeader(&file); + Data result = Data::LoadFromStream(file); + return result; +} + +void OnDiskBlock::_checkHeader(istream *str) { + Data header(formatVersionHeaderSize()); + str->read(reinterpret_cast(header.data()), formatVersionHeaderSize()); + if (!_isAcceptedCryfsHeader(header)) { + if (_isOtherCryfsHeader(header)) { + throw std::runtime_error("This block is not supported yet. Maybe it was created with a newer version of CryFS?"); + } else { + throw std::runtime_error("This is not a valid block."); + } + } +} + +bool OnDiskBlock::_isAcceptedCryfsHeader(const Data &data) { + ASSERT(data.size() == formatVersionHeaderSize(), "We extracted the wrong header size from the block."); + return 0 == std::memcmp(data.data(), FORMAT_VERSION_HEADER.c_str(), formatVersionHeaderSize()); +} + +bool OnDiskBlock::_isOtherCryfsHeader(const Data &data) { + ASSERT(data.size() >= FORMAT_VERSION_HEADER_PREFIX.size(), "We extracted the wrong header size from the block."); + return 0 == std::memcmp(data.data(), FORMAT_VERSION_HEADER_PREFIX.c_str(), FORMAT_VERSION_HEADER_PREFIX.size()); +} + +unsigned int OnDiskBlock::formatVersionHeaderSize() { + return FORMAT_VERSION_HEADER.size() + 1; // +1 because of the null byte } void OnDiskBlock::flush() { diff --git a/src/blockstore/implementations/ondisk/OnDiskBlock.h b/src/blockstore/implementations/ondisk/OnDiskBlock.h index 03f958dc..8cfb9ac1 100644 --- a/src/blockstore/implementations/ondisk/OnDiskBlock.h +++ b/src/blockstore/implementations/ondisk/OnDiskBlock.h @@ -19,6 +19,10 @@ public: OnDiskBlock(const Key &key, const boost::filesystem::path &filepath, cpputils::Data data); ~OnDiskBlock(); + static const std::string FORMAT_VERSION_HEADER_PREFIX; + static const std::string FORMAT_VERSION_HEADER; + static unsigned int formatVersionHeaderSize(); + static boost::optional> LoadFromDisk(const boost::filesystem::path &rootdir, const Key &key); static boost::optional> CreateOnDisk(const boost::filesystem::path &rootdir, const Key &key, cpputils::Data data); static void RemoveFromDisk(const boost::filesystem::path &rootdir, const Key &key); @@ -32,11 +36,16 @@ public: void resize(size_t newSize) override; private: + + static bool _isAcceptedCryfsHeader(const cpputils::Data &data); + static bool _isOtherCryfsHeader(const cpputils::Data &data); + static void _checkHeader(std::istream *str); + const boost::filesystem::path _filepath; cpputils::Data _data; bool _dataChanged; - void _fillDataWithZeroes(); + static boost::optional _loadFromDisk(const boost::filesystem::path &filepath); void _storeToDisk() const; std::mutex _mutex; diff --git a/src/cryfs/CMakeLists.txt b/src/cryfs/CMakeLists.txt index 9775c6dc..b60a658d 100644 --- a/src/cryfs/CMakeLists.txt +++ b/src/cryfs/CMakeLists.txt @@ -26,6 +26,7 @@ set(SOURCES filesystem/fsblobstore/utils/DirEntry.cpp filesystem/fsblobstore/utils/DirEntryList.cpp filesystem/fsblobstore/FsBlobStore.cpp + filesystem/fsblobstore/FsBlobView.cpp filesystem/fsblobstore/FileBlob.cpp filesystem/fsblobstore/FsBlob.cpp filesystem/fsblobstore/SymlinkBlob.cpp diff --git a/src/cryfs/filesystem/fsblobstore/DirBlob.cpp b/src/cryfs/filesystem/fsblobstore/DirBlob.cpp index e602913c..3b92b3bd 100644 --- a/src/cryfs/filesystem/fsblobstore/DirBlob.cpp +++ b/src/cryfs/filesystem/fsblobstore/DirBlob.cpp @@ -6,7 +6,6 @@ #include #include -#include "MagicNumbers.h" #include "../CryDevice.h" #include "FileBlob.h" #include "SymlinkBlob.h" @@ -30,7 +29,7 @@ constexpr off_t DirBlob::DIR_LSTAT_SIZE; DirBlob::DirBlob(unique_ref blob, std::function getLstatSize) : FsBlob(std::move(blob)), _getLstatSize(getLstatSize), _entries(), _mutex(), _changed(false) { - ASSERT(magicNumber() == MagicNumbers::DIR, "Loaded blob is not a directory"); + ASSERT(baseBlob().blobType() == FsBlobView::BlobType::DIR, "Loaded blob is not a directory"); _readEntriesFromBlob(); } @@ -46,15 +45,15 @@ void DirBlob::flush() { } unique_ref DirBlob::InitializeEmptyDir(unique_ref blob, std::function getLstatSize) { - InitializeBlobWithMagicNumber(blob.get(), MagicNumbers::DIR); + InitializeBlob(blob.get(), FsBlobView::BlobType::DIR); return make_unique_ref(std::move(blob), getLstatSize); } void DirBlob::_writeEntriesToBlob() { if (_changed) { Data serialized = _entries.serialize(); - baseBlob().resize(1 + serialized.size()); - baseBlob().write(serialized.data(), 1, serialized.size()); + baseBlob().resize(serialized.size()); + baseBlob().write(serialized.data(), 0, serialized.size()); _changed = false; } } @@ -62,7 +61,7 @@ void DirBlob::_writeEntriesToBlob() { void DirBlob::_readEntriesFromBlob() { //No lock needed, because this is only called from the constructor. Data data = baseBlob().readAll(); - _entries.deserializeFrom(static_cast(data.data()) + 1, data.size() - 1); // data+1/size-1 because the first byte is the magic number + _entries.deserializeFrom(static_cast(data.data()), data.size()); } void DirBlob::AddChildDir(const std::string &name, const Key &blobKey, mode_t mode, uid_t uid, gid_t gid) { diff --git a/src/cryfs/filesystem/fsblobstore/FileBlob.cpp b/src/cryfs/filesystem/fsblobstore/FileBlob.cpp index 22e20cbe..6f951c04 100644 --- a/src/cryfs/filesystem/fsblobstore/FileBlob.cpp +++ b/src/cryfs/filesystem/fsblobstore/FileBlob.cpp @@ -1,6 +1,5 @@ #include "FileBlob.h" -#include "MagicNumbers.h" #include #include @@ -14,19 +13,20 @@ namespace fsblobstore { FileBlob::FileBlob(unique_ref blob) : FsBlob(std::move(blob)) { + ASSERT(baseBlob().blobType() == FsBlobView::BlobType::FILE, "Loaded blob is not a file"); } unique_ref FileBlob::InitializeEmptyFile(unique_ref blob) { - InitializeBlobWithMagicNumber(blob.get(), MagicNumbers::FILE); + InitializeBlob(blob.get(), FsBlobView::BlobType::FILE); return make_unique_ref(std::move(blob)); } ssize_t FileBlob::read(void *target, uint64_t offset, uint64_t count) const { - return baseBlob().tryRead(target, offset + 1, count); + return baseBlob().tryRead(target, offset, count); } void FileBlob::write(const void *source, uint64_t offset, uint64_t count) { - baseBlob().write(source, offset + 1, count); + baseBlob().write(source, offset, count); } void FileBlob::flush() { @@ -34,7 +34,7 @@ void FileBlob::flush() { } void FileBlob::resize(off_t size) { - baseBlob().resize(size+1); + baseBlob().resize(size); } off_t FileBlob::lstat_size() const { @@ -42,7 +42,7 @@ off_t FileBlob::lstat_size() const { } off_t FileBlob::size() const { - return baseBlob().size()-1; + return baseBlob().size(); } } diff --git a/src/cryfs/filesystem/fsblobstore/FsBlob.h b/src/cryfs/filesystem/fsblobstore/FsBlob.h index a9b38071..9178134a 100644 --- a/src/cryfs/filesystem/fsblobstore/FsBlob.h +++ b/src/cryfs/filesystem/fsblobstore/FsBlob.h @@ -4,6 +4,7 @@ #include #include +#include "FsBlobView.h" namespace cryfs { namespace fsblobstore { @@ -17,20 +18,17 @@ namespace cryfs { protected: FsBlob(cpputils::unique_ref baseBlob); - blobstore::Blob &baseBlob(); - const blobstore::Blob &baseBlob() const; + FsBlobView &baseBlob(); + const FsBlobView &baseBlob() const; - unsigned char magicNumber() const; - static unsigned char magicNumber(const blobstore::Blob &blob); - - static void InitializeBlobWithMagicNumber(blobstore::Blob *blob, unsigned char magicNumber); + static void InitializeBlob(blobstore::Blob *blob, FsBlobView::BlobType magicNumber); friend class FsBlobStore; virtual cpputils::unique_ref releaseBaseBlob(); private: - cpputils::unique_ref _baseBlob; + FsBlobView _baseBlob; DISALLOW_COPY_AND_ASSIGN(FsBlob); }; @@ -48,34 +46,23 @@ namespace cryfs { } inline const blockstore::Key &FsBlob::key() const { - return _baseBlob->key(); + return _baseBlob.key(); } - inline const blobstore::Blob &FsBlob::baseBlob() const { - return *_baseBlob; + inline const FsBlobView &FsBlob::baseBlob() const { + return _baseBlob; } - inline blobstore::Blob &FsBlob::baseBlob() { - return *_baseBlob; + inline FsBlobView &FsBlob::baseBlob() { + return _baseBlob; } - inline unsigned char FsBlob::magicNumber(const blobstore::Blob &blob) { - unsigned char value; - blob.read(&value, 0, 1); - return value; - } - - inline unsigned char FsBlob::magicNumber() const { - return magicNumber(*_baseBlob); - } - - inline void FsBlob::InitializeBlobWithMagicNumber(blobstore::Blob *blob, unsigned char magicNumber) { - blob->resize(1); - blob->write(&magicNumber, 0, 1); + inline void FsBlob::InitializeBlob(blobstore::Blob *blob, FsBlobView::BlobType magicNumber) { + FsBlobView::InitializeBlob(blob, magicNumber); } inline cpputils::unique_ref FsBlob::releaseBaseBlob() { - return std::move(_baseBlob); + return _baseBlob.releaseBaseBlob(); } } } diff --git a/src/cryfs/filesystem/fsblobstore/FsBlobStore.cpp b/src/cryfs/filesystem/fsblobstore/FsBlobStore.cpp index 7ed5c088..d24f2df9 100644 --- a/src/cryfs/filesystem/fsblobstore/FsBlobStore.cpp +++ b/src/cryfs/filesystem/fsblobstore/FsBlobStore.cpp @@ -2,7 +2,6 @@ #include "FileBlob.h" #include "DirBlob.h" #include "SymlinkBlob.h" -#include "MagicNumbers.h" namespace bf = boost::filesystem; using cpputils::unique_ref; @@ -38,12 +37,12 @@ boost::optional> FsBlobStore::load(const blockstore::Key &key if (blob == none) { return none; } - unsigned char magicNumber = FsBlob::magicNumber(**blob); - if (magicNumber == MagicNumbers::FILE) { + FsBlobView::BlobType blobType = FsBlobView::blobType(**blob); + if (blobType == FsBlobView::BlobType::FILE) { return unique_ref(make_unique_ref(std::move(*blob))); - } else if (magicNumber == MagicNumbers::DIR) { + } else if (blobType == FsBlobView::BlobType::DIR) { return unique_ref(make_unique_ref(std::move(*blob), _getLstatSize())); - } else if (magicNumber == MagicNumbers::SYMLINK) { + } else if (blobType == FsBlobView::BlobType::SYMLINK) { return unique_ref(make_unique_ref(std::move(*blob))); } else { ASSERT(false, "Unknown magic number"); diff --git a/src/cryfs/filesystem/fsblobstore/FsBlobView.cpp b/src/cryfs/filesystem/fsblobstore/FsBlobView.cpp new file mode 100644 index 00000000..c9cb7fbb --- /dev/null +++ b/src/cryfs/filesystem/fsblobstore/FsBlobView.cpp @@ -0,0 +1,5 @@ +#include "FsBlobView.h" + +namespace cryfs { + constexpr uint16_t FsBlobView::FORMAT_VERSION_HEADER; +} \ No newline at end of file diff --git a/src/cryfs/filesystem/fsblobstore/FsBlobView.h b/src/cryfs/filesystem/fsblobstore/FsBlobView.h new file mode 100644 index 00000000..12bd160b --- /dev/null +++ b/src/cryfs/filesystem/fsblobstore/FsBlobView.h @@ -0,0 +1,105 @@ +#pragma once +#ifndef MESSMER_CRYFS_FILESYSTEM_FSBLOBSTORE_FSBLOBVIEW_H +#define MESSMER_CRYFS_FILESYSTEM_FSBLOBSTORE_FSBLOBVIEW_H + +#include +#include + +namespace cryfs { + + //TODO Test + class FsBlobView final : public blobstore::Blob { + public: + //TODO Rename to "Type" or similar + enum class BlobType : uint8_t { + DIR = 0x00, + FILE = 0x01, + SYMLINK = 0x02 + }; + + FsBlobView(cpputils::unique_ref baseBlob): _baseBlob(std::move(baseBlob)) { + _checkHeader(*_baseBlob); + } + + static void InitializeBlob(blobstore::Blob *baseBlob, BlobType blobType) { + baseBlob->resize(sizeof(FORMAT_VERSION_HEADER) + 1); + baseBlob->write(&FORMAT_VERSION_HEADER, 0, sizeof(FORMAT_VERSION_HEADER)); + uint8_t blobTypeInt = static_cast(blobType); + baseBlob->write(&blobTypeInt, sizeof(FORMAT_VERSION_HEADER), 1); + } + + static BlobType blobType(const blobstore::Blob &blob) { + _checkHeader(blob); + return _blobType(blob); + } + + BlobType blobType() const { + return _blobType(*_baseBlob); + } + + const blockstore::Key &key() const override { + return _baseBlob->key(); + } + + uint64_t size() const override { + return _baseBlob->size() - sizeof(FORMAT_VERSION_HEADER) - 1; + } + + void resize(uint64_t numBytes) override { + return _baseBlob->resize(numBytes + sizeof(FORMAT_VERSION_HEADER) + 1); + } + + cpputils::Data readAll() const override { + cpputils::Data data = _baseBlob->readAll(); + cpputils::Data dataWithoutHeader(data.size() - sizeof(FORMAT_VERSION_HEADER) - 1); + std::memcpy(dataWithoutHeader.data(), data.dataOffset(sizeof(FORMAT_VERSION_HEADER) + 1), dataWithoutHeader.size()); + return dataWithoutHeader; + } + + void read(void *target, uint64_t offset, uint64_t size) const override { + return _baseBlob->read(target, offset + sizeof(FORMAT_VERSION_HEADER) + 1, size); + } + + uint64_t tryRead(void *target, uint64_t offset, uint64_t size) const override { + return _baseBlob->tryRead(target, offset + sizeof(FORMAT_VERSION_HEADER) + 1, size); + } + + void write(const void *source, uint64_t offset, uint64_t size) override { + return _baseBlob->write(source, offset + sizeof(FORMAT_VERSION_HEADER) + 1, size); + } + + void flush() override { + return _baseBlob->flush(); + } + + cpputils::unique_ref releaseBaseBlob() { + return std::move(_baseBlob); + } + + private: + static constexpr uint16_t FORMAT_VERSION_HEADER = 0; + + static void _checkHeader(const blobstore::Blob &blob) { + static_assert(sizeof(uint16_t) == sizeof(FORMAT_VERSION_HEADER), "Wrong type used to read format version header"); + uint16_t actualFormatVersion; + blob.read(&actualFormatVersion, 0, sizeof(FORMAT_VERSION_HEADER)); + if (FORMAT_VERSION_HEADER != actualFormatVersion) { + throw std::runtime_error("This file system entity has the wrong format. Was it created with a newer version of CryFS?"); + } + } + + static BlobType _blobType(const blobstore::Blob &blob) { + uint8_t result; + blob.read(&result, sizeof(FORMAT_VERSION_HEADER), 1); + return static_cast(result); + } + + cpputils::unique_ref _baseBlob; + + DISALLOW_COPY_AND_ASSIGN(FsBlobView); + }; + +} + + +#endif diff --git a/src/cryfs/filesystem/fsblobstore/MagicNumbers.h b/src/cryfs/filesystem/fsblobstore/MagicNumbers.h deleted file mode 100644 index 44c2d263..00000000 --- a/src/cryfs/filesystem/fsblobstore/MagicNumbers.h +++ /dev/null @@ -1,20 +0,0 @@ -#pragma once -#ifndef MESSMER_CRYFS_FILESYSTEM_FSBLOBSTORE_MAGICNUMBERS_H_ -#define MESSMER_CRYFS_FILESYSTEM_FSBLOBSTORE_MAGICNUMBERS_H_ - -namespace cryfs { -namespace fsblobstore { - -//TODO enum class -enum MagicNumbers { - DIR = 0x00, - FILE = 0x01, - SYMLINK = 0x02 -}; - -} -} - - - -#endif diff --git a/src/cryfs/filesystem/fsblobstore/SymlinkBlob.cpp b/src/cryfs/filesystem/fsblobstore/SymlinkBlob.cpp index 459b7d5f..9dee2a81 100644 --- a/src/cryfs/filesystem/fsblobstore/SymlinkBlob.cpp +++ b/src/cryfs/filesystem/fsblobstore/SymlinkBlob.cpp @@ -1,6 +1,5 @@ #include "SymlinkBlob.h" -#include "MagicNumbers.h" #include #include @@ -16,29 +15,22 @@ namespace fsblobstore { SymlinkBlob::SymlinkBlob(unique_ref blob) : FsBlob(std::move(blob)), _target(_readTargetFromBlob(baseBlob())) { + ASSERT(baseBlob().blobType() == FsBlobView::BlobType::SYMLINK, "Loaded blob is not a symlink"); } unique_ref SymlinkBlob::InitializeSymlink(unique_ref blob, const bf::path &target) { + InitializeBlob(blob.get(), FsBlobView::BlobType::SYMLINK); + FsBlobView symlinkBlobView(std::move(blob)); string targetStr = target.native(); - blob->resize(1 + targetStr.size()); - unsigned char magicNumber = MagicNumbers::SYMLINK; - blob->write(&magicNumber, 0, 1); - blob->write(targetStr.c_str(), 1, targetStr.size()); - return make_unique_ref(std::move(blob)); + symlinkBlobView.resize(targetStr.size()); + symlinkBlobView.write(targetStr.c_str(), 0, targetStr.size()); + return make_unique_ref(symlinkBlobView.releaseBaseBlob()); } -void SymlinkBlob::_checkMagicNumber(const Blob &blob) { - unsigned char value; - blob.read(&value, 0, 1); - ASSERT(value == MagicNumbers::SYMLINK, "Blob is not a symlink blob"); -} - -bf::path SymlinkBlob::_readTargetFromBlob(const blobstore::Blob &blob) { - _checkMagicNumber(blob); - size_t targetStrSize = blob.size() - 1; // -1 because of the magic number - char targetStr[targetStrSize + 1]; // +1 because of the nullbyte - blob.read(targetStr, 1, targetStrSize); - targetStr[targetStrSize] = '\0'; +bf::path SymlinkBlob::_readTargetFromBlob(const FsBlobView &blob) { + char targetStr[blob.size() + 1]; // +1 because of the nullbyte + blob.read(targetStr, 0, blob.size()); + targetStr[blob.size()] = '\0'; return targetStr; } diff --git a/src/cryfs/filesystem/fsblobstore/SymlinkBlob.h b/src/cryfs/filesystem/fsblobstore/SymlinkBlob.h index 66b27175..19186b4b 100644 --- a/src/cryfs/filesystem/fsblobstore/SymlinkBlob.h +++ b/src/cryfs/filesystem/fsblobstore/SymlinkBlob.h @@ -22,9 +22,7 @@ namespace cryfs { private: boost::filesystem::path _target; - static void _checkMagicNumber(const blobstore::Blob &blob); - - static boost::filesystem::path _readTargetFromBlob(const blobstore::Blob &blob); + static boost::filesystem::path _readTargetFromBlob(const FsBlobView &blob); DISALLOW_COPY_AND_ASSIGN(SymlinkBlob); }; diff --git a/test/blockstore/implementations/ondisk/OnDiskBlockTest/OnDiskBlockCreateTest.cpp b/test/blockstore/implementations/ondisk/OnDiskBlockTest/OnDiskBlockCreateTest.cpp index 5de0ede1..663899df 100644 --- a/test/blockstore/implementations/ondisk/OnDiskBlockTest/OnDiskBlockCreateTest.cpp +++ b/test/blockstore/implementations/ondisk/OnDiskBlockTest/OnDiskBlockCreateTest.cpp @@ -62,12 +62,14 @@ INSTANTIATE_TEST_CASE_P(OnDiskBlockCreateSizeTest, OnDiskBlockCreateSizeTest, Va TEST_P(OnDiskBlockCreateSizeTest, OnDiskSizeIsCorrect) { Data fileContent = Data::LoadFromFile(file.path()).value(); - EXPECT_EQ(GetParam(), fileContent.size()); + EXPECT_EQ(GetParam() + OnDiskBlock::formatVersionHeaderSize(), fileContent.size()); } TEST_P(OnDiskBlockCreateSizeTest, OnDiskBlockIsZeroedOut) { Data fileContent = Data::LoadFromFile(file.path()).value(); - EXPECT_EQ(ZEROES, fileContent); + Data fileContentWithoutHeader(fileContent.size() - OnDiskBlock::formatVersionHeaderSize()); + std::memcpy(fileContentWithoutHeader.data(), fileContent.dataOffset(OnDiskBlock::formatVersionHeaderSize()), fileContentWithoutHeader.size()); + EXPECT_EQ(ZEROES, fileContentWithoutHeader); } // This test is also tested by OnDiskBlockStoreTest, but there the block is created using the BlockStore interface. diff --git a/test/blockstore/implementations/ondisk/OnDiskBlockTest/OnDiskBlockFlushTest.cpp b/test/blockstore/implementations/ondisk/OnDiskBlockTest/OnDiskBlockFlushTest.cpp index 6300e034..e49856ae 100644 --- a/test/blockstore/implementations/ondisk/OnDiskBlockTest/OnDiskBlockFlushTest.cpp +++ b/test/blockstore/implementations/ondisk/OnDiskBlockTest/OnDiskBlockFlushTest.cpp @@ -56,8 +56,10 @@ public: } void EXPECT_STORED_FILE_DATA_CORRECT() { - Data actual = Data::LoadFromFile(file.path()).value(); - EXPECT_EQ(randomData, actual); + Data fileContent = Data::LoadFromFile(file.path()).value(); + Data fileContentWithoutHeader(fileContent.size() - OnDiskBlock::formatVersionHeaderSize()); + std::memcpy(fileContentWithoutHeader.data(), fileContent.dataOffset(OnDiskBlock::formatVersionHeaderSize()), fileContentWithoutHeader.size()); + EXPECT_EQ(randomData, fileContentWithoutHeader); } }; INSTANTIATE_TEST_CASE_P(OnDiskBlockFlushTest, OnDiskBlockFlushTest, Values((size_t)0, (size_t)1, (size_t)1024, (size_t)4096, (size_t)10*1024*1024)); diff --git a/test/blockstore/implementations/ondisk/OnDiskBlockTest/OnDiskBlockLoadTest.cpp b/test/blockstore/implementations/ondisk/OnDiskBlockTest/OnDiskBlockLoadTest.cpp index a73d9a20..c032e28a 100644 --- a/test/blockstore/implementations/ondisk/OnDiskBlockTest/OnDiskBlockLoadTest.cpp +++ b/test/blockstore/implementations/ondisk/OnDiskBlockTest/OnDiskBlockLoadTest.cpp @@ -31,19 +31,19 @@ public: OnDiskBlockLoadTest(): dir(), key(Key::FromString("1491BB4932A389EE14BC7090AC772972")), - file(dir.path() / key.ToString()) { + file(dir.path() / key.ToString(), false) { } TempDir dir; Key key; TempFile file; - void SetFileSize(size_t size) { + void CreateBlockWithSize(size_t size) { Data data(size); - data.StoreToFile(file.path()); + OnDiskBlock::CreateOnDisk(dir.path(), key, std::move(data)); } - void StoreData(const Data &data) { - data.StoreToFile(file.path()); + void StoreData(Data data) { + OnDiskBlock::CreateOnDisk(dir.path(), key, std::move(data)); } unique_ref LoadBlock() { @@ -57,8 +57,8 @@ public: }; INSTANTIATE_TEST_CASE_P(OnDiskBlockLoadTest, OnDiskBlockLoadTest, Values(0, 1, 5, 1024, 10*1024*1024)); -TEST_P(OnDiskBlockLoadTest, FileSizeIsCorrect) { - SetFileSize(GetParam()); +TEST_P(OnDiskBlockLoadTest, LoadsCorrectSize) { + CreateBlockWithSize(GetParam()); auto block = LoadBlock(); @@ -67,7 +67,7 @@ TEST_P(OnDiskBlockLoadTest, FileSizeIsCorrect) { TEST_P(OnDiskBlockLoadTest, LoadedDataIsCorrect) { Data randomData = DataFixture::generate(GetParam()); - StoreData(randomData); + StoreData(randomData.copy()); auto block = LoadBlock(); diff --git a/test/cryfs/cli/CliTest_Setup.cpp b/test/cryfs/cli/CliTest_Setup.cpp index c7023fe5..fcf7ae97 100644 --- a/test/cryfs/cli/CliTest_Setup.cpp +++ b/test/cryfs/cli/CliTest_Setup.cpp @@ -6,34 +6,34 @@ using cpputils::TempFile; using CliTest_Setup = CliTest; TEST_F(CliTest_Setup, NoSpecialOptions) { - //Specify --cipher and --extpass parameters to make it non-interactive + //Specify --cipher parameter to make it non-interactive //TODO Remove "-f" parameter, once EXPECT_RUN_SUCCESS can handle that - EXPECT_RUN_SUCCESS({basedir.c_str(), mountdir.c_str(), "--cipher", "aes-256-gcm", "--extpass", "echo mypassword", "-f"}, mountdir); + EXPECT_RUN_SUCCESS({basedir.c_str(), mountdir.c_str(), "--cipher", "aes-256-gcm", "-f"}, mountdir); } TEST_F(CliTest_Setup, NotexistingLogfileGiven) { TempFile notexisting_logfile(false); - //Specify --cipher and --extpass parameters to make it non-interactive + //Specify --cipher parameter to make it non-interactive //TODO Remove "-f" parameter, once EXPECT_RUN_SUCCESS can handle that - EXPECT_RUN_SUCCESS({basedir.c_str(), mountdir.c_str(), "-f", "--cipher", "aes-256-gcm", "--extpass", "echo mypassword", "--logfile", notexisting_logfile.path().c_str()}, mountdir); + EXPECT_RUN_SUCCESS({basedir.c_str(), mountdir.c_str(), "-f", "--cipher", "aes-256-gcm", "--logfile", notexisting_logfile.path().c_str()}, mountdir); //TODO Expect logfile is used (check logfile content) } TEST_F(CliTest_Setup, ExistingLogfileGiven) { - //Specify --cipher and --extpass parameters to make it non-interactive + //Specify --cipher parameter to make it non-interactive //TODO Remove "-f" parameter, once EXPECT_RUN_SUCCESS can handle that - EXPECT_RUN_SUCCESS({basedir.c_str(), mountdir.c_str(), "-f", "--cipher", "aes-256-gcm", "--extpass", "echo mypassword", "--logfile", logfile.path().c_str()}, mountdir); + EXPECT_RUN_SUCCESS({basedir.c_str(), mountdir.c_str(), "-f", "--cipher", "aes-256-gcm", "--logfile", logfile.path().c_str()}, mountdir); //TODO Expect logfile is used (check logfile content) } TEST_F(CliTest_Setup, ConfigfileGiven) { - //Specify --cipher and --extpass parameters to make it non-interactive + //Specify --cipher parameter to make it non-interactive //TODO Remove "-f" parameter, once EXPECT_RUN_SUCCESS can handle that - EXPECT_RUN_SUCCESS({basedir.c_str(), mountdir.c_str(), "-f", "--cipher", "aes-256-gcm", "--extpass", "echo mypassword", "--config", configfile.path().c_str()}, mountdir); + EXPECT_RUN_SUCCESS({basedir.c_str(), mountdir.c_str(), "-f", "--cipher", "aes-256-gcm", "--config", configfile.path().c_str()}, mountdir); } TEST_F(CliTest_Setup, FuseOptionGiven) { - //Specify --cipher and --extpass parameters to make it non-interactive + //Specify --cipher parameter to make it non-interactive //TODO Remove "-f" parameter, once EXPECT_RUN_SUCCESS can handle that - EXPECT_RUN_SUCCESS({basedir.c_str(), mountdir.c_str(), "-f", "--cipher", "aes-256-gcm", "--extpass", "echo mypassword", "--", "-f"}, mountdir); + EXPECT_RUN_SUCCESS({basedir.c_str(), mountdir.c_str(), "-f", "--cipher", "aes-256-gcm", "--", "-f"}, mountdir); } \ No newline at end of file diff --git a/test/cryfs/cli/CliTest_WrongEnvironment.cpp b/test/cryfs/cli/CliTest_WrongEnvironment.cpp index 8a4b7526..63d0e5eb 100644 --- a/test/cryfs/cli/CliTest_WrongEnvironment.cpp +++ b/test/cryfs/cli/CliTest_WrongEnvironment.cpp @@ -60,11 +60,9 @@ public: if (GetParam().runningInForeground) { result.push_back("-f"); } - // Test case should be non-interactive, so don't ask for cipher or password. + // Test case should be non-interactive, so don't ask for cipher. result.push_back("--cipher"); result.push_back("aes-256-gcm"); - result.push_back("--extpass"); - result.push_back("echo mypassword"); return result; } }; diff --git a/test/cryfs/cli/testutils/CliTest.h b/test/cryfs/cli/testutils/CliTest.h index af6a29cd..85a3ee49 100644 --- a/test/cryfs/cli/testutils/CliTest.h +++ b/test/cryfs/cli/testutils/CliTest.h @@ -39,6 +39,10 @@ public: _args.push_back(const_cast(arg)); } auto &keyGenerator = cpputils::Random::PseudoRandom(); + // Write 2x 'pass\n' to stdin so Cryfs can read it as password (+ password confirmation prompt) + 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()); }