hs: Use AES256 prop224 descriptors
Support got added recently so use it in hs_descriptor.c instead of AES128. Proposal 224 has been updated to use aes256.
* Instantiate STREAM with AES256-CTR.Activity
changed milestone to %Tor: 0.3.0.x-final
NAK. This doesn't increase the length of secret_key in encrypt_descriptor_data, or the length of secret_key in desc_decrypt_data_v3. So you're still passing a 128-bit value to the AES constructor.
You need to make sure that the inputs that are supposed to be 256-bit keys really are 32 bytes long.
Trac:
Status: merge_ready to needs_revisionA couple thoughts, feel free to take or leave what is useful:
-
The iv for AES Counter Mode (in principle) does not add additional security properties if it is secret. I see that the iv in encrypt_descriptor_data and desc_decrypt_data_v3 is named secret_iv, but this is misleading if it does not need to be secret. (Please correct me if this is not the case for this protocol)
-
As far as options to ensure the expected key length is being used, here are a couple:
a) One option could be to write unit tests which mock crypto_cipher_new_with_iv_and_bits (for example) and test functions like build_encrypted, asserting in the test that crypto_cipher_new_with_iv_and_bits is called with the expected key size. b) This would require more thought/better naming, but another option could be to centralize hs configuration, such as key size, iv length, etc. For example, these could be constant values such as HS_DESCRIPTOR_KEY_LENGTH, or a function such as get_hs_configuration(). This can be implemented in other ways, but the general idea is to minimize the number of places which have to change if we need to update values such as key size again in the future.
-
Replying to chelseakomlo:
A couple thoughts, feel free to take or leave what is useful:
- The iv for AES Counter Mode (in principle) does not add additional security properties if it is secret. I see that the iv in encrypt_descriptor_data and desc_decrypt_data_v3 is named secret_iv, but this is misleading if it does not need to be secret. (Please correct me if this is not the case for this protocol)
So good question. It could be indeed misleading but I'll explain where it comes from and see. http://jqs44zhtxl2uo6gk.onion/torspec.git/tree/proposals/224-rend-spec-ng.txt#n953
From the blinded key which is ultimately the .onion with some computation made with it, you derive some bytes (KDF) and out of which we extract the key/iv and then used to decrypt the encrypted section in the descriptor. Only the client and service can do such a thing as they know the .onion address. So considering that we have at least two actors accessing it, I'm not sure if I would qualify this as "secret". It is secret in the sense that you need so prior knowledge to derive that key.
Makes sense. Do you have a better name in mind?
- As far as options to ensure the expected key length is being used, here are a couple:
a) One option could be to write unit tests which mock crypto_cipher_new_with_iv_and_bits (for example) and test functions like build_encrypted, asserting in the test that crypto_cipher_new_with_iv_and_bits is called with the expected key size.
Yes! I'm personally not liking that function naming as well and how the API is kind of non symmetric for 128 and 256 bit. But I would recommend opening a ticket for this on refactoring and better testing our stream cipher API which is "clunky".
b) This would require more thought/better naming, but another option could be to centralize hs configuration, such as key size, iv length, etc. For example, these could be constant values such as HS_DESCRIPTOR_KEY_LENGTH, or a function such as get_hs_configuration(). This can be implemented in other ways, but the general idea is to minimize the number of places which have to change if we need to update values such as key size again in the future.
Yes, the current name of the #define value is really not good... not sure what I was thinking there. I would go with something like
HS_DESC_ENCRYPTED_BIT_SIZE 256. It has to be a bit size as our API takes bits because the "key length" semantic is in bytes in our code like#define CIPHER_KEY_LEN 16. Although, we could make this nicer with a refactor of a).Note that with client authorization coming up, we'll most likely end up with a super encrypted section that is the encrypted section of the descriptor inside another encrypted section for client authorization. Chances are that they will be same cipher but very different semantic so I wouldn't be surprise that we could end up with:
HS_DESC_ENCRYPTED_BIT_SIZE 256andHS_DESC_SUPERENCRYPTED_BIT_SIZE 256Ok, I won't make any of the suggested fixes to the branch yet because asn raised concerns with AES-256 specifically regarding the size of the output and how it will influence the final size of the descriptor. Let's figure that one out and then fix our code.
Trac:
Keywords: easy deleted, N/A added
Status: needs_review to needs_information
Replying to dgoulet:
Replying to chelseakomlo:
Ok, I won't make any of the suggested fixes to the branch yet because asn raised concerns with AES-256 specifically regarding the size of the output and how it will influence the final size of the descriptor. Let's figure that one out and then fix our code.
FWIW, after all there are no serious concerns about AES-256 and the size of the output, so I think it's fine to use AES-256.
moved to tpo/core/tor#20569 (closed)