Deprecated ActiveSupport::MessageEncryptor#encrypt and decrypt.

3 files changed, 70 insertions, 41 deletions

diff --git a/activesupport/CHANGELOG.md b/activesupport/CHANGELOG.md
index e03bfdee92..18a115b369 100644
--- a/activesupport/CHANGELOG.md
+++ b/activesupport/CHANGELOG.md
@@ -1,5 +1,7 @@
 ## Rails 3.2.0 (unreleased) ##
 
+*   Deprecated ActiveSupport::MessageEncryptor#encrypt and decrypt. *José Valim*
+
 *   ActiveSupport::Notifications.subscribed provides subscriptions to events while a block runs. *fxn*
 
 *   Module#qualified_const_(defined?|get|set) are analogous to the corresponding methods
diff --git a/activesupport/lib/active_support/message_encryptor.rb b/activesupport/lib/active_support/message_encryptor.rb
index e14386a85d..5ab0cc83a7 100644
--- a/activesupport/lib/active_support/message_encryptor.rb
+++ b/activesupport/lib/active_support/message_encryptor.rb
@@ -18,13 +18,39 @@ module ActiveSupport
         ActiveSupport::Deprecation.warn "The second parameter should be an options hash. Use :cipher => 'algorithm' to specify the cipher algorithm."
         options = { :cipher => options }
       end
-      
+
       @secret = secret
       @cipher = options[:cipher] || 'aes-256-cbc'
       @serializer = options[:serializer] || Marshal
     end
 
     def encrypt(value)
+      ActiveSupport::Deprecation.warn "MessageEncryptor#encrypt is deprecated as it is not safe without a signature. " \
+        "Please use MessageEncryptor#encrypt_and_sign instead."
+      _encrypt(value)
+    end
+
+    def decrypt(value)
+      ActiveSupport::Deprecation.warn "MessageEncryptor#decrypt is deprecated as it is not safe without a signature. " \
+        "Please use MessageEncryptor#decrypt_and_verify instead."
+      _decrypt(value)
+    end
+
+    # Encrypt and sign a message. We need to sign the message in order to avoid padding attacks.
+    # Reference: http://www.limited-entropy.com/padding-oracle-attacks
+    def encrypt_and_sign(value)
+      verifier.generate(_encrypt(value))
+    end
+
+    # Decrypt and verify a message. We need to verify the message in order to avoid padding attacks.
+    # Reference: http://www.limited-entropy.com/padding-oracle-attacks
+    def decrypt_and_verify(value)
+      _decrypt(verifier.verify(value))
+    end
+
+    private
+
+    def _encrypt(value)
       cipher = new_cipher
       # Rely on OpenSSL for the initialization vector
       iv = cipher.random_iv
@@ -39,7 +65,7 @@ module ActiveSupport
       [encrypted_data, iv].map {|v| ActiveSupport::Base64.encode64s(v)}.join("--")
     end
 
-    def decrypt(encrypted_message)
+    def _decrypt(encrypted_message)
       cipher = new_cipher
       encrypted_data, iv = encrypted_message.split("--").map {|v| ActiveSupport::Base64.decode64(v)}
 
@@ -55,23 +81,12 @@ module ActiveSupport
       raise InvalidMessage
     end
 
-    def encrypt_and_sign(value)
-      verifier.generate(encrypt(value))
+    def new_cipher
+      OpenSSL::Cipher::Cipher.new(@cipher)
     end
 
-    def decrypt_and_verify(value)
-      decrypt(verifier.verify(value))
+    def verifier
+      MessageVerifier.new(@secret)
     end
-
-
-
-    private
-      def new_cipher
-        OpenSSL::Cipher::Cipher.new(@cipher)
-      end
-
-      def verifier
-        MessageVerifier.new(@secret)
-      end
   end
 end
diff --git a/activesupport/test/message_encryptor_test.rb b/activesupport/test/message_encryptor_test.rb
index 83a19f8106..55c4d42500 100644
--- a/activesupport/test/message_encryptor_test.rb
+++ b/activesupport/test/message_encryptor_test.rb
@@ -11,46 +11,51 @@ require 'active_support/time'
 require 'active_support/json'
 
 class MessageEncryptorTest < ActiveSupport::TestCase
-  
+
   class JSONSerializer
     def dump(value)
       ActiveSupport::JSON.encode(value)
     end
-    
+
     def load(value)
       ActiveSupport::JSON.decode(value)
     end
   end
-  
+
   def setup
-    @encryptor = ActiveSupport::MessageEncryptor.new(SecureRandom.hex(64))
+    @secret    = SecureRandom.hex(64)
+    @verifier  = ActiveSupport::MessageVerifier.new(@secret)
+    @encryptor = ActiveSupport::MessageEncryptor.new(@secret)
     @data = { :some => "data", :now => Time.local(2010) }
   end
 
-  def test_simple_round_tripping
-    message = @encryptor.encrypt(@data)
-    assert_equal @data, @encryptor.decrypt(message)
-  end
-
   def test_encrypting_twice_yields_differing_cipher_text
-    first_messqage = @encryptor.encrypt(@data)
-    second_message = @encryptor.encrypt(@data)
+    first_messqage = @encryptor.encrypt_and_sign(@data).split("--").first
+    second_message = @encryptor.encrypt_and_sign(@data).split("--").first
     assert_not_equal first_messqage, second_message
   end
 
-  def test_messing_with_either_value_causes_failure
-    text, iv = @encryptor.encrypt(@data).split("--")
+  def test_messing_with_either_encrypted_values_causes_failure
+    text, iv = @verifier.verify(@encryptor.encrypt_and_sign(@data)).split("--")
     assert_not_decrypted([iv, text] * "--")
     assert_not_decrypted([text, munge(iv)] * "--")
     assert_not_decrypted([munge(text), iv] * "--")
     assert_not_decrypted([munge(text), munge(iv)] * "--")
   end
 
+  def test_messing_with_verified_values_causes_failures
+    text, iv = @encryptor.encrypt_and_sign(@data).split("--")
+    assert_not_verified([iv, text] * "--")
+    assert_not_verified([text, munge(iv)] * "--")
+    assert_not_verified([munge(text), iv] * "--")
+    assert_not_verified([munge(text), munge(iv)] * "--")
+  end
+
   def test_signed_round_tripping
     message = @encryptor.encrypt_and_sign(@data)
     assert_equal @data, @encryptor.decrypt_and_verify(message)
   end
-  
+
   def test_alternative_serialization_method
     encryptor = ActiveSupport::MessageEncryptor.new(SecureRandom.hex(64), :serializer => JSONSerializer.new)
     message = encryptor.encrypt_and_sign({ :foo => 123, 'bar' => Time.utc(2010) })
@@ -62,19 +67,26 @@ class MessageEncryptorTest < ActiveSupport::TestCase
       ActiveSupport::MessageEncryptor.new(SecureRandom.hex(64), 'aes-256-cbc')
     end
   end
-  
+
   private
-    def assert_not_decrypted(value)
-      assert_raise(ActiveSupport::MessageEncryptor::InvalidMessage) do
-        @encryptor.decrypt(value)
-      end
+
+  def assert_not_decrypted(value)
+    assert_raise(ActiveSupport::MessageEncryptor::InvalidMessage) do
+      @encryptor.decrypt_and_verify(@verifier.generate(value))
     end
+  end
 
-    def munge(base64_string)
-      bits = ActiveSupport::Base64.decode64(base64_string)
-      bits.reverse!
-      ActiveSupport::Base64.encode64s(bits)
+  def assert_not_verified(value)
+    assert_raise(ActiveSupport::MessageVerifier::InvalidSignature) do
+      @encryptor.decrypt_and_verify(value)
     end
-end
+  end
 
+  def munge(base64_string)
+    bits = ActiveSupport::Base64.decode64(base64_string)
+    bits.reverse!
+    ActiveSupport::Base64.encode64s(bits)
+  end
 end
+
+end
\ No newline at end of file