Opened 4 years ago

Closed 4 years ago

#20569 closed defect (fixed)

hs: Use AES256 prop224 descriptors

Reported by: dgoulet Owned by: dgoulet
Priority: High Milestone: Tor: 0.3.0.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-hs, prop224, TorCoreTeam201612, review-group-14
Cc: Actual Points:
Parent ID: Points: 0.1
Reviewer: nickm Sponsor: SponsorR-must

Description

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.

Child Tickets

Change History (17)

comment:1 Changed 4 years ago by dgoulet

Keywords: TorCoreTeam201611 added

comment:2 Changed 4 years ago by dgoulet

Status: newmerge_ready

See branch bug20569_030_01.

Test passes correctly with this.

comment:3 Changed 4 years ago by nickm

Status: merge_readyneeds_revision

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.

comment:4 Changed 4 years ago by dgoulet

Status: needs_revisionneeds_review

Very good catch! Fixup commit in bug20569_030_01.

Tests passes with expensive hardening as well. I'm very open to suggestion on how to make this bulletproof or any refactoring that could make us NOT make that mistake again.

comment:5 Changed 4 years ago by chelseakomlo

A couple thoughts, feel free to take or leave what is useful:

  1. 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)
  1. 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.

comment:6 Changed 4 years ago by nickm

Keywords: review-group-12 added

comment:7 in reply to:  5 ; Changed 4 years ago by dgoulet

Keywords: easy removed
Status: needs_reviewneeds_information

Replying to chelseakomlo:

A couple thoughts, feel free to take or leave what is useful:

  1. 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?

  1. 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 256 and
HS_DESC_SUPERENCRYPTED_BIT_SIZE 256

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.

comment:8 Changed 4 years ago by dgoulet

Owner: set to dgoulet
Status: needs_informationaccepted

Taking ownership and will fix soon the issues from feedback above.

comment:9 in reply to:  7 Changed 4 years ago by asn

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.

comment:10 Changed 4 years ago by nickm

Keywords: review-group-12 removed

comment:11 Changed 4 years ago by dgoulet

Keywords: TorCoreTeam201612 added; TorCoreTeam201611 removed
Status: acceptedneeds_revision

comment:12 Changed 4 years ago by dgoulet

Status: needs_revisionneeds_review

I've rebased on current master and made the change for a better name for the bit size define.

See branch: bug20569_030_02

comment:13 Changed 4 years ago by nickm

Keywords: review-group-14 added

comment:14 Changed 4 years ago by nickm

Status: needs_reviewneeds_revision

Looks good. Two tweaks:

  • AES256-CTR doesn't use a 256-bit IV. Only the key is 256 bit.
  • Maybe instead of using CIPHER256_KEY_LEN all over hs_descriptor.c, it would be a good idea to have a HS_DESC_ENC_KEY_LEN or something?

comment:15 Changed 4 years ago by dgoulet

Status: needs_revisionneeds_review

Thanks nickm!

Fixup commit 78bef6d5 addressing the above. Still in branch bug20569_030_02.

comment:16 Changed 4 years ago by nickm

Reviewer: nickm

comment:17 Changed 4 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Merged, and also removed the definition of CIPHER256_IV_LEN.

Note: See TracTickets for help on using tickets.