feat: security fixes and improved test coverages

CMakeLists.txt

@@ -29,7 +29,7 @@ target_include_directories(blowfish PUBLIC ${BF_INCLUDE_DIR})
 
 # ===== blowfish2 library =====
 add_library(blowfish2)
-target_compile_features(blowfish2 PUBLIC cxx_std_14)
+target_compile_features(blowfish2 PUBLIC cxx_std_17)
 
 target_sources(blowfish2
     PRIVATE

include/blowfish/blowfish.h

@@ -5,16 +5,17 @@
 // SPDX-FileCopyrightText: 1997 Paul Kocher
 
 #pragma once
+
+#if !defined(BLOWFISH_BLOWFISH_H_)
+#define BLOWFISH_BLOWFISH_H_
+
 #include <array>
 #include <cstdint>
 #include <string>
 
 static constexpr uint32_t BF_NUM_ROUNDS = 16;
 static constexpr uint32_t BF_MAX_KEYBYTES = 56;
 
-#if !defined(BLOWFISH_BLOWFISH_H_)
-#define BLOWFISH_BLOWFISH_H_
-
 class Blowfish {
 private:
   std::array<uint32_t, BF_NUM_ROUNDS + 2> PArray{};
@@ -25,6 +26,7 @@ class Blowfish {
   Blowfish() = default;
   explicit Blowfish(std::string const &key);
   Blowfish(Blowfish const &) = delete;
+  Blowfish &operator=(const Blowfish &) = delete;
 
   void initialize(const uint8_t *key, size_t keylen);
   void initialize(const std::string &key);

include/blowfish/blowfish2.h

@@ -5,16 +5,17 @@
 // SPDX-FileCopyrightText: 2005 Alexander Pukall
 
 #pragma once
+
+#if !defined(BLOWFISH_BLOWFISH2_H_)
+#define BLOWFISH_BLOWFISH2_H_
+
 #include <array>
 #include <cstdint>
 #include <string>
 
 static constexpr uint64_t BF2_NUM_ROUNDS = 64;
 static constexpr uint64_t BF2_MAX_KEYBYTES = 56;
 
-#if !defined(BLOWFISH_BLOWFISH2_H_)
-#define BLOWFISH_BLOWFISH2_H_
-
 class Blowfish2 {
 private:
   std::array<uint64_t, BF2_NUM_ROUNDS + 2> PArray{};
@@ -33,6 +34,7 @@ class Blowfish2 {
 
   void encrypt(uint64_t &xl, uint64_t &xr) noexcept;
   void decrypt(uint64_t &xl, uint64_t &xr) noexcept;
+  ~Blowfish2();
 };
 
 #endif // BLOWFISH_BLOWFISH2_H_

src/blowfish.cc

@@ -5,14 +5,15 @@
 // SPDX-FileCopyrightText: 1997 Paul Kocher
 
 #include <blowfish/blowfish.h>
+#include <stdexcept>
 
 static const std::array<uint32_t, 16 + 2> BF_PARRAY_INIT = {
     0x243F6A88L, 0x85A308D3L, 0x13198A2EL, 0x03707344L, 0xA4093822L,
     0x299F31D0L, 0x082EFA98L, 0xEC4E6C89L, 0x452821E6L, 0x38D01377L,
     0xBE5466CFL, 0x34E90C6CL, 0xC0AC29B7L, 0xC97C50DDL, 0x3F84D5B5L,
     0xB5470917L, 0x9216D5D9L, 0x8979FB1BL};
 
-static const std::array<std::array<uint32_t, 256>, 4> BF_SBOX_INT = {
+static const std::array<std::array<uint32_t, 256>, 4> BF_SBOX_INIT = {
     {{0xD1310BA6L, 0x98DFB5ACL, 0x2FFD72DBL, 0xD01ADFB7L, 0xB8E1AFEDL,
       0x6A267E96L, 0xBA7C9045L, 0xF12C7F99L, 0x24A19947L, 0xB3916CF7L,
       0x0801F2E2L, 0x858EFC16L, 0x636920D8L, 0x71574E69L, 0xA458FEA3L,
@@ -226,11 +227,15 @@ static const std::array<std::array<uint32_t, 256>, 4> BF_SBOX_INT = {
       0x3AC372E6L}}};
 
 void Blowfish::initialize(const uint8_t *key, size_t keylen) {
+  if (key == nullptr || keylen == 0 || keylen > BF_MAX_KEYBYTES)
+    throw std::invalid_argument(
+        "Blowfish key must be non-null and between 1 and 56 bytes");
+
   uint32_t data = 0;
   uint32_t datal = 0;
   uint32_t datar = 0;
 
-  Sboxes = BF_SBOX_INT;
+  Sboxes = BF_SBOX_INIT;
 
   size_t j = 0;
   for (uint32_t i = 0; i < BF_NUM_ROUNDS + 2; ++i) {
@@ -312,8 +317,12 @@ void Blowfish::decrypt(uint32_t &xl, uint32_t &xr) noexcept {
   xr = Xr;
 }
 Blowfish::~Blowfish() {
-  std::fill(PArray.begin(), PArray.end(), 0);
+  volatile uint32_t *p = PArray.data();
+  for (size_t i = 0; i < PArray.size(); ++i)
+    p[i] = 0;
   for (auto &row : Sboxes) {
-    std::fill(row.begin(), row.end(), 0);
+    volatile uint32_t *s = row.data();
+    for (size_t i = 0; i < row.size(); ++i)
+      s[i] = 0;
   }
 }

src/blowfish2.cc

@@ -5,6 +5,7 @@
 // SPDX-FileCopyrightText: 2005 Alexander Pukall
 
 #include <blowfish/blowfish2.h>
+#include <stdexcept>
 
 static const std::array<uint64_t, 64 + 2> BF2_PARRAY_INIT = {
     0x243F6A8885A308D3, 0x13198A2E03707344, 0xA4093822299F31D0,
@@ -732,11 +733,15 @@ void Blowfish2::initialize(const std::string &key) {
 }
 
 void Blowfish2::initialize(const uint8_t *key, size_t keylen) {
+  if (key == nullptr || keylen == 0 || keylen > BF2_MAX_KEYBYTES)
+    throw std::invalid_argument(
+        "Blowfish2 key must be non-null and between 1 and 56 bytes");
+
   uint64_t data = 0;
   uint64_t datal = 0;
   uint64_t datar = 0;
 
-  Sboxes = BF2_SBOX_INIT; // assumes static const S exists
+  Sboxes = BF2_SBOX_INIT;
 
   size_t j = 0;
 
@@ -827,3 +832,14 @@ void Blowfish2::decrypt(uint64_t &xl, uint64_t &xr) noexcept {
   xl = Xl;
   xr = Xr;
 }
+
+Blowfish2::~Blowfish2() {
+  volatile uint64_t *p = PArray.data();
+  for (size_t i = 0; i < PArray.size(); ++i)
+    p[i] = 0;
+  for (auto &row : Sboxes) {
+    volatile uint64_t *s = row.data();
+    for (size_t i = 0; i < row.size(); ++i)
+      s[i] = 0;
+  }
+}

tests/bf2_test_avalanche.cpp

@@ -8,9 +8,9 @@ static int hamming128(uint64_t a1, uint64_t b1, uint64_t a2, uint64_t b2) {
   return __builtin_popcountll(a1 ^ a2) + __builtin_popcountll(b1 ^ b2);
 }
 
-// Check that flipping one bit in plaintext or key
+// Check that flipping one bit in plaintext
 // causes large, unpredictable changes in ciphertext.
-TEST("Blowfish2 Avalanche Effect") {
+TEST("Blowfish2 Plaintext Avalanche Effect") {
   Blowfish2 bf("key-for-avalanche");
 
   uint64_t L = 0x1122334455667788ULL;
@@ -32,3 +32,33 @@ TEST("Blowfish2 Avalanche Effect") {
     EXPECT_TRUE(hd > 40);
   }
 }
+
+// Check that flipping one bit in the key
+// causes large, unpredictable changes in ciphertext.
+TEST("Blowfish2 Key Avalanche Effect") {
+  uint8_t basekey[8] = {0x01, 0x23, 0x45, 0x67, 0x89, 0xAB, 0xCD, 0xEF};
+  uint64_t L = 0x1122334455667788ULL;
+  uint64_t R = 0x99AABBCCDDEEFF00ULL;
+
+  Blowfish2 bf_base;
+  bf_base.initialize(basekey, 8);
+  uint64_t L0 = L, R0 = R;
+  bf_base.encrypt(L0, R0);
+
+  for (int byte = 0; byte < 8; ++byte) {
+    for (int bit = 0; bit < 8; ++bit) {
+      uint8_t flipped[8];
+      std::copy(basekey, basekey + 8, flipped);
+      flipped[byte] ^= (1u << bit);
+
+      Blowfish2 bf_flip;
+      bf_flip.initialize(flipped, 8);
+
+      uint64_t L1 = L, R1 = R;
+      bf_flip.encrypt(L1, R1);
+
+      int hd = hamming128(L0, R0, L1, R1);
+      EXPECT_TRUE(hd > 40);
+    }
+  }
+}

tests/bf2_test_properties.cpp

@@ -3,6 +3,143 @@
 
 #include "test_framework.h"
 #include <blowfish/blowfish2.h>
+#include <stdexcept>
+
+TEST("Blowfish2 varying key lengths") {
+  uint64_t L = 0xDEADBEEFCAFEBABEULL, R = 0x0123456789ABCDEFULL;
+
+  // 1-byte key (minimum)
+  {
+    Blowfish2 bf;
+    bf.initialize(reinterpret_cast<const uint8_t *>("A"), 1);
+    uint64_t l = L, r = R;
+    bf.encrypt(l, r);
+    EXPECT_TRUE(l != L || r != R);
+    bf.decrypt(l, r);
+    EXPECT_EQ(l, L);
+    EXPECT_EQ(r, R);
+  }
+
+  // 56-byte key (maximum)
+  {
+    const uint8_t maxkey[56] = {
+        0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09, 0x0A,
+        0x0B, 0x0C, 0x0D, 0x0E, 0x0F, 0x10, 0x11, 0x12, 0x13, 0x14,
+        0x15, 0x16, 0x17, 0x18, 0x19, 0x1A, 0x1B, 0x1C, 0x1D, 0x1E,
+        0x1F, 0x20, 0x21, 0x22, 0x23, 0x24, 0x25, 0x26, 0x27, 0x28,
+        0x29, 0x2A, 0x2B, 0x2C, 0x2D, 0x2E, 0x2F, 0x30, 0x31, 0x32,
+        0x33, 0x34, 0x35, 0x36, 0x37, 0x38};
+    Blowfish2 bf;
+    bf.initialize(maxkey, 56);
+    uint64_t l = L, r = R;
+    bf.encrypt(l, r);
+    EXPECT_TRUE(l != L || r != R);
+    bf.decrypt(l, r);
+    EXPECT_EQ(l, L);
+    EXPECT_EQ(r, R);
+  }
+
+  // Different key lengths produce different ciphertext
+  {
+    Blowfish2 bf4, bf8;
+    bf4.initialize(reinterpret_cast<const uint8_t *>("ABCD"), 4);
+    bf8.initialize(reinterpret_cast<const uint8_t *>("ABCDEFGH"), 8);
+    uint64_t l4 = L, r4 = R, l8 = L, r8 = R;
+    bf4.encrypt(l4, r4);
+    bf8.encrypt(l8, r8);
+    EXPECT_TRUE(l4 != l8 || r4 != r8);
+  }
+}
+
+TEST("Blowfish2 rejects invalid keys") {
+  bool caught = false;
+
+  // Empty key
+  try {
+    Blowfish2 bf("");
+    (void)bf;
+  } catch (const std::invalid_argument &) {
+    caught = true;
+  }
+  EXPECT_TRUE(caught);
+
+  // Over-length key (57 bytes)
+  caught = false;
+  try {
+    Blowfish2 bf;
+    uint8_t bigkey[57] = {};
+    bf.initialize(bigkey, 57);
+  } catch (const std::invalid_argument &) {
+    caught = true;
+  }
+  EXPECT_TRUE(caught);
+
+  // Null pointer
+  caught = false;
+  try {
+    Blowfish2 bf;
+    bf.initialize(nullptr, 8);
+  } catch (const std::invalid_argument &) {
+    caught = true;
+  }
+  EXPECT_TRUE(caught);
+}
+
+TEST("Blowfish2 edge-case blocks") {
+  Blowfish2 bf("edge-test-2");
+
+  // All-zero block
+  {
+    uint64_t L = 0, R = 0;
+    uint64_t L2 = L, R2 = R;
+    bf.encrypt(L2, R2);
+    EXPECT_TRUE(L2 != 0 || R2 != 0);
+    bf.decrypt(L2, R2);
+    EXPECT_EQ(L2, L);
+    EXPECT_EQ(R2, R);
+  }
+
+  // All-ones block
+  {
+    uint64_t L = 0xFFFFFFFFFFFFFFFFULL, R = 0xFFFFFFFFFFFFFFFFULL;
+    uint64_t L2 = L, R2 = R;
+    bf.encrypt(L2, R2);
+    EXPECT_TRUE(L2 != L || R2 != R);
+    bf.decrypt(L2, R2);
+    EXPECT_EQ(L2, L);
+    EXPECT_EQ(R2, R);
+  }
+}
+
+TEST("Blowfish2 cross-instance consistency") {
+  Blowfish2 bf1("same-key-2");
+  Blowfish2 bf2("same-key-2");
+
+  uint64_t L = 0xAAAAAAAAAAAAAAAAULL, R = 0x5555555555555555ULL;
+  uint64_t L1 = L, R1 = R, L2 = L, R2 = R;
+  bf1.encrypt(L1, R1);
+  bf2.encrypt(L2, R2);
+  EXPECT_EQ(L1, L2);
+  EXPECT_EQ(R1, R2);
+}
+
+TEST("Blowfish2 re-initialization") {
+  Blowfish2 bf("key-one");
+
+  uint64_t L = 0x1111111111111111ULL, R = 0x2222222222222222ULL;
+  uint64_t L1 = L, R1 = R;
+  bf.encrypt(L1, R1);
+
+  bf.initialize("key-two");
+  uint64_t L2 = L, R2 = R;
+  bf.encrypt(L2, R2);
+
+  EXPECT_TRUE(L1 != L2 || R1 != R2);
+
+  bf.decrypt(L2, R2);
+  EXPECT_EQ(L2, L);
+  EXPECT_EQ(R2, R);
+}
 
 TEST("Blowfish2 no fixed points") {
   Blowfish2 bf("fixed-point-check");

tests/test_avalanche.cpp

@@ -8,9 +8,9 @@ static int hamming(uint64_t a, uint64_t b) {
   return __builtin_popcountll(a ^ b);
 }
 
-// Check that flipping one bit in plaintext or key
+// Check that flipping one bit in plaintext
 // causes large, unpredictable changes in ciphertext.
-TEST("Blowfish Avalanche Effect") {
+TEST("Blowfish Plaintext Avalanche Effect") {
   Blowfish bf("key");
 
   uint32_t L = 0x11223344, R = 0x55667788;
@@ -32,3 +32,34 @@ TEST("Blowfish Avalanche Effect") {
     EXPECT_TRUE(hd > 20); // Strong avalanche threshold
   }
 }
+
+// Check that flipping one bit in the key
+// causes large, unpredictable changes in ciphertext.
+TEST("Blowfish Key Avalanche Effect") {
+  uint8_t basekey[8] = {0x01, 0x23, 0x45, 0x67, 0x89, 0xAB, 0xCD, 0xEF};
+  uint32_t L = 0x11223344, R = 0x55667788;
+
+  Blowfish bf_base;
+  bf_base.initialize(basekey, 8);
+  uint32_t Lc = L, Rc = R;
+  bf_base.encrypt(Lc, Rc);
+  uint64_t C1 = (uint64_t(Lc) << 32) | Rc;
+
+  for (int byte = 0; byte < 8; ++byte) {
+    for (int bit = 0; bit < 8; ++bit) {
+      uint8_t flipped[8];
+      std::copy(basekey, basekey + 8, flipped);
+      flipped[byte] ^= (1u << bit);
+
+      Blowfish bf_flip;
+      bf_flip.initialize(flipped, 8);
+
+      uint32_t L2 = L, R2 = R;
+      bf_flip.encrypt(L2, R2);
+      uint64_t C2 = (uint64_t(L2) << 32) | R2;
+
+      int hd = hamming(C1, C2);
+      EXPECT_TRUE(hd > 20);
+    }
+  }
+}