Skip to content

Commit

Permalink
Added salts and IV. This is what attr_encrypted will use by default
Browse files Browse the repository at this point in the history
  • Loading branch information
Daniel Palacio committed Dec 15, 2011
1 parent 51ac7a0 commit c1b7e39
Show file tree
Hide file tree
Showing 2 changed files with 13 additions and 11 deletions.
3 changes: 2 additions & 1 deletion lib/encryptor.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,9 @@ def crypt(cipher_method, *args) #:nodoc:
cipher = OpenSSL::Cipher::Cipher.new(options[:algorithm])
cipher.send(cipher_method)
if options[:iv]
cipher.key = options[:key]
raise ArgumentError.new('you must specify a :salt') if options[:salt].nil?
cipher.iv = options[:iv]
cipher.key = OpenSSL::PKCS5.pbkdf2_hmac_sha1(options[:key], options[:salt], 2000, cipher.key_len)

This comment has been minimized.

Sign in to view

Copy link
@nahi

nahi Dec 17, 2011

'key' should be used for secret key, not for password of PBKDF. Arguments should be either of 'key and iv', 'password, salt and iv' and 'password'. The correct way is between encryptor and encryptor2...

This comment has been minimized.

Sign in to view

Copy link
@danpal

danpal Dec 17, 2011

Owner

Hm, the key here is really just a secret, not really a key in the cryptography sense. An example is :key => "secret". this would be translated as:

  •    cipher.key = OpenSSL::PKCS5.pbkdf2_hmac_sha1("secret", "unique_salt", 2000, cipher.key_len)

The reason to use the name :key is such that encryptor was already using key.
But please explain further if I messed up something...I didn't fully understood you.

This comment has been minimized.

Sign in to view

Copy link
@nahi

nahi Dec 19, 2011

In encryptor, :key is used as a secret key. In encryptor2, :key is used for a password for generating a secret key. PBKDF stands for Password-Based Key Derivation Function, so an input should be called 'password' and the output should be called 'key'.

encryptor2 does not handle :key as a key so it's not interoperable with other AES implementations...

This comment has been minimized.

Sign in to view

Copy link
@danpal

danpal Dec 19, 2011

Owner

Thanks, I need to think of a way of fixing this. It's going to break a lot of things, the reason :key was choosen is because I cloned this from:

https://github.com/shuber/encryptor

He was using :key to mean :pass, and worst not even using :iv or :salts.
Encryptor is used mostly in attr_encrypted:
https://github.com/shuber/attr_encrypted

In attr_encrypted you can't specify an :iv or a :salt so you will actually be doing:

 else
    cipher.pkcs5_keyivgen(options[:key])
  end

Here :key is also treated as a password, and even worse not being interoperable it is actually pretty insecure to do cbc_mode without an iv or salt.

But I agree, I need to fix this to make things more explicit.

This comment has been minimized.

Sign in to view

Copy link
@nahi

nahi Dec 19, 2011

Ah, I was not aware of that. Sorry, it's already used in wrong from the beginning...

OT: pkcs5_keyivgen sets key and iv that are generated from given password.

This comment has been minimized.

Sign in to view

Copy link
@danpal

danpal Dec 19, 2011

Owner

Could you explain how pkcs5_keyivgen works, it seems it uses

  EVP_BytesToKey(EVP_CIPHER_CTX_cipher(ctx), digest, salt, (unsigned char *)RSTRING_PTR(vpass), RSTRING_LEN(vpass), iter, key, iv);

to derive the :iv and the :key.

So the key and iv are going to be derived by concatenating multiple md5 hashes of the password. But without a :salt here this is totally insecure. If someone uses the same password multiple times, he will leak the first block of the cypher-text.
It seems this is confusingly documented.


/*
 *  call-seq:
 *     cipher.pkcs5_keyivgen(pass [, salt [, iterations [, digest]]] ) -> nil
 *
 *  Generates and sets the key/iv based on a password.
 *
 *  WARNING: This method is only PKCS5 v1.5 compliant when using RC2, RC4-40, or DES
 *  with MD5 or SHA1.  Using anything else (like AES) will generate the key/iv using an
 *  OpenSSL specific method.  Use a PKCS5 v2 key generation method instead.
 *
 *  === Parameters
 *  +salt+ must be an 8 byte string if provided.
 *  +iterations+ is a integer with a default of 2048.
 *  +digest+ is a Digest object that defaults to 'MD5'
 *
 *  A minimum of 1000 iterations is recommended.
 *
 */
static VALUE
ossl_cipher_pkcs5_keyivgen(int argc, VALUE *argv, VALUE self)
{
    EVP_CIPHER_CTX *ctx;
    const EVP_MD *digest;
    VALUE vpass, vsalt, viter, vdigest;
    unsigned char key[EVP_MAX_KEY_LENGTH], iv[EVP_MAX_IV_LENGTH], *salt = NULL;
    int iter;

    rb_scan_args(argc, argv, "13", &vpass, &vsalt, &viter, &vdigest);
    StringValue(vpass);
    if(!NIL_P(vsalt)){
        StringValue(vsalt);
        if(RSTRING_LEN(vsalt) != PKCS5_SALT_LEN)
            rb_raise(eCipherError, "salt must be an 8-octet string");
        salt = (unsigned char *)RSTRING_PTR(vsalt);
    }
    iter = NIL_P(viter) ? 2048 : NUM2INT(viter);
    digest = NIL_P(vdigest) ? EVP_md5() : GetDigestPtr(vdigest);
    GetCipher(self, ctx);
    EVP_BytesToKey(EVP_CIPHER_CTX_cipher(ctx), digest, salt,
                   (unsigned char *)RSTRING_PTR(vpass), RSTRING_LEN(vpass), iter, key, iv);
    if (EVP_CipherInit_ex(ctx, NULL, NULL, key, iv, -1) != 1)
        ossl_raise(eCipherError, NULL);
    OPENSSL_cleanse(key, sizeof key);
    OPENSSL_cleanse(iv, sizeof iv);

    return Qnil;
}

This comment has been minimized.

Sign in to view

Copy link
@nahi

nahi Dec 19, 2011

Yes, pkcs5_keyivgen uses fixed IV for encryption. And fixed IV + CBC mode is vulnerable when an attacker can do chosen text attack. You need to use another mode or Cipher#random_iv and keep the generated IV with key for decryption.
else
cipher.pkcs5_keyivgen(options[:key])
end
Expand Down
21 changes: 11 additions & 10 deletions test/encryptor_test.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,17 @@ class EncryptorTest < Test::Unit::TestCase
algorithms = %x(openssl list-cipher-commands).split
key = Digest::SHA256.hexdigest(([Time.now.to_s] * rand(3)).join)
iv = Digest::SHA256.hexdigest(([Time.now.to_s] * rand(3)).join)
salt = Time.now.to_i.to_s
original_value = Digest::SHA256.hexdigest(([Time.now.to_s] * rand(3)).join)

algorithms.reject { |algorithm| algorithm == 'base64' }.each do |algorithm|
encrypted_value_with_iv = Encryptor.encrypt(:value => original_value, :key => key, :iv => iv, :algorithm => algorithm)
encrypted_value_with_iv = Encryptor.encrypt(:value => original_value, :key => key, :iv => iv, :salt => salt, :algorithm => algorithm)
encrypted_value_without_iv = Encryptor.encrypt(:value => original_value, :key => key, :algorithm => algorithm)

define_method "test_should_crypt_with_the_#{algorithm}_algorithm_with_iv" do
assert_not_equal original_value, encrypted_value_with_iv
assert_not_equal encrypted_value_without_iv, encrypted_value_with_iv
assert_equal original_value, Encryptor.decrypt(:value => encrypted_value_with_iv, :key => key, :iv => iv, :algorithm => algorithm)
assert_equal original_value, Encryptor.decrypt(:value => encrypted_value_with_iv, :key => key, :iv => iv, :salt => salt, :algorithm => algorithm)
end

define_method "test_should_crypt_with_the_#{algorithm}_algorithm_without_iv" do
Expand All @@ -23,31 +24,31 @@ class EncryptorTest < Test::Unit::TestCase
end

define_method "test_should_encrypt_with_the_#{algorithm}_algorithm_with_iv_with_the_first_arg_as_the_value" do
assert_equal encrypted_value_with_iv, Encryptor.encrypt(original_value, :key => key, :iv => iv, :algorithm => algorithm)
assert_equal encrypted_value_with_iv, Encryptor.encrypt(original_value, :key => key, :iv => iv, :salt => salt, :algorithm => algorithm)
end

define_method "test_should_encrypt_with_the_#{algorithm}_algorithm_without_iv_with_the_first_arg_as_the_value" do
assert_equal encrypted_value_without_iv, Encryptor.encrypt(original_value, :key => key, :algorithm => algorithm)
end

define_method "test_should_decrypt_with_the_#{algorithm}_algorithm_with_iv_with_the_first_arg_as_the_value" do
assert_equal original_value, Encryptor.decrypt(encrypted_value_with_iv, :key => key, :iv => iv, :algorithm => algorithm)
assert_equal original_value, Encryptor.decrypt(encrypted_value_with_iv, :key => key, :iv => iv, :salt => salt, :algorithm => algorithm)
end

define_method "test_should_decrypt_with_the_#{algorithm}_algorithm_without_iv_with_the_first_arg_as_the_value" do
assert_equal original_value, Encryptor.decrypt(encrypted_value_without_iv, :key => key, :algorithm => algorithm)
end

define_method "test_should_call_encrypt_on_a_string_with_the_#{algorithm}_algorithm_with_iv" do
assert_equal encrypted_value_with_iv, original_value.encrypt(:key => key, :iv => iv, :algorithm => algorithm)
assert_equal encrypted_value_with_iv, original_value.encrypt(:key => key, :iv => iv, :salt => salt, :algorithm => algorithm)
end

define_method "test_should_call_encrypt_on_a_string_with_the_#{algorithm}_algorithm_without_iv" do
assert_equal encrypted_value_without_iv, original_value.encrypt(:key => key, :algorithm => algorithm)
end

define_method "test_should_call_decrypt_on_a_string_with_the_#{algorithm}_algorithm_with_iv" do
assert_equal original_value, encrypted_value_with_iv.decrypt(:key => key, :iv => iv, :algorithm => algorithm)
assert_equal original_value, encrypted_value_with_iv.decrypt(:key => key, :iv => iv, :salt => salt, :algorithm => algorithm)
end

define_method "test_should_call_decrypt_on_a_string_with_the_#{algorithm}_algorithm_without_iv" do
Expand All @@ -56,8 +57,8 @@ class EncryptorTest < Test::Unit::TestCase

define_method "test_string_encrypt!_on_a_string_with_the_#{algorithm}_algorithm_with_iv" do
original_value_dup = original_value.dup
original_value_dup.encrypt!(:key => key, :iv => iv, :algorithm => algorithm)
assert_equal original_value.encrypt(:key => key, :iv => iv, :algorithm => algorithm), original_value_dup
original_value_dup.encrypt!(:key => key, :iv => iv, :salt => salt, :algorithm => algorithm)
assert_equal original_value.encrypt(:key => key, :iv => iv, :salt => salt, :algorithm => algorithm), original_value_dup
end

define_method "test_string_encrypt!_on_a_string_with_the_#{algorithm}_algorithm_without_iv" do
Expand All @@ -68,7 +69,7 @@ class EncryptorTest < Test::Unit::TestCase

define_method "test_string_decrypt!_on_a_string_with_the_#{algorithm}_algorithm_with_iv" do
encrypted_value_with_iv_dup = encrypted_value_with_iv.dup
encrypted_value_with_iv_dup.decrypt!(:key => key, :iv => iv, :algorithm => algorithm)
encrypted_value_with_iv_dup.decrypt!(:key => key, :iv => iv, :salt => salt, :algorithm => algorithm)
assert_equal original_value, encrypted_value_with_iv_dup
end

Expand Down Expand Up @@ -101,4 +102,4 @@ def test_should_yield_block_with_cipher_and_options
assert called
end

end
end

0 comments on commit c1b7e39

Please sign in to comment.