librbd: don't decay LUKS{1,2}EncryptionFormat into LUKSEncryptionFormat

Commit 9892ead ("librbd/crypto: allow loading luks format without specifying version") introduced RBD_ENCRYPTION_FORMAT_LUKS format identifier, matching cryptsetup's CRYPT_LUKS ("load any LUKS version happens to be there"). However, in an effort to enable an obscure "layered encryption with the same passphrase + old QEMU" use case, it also introduced decaying of RBD_ENCRYPTION_FORMAT_LUKS1 and RBD_ENCRYPTION_FORMAT_LUKS2 format identifiers, making it impossible to assert on the format that is being loaded. This new behavior was then extended to standalone images. Treating RBD_ENCRYPTION_FORMAT_LUKS1, RBD_ENCRYPTION_FORMAT_LUKS2 and RBD_ENCRYPTION_FORMAT_LUKS the same when loading encryption can be construed as an opening for a format downgrade attack. Let's resurrect the previous standalone images behavior and extend it to layered encryption instead. Signed-off-by: Ilya Dryomov <[email protected]>
senax · Dec 4, 2022 · 75acf7b · 75acf7b
1 parent 0230c17
commit 75acf7b
Show file tree

Hide file tree

Showing 7 changed files with 337 additions and 87 deletions.
diff --git a/src/librbd/crypto/EncryptionFormat.h b/src/librbd/crypto/EncryptionFormat.h
@@ -5,7 +5,6 @@
 #define CEPH_LIBRBD_CRYPTO_ENCRYPTION_FORMAT_H
 
 #include <memory>
-#include "common/ref.h"
 
 struct Context;
 

diff --git a/src/librbd/crypto/luks/LUKSEncryptionFormat.cc b/src/librbd/crypto/luks/LUKSEncryptionFormat.cc
@@ -19,42 +19,60 @@ namespace crypto {
 namespace luks {
 
 template <typename I>
-LUKSEncryptionFormat<I>::LUKSEncryptionFormat(
-        encryption_algorithm_t alg,
-        std::string_view passphrase) : m_passphrase(passphrase), m_alg(alg) {
+void EncryptionFormat<I>::flatten(I* image_ctx, Context* on_finish) {
+  auto req = luks::FlattenRequest<I>::create(image_ctx, on_finish);
+  req->send();
 }
 
 template <typename I>
-LUKSEncryptionFormat<I>::LUKSEncryptionFormat(
-        std::string_view passphrase) : m_passphrase(passphrase) {
+void LUKSEncryptionFormat<I>::format(I* image_ctx, Context* on_finish) {
+  lderr(image_ctx->cct) << "explicit LUKS version required for format" << dendl;
+  on_finish->complete(-EINVAL);
 }
 
 template <typename I>
-void LUKSEncryptionFormat<I>::format(I* image_ctx, Context* on_finish) {
-  if (get_format() == RBD_ENCRYPTION_FORMAT_LUKS) {
-    lderr(image_ctx->cct) << "explicit LUKS version required for format"
-                          << dendl;
-    on_finish->complete(-EINVAL);
-    return;
-  }
+void LUKSEncryptionFormat<I>::load(I* image_ctx,
+                                   std::string* detected_format_name,
+                                   Context* on_finish) {
+  auto req = luks::LoadRequest<I>::create(image_ctx, RBD_ENCRYPTION_FORMAT_LUKS,
+                                          m_passphrase, &this->m_crypto,
+                                          detected_format_name, on_finish);
+  req->send();
+}
 
+template <typename I>
+void LUKS1EncryptionFormat<I>::format(I* image_ctx, Context* on_finish) {
   auto req = luks::FormatRequest<I>::create(
-          image_ctx, get_format(), m_alg, m_passphrase, &m_crypto, on_finish,
-          false);
+      image_ctx, RBD_ENCRYPTION_FORMAT_LUKS1, m_alg, m_passphrase,
+      &this->m_crypto, on_finish, false);
   req->send();
 }
 
 template <typename I>
-void LUKSEncryptionFormat<I>::load(
-        I* image_ctx, std::string* detected_format_name, Context* on_finish) {
+void LUKS1EncryptionFormat<I>::load(I* image_ctx,
+                                    std::string* detected_format_name,
+                                    Context* on_finish) {
   auto req = luks::LoadRequest<I>::create(
-          image_ctx, m_passphrase, &m_crypto, detected_format_name, on_finish);
+      image_ctx, RBD_ENCRYPTION_FORMAT_LUKS1, m_passphrase, &this->m_crypto,
+      detected_format_name, on_finish);
   req->send();
 }
 
 template <typename I>
-void LUKSEncryptionFormat<I>::flatten(I* image_ctx, Context* on_finish) {
-  auto req = luks::FlattenRequest<I>::create(image_ctx, on_finish);
+void LUKS2EncryptionFormat<I>::format(I* image_ctx, Context* on_finish) {
+  auto req = luks::FormatRequest<I>::create(
+      image_ctx, RBD_ENCRYPTION_FORMAT_LUKS2, m_alg, m_passphrase,
+      &this->m_crypto, on_finish, false);
+  req->send();
+}
+
+template <typename I>
+void LUKS2EncryptionFormat<I>::load(I* image_ctx,
+                                    std::string* detected_format_name,
+                                    Context* on_finish) {
+  auto req = luks::LoadRequest<I>::create(
+      image_ctx, RBD_ENCRYPTION_FORMAT_LUKS2, m_passphrase, &this->m_crypto,
+      detected_format_name, on_finish);
   req->send();
 }
 

diff --git a/src/librbd/crypto/luks/LUKSEncryptionFormat.h b/src/librbd/crypto/luks/LUKSEncryptionFormat.h
@@ -16,61 +16,74 @@ struct ImageCtx;
 namespace crypto {
 namespace luks {
 
-// This class is derived from only for the sake of the 'format' operation.
-// The 'load' and 'flatten' operations are handled the same for all three
-// variants.
 template <typename ImageCtxT>
-class LUKSEncryptionFormat : public crypto::EncryptionFormat<ImageCtxT> {
-
+class EncryptionFormat : public crypto::EncryptionFormat<ImageCtxT> {
 public:
-    LUKSEncryptionFormat(std::string_view passphrase);
-    LUKSEncryptionFormat(encryption_algorithm_t alg,
-                         std::string_view passphrase);
-
-    std::unique_ptr<crypto::EncryptionFormat<ImageCtxT>>
-    clone() const override {
-      // clone() should be called only when handling the 'load' operation,
-      // so decaying LUKS{1,2}EncryptionFormat into LUKSEncryptionFormat is fine
-      return std::unique_ptr<crypto::EncryptionFormat<ImageCtxT>>(
-              new LUKSEncryptionFormat(m_passphrase));
-    }
-
-    void format(ImageCtxT* ictx, Context* on_finish) override;
-    void load(ImageCtxT* ictx, std::string* detected_format_name,
-              Context* on_finish) override;
-    void flatten(ImageCtxT* ictx, Context* on_finish) override;
-
-    CryptoInterface* get_crypto() override {
-      ceph_assert(m_crypto);
-      return m_crypto.get();
-    }
+  void flatten(ImageCtxT* ictx, Context* on_finish) override;
+
+  CryptoInterface* get_crypto() override {
+    ceph_assert(m_crypto);
+    return m_crypto.get();
+  }
 
 protected:
-    virtual encryption_format_t get_format() const {
-      return RBD_ENCRYPTION_FORMAT_LUKS;
-    }
+  std::unique_ptr<CryptoInterface> m_crypto;
+};
+
+template <typename ImageCtxT>
+class LUKSEncryptionFormat : public EncryptionFormat<ImageCtxT> {
+public:
+  LUKSEncryptionFormat(std::string_view passphrase)
+      : m_passphrase(passphrase) {}
+
+  std::unique_ptr<crypto::EncryptionFormat<ImageCtxT>> clone() const override {
+    return std::make_unique<LUKSEncryptionFormat>(m_passphrase);
+  }
 
-    std::string_view m_passphrase;
-    encryption_algorithm_t m_alg;
-    std::unique_ptr<CryptoInterface> m_crypto;
+  void format(ImageCtxT* ictx, Context* on_finish) override;
+  void load(ImageCtxT* ictx, std::string* detected_format_name,
+            Context* on_finish) override;
+
+private:
+  std::string_view m_passphrase;
 };
 
 template <typename ImageCtxT>
-class LUKS1EncryptionFormat : public LUKSEncryptionFormat<ImageCtxT> {
-    using LUKSEncryptionFormat<ImageCtxT>::LUKSEncryptionFormat;
+class LUKS1EncryptionFormat : public EncryptionFormat<ImageCtxT> {
+public:
+  LUKS1EncryptionFormat(encryption_algorithm_t alg, std::string_view passphrase)
+      : m_alg(alg), m_passphrase(passphrase) {}
+
+  std::unique_ptr<crypto::EncryptionFormat<ImageCtxT>> clone() const override {
+    return std::make_unique<LUKS1EncryptionFormat>(m_alg, m_passphrase);
+  }
+
+  void format(ImageCtxT* ictx, Context* on_finish) override;
+  void load(ImageCtxT* ictx, std::string* detected_format_name,
+            Context* on_finish) override;
 
-    encryption_format_t get_format() const override {
-      return RBD_ENCRYPTION_FORMAT_LUKS1;
-    }
+private:
+  encryption_algorithm_t m_alg;
+  std::string_view m_passphrase;
 };
 
 template <typename ImageCtxT>
-class LUKS2EncryptionFormat : public LUKSEncryptionFormat<ImageCtxT> {
-    using LUKSEncryptionFormat<ImageCtxT>::LUKSEncryptionFormat;
+class LUKS2EncryptionFormat : public EncryptionFormat<ImageCtxT> {
+public:
+  LUKS2EncryptionFormat(encryption_algorithm_t alg, std::string_view passphrase)
+      : m_alg(alg), m_passphrase(passphrase) {}
+
+  std::unique_ptr<crypto::EncryptionFormat<ImageCtxT>> clone() const override {
+    return std::make_unique<LUKS2EncryptionFormat>(m_alg, m_passphrase);
+  }
+
+  void format(ImageCtxT* ictx, Context* on_finish) override;
+  void load(ImageCtxT* ictx, std::string* detected_format_name,
+            Context* on_finish) override;
 
-    encryption_format_t get_format() const override {
-      return RBD_ENCRYPTION_FORMAT_LUKS2;
-    }
+private:
+  encryption_algorithm_t m_alg;
+  std::string_view m_passphrase;
 };
 
 } // namespace luks

diff --git a/src/librbd/crypto/luks/LoadRequest.cc b/src/librbd/crypto/luks/LoadRequest.cc
@@ -26,10 +26,11 @@ using librbd::util::create_context_callback;
 
 template <typename I>
 LoadRequest<I>::LoadRequest(
-        I* image_ctx, std::string_view passphrase,
+        I* image_ctx, encryption_format_t format, std::string_view passphrase,
         std::unique_ptr<CryptoInterface>* result_crypto,
         std::string* detected_format_name,
         Context* on_finish) : m_image_ctx(image_ctx),
+                              m_format(format),
                               m_passphrase(passphrase),
                               m_on_finish(on_finish),
                               m_result_crypto(result_crypto),
@@ -140,8 +141,26 @@ void LoadRequest<I>::handle_read_header(int r) {
     return;
   }
 
+  const char* type;
+  switch (m_format) {
+  case RBD_ENCRYPTION_FORMAT_LUKS:
+    type = CRYPT_LUKS;
+    break;
+  case RBD_ENCRYPTION_FORMAT_LUKS1:
+    type = CRYPT_LUKS1;
+    break;
+  case RBD_ENCRYPTION_FORMAT_LUKS2:
+    type = CRYPT_LUKS2;
+    break;
+  default:
+    lderr(m_image_ctx->cct) << "unsupported format type: " << m_format
+                            << dendl;
+    finish(-EINVAL);
+    return;
+  }
+
   // parse header via libcryptsetup
-  r = m_header.load(CRYPT_LUKS);
+  r = m_header.load(type);
   if (r != 0) {
     if (m_offset < MAXIMUM_HEADER_SIZE) {
       // perhaps we did not feed the entire header to libcryptsetup, retry

diff --git a/src/librbd/crypto/luks/LoadRequest.h b/src/librbd/crypto/luks/LoadRequest.h
@@ -26,15 +26,17 @@ template <typename I>
 class LoadRequest {
 public:
     static LoadRequest* create(
-            I* image_ctx, std::string_view passphrase,
+            I* image_ctx, encryption_format_t format,
+            std::string_view passphrase,
             std::unique_ptr<CryptoInterface>* result_crypto,
             std::string* detected_format_name,
             Context* on_finish) {
-      return new LoadRequest(image_ctx, passphrase, result_crypto,
+      return new LoadRequest(image_ctx, format, passphrase, result_crypto,
                              detected_format_name, on_finish);
     }
 
-    LoadRequest(I* image_ctx, std::string_view passphrase,
+    LoadRequest(I* image_ctx, encryption_format_t format,
+                std::string_view passphrase,
                 std::unique_ptr<CryptoInterface>* result_crypto,
                 std::string* detected_format_name, Context* on_finish);
     void send();
@@ -43,6 +45,7 @@ class LoadRequest {
 
 private:
     I* m_image_ctx;
+    encryption_format_t m_format;
     std::string_view m_passphrase;
     Context* m_on_finish;
     ceph::bufferlist m_bl;

diff --git a/src/test/librbd/crypto/luks/test_mock_LoadRequest.cc b/src/test/librbd/crypto/luks/test_mock_LoadRequest.cc
@@ -49,8 +49,8 @@ struct TestMockCryptoLuksLoadRequest : public TestMockFixture {
     ASSERT_EQ(0, open_image(m_image_name, &ictx));
     mock_image_ctx = new MockImageCtx(*ictx);
     mock_load_request = MockLoadRequest::create(
-            mock_image_ctx, std::move(passphrase), &crypto,
-            &detected_format_name, on_finish);
+        mock_image_ctx, RBD_ENCRYPTION_FORMAT_LUKS2, std::move(passphrase),
+        &crypto, &detected_format_name, on_finish);
     detected_format_name = "";
   }
 
@@ -142,8 +142,8 @@ TEST_F(TestMockCryptoLuksLoadRequest, AES256) {
 TEST_F(TestMockCryptoLuksLoadRequest, LUKS1) {
   delete mock_load_request;
   mock_load_request = MockLoadRequest::create(
-          mock_image_ctx, {passphrase_cstr}, &crypto, &detected_format_name,
-          on_finish);
+      mock_image_ctx, RBD_ENCRYPTION_FORMAT_LUKS1, {passphrase_cstr}, &crypto,
+      &detected_format_name, on_finish);
   generate_header(CRYPT_LUKS1, "aes", 32, "xts-plain64", 512, false);
   expect_image_read(0, DEFAULT_INITIAL_READ_SIZE);
   expect_get_image_size(OBJECT_SIZE << 5);
@@ -155,7 +155,23 @@ TEST_F(TestMockCryptoLuksLoadRequest, LUKS1) {
   ASSERT_EQ("LUKS1", detected_format_name);
 }
 
-TEST_F(TestMockCryptoLuksLoadRequest, WrongFormat) {
+TEST_F(TestMockCryptoLuksLoadRequest, LUKS1ViaLUKS) {
+  delete mock_load_request;
+  mock_load_request = MockLoadRequest::create(
+      mock_image_ctx, RBD_ENCRYPTION_FORMAT_LUKS, {passphrase_cstr}, &crypto,
+      &detected_format_name, on_finish);
+  generate_header(CRYPT_LUKS1, "aes", 32, "xts-plain64", 512, false);
+  expect_image_read(0, DEFAULT_INITIAL_READ_SIZE);
+  expect_get_image_size(OBJECT_SIZE << 5);
+  expect_get_stripe_period(OBJECT_SIZE);
+  mock_load_request->send();
+  image_read_request->complete(DEFAULT_INITIAL_READ_SIZE);
+  ASSERT_EQ(0, finished_cond.wait());
+  ASSERT_NE(crypto.get(), nullptr);
+  ASSERT_EQ("LUKS1", detected_format_name);
+}
+
+TEST_F(TestMockCryptoLuksLoadRequest, UnknownFormat) {
   header_bl.append_zero(MAXIMUM_HEADER_SIZE);
   expect_image_read(0, DEFAULT_INITIAL_READ_SIZE);
   mock_load_request->send();
@@ -167,6 +183,21 @@ TEST_F(TestMockCryptoLuksLoadRequest, WrongFormat) {
   ASSERT_EQ("<unknown>", detected_format_name);
 }
 
+TEST_F(TestMockCryptoLuksLoadRequest, WrongFormat) {
+  generate_header(CRYPT_LUKS1, "aes", 32, "xts-plain64", 512, false);
+  expect_image_read(0, DEFAULT_INITIAL_READ_SIZE);
+  mock_load_request->send();
+
+  expect_image_read(DEFAULT_INITIAL_READ_SIZE,
+                    MAXIMUM_HEADER_SIZE - DEFAULT_INITIAL_READ_SIZE);
+  image_read_request->complete(DEFAULT_INITIAL_READ_SIZE);
+  image_read_request->complete(MAXIMUM_HEADER_SIZE - DEFAULT_INITIAL_READ_SIZE);
+
+  ASSERT_EQ(-EINVAL, finished_cond.wait());
+  ASSERT_EQ(crypto.get(), nullptr);
+  ASSERT_EQ("LUKS", detected_format_name);
+}
+
 TEST_F(TestMockCryptoLuksLoadRequest, UnsupportedAlgorithm) {
   generate_header(CRYPT_LUKS2, "twofish", 32, "xts-plain64", 4096, false);
   expect_image_read(0, DEFAULT_INITIAL_READ_SIZE);
@@ -231,8 +262,8 @@ TEST_F(TestMockCryptoLuksLoadRequest, LUKS1FormattedClone) {
   mock_image_ctx->parent = mock_image_ctx;
   delete mock_load_request;
   mock_load_request = MockLoadRequest::create(
-          mock_image_ctx, {passphrase_cstr}, &crypto, &detected_format_name,
-          on_finish);
+      mock_image_ctx, RBD_ENCRYPTION_FORMAT_LUKS1, {passphrase_cstr}, &crypto,
+      &detected_format_name, on_finish);
   generate_header(CRYPT_LUKS1, "aes", 64, "xts-plain64", 512, true);
   expect_image_read(0, DEFAULT_INITIAL_READ_SIZE);
   expect_get_image_size(OBJECT_SIZE << 5);
@@ -277,7 +308,8 @@ TEST_F(TestMockCryptoLuksLoadRequest, KeyslotsBiggerThanInitialRead) {
 TEST_F(TestMockCryptoLuksLoadRequest, WrongPassphrase) {
   delete mock_load_request;
   mock_load_request = MockLoadRequest::create(
-        mock_image_ctx, "wrong", &crypto, &detected_format_name, on_finish);
+      mock_image_ctx, RBD_ENCRYPTION_FORMAT_LUKS2, "wrong", &crypto,
+      &detected_format_name, on_finish);
 
   generate_header(CRYPT_LUKS2, "aes", 64, "xts-plain64", 4096, false);
   expect_image_read(0, DEFAULT_INITIAL_READ_SIZE);