Fix vulnerabilities #16
Conversation
|
|
||
| gem.files = `git ls-files`.split("\n") |
There was a problem hiding this comment.
I mildly prefer the git ls-files-based version, as it honours .gitignore.
| def self.encrypt(message, password) | ||
| Base64.encode64(self.encrypt_data(message.to_s.strip, self.key_digest(password), nil, "AES-256-CBC")) | ||
| def self.encrypt(message, password, salt = nil, iv = nil) | ||
| iv ||= SecureRandom.bytes 16 |
There was a problem hiding this comment.
I don't know the SecureRandom implementation without checking. But I notice that in the README.md you suggest using OpenSSL::Random.random_bytes(16). Based on knowing OpenSSL a bit, I'd use the latter.
There was a problem hiding this comment.
SecureRandom.bytes only for ruby 2.4 ?
| def self.key_digest(password) | ||
| OpenSSL::Digest::SHA256.new(password).digest | ||
| def self.key_digest(password, salt = nil) | ||
| salt ||= SecureRandom.bytes 32 |
There was a problem hiding this comment.
See above: choose either SecureRandom or OpenSSL::Random.
| def self.key_digest(password, salt = nil) | ||
| salt ||= SecureRandom.bytes 32 | ||
| digest = OpenSSL::Digest::SHA256.new | ||
| return salt, OpenSSL::PKCS5.pbkdf2_hmac(password, salt, 10000, digest.digest_length, digest) |
There was a problem hiding this comment.
An HMAC is a message authentication code. You probably took this from the PKCS5 example.
It's not that what you're doing is wrong, exactly, but I'm not sure it is what you need. The purposes of an HMAC is that given a shared secret (here: the salt), you can authenticate that a message (here: the password) hasn't been tampered with.
You would normally send the message and the digest across an insecure medium, relying on the HMAC (and the shared secret that's been communicated securely beforehand) to verify the message has not been tampered with.
The example also suggests to generate cipher keys this way (that's questionable), and to store passwords that way.
Storing passwords in the form of salt and HMAC works just fine. If that's your intended use, go for it.
Generating cipher keys with an HMAC is simply unnecessary, and forces you to also deal with the salt.
| Base64.encode64(self.encrypt_data(message.to_s.strip, self.key_digest(password), nil, "AES-256-CBC")) | ||
| def self.encrypt(message, password, salt = nil, iv = nil) | ||
| iv ||= SecureRandom.bytes 16 | ||
| salt, key = self.key_digest(password, salt) |
There was a problem hiding this comment.
See my comment below; an HMAC (with salt) is unnecessary for generating cipher keys.
The point of a key derivation function is to expand or contract the bit length of the key to something that's the block length of cipher. If slightly different keys yield vastly different results from the key derivation function, that's also a benefit. So running the key through such a function is great!
But the use of an HMAC requires you to deal with the salt; a simple SHA2-256 for AES-256 would be sufficient and no salt would be required. You can hash for a few iterations much like the OpenSSL::PKCS5.pbkdf2_hmac function does, but that's about all that's required.
| aes = OpenSSL::Cipher::Cipher.new(cipher_type) | ||
| self.warn_if_ecb cipher_type | ||
|
|
||
| aes = OpenSSL::Cipher.new(cipher_type) |
There was a problem hiding this comment.
Technically, aes is not necessarily the right variable name, as cipher_type might specify non-AES.
| aes = OpenSSL::Cipher::Cipher.new(cipher_type) | ||
| self.warn_if_ecb cipher_type | ||
|
|
||
| aes = OpenSSL::Cipher.new(cipher_type) |
There was a problem hiding this comment.
Technically, aes is not necessarily the right variable name, as cipher_type might specify non-AES.
| aes = OpenSSL::Cipher::Cipher.new(cipher_type) | ||
| self.warn_if_ecb cipher_type | ||
|
|
||
| aes = OpenSSL::Cipher.new(cipher_type) | ||
| aes.decrypt | ||
| aes.key = key |
There was a problem hiding this comment.
Perhaps run the key derivation function (mentioned above) here, so that all uses of decrypt_data are safe, not just the simple uses. But perhaps you want the advanced functions to stay advanced. So alternatively assert that they key is non-nil and of the cipher's block length (some ciphers require different lengths).
| aes = OpenSSL::Cipher::Cipher.new(cipher_type) | ||
| self.warn_if_ecb cipher_type | ||
|
|
||
| aes = OpenSSL::Cipher.new(cipher_type) | ||
| aes.encrypt | ||
| aes.key = key |
There was a problem hiding this comment.
Perhaps run the key derivation function (mentioned above) here, so that all uses of encrypt_data are safe, not just the simple uses. But perhaps you want the advanced functions to stay advanced. So alternatively assert that they key is non-nil and of the cipher's block length (some ciphers require different lengths).
|
|
||
| private | ||
| def self.warn_if_ecb(cipher_type) | ||
| if cipher_type.downcase.include? 'ecb' |
There was a problem hiding this comment.
And if it includes 'aes'.
Actually, ECB mode is not a good choice with any cipher :)
I'm not sure this will ever be merged, but at least it gives me the ability to review/comment :)