LibCrypto: Fix issues in the Crypto stack

This commit fixes up the following:
- HMAC should not reuse a single hasher when successively updating
- AES Key should not assume its user key is valid signed char*
- Mode should have a virtual destructor
And adds a RFC5246 padding mode, which is required for TLS

Libraries/LibCrypto/Cipher/AES.cpp

@@ -59,7 +59,7 @@ namespace Cipher {
         return builder.build();
     }
 
-    void AESCipherKey::expand_encrypt_key(const StringView& user_key, size_t bits)
+    void AESCipherKey::expand_encrypt_key(const ByteBuffer& user_key, size_t bits)
     {
         u32* round_key;
         u32 temp;
@@ -78,10 +78,10 @@ namespace Cipher {
             m_rounds = 14;
         }
 
-        round_key[0] = get_key(user_key.substring_view(0, 4).characters_without_null_termination());
+        round_key[0] = get_key(user_key.slice_view(0, 4).data());
-        round_key[1] = get_key(user_key.substring_view(4, 4).characters_without_null_termination());
+        round_key[1] = get_key(user_key.slice_view(4, 4).data());
-        round_key[2] = get_key(user_key.substring_view(8, 4).characters_without_null_termination());
+        round_key[2] = get_key(user_key.slice_view(8, 4).data());
-        round_key[3] = get_key(user_key.substring_view(12, 4).characters_without_null_termination());
+        round_key[3] = get_key(user_key.slice_view(12, 4).data());
         if (bits == 128) {
             for (;;) {
                 temp = round_key[3];
@@ -103,8 +103,8 @@ namespace Cipher {
             return;
         }
 
-        round_key[4] = get_key(user_key.substring_view(16, 4).characters_without_null_termination());
+        round_key[4] = get_key(user_key.slice_view(16, 4).data());
-        round_key[5] = get_key(user_key.substring_view(20, 4).characters_without_null_termination());
+        round_key[5] = get_key(user_key.slice_view(20, 4).data());
         if (bits == 192) {
             for (;;) {
                 temp = round_key[5];
@@ -131,8 +131,8 @@ namespace Cipher {
             return;
         }
 
-        round_key[6] = get_key(user_key.substring_view(24, 4).characters_without_null_termination());
+        round_key[6] = get_key(user_key.slice_view(24, 4).data());
-        round_key[7] = get_key(user_key.substring_view(28, 4).characters_without_null_termination());
+        round_key[7] = get_key(user_key.slice_view(28, 4).data());
         if (true) { // bits == 256
             for (;;) {
                 temp = round_key[7];
@@ -169,7 +169,7 @@ namespace Cipher {
         }
     }
 
-    void AESCipherKey::expand_decrypt_key(const StringView& user_key, size_t bits)
+    void AESCipherKey::expand_decrypt_key(const ByteBuffer& user_key, size_t bits)
     {
         u32* round_key;
 
@@ -414,6 +414,10 @@ namespace Cipher {
                 // fill with the length of the padding bytes
                 __builtin_memset(m_data.data() + length, m_data.size() - length, m_data.size() - length);
                 break;
+            case PaddingMode::RFC5246:
+                // fill with the length of the padding bytes minus one
+                __builtin_memset(m_data.data() + length, m_data.size() - length - 1, m_data.size() - length);
+                break;
             default:
                 // FIXME: We should handle the rest of the common padding modes
                 ASSERT_NOT_REACHED();

Libraries/LibCrypto/Cipher/AES.h

@@ -71,8 +71,8 @@ namespace Cipher {
 
     struct AESCipherKey : public CipherKey {
         virtual ByteBuffer data() const override { return ByteBuffer::copy(m_rd_keys, sizeof(m_rd_keys)); };
-        virtual void expand_encrypt_key(const StringView& user_key, size_t bits) override;
+        virtual void expand_encrypt_key(const ByteBuffer& user_key, size_t bits) override;
-        virtual void expand_decrypt_key(const StringView& user_key, size_t bits) override;
+        virtual void expand_decrypt_key(const ByteBuffer& user_key, size_t bits) override;
         static bool is_valid_key_size(size_t bits) { return bits == 128 || bits == 192 || bits == 256; };
         String to_string() const;
         const u32* round_keys() const
@@ -80,7 +80,8 @@ namespace Cipher {
             return (const u32*)m_rd_keys;
         }
 
-        AESCipherKey(const StringView& user_key, size_t key_bits, Intent intent)
+        AESCipherKey(const ByteBuffer& user_key, size_t key_bits, Intent intent)
+            : m_bits(key_bits)
         {
             if (intent == Intent::Encryption)
                 expand_encrypt_key(user_key, key_bits);
@@ -91,6 +92,7 @@ namespace Cipher {
         virtual ~AESCipherKey() override {}
 
         size_t rounds() const { return m_rounds; }
+        size_t length() const { return m_bits / 8; }
 
     protected:
         u32* round_keys()
@@ -102,13 +104,14 @@ namespace Cipher {
         static constexpr size_t MAX_ROUND_COUNT = 14;
         u32 m_rd_keys[(MAX_ROUND_COUNT + 1) * 4] { 0 };
         size_t m_rounds;
+        size_t m_bits;
     };
 
     class AESCipher final : public Cipher<AESCipherKey, AESCipherBlock> {
     public:
         using CBCMode = CBC<AESCipher>;
 
-        AESCipher(const StringView& user_key, size_t key_bits, Intent intent = Intent::Encryption, PaddingMode mode = PaddingMode::CMS)
+        AESCipher(const ByteBuffer& user_key, size_t key_bits, Intent intent = Intent::Encryption, PaddingMode mode = PaddingMode::CMS)
             : Cipher<AESCipherKey, AESCipherBlock>(mode)
             , m_key(user_key, key_bits, intent)
         {

Libraries/LibCrypto/Cipher/Cipher.h

@@ -40,6 +40,7 @@ namespace Cipher {
 
     enum class PaddingMode {
         CMS, // RFC 1423
+        RFC5246, // very similar to CMS, but filled with |length - 1|, instead of |length|
         Null,
         // FIXME: We do not implement these yet
         Bit,

Libraries/LibCrypto/Cipher/Mode/CBC.h

@@ -36,6 +36,7 @@ namespace Cipher {
     template <typename T>
     class CBC : public Mode<T> {
     public:
+        virtual ~CBC() {}
         template <typename... Args>
         explicit constexpr CBC<T>(Args... args)
             : Mode<T>(args...)

Libraries/LibCrypto/Cipher/Mode/Mode.h

@@ -35,6 +35,8 @@ namespace Cipher {
     template <typename T>
     class Mode {
     public:
+        virtual ~Mode() {}
+
         // FIXME: Somehow communicate that encrypt returns the last initialization vector (if the mode supports it)
         virtual Optional<ByteBuffer> encrypt(const ByteBuffer& in, ByteBuffer& out, Optional<ByteBuffer> ivec = {}) = 0;
         virtual void decrypt(const ByteBuffer& in, ByteBuffer& out, Optional<ByteBuffer> ivec = {}) = 0;
@@ -51,10 +53,9 @@ namespace Cipher {
         }
 
         virtual String class_name() const = 0;
-
-    protected:
         T& cipher() { return m_cipher; }
 
+    protected:
         virtual void prune_padding(ByteBuffer& data)
         {
             auto size = data.size();
@@ -74,6 +75,22 @@ namespace Cipher {
                 data.trim(size - maybe_padding_length);
                 break;
             }
+            case PaddingMode::RFC5246: {
+                auto maybe_padding_length = data[size - 1];
+                if (maybe_padding_length >= T::block_size() - 1) {
+                    // cannot be padding (the entire block cannot be padding)
+                    return;
+                }
+                // FIXME: If we want to constant-time operations, this loop should not stop
+                for (auto i = maybe_padding_length; i > 0; --i) {
+                    if (data[size - i - 1] != maybe_padding_length) {
+                        // note that this is likely invalid padding
+                        return;
+                    }
+                }
+                data.trim(size - maybe_padding_length - 1);
+                break;
+            }
             case PaddingMode::Null: {
                 while (data[size - 1] == 0)
                     --size;

Libraries/LibCrypto/Hash/HashFunction.h

@@ -51,6 +51,8 @@ namespace Hash {
         virtual DigestType peek() = 0;
         virtual DigestType digest() = 0;
 
+        virtual void reset() = 0;
+
         virtual String class_name() const = 0;
     };
 }

Libraries/LibCrypto/Hash/MD5.cpp

@@ -220,18 +220,5 @@ namespace Hash {
         __builtin_memset(x, 0, sizeof(x));
     }
 
-    void MD5::reset()
-    {
-        m_A = MD5Constants::init_A;
-        m_B = MD5Constants::init_B;
-        m_C = MD5Constants::init_C;
-        m_D = MD5Constants::init_D;
-
-        m_count[0] = 0;
-        m_count[1] = 0;
-
-        __builtin_memset(m_data_buffer, 0, sizeof(m_data_buffer));
-    }
-
 }
 }

Libraries/LibCrypto/Hash/MD5.h

@@ -92,10 +92,21 @@ namespace Hash {
 
         inline static DigestType hash(const ByteBuffer& buffer) { return hash(buffer.data(), buffer.size()); }
         inline static DigestType hash(const StringView& buffer) { return hash((const u8*)buffer.characters_without_null_termination(), buffer.length()); }
+        inline virtual void reset() override
+        {
+            m_A = MD5Constants::init_A;
+            m_B = MD5Constants::init_B;
+            m_C = MD5Constants::init_C;
+            m_D = MD5Constants::init_D;
+
+            m_count[0] = 0;
+            m_count[1] = 0;
+
+            __builtin_memset(m_data_buffer, 0, sizeof(m_data_buffer));
+        }
 
     private:
         inline void transform(const u8*);
-        inline void reset();
 
         static void encode(const u32* from, u8* to, size_t length);
         static void decode(const u8* from, u32* to, size_t length);

Libraries/LibCrypto/Hash/SHA2.h

@@ -123,10 +123,7 @@ namespace Hash {
             builder.appendf("%zu", this->DigestSize * 8);
             return builder.build();
         };
-
+        inline virtual void reset() override
-    private:
-        inline void transform(const u8*);
-        inline void reset()
         {
             m_data_length = 0;
             m_bit_length = 0;
@@ -134,6 +131,9 @@ namespace Hash {
                 m_state[i] = SHA256Constants::InitializationHashes[i];
         }
 
+    private:
+        inline void transform(const u8*);
+
         u8 m_data_buffer[BlockSize];
         size_t m_data_length { 0 };
 
@@ -176,10 +176,7 @@ namespace Hash {
             builder.appendf("%zu", this->DigestSize * 8);
             return builder.build();
         };
-
+        inline virtual void reset() override
-    private:
-        inline void transform(const u8*);
-        inline void reset()
         {
             m_data_length = 0;
             m_bit_length = 0;
@@ -187,6 +184,9 @@ namespace Hash {
                 m_state[i] = SHA512Constants::InitializationHashes[i];
         }
 
+    private:
+        inline void transform(const u8*);
+
         u8 m_data_buffer[BlockSize];
         size_t m_data_length { 0 };
 

Userland/test-crypto.cpp

@@ -50,10 +50,16 @@ void print_buffer(const ByteBuffer& buffer, int split)
 {
     for (size_t i = 0; i < buffer.size(); ++i) {
         if (split > 0) {
-            if (i % split == 0 && i)
+            if (i % split == 0 && i) {
+                printf("    ");
+                for (size_t j = i - split; j < i; ++j) {
+                    auto ch = buffer[j];
+                    printf("%c", ch >= 32 && ch <= 127 ? ch : '.'); // silly hack
+                }
                 puts("");
             }
-        printf("%02x", buffer[i]);
+        }
+        printf("%02x ", buffer[i]);
     }
     puts("");
 }
@@ -90,7 +96,7 @@ void aes_cbc(const char* message, size_t len)
     auto iv = ByteBuffer::create_zeroed(Crypto::Cipher::AESCipher::block_size());
 
     if (encrypting) {
-        Crypto::Cipher::AESCipher::CBCMode cipher(secret_key, key_bits, Crypto::Cipher::Intent::Encryption);
+        Crypto::Cipher::AESCipher::CBCMode cipher(ByteBuffer::wrap(secret_key, strlen(secret_key)), key_bits, Crypto::Cipher::Intent::Encryption);
 
         auto enc = cipher.create_aligned_buffer(buffer.size());
         cipher.encrypt(buffer, enc, iv);
@@ -100,7 +106,7 @@ void aes_cbc(const char* message, size_t len)
         else
             print_buffer(enc, Crypto::Cipher::AESCipher::block_size());
     } else {
-        Crypto::Cipher::AESCipher::CBCMode cipher(secret_key, key_bits, Crypto::Cipher::Intent::Decryption);
+        Crypto::Cipher::AESCipher::CBCMode cipher(ByteBuffer::wrap(secret_key, strlen(secret_key)), key_bits, Crypto::Cipher::Intent::Decryption);
         auto dec = cipher.create_aligned_buffer(buffer.size());
         cipher.decrypt(buffer, dec, iv);
         printf("%.*s\n", (int)dec.size(), dec.data());
@@ -341,7 +347,7 @@ int aes_cbc_tests()
 void aes_cbc_test_name()
 {
     I_TEST((AES CBC class name));
-    Crypto::Cipher::AESCipher::CBCMode cipher("WellHelloFriends", 128, Crypto::Cipher::Intent::Encryption);
+    Crypto::Cipher::AESCipher::CBCMode cipher("WellHelloFriends"_b, 128, Crypto::Cipher::Intent::Encryption);
     if (cipher.class_name() != "AES_CBC")
         FAIL(Invalid class name);
     else
@@ -371,7 +377,7 @@ void aes_cbc_test_encrypt()
             0x8b, 0xd3, 0x70, 0x45, 0xf0, 0x79, 0x65, 0xca, 0xb9, 0x03, 0x88, 0x72, 0x1c, 0xdd, 0xab,
             0x45, 0x6b, 0x1c
         };
-        Crypto::Cipher::AESCipher::CBCMode cipher("WellHelloFriends", 128, Crypto::Cipher::Intent::Encryption);
+        Crypto::Cipher::AESCipher::CBCMode cipher("WellHelloFriends"_b, 128, Crypto::Cipher::Intent::Encryption);
         test_it(cipher, result);
     }
     {
@@ -382,7 +388,7 @@ void aes_cbc_test_encrypt()
             0x68, 0x51, 0x09, 0xd7, 0x3b, 0x48, 0x1b, 0x8a, 0xd3, 0x50, 0x09, 0xba, 0xfc, 0xde, 0x11,
             0xe0, 0x3f, 0xcb
         };
-        Crypto::Cipher::AESCipher::CBCMode cipher("Well Hello Friends! whf!", 192, Crypto::Cipher::Intent::Encryption);
+        Crypto::Cipher::AESCipher::CBCMode cipher("Well Hello Friends! whf!"_b, 192, Crypto::Cipher::Intent::Encryption);
         test_it(cipher, result);
     }
     {
@@ -393,7 +399,19 @@ void aes_cbc_test_encrypt()
             0x47, 0x9f, 0xc2, 0x21, 0xe6, 0x19, 0x62, 0xc3, 0x75, 0xca, 0xab, 0x2d, 0x18, 0xa1, 0x54,
             0xd1, 0x41, 0xe6
         };
-        Crypto::Cipher::AESCipher::CBCMode cipher("WellHelloFriendsWellHelloFriends", 256, Crypto::Cipher::Intent::Encryption);
+        Crypto::Cipher::AESCipher::CBCMode cipher("WellHelloFriendsWellHelloFriends"_b, 256, Crypto::Cipher::Intent::Encryption);
+        test_it(cipher, result);
+    }
+    {
+        I_TEST((AES CBC with 256 bit key | Specialized Encrypt))
+        u8 result[] {
+            0x0a, 0x44, 0x4d, 0x62, 0x9e, 0x8b, 0xd8, 0x11, 0x80, 0x48, 0x2a, 0x32, 0x53, 0x61, 0xe7,
+            0x59, 0x62, 0x55, 0x9e, 0xf4, 0xe6, 0xad, 0xea, 0xc5, 0x0b, 0xf6, 0xbc, 0x6a, 0xcb, 0x9c,
+            0x47, 0x9f, 0xc2, 0x21, 0xe6, 0x19, 0x62, 0xc3, 0x75, 0xca, 0xab, 0x2d, 0x18, 0xa1, 0x54,
+            0xd1, 0x41, 0xe6
+        };
+        u8 key[] { 0x0a, 0x8c, 0x5b, 0x0d, 0x8a, 0x68, 0x43, 0xf7, 0xaf, 0xc0, 0xe3, 0x4e, 0x4b, 0x43, 0xaa, 0x28, 0x69, 0x9b, 0x6f, 0xe7, 0x24, 0x82, 0x1c, 0x71, 0x86, 0xf6, 0x2b, 0x87, 0xd6, 0x8b, 0x8f, 0xf1 };
+        Crypto::Cipher::AESCipher::CBCMode cipher(ByteBuffer::wrap(key, 32), 256, Crypto::Cipher::Intent::Encryption);
         test_it(cipher, result);
     }
     // TODO: Test non-CMS padding options
@@ -423,7 +441,7 @@ void aes_cbc_test_decrypt()
             0x8b, 0xd3, 0x70, 0x45, 0xf0, 0x79, 0x65, 0xca, 0xb9, 0x03, 0x88, 0x72, 0x1c, 0xdd, 0xab,
             0x45, 0x6b, 0x1c
         };
-        Crypto::Cipher::AESCipher::CBCMode cipher("WellHelloFriends", 128, Crypto::Cipher::Intent::Decryption);
+        Crypto::Cipher::AESCipher::CBCMode cipher("WellHelloFriends"_b, 128, Crypto::Cipher::Intent::Decryption);
         test_it(cipher, result, 48);
     }
     {
@@ -434,7 +452,7 @@ void aes_cbc_test_decrypt()
             0x68, 0x51, 0x09, 0xd7, 0x3b, 0x48, 0x1b, 0x8a, 0xd3, 0x50, 0x09, 0xba, 0xfc, 0xde, 0x11,
             0xe0, 0x3f, 0xcb
         };
-        Crypto::Cipher::AESCipher::CBCMode cipher("Well Hello Friends! whf!", 192, Crypto::Cipher::Intent::Decryption);
+        Crypto::Cipher::AESCipher::CBCMode cipher("Well Hello Friends! whf!"_b, 192, Crypto::Cipher::Intent::Decryption);
         test_it(cipher, result, 48);
     }
     {
@@ -445,7 +463,7 @@ void aes_cbc_test_decrypt()
             0x47, 0x9f, 0xc2, 0x21, 0xe6, 0x19, 0x62, 0xc3, 0x75, 0xca, 0xab, 0x2d, 0x18, 0xa1, 0x54,
             0xd1, 0x41, 0xe6
         };
-        Crypto::Cipher::AESCipher::CBCMode cipher("WellHelloFriendsWellHelloFriends", 256, Crypto::Cipher::Intent::Decryption);
+        Crypto::Cipher::AESCipher::CBCMode cipher("WellHelloFriendsWellHelloFriends"_b, 256, Crypto::Cipher::Intent::Decryption);
         test_it(cipher, result, 48);
     }
     // TODO: Test non-CMS padding options