Remember hashed filesystem key in local state so attacker can't replace it

src/cpp-utils/CMakeLists.txt

@@ -7,6 +7,7 @@ set(SOURCES
         crypto/kdf/PasswordBasedKDF.cpp
         crypto/RandomPadding.cpp
         crypto/symmetric/EncryptionKey.cpp
+        crypto/hash/Hash.cpp
         process/daemonize.cpp
         process/subprocess.cpp
         tempfile/TempFile.cpp

src/cpp-utils/crypto/hash/Hash.cpp

@@ -0,0 +1,30 @@
+#include "Hash.h"
+#include <cpp-utils/random/Random.h>
+#include <cryptopp/sha.h>
+
+using cpputils::Random;
+using CryptoPP::SHA512;
+
+namespace cpputils {
+namespace hash {
+
+Hash hash(const Data& data, Salt salt) {
+  SHA512 hasher;
+  hasher.Update((CryptoPP::byte*)salt.data(), Salt::BINARY_LENGTH);
+  hasher.Update((CryptoPP::byte*)data.data(), data.size());
+
+  Digest digest = Digest::Null();
+  hasher.Final((CryptoPP::byte*)digest.data());
+
+  return Hash{
+      .digest = std::move(digest),
+      .salt = std::move(salt)
+  };
+}
+
+Salt generateSalt() {
+  return Random::PseudoRandom().getFixedSize<8>();
+}
+
+}
+}

src/cpp-utils/crypto/hash/Hash.h

@@ -0,0 +1,28 @@
+#pragma once
+#ifndef MESSMER_CPPUTILS_CRYPTO_HASH_HASH_H
+#define MESSMER_CPPUTILS_CRYPTO_HASH_HASH_H
+
+#include <cpp-utils/data/FixedSizeData.h>
+#include <cpp-utils/data/Data.h>
+
+namespace cpputils {
+namespace hash {
+
+using Digest = FixedSizeData<64>;
+using Salt = FixedSizeData<8>;
+
+struct Hash final {
+  Digest digest;
+  Salt salt;
+};
+
+
+Salt generateSalt();
+Hash hash(const cpputils::Data& data, Salt salt);
+
+
+}
+}
+
+
+#endif

src/cpp-utils/data/Data.cpp

@@ -1,5 +1,7 @@
 #include "Data.h"
 #include <stdexcept>
+#include <cryptopp/hex.h>
+#include <cpp-utils/crypto/cryptopp_byte.h>
 
 using std::istream;
 using std::ofstream;
@@ -42,4 +44,26 @@ Data Data::LoadFromStream(istream &stream, size_t size) {
   return result;
 }
 
+Data Data::FromString(const std::string &data) {
+  ASSERT(data.size() % 2 == 0, "hex encoded data cannot have odd number of characters");
+  Data result(data.size() / 2);
+  CryptoPP::StringSource(data, true,
+    new CryptoPP::HexDecoder(
+      new CryptoPP::ArraySink((CryptoPP::byte*)result._data, result.size())
+    )
+  );
+  return result;
+}
+
+std::string Data::ToString() const {
+  std::string result;
+  CryptoPP::ArraySource((CryptoPP::byte*)_data, _size, true,
+    new CryptoPP::HexEncoder(
+        new CryptoPP::StringSink(result)
+    )
+  );
+  ASSERT(result.size() == 2 * _size, "Created wrongly sized string");
+  return result;
+}
+
 }

src/cpp-utils/data/Data.h

@@ -45,6 +45,10 @@ public:
   static Data LoadFromStream(std::istream &stream, size_t size);
   void StoreToStream(std::ostream &stream) const;
 
+  // TODO Unify ToString/FromString functions from Data/FixedSizeData using free functions
+  static Data FromString(const std::string &data);
+  std::string ToString() const;
+
 private:
   size_t _size;
   void *_data;

src/cryfs/config/CryConfigCreator.cpp

@@ -28,10 +28,11 @@ namespace cryfs {
         config.SetCreatedWithVersion(gitversion::VersionString());
         config.SetBlocksizeBytes(_generateBlocksizeBytes(blocksizeBytesFromCommandLine));
         config.SetRootBlob(_generateRootBlobId());
-        config.SetEncryptionKey(_generateEncKey(config.Cipher()));
         config.SetFilesystemId(_generateFilesystemID());
-        auto localState = LocalStateMetadata::loadOrGenerate(LocalStateDir::forFilesystemId(config.FilesystemId()));
+        auto encryptionKey = _generateEncKey(config.Cipher());
+        auto localState = LocalStateMetadata::loadOrGenerate(LocalStateDir::forFilesystemId(config.FilesystemId()), cpputils::Data::FromString(encryptionKey));
         uint32_t myClientId = localState.myClientId();
+        config.SetEncryptionKey(std::move(encryptionKey));
         config.SetExclusiveClientId(_generateExclusiveClientId(missingBlockIsIntegrityViolationFromCommandLine, myClientId));
 #ifndef CRYFS_NO_COMPATIBILITY
         config.SetHasVersionNumbers(true);

src/cryfs/config/CryConfigLoader.cpp

@@ -57,7 +57,7 @@ optional<CryConfigLoader::ConfigLoadResult> CryConfigLoader::_loadConfig(const b
     config->save();
   }
   _checkCipher(*config->config());
-  auto localState = LocalStateMetadata::loadOrGenerate(LocalStateDir::forFilesystemId(config->config()->FilesystemId()));
+  auto localState = LocalStateMetadata::loadOrGenerate(LocalStateDir::forFilesystemId(config->config()->FilesystemId()), cpputils::Data::FromString(config->config()->EncryptionKey()));
   uint32_t myClientId = localState.myClientId();
   _checkMissingBlocksAreIntegrityViolations(&*config, myClientId);
   return ConfigLoadResult {std::move(*config), myClientId};

src/cryfs/localstate/LocalStateMetadata.cpp

@@ -14,23 +14,30 @@ using std::ifstream;
 using std::ofstream;
 using std::istream;
 using std::ostream;
-using cpputils::Random;
+using std::string;
 using blockstore::integrity::KnownBlockVersions;
+using cpputils::hash::Hash;
+using cpputils::Data;
+using cpputils::Random;
 namespace bf = boost::filesystem;
 
 namespace cryfs {
 
-LocalStateMetadata::LocalStateMetadata(uint32_t myClientId)
+LocalStateMetadata::LocalStateMetadata(uint32_t myClientId, Hash encryptionKeyHash)
-: _myClientId(myClientId) {}
+: _myClientId(myClientId), _encryptionKeyHash(std::move(encryptionKeyHash)) {}
 
-LocalStateMetadata LocalStateMetadata::loadOrGenerate(const bf::path &statePath) {
+LocalStateMetadata LocalStateMetadata::loadOrGenerate(const bf::path &statePath, const Data& encryptionKey) {
   auto metadataFile = statePath / "metadata";
   auto loaded = _load(metadataFile);
-  if (loaded != none) {
+  if (loaded == none) {
-      return *loaded;
+    // If it couldn't be loaded, generate a new client id.
+    return _generate(metadataFile, encryptionKey);
   }
-  // If it couldn't be loaded, generate a new client id.
+
-  return _generate(metadataFile);
+  if (loaded->_encryptionKeyHash.digest != cpputils::hash::hash(encryptionKey, loaded->_encryptionKeyHash.salt).digest) {
+    throw std::runtime_error("The filesystem encryption key differs from the last time we loaded this filesystem. Did an attacker replace the file system?");
+  }
+  return *loaded;
 }
 
 optional<LocalStateMetadata> LocalStateMetadata::_load(const bf::path &metadataFilePath) {
@@ -73,7 +80,7 @@ optional<uint32_t> _tryLoadClientIdFromLegacyFile(const bf::path &metadataFilePa
 #endif
 }
 
-LocalStateMetadata LocalStateMetadata::_generate(const bf::path &metadataFilePath) {
+LocalStateMetadata LocalStateMetadata::_generate(const bf::path &metadataFilePath, const Data& encryptionKey) {
   uint32_t myClientId = _generateClientId();
 #ifndef CRYFS_NO_COMPATIBILITY
   // In the old format, this was stored in a "myClientId" file. If that file exists, load it from there.
@@ -83,7 +90,7 @@ LocalStateMetadata LocalStateMetadata::_generate(const bf::path &metadataFilePat
   }
 #endif
 
-  LocalStateMetadata result(myClientId);
+  LocalStateMetadata result(myClientId, cpputils::hash::hash(encryptionKey, cpputils::hash::generateSalt()));
   result._save(metadataFilePath);
   return result;
 }
@@ -91,6 +98,8 @@ LocalStateMetadata LocalStateMetadata::_generate(const bf::path &metadataFilePat
 void LocalStateMetadata::_serialize(ostream& stream) const {
   ptree pt;
   pt.put<uint32_t>("myClientId", myClientId());
+  pt.put<string>("encryptionKey.salt", _encryptionKeyHash.salt.ToString());
+  pt.put<string>("encryptionKey.hash", _encryptionKeyHash.digest.ToString());
 
   write_json(stream, pt);
 }
@@ -100,9 +109,13 @@ LocalStateMetadata LocalStateMetadata::_deserialize(istream& stream) {
   read_json(stream, pt);
 
   uint32_t myClientId = pt.get<uint32_t>("myClientId");
+  string encryptionKeySalt = pt.get<string>("encryptionKey.salt");
+  string encryptionKeyDigest = pt.get<string>("encryptionKey.hash");
 
-  return LocalStateMetadata(myClientId);
+  return LocalStateMetadata(myClientId, Hash{
+      .digest = cpputils::hash::Digest::FromString(std::move(encryptionKeyDigest)),
+      .salt = cpputils::hash::Salt::FromString(std::move(encryptionKeySalt))
+  });
 }
 
-
 }

src/cryfs/localstate/LocalStateMetadata.h

@@ -5,26 +5,28 @@
 #include <boost/filesystem/path.hpp>
 #include <boost/optional.hpp>
 #include <iostream>
+#include <cpp-utils/crypto/hash/Hash.h>
 
 namespace cryfs {
 
 class LocalStateMetadata final {
 public:
 
-  static LocalStateMetadata loadOrGenerate(const boost::filesystem::path &statePath);
+  static LocalStateMetadata loadOrGenerate(const boost::filesystem::path &statePath, const cpputils::Data& encryptionKey);
 
   uint32_t myClientId() const;
 
 private:
-  LocalStateMetadata(uint32_t myClientId);
+  const uint32_t _myClientId;
+  const cpputils::hash::Hash _encryptionKeyHash;
 
   static boost::optional<LocalStateMetadata> _load(const boost::filesystem::path &metadataFilePath);
   static LocalStateMetadata _deserialize(std::istream& stream);
-  static LocalStateMetadata _generate(const boost::filesystem::path &metadataFilePath);
+  static LocalStateMetadata _generate(const boost::filesystem::path &metadataFilePath, const cpputils::Data& encryptionKey);
   void _save(const boost::filesystem::path &metadataFilePath) const;
   void _serialize(std::ostream& stream) const;
 
-  const uint32_t _myClientId;
+  LocalStateMetadata(uint32_t myClientId, cpputils::hash::Hash encryptionKey);
 };
 
 inline uint32_t LocalStateMetadata::myClientId() const {

test/cpp-utils/CMakeLists.txt

@@ -6,6 +6,7 @@ set(SOURCES
     crypto/symmetric/testutils/FakeAuthenticatedCipher.cpp
     crypto/kdf/SCryptTest.cpp
     crypto/kdf/SCryptParametersTest.cpp
+    crypto/hash/HashTest.cpp
     MacrosIncludeTest.cpp
     pointer/unique_ref_test.cpp
     pointer/cast_include_test.cpp

test/cpp-utils/crypto/hash/HashTest.cpp

@@ -0,0 +1,45 @@
+#include <gtest/gtest.h>
+#include <cpp-utils/crypto/hash/Hash.h>
+#include <cpp-utils/data/DataFixture.h>
+
+using namespace cpputils::hash;
+using cpputils::DataFixture;
+using cpputils::Data;
+
+TEST(HashTest, generateSalt_isIndeterministic) {
+  EXPECT_NE(generateSalt(), generateSalt());
+}
+
+TEST(HashTest, hash_setsSaltCorrectly) {
+  Salt salt = generateSalt();
+  Data data = DataFixture::generate(1024);
+  EXPECT_EQ(salt, hash(data, salt).salt);
+}
+
+TEST(HashTest, hash_isDeterministicWithSameDataSameSalt) {
+  Salt salt = generateSalt();
+  Data data = DataFixture::generate(1024);
+  EXPECT_EQ(hash(data, salt).digest, hash(data, salt).digest);
+}
+
+TEST(HashTest, hash_isIndeterministicWithSameDataDifferentSalt) {
+  Salt salt1 = generateSalt();
+  Salt salt2 = generateSalt();
+  Data data = DataFixture::generate(1024);
+  EXPECT_NE(hash(data, salt1).digest, hash(data, salt2).digest);
+}
+
+TEST(HashTest, hash_isIndeterministicWithDifferentDataSameSalt) {
+  Salt salt = generateSalt();
+  Data data1 = DataFixture::generate(1024, 1);
+  Data data2 = DataFixture::generate(1024, 2);
+  EXPECT_NE(hash(data1, salt).digest, hash(data2, salt).digest);
+}
+
+TEST(HashTest, hash_isIndeterministicWithDifferentDataDifferentSalt) {
+  Salt salt1 = generateSalt();
+  Salt salt2 = generateSalt();
+  Data data1 = DataFixture::generate(1024, 1);
+  Data data2 = DataFixture::generate(1024, 2);
+  EXPECT_NE(hash(data1, salt1).digest, hash(data2, salt2).digest);
+}

test/cpp-utils/data/DataTest.cpp

@@ -14,6 +14,7 @@ using cpputils::TempFile;
 
 using std::ifstream;
 using std::ofstream;
+using std::string;
 
 namespace bf = boost::filesystem;
 
@@ -206,3 +207,17 @@ TEST_F(DataTest, LoadingNonexistingFile) {
   TempFile file(false); // Pass false to constructor, so the tempfile is not created
   EXPECT_FALSE(Data::LoadFromFile(file.path()));
 }
+
+class DataTestWithStringParam: public DataTest, public WithParamInterface<string> {};
+INSTANTIATE_TEST_CASE_P(DataTestWithStringParam, DataTestWithStringParam, Values("", "2898B4B8A13C0F0278CCE465DB", "6FFEBAD90C0DAA2B79628F0627CE9841"));
+
+TEST_P(DataTestWithStringParam, FromAndToString) {
+  Data data = Data::FromString(GetParam());
+  EXPECT_EQ(GetParam(), data.ToString());
+}
+
+TEST_P(DataTestWithStringParam, ToAndFromString) {
+  Data data = Data::FromString(GetParam());
+  Data data2 = Data::FromString(data.ToString());
+  EXPECT_EQ(data, data2);
+}

test/cryfs/localstate/LocalStateMetadataTest.cpp

@@ -3,9 +3,12 @@
 #include <cryfs/localstate/LocalStateMetadata.h>
 #include <cpp-utils/tempfile/TempDir.h>
 #include <fstream>
+#include <cpp-utils/data/DataFixture.h>
 
 using cpputils::TempDir;
+using cpputils::Data;
 using cryfs::LocalStateMetadata;
+using cpputils::DataFixture;
 using std::ofstream;
 
 class LocalStateMetadataTest : public ::testing::Test {
@@ -15,14 +18,14 @@ public:
 };
 
 TEST_F(LocalStateMetadataTest, myClientId_ValueIsConsistent) {
-    LocalStateMetadata metadata1 = LocalStateMetadata::loadOrGenerate(stateDir.path());
+    LocalStateMetadata metadata1 = LocalStateMetadata::loadOrGenerate(stateDir.path(), Data(0));
-    LocalStateMetadata metadata2 = LocalStateMetadata::loadOrGenerate(stateDir.path());
+    LocalStateMetadata metadata2 = LocalStateMetadata::loadOrGenerate(stateDir.path(), Data(0));
     EXPECT_EQ(metadata1.myClientId(), metadata2.myClientId());
 }
 
 TEST_F(LocalStateMetadataTest, myClientId_ValueIsRandomForNewClient) {
-    LocalStateMetadata metadata1 = LocalStateMetadata::loadOrGenerate(stateDir.path());
+    LocalStateMetadata metadata1 = LocalStateMetadata::loadOrGenerate(stateDir.path(), Data(0));
-    LocalStateMetadata metadata2 = LocalStateMetadata::loadOrGenerate(stateDir2.path());
+    LocalStateMetadata metadata2 = LocalStateMetadata::loadOrGenerate(stateDir2.path(), Data(0));
     EXPECT_NE(metadata1.myClientId(), metadata2.myClientId());
 }
 
@@ -32,7 +35,20 @@ TEST_F(LocalStateMetadataTest, myClientId_TakesLegacyValueIfSpecified) {
   file << 12345u;
   file.close();
 
-  LocalStateMetadata metadata = LocalStateMetadata::loadOrGenerate(stateDir.path());
+  LocalStateMetadata metadata = LocalStateMetadata::loadOrGenerate(stateDir.path(), Data(0));
   EXPECT_EQ(12345u, metadata.myClientId());
 }
 #endif
+
+TEST_F(LocalStateMetadataTest, encryptionKeyHash_whenLoadingWithSameKey_thenDoesntCrash) {
+  LocalStateMetadata::loadOrGenerate(stateDir.path(), DataFixture::generate(1024));
+  LocalStateMetadata::loadOrGenerate(stateDir.path(), DataFixture::generate(1024));
+}
+
+TEST_F(LocalStateMetadataTest, encryptionKeyHash_whenLoadingWithDifferentKey_thenCrashes) {
+  LocalStateMetadata::loadOrGenerate(stateDir.path(), DataFixture::generate(1024, 1));
+  EXPECT_THROW(
+    LocalStateMetadata::loadOrGenerate(stateDir.path(), DataFixture::generate(1024, 2)),
+    std::runtime_error
+  );
+}