OpenSSL::Cipher::Cipher produces garbage result if #encrypt is called after setting key

[tarcieri]

OpenSSL::Cipher::Cipher ostensibly depends on #encrypt or #decrypt being called first before setting #key=. However, if #key= is called first, it seems to be silently replaced with a different value, and the encrypt operation instead produces a garbage result.

Personally I would prefer #key= to raise an exception unless #encrypt or #decrypt has already been called. A garbage encryption is undesirable and confusing.

Repro code:

require 'openssl'

# AES-128 ECB mode test vectors
# Taken from https://boringssl.googlesource.com/boringssl/+/2272/crypto/cipher/cipher_test.txt#24
KEY        = ["2b7e151628aed2a6abf7158809cf4f3c"].pack("H*")
PLAINTEXT  = ["6bc1bee22e409f96e93d7e117393172a"].pack("H*")
CIPHERTEXT = ["3ad77bb40d7a3660a89ecaf32466ef97"].pack("H*")

cipher = OpenSSL::Cipher::Cipher.new("aes-128-ecb")
cipher.key = KEY
cipher.padding = 0 # Padding is enabled by default, but makes no sense with ECB mode

print "Testing encryption: "

cipher.encrypt
ciphertext = cipher.update(PLAINTEXT) << cipher.final

if ciphertext == CIPHERTEXT
  puts "OK!"
else
  puts "FAILED! Got #{ciphertext.inspect} instead of #{CIPHERTEXT.inspect}"
end

[tarcieri]

Before anyone asks, yes I know ECB mode lets you see penguins. I actually ran into this three years ago trying to do the Matasano crypto challenges in Ruby.

[rhenium]

An open ticket at bugs.ruby-lang.org for the same problem: https://bugs.ruby-lang.org/issues/8720
/cc @zzak

I fully agree the current behavior is not good, but I unfortunately haven't come up with a good solution.

A straightforward fix would be to introduce is_initialized flag and check it in all methods, or, defer the actual initialization of EVP_CIPHER_CTX until Cipher#encrypt or Cipher#decrypt is called.

But, since we currently allow calling Cipher#encrypt and Cipher#decrypt multiple times to change the mode (at least the test code does this, though the docs don't say explicitly), they don't fix the entire problem. I'm afraid a halfway workaround might be even more confusing.

Another idea is to discourage use of Cipher#{encrypt,decrypt}, and allow specifying the mode in Cipher.new (like this: Cipher.new("aes-128-ecb", :encrypt)) or to introduce new dedicated factory methods for each mode (Cipher.encrypt and Cipher.decrypt). But people may want to stick to the current interface because of the compatibility with older versions of Ruby and continue generating garbage.

Any ideas?

[sevk]

how to fix this ?

[jeremyevans]

This should be fixed by #263.