[tor-bugs] #20569 [Core Tor/Tor]: hs: Use AES256 prop224 descriptors

[Tor Bug Tracker & Wiki]

#20569: hs: Use AES256 prop224 descriptors
-------------------------------------------------+-------------------------
 Reporter:  dgoulet                              |          Owner:
     Type:  defect                               |         Status:
                                                 |  needs_information
 Priority:  High                                 |      Milestone:  Tor:
                                                 |  0.3.0.x-final
Component:  Core Tor/Tor                         |        Version:
 Severity:  Normal                               |     Resolution:
 Keywords:  tor-hs, prop224, TorCoreTeam201611,  |  Actual Points:
  review-group-12                                |
Parent ID:                                       |         Points:  0.1
 Reviewer:                                       |        Sponsor:
                                                 |  SponsorR-must
-------------------------------------------------+-------------------------
Changes (by dgoulet):

 * status:  needs_review => needs_information
 * keywords:  tor-hs, easy, prop224, TorCoreTeam201611, review-group-12 =>
     tor-hs, prop224, TorCoreTeam201611, review-group-12


Comment:

 Replying to [comment:5 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?

 >
 > 2. 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.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/20569#comment:7>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online