[CHEF-3858] ensure invalid key always fails to decrypt

In Ci, we occasionally see test failures when decryption with an
incorrect key does not raise an error, but instead returns garbage.

This fixes that issue by adding an HMAC-SHA2-256 of the encrypted data
to the version 1 format. For backwards compatibility, decryption will
continue if the hmac is missing; therefore, this does not increase the
security of encrypted data bag items.

lib/chef/encrypted_data_bag_item.rb

@@ -86,6 +86,7 @@ def initialize(plaintext_data, key, iv=nil)
     def for_encrypted_item
       {
         "encrypted_data" => encrypted_data,
+        "hmac" => hmac,
         "iv" => Base64.encode64(iv),
         "version" => 1,
         "cipher" => ALGORITHM
@@ -122,6 +123,15 @@ def encrypted_data
       end
     end
 
+    # Generates an HMAC-SHA2-256 of the encrypted data (encrypt-then-mac)
+    def hmac
+      @hmac ||= begin
+        digest = OpenSSL::Digest::Digest.new("sha256")
+        raw_hmac = OpenSSL::HMAC.digest(digest, key, encrypted_data)
+        Base64.encode64(raw_hmac)
+      end
+    end
+
     # Wraps the data in a single key Hash (JSON Object) and converts to JSON.
     # The wrapper is required because we accept values (such as Integers or
     # Strings) that do not produce valid JSON when serialized without the
@@ -182,7 +192,6 @@ def for_decrypted_item
         raise DecryptionFailure, "Error decrypting data bag value. Most likely the provided key is incorrect"
       end
 
-
       def encrypted_bytes
         Base64.decode64(@encrypted_data["encrypted_data"])
       end
@@ -193,13 +202,32 @@ def iv
 
       def decrypted_data
         @decrypted_data ||= begin
+          validate_hmac!
           plaintext = openssl_decryptor.update(encrypted_bytes)
           plaintext << openssl_decryptor.final
         rescue OpenSSL::Cipher::CipherError => e
           raise DecryptionFailure, "Error decrypting data bag value: '#{e.message}'. Most likely the provided key is incorrect"
         end
       end
 
+      def validate_hmac!
+        return nil unless @encrypted_data.key?("hmac")
+
+        digest = OpenSSL::Digest::Digest.new("sha256")
+        raw_hmac = OpenSSL::HMAC.digest(digest, key, @encrypted_data["encrypted_data"])
+        expected_bytes = raw_hmac.bytes.to_a
+        actual_bytes = Base64.decode64(@encrypted_data["hmac"]).bytes.to_a
+        valid = expected_bytes.size ^ actual_bytes.size
+        expected_bytes.zip(actual_bytes) { |x, y| valid |= x ^ y.to_i }
+        if valid == 0
+          # correct hmac
+          true
+        else
+          raise DecryptionFailure, "Error decrypting data bag value: invalid hmac. Most likely the provided key is incorrect"
+        end
+      end
+
+
       def openssl_decryptor
         @openssl_decryptor ||= begin
           assert_valid_cipher!

spec/unit/encrypted_data_bag_item_spec.rb

@@ -46,6 +46,7 @@ def self.encrypt_value(plaintext_data, key)
       # out. Instead we test if the encrypted data is the same. If it *is* the
       # same, we assume the IV was the same each time.
       encryptor.encrypted_data.should_not eq encryptor2.encrypted_data
+      encryptor.hmac.should_not eq(encryptor2.hmac)
     end
   end
 
@@ -64,6 +65,7 @@ def self.encrypt_value(plaintext_data, key)
       final_data["iv"].should eq Base64.encode64(encryptor.iv)
       final_data["version"].should eq 1
       final_data["cipher"].should eq"aes-256-cbc"
+      final_data["hmac"].should eq(encryptor.hmac)
     end
   end
 
@@ -100,6 +102,27 @@ def self.encrypt_value(plaintext_data, key)
         decryptor.should_receive(:decrypted_data).and_return("lksajdf")
         lambda { decryptor.for_decrypted_item }.should raise_error(Chef::EncryptedDataBagItem::DecryptionFailure)
       end
+    end
+
+    context "and an hmac is present" do
+      let(:bogus_hmac) do
+        digest = OpenSSL::Digest::Digest.new("sha256")
+        raw_hmac = OpenSSL::HMAC.digest(digest, "WRONG", encrypted_value["encrypted_data"])
+        Base64.encode64(raw_hmac)
+      end
+
+      it "rejects the data if the hmac is wrong" do
+        encrypted_value["hmac"] = bogus_hmac
+        lambda { decryptor.for_decrypted_item }.should raise_error(Chef::EncryptedDataBagItem::DecryptionFailure)
+      end
+    end
+
+    context "and an hmac is not present" do
+
+      it "decrypts the data" do
+        encrypted_value.delete("hmac")
+        lambda { decryptor.for_decrypted_item }.should_not raise_error
+      end
 
     end