-
Notifications
You must be signed in to change notification settings - Fork 165
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Authenticated Encryption should check for tag length #63
Comments
@rhenium I am not sure about other ciphers, but the current implementation also has a default value of 16, see openssl/ext/openssl/ossl_cipher.c Line 575 in 3338994
If we add other AE ciphers in the future we could still change the default value depending on the used cipher.
I just want to make sure that users of There is another problem with
If we make sure that the API has secure defaults, I think we can limit the amount of additional documentation to a minimum. |
This issue was identified at the next link It seems to me that the idea to use secure defaults set to the maximum authentication tag size (16 bytes, the block size of AES) is the right one. Shorter tag lengths quickly degrade the security of GCM, I would advice that this information is included in the documentation for setting smaller tag sizes. Note that this is both a very much needed bug fix as well as one that may break compatibility for shorter tag sizes (although those implementations may also be vulnerable right now). I would recommend saying so in release notes in case anything breaks functionally speaking. Maarten |
That's one way to do it - always use a fixed size tag. But the overall scheme may have gaps.
NIST allows tag sizes as small as 32-bits, but they caution against it. 64-bit is usually the floor of the range, so they range in practice is usually
Yes, it should derive a unique key. That is, version 1 of the protocol should derive a distinct key from version 2 of the protocol. That's by design. To avoid future problems, you should consider making all security parameters contribute to the keying material. For example:
Then, when you have a secret (from Password Derivation, Key Encapsulation, Diffie-Hellman, etc), use the security parameters and a usage label:
Using the version information and providing unique keys per version is important to stop downgrade attacks. For example, see Attacking and Repairing the WinZip Encryption Scheme. You might also want to look at Kelsey's work on Truncated Hashes. Modern hashes, like BLAKE2, use the truncated size as a security parameter, so the hashes are distinct depending on the output hash size). |
Thank you for creating this issue. As a general wrapper for EVP_CIPHER_CTX, a cipher-specific code or magic numbers should be minimized. In this point, in my opinion, the current default value of 16 in Cipher#auth_tag wasn't a good decision. It's hard to change once added API. Changing the default value will certainly break any existing applications using AES-GCM. And who decide what cipher is the most used one? Although OpenSSL::Cipher doesn't support now (our API doesn't fit, needs consideration), there is an usage of AES-CCM with 8 bytes authentication tag in TLS. |
If I read the code correctly, the only AE cipher that is implemented is currently AES-GCM. Please correct me if I'm wrong. I honestly don't see any problem with the default value of 16 for
Other ciphers could set different default values for cipher = OpenSSL::Cipher.new('aes-256-gcm')
cipher.auth_tag_len #=> 16
# ...
cipher = OpenSSL::Cipher.new('aes-256-ccm')
cipher.auth_tag_len #=> 8
# ... |
@rhenium I agree that having cipher specific code in the general |
It was. With 557e2de, if you build ext/openssl with a capable OpenSSL, (for instance) ChaCha20-Poly1305 should work.
It doesn't resolve the problem. Users may use newer OpenSSL or use engines. What should be the default value for unknown AE ciphers? |
16 bytes is the only sensible default which is mostly backwards compatible with the current 16 byte authentication tag generated during GCM encryption, and setting no default would break backwards compatibility. So I don't think there is any choice here. I'd say that defining the default as the maximum value possible would create a clear indication for future AEAD ciphers, although most will likely be 128 bit anyway (as 128 bit is the most used block size at the moment). |
+1
|
For Cipher#auth_tag(n), I'm not seeing the need to change the default length now, though I don't think it should have a default value.
As far as I know OpenSSL does not give the information of the maximal possible length. We should avoid any heuristics in this library, especially in primitives. If we introduce a tag length check, it should be in each subclass of OpenSSL::Cipher because it can't be common to all AE ciphers, and should be done always with the maximal allowed in the algorithm. |
I think changing tag lengths and addressing the truncation attacks and potential forgeries are two separate issues. I think the thrust of @padde's issue was due to the latter. My apologies if I am not parsing things correctly. |
There seems to be some misunderstanding about what I am trying to suggest. I don't want to change any default values, I just care about preventing truncation attacks. The reason I suggested the default value (16 bytes/128 bits) is to keep backwards compatibility.
@rhenium how about we still add We can then additionally set default values for known ciphers (whitelist), which provides backwards compatibility. If possible, we can move the defaults into specialized subclasses which seems the cleanest solution. However, if this means we don't get the default values with
@tarcieri although I get your point, I think we should not enforce a lower bound. Ruby-OpenSSL is supposed to be a general purpose tool, and it should be able to decrypt/authenticate any valid AES-GCM ciphertext. This includes ciphertext with auth tags shorter than what would be considered secure. For the same reason, OpenSSL will not prevent you from using ECB mode and so on. I think the point is to provide secure defaults, while keeping backwards compatibility and full flexibility. |
Add a note about GCM mode - warn of the risk of reusing nonce and authentication tag truncation. [GH ruby#63]
Add a note about GCM mode - warn of the risk of reusing nonce and authentication tag truncation. [GH #63]
Sorry for the confusion. It seems I misread @owlstead's comment.
Why should the auth_tag_len be required even when encrypting? I don't think users want to be requested to set the expected tag length just for a fail-safe (and especially when the algorithm doesn't need such check). I feel it could lead lazy users to write such code to suppress exceptions:
A white list is not an option for the generic OpenSSL::Cipher. It requires us to keep up to date forever which we want to avoid. Even if we could, it still can't cover ciphers added by engines. So, the only possible is to add subclasses and encourage the use of them in the documentation. By the way, I'm going to include the documentation update anyway in the next release (hopefully in a week). Could you check it? |
To me this seems to imply that you have to keep the IV as secret as the key, while the "Choosing an IV" section of the docs explain this is not the case. It also might be confusing to suddenly switch to |
@bdewater Thanks for reviewing!
Right, it is confusing.
I intentionally renamed it to diff --git a/ext/openssl/ossl_cipher.c b/ext/openssl/ossl_cipher.c
index c09fb2d..67bbd64 100644
--- a/ext/openssl/ossl_cipher.c
+++ b/ext/openssl/ossl_cipher.c
@@ -958,9 +958,11 @@ Init_ossl_cipher(void)
* could otherwise be exploited to modify ciphertexts in ways beneficial to
* potential attackers.
*
- * If no associated data is needed for encryption and later decryption,
- * the OpenSSL library still requires a value to be set - "" may be used in
- * case none is available.
+ * An associated data is used where there is additional information, such as
+ * headers or some metadata, that must be also authenticated but not
+ * necessarily need to be encrypted. If no associated data is needed for
+ * encryption and later decryption, the OpenSSL library still requires a
+ * value to be set - "" may be used in case none is available.
*
* An example using the GCM (Galois/Counter Mode). You have 16 bytes +key+,
* 12 bytes (96 bits) +nonce+ and the associated data +auth_data+. Be sure
@@ -975,9 +977,9 @@ Init_ossl_cipher(void)
* encrypted = cipher.update(data) + cipher.final
* tag = cipher.auth_tag # produces 16 bytes tag by default
*
- * Now you are the receiver. You know the +key+ and +nonce+, and have
- * received +encrypted+ and +tag+ through an untrusted network. Note that
- * GCM accepts an arbitrary length tag between 1 and 16 bytes. You may
+ * Now you are the receiver. You know the +key+ and have received +nonce+,
+ * +auth_data+, +encrypted+ and +tag+ through an untrusted network. Note
+ * that GCM accepts an arbitrary length tag between 1 and 16 bytes. You may
* additionally need to check that the received tag has the correct length,
* or you allow attackers to forge a valid single byte tag for the tampered
* ciphertext with a probability of 1/256. Does this fix them? Or you may have a better idea. |
+1 on using |
As described in ruby/openssl#63 without this check, only a single byte needs to be supplied to make the authentication pass. This means that an attacker needs at most 256 attempts in order to forge a valid authentication tag. The JWE spec example prescribes 128 bits (16 bytes) for the tag: https://tools.ietf.org/html/rfc7516#section-3.3
This cipher suite is an authenticated encryption with associated data (AEAD) one. Keys that are prefixed with 'AES-256-GCM:' will use this new Cryptor. This is an abstraction and technique created after the Fernet coding, which attr_vault borrows in part, was designed. This predecessor coding is AES-128-CBC with an extra HMAC, to attain authentication. AES-256-GCM eliminates the HMAC as a separate cryptography primitive, and the decryption and authentication check are thus simplified. A separate HMAC secret is no longer required, so the entire key text can be used to expand the key width, to 256 bits, for AES-256. An important attack vector to close is truncation of the "authentication tag" (`auth_tag`), which can cause any message to authenticate. Its length is checked, and if it is not the specification maximum (16), the ciphertext is rejected. For more details, see ruby/openssl#63.
This cipher suite is an authenticated encryption with associated data (AEAD) one. Keys that are prefixed with 'AES-256-GCM:' will use this new Cryptor. This is an abstraction and technique created after the Fernet coding, which attr_vault borrows in part, was designed. This predecessor coding is AES-128-CBC with an extra HMAC, to attain authentication. AES-256-GCM eliminates the HMAC as a separate cryptography primitive, and the decryption and authentication check are thus simplified. A separate HMAC secret is no longer required, so the entire key text can be used to expand the key width, to 256 bits, for AES-256. An important attack vector to close is truncation of the "authentication tag" (`auth_tag`), which can cause any message to authenticate. Its length is checked, and if it is not the specification maximum (16), the ciphertext is rejected. For more details, see ruby/openssl#63.
The current API for using ciphers with Authenticated Encryption (currently only AES-GCM) is rather misleading and quickly leads to subtle bugs related to the length of
auth_tag
.In particular, the current implementation will not check for the length of the
auth_tag
. Because GCM mode allows arbitrary sizes of theauth_tag
up to 128 bytes, only a single byte needs to be supplied to make the authentication pass. This means that an attacker needs at most 256 attempts in order to forge a validauth_tag
.Currently, the only way to prevent such attacks is to manually assert the correct
auth_tag
length when decrypting/authenticating.I suggest the following improvements:
Documentation should mention the importance of manually checking
auth_tag
lengthThis can/should be done immediately even if the API should not change.
Authentication tag length should be an input parameter to the cipher
To improve the usability of the API and unburden users from performing additional manual checks without compromising security, I suggest to add an
auth_tag_len
accessor. This can be used to determine the size of theauth_tag
both when generating and when authenticating theauth_tag
. The default value should be 16 bytes (see below).#auth_tag
should useauth_tag_len
to determine the output lengthDuring encryption:
If no parameter is given,
#auth_tag
should return an authentication tag according to the length configured inauth_tag_len
.If a length parameter is given,
#auth_tag
should use the supplied parameter to determine the length of the authentication tag. Although this parameter is not as useful any more it should be kept for backwards compatibility. Maybe it should be deprecated.Currently the API supports different tag lengths by passing the length parameter to
#auth_tag
. This currently defaults to 16 bytes, which should be the default value forauth_tag_len
in order to keep backwards compatibility.#final
should useauth_tag_len
to assert the correct length of theauth_tag
During decryption:
auth_tag_len
should be used to assert that the suppliedauth_tag
has the correct length. The big difference to the existing API lies here, because users need to actively change the value ofauth_tag_len
in order to allow shorter tags.When the check fails, an
OpenSSL::Cipher::CipherError
should be raised. The same type of error is already raised when authentication fails, so existing users should be fine without having to touch their error handling. A descriptive error message should be helpful. In order to distinguish between such errors and "actual" verification errors, we could also add a descriptive message for the latter.I'd be happy to implement these changes, but I wanted to discuss them first.
For reference: This issue was copied over from the Ruby Issue Tracker https://bugs.ruby-lang.org/issues/12582
The text was updated successfully, but these errors were encountered: