prop224: Add a trunnel subdirectory specifically for HS
We'll be using trunnel for all cells parsing in proposal 224. This ticket is to add a subdirectory in src/trunnel/hs that will contain one .trunnel file per cell and most likely a common file for shared binary structure.
This is the starting point for #17241 (moved) so if we can get this upstream as soon as possible, it makes thing much more easier to split the work between developers and branches instead of carrying a commit that adds that directory to the build system.
The plan is to add two cell definition at first ESTABLISH_INTRO and INTRODUCE1 and then add more as we implement them.
Activity
changed milestone to %Tor: 0.3.0.x-final
Started here.
ESTABLISH_INTROtrunnel file is done (inspired by patch in #19043 (moved)). MissingINTRODUCE1cell and we should be good.Branch:
ticket20004_029_01-
Ok I ended up adding all cells from 224 which is 4 of them:
- ESTABLISH_INTRO
- INTRO_ESTABLISHED
- INTRODUCE1 (also INTRODUCE2)
- INTRODUCE_ACK
Reminder that this branch only adds trunnel definition for those cells.
Still branch:
ticket20004_029_01Trac:
Status: new to needs_review
Actualpoints: N/A to 0.5 
I think that the trunnel files carry outdated definitions, or well there seems to be a disconnect between the generated code and the trunnel defs.
Example trunnel def:
/* ESTABLISH_INTRO payload. See details in section 3.1.1 */ struct hs_cell_establish_intro { /* Indicate the start of the handshake authentication data. */ @ptr start_mac_data; /* Authentication key material. */ u8 auth_key_type IN [0x00, 0x01, 0x02]; u16 auth_key_len; u8 auth_key[auth_key_len]; /* Extension(s). Reserved fields. */ struct cell_extension extensions; /* Handshake type. Size of SHA3-256. */ u8 handshake[32]; /* Signature */ u16 sig_len; /* Indicate the end of the handshake authentication data. */ @ptr end_mac_data; u8 sig[sig_len]; };Generated code:
struct hs_cell_establish_intro_st { const uint8_t *start_mac_data; uint8_t auth_key_type; uint16_t auth_key_len; TRUNNEL_DYNARRAY_HEAD(, uint8_t) auth_key; struct cell_extension_st *extensions; uint8_t handshake_mac[SHA3_256_LEN]; uint16_t sig_len; const uint8_t *end_mac_data; TRUNNEL_DYNARRAY_HEAD(, uint8_t) sig; uint8_t trunnel_error_code_; };See how
handshakeishandshake_macin the code.Trac:
Status: needs_review to needs_revision-
Another issue with ESTABLISH_INTRO:
In the spec we say:
The HANDSHAKE_AUTH field contains the MAC of all earlier fields in the cell using as its key the shared per-circuit material ("KH") generated during the circuit extension protocol; see tor-spec.txt section 5.2, "Setting circuit keys". It prevents replays of ESTABLISH_INTRO cells.In this case,
end_mac_datashould be right before thehandshake_macfield and not in the end. Also, there should probably be anotherptrcalledend_sig_dataright before the sig. Or do you think the spec is wrong?Here is how Alex had his pointers, and that's how I have it in my code. Let's keep the same ptr position if possible:
struct hs_establish_intro_cell { @ptr start_cell; u8 auth_key_type; u16 auth_key_len; u8 auth_key[auth_key_len]; u8 n_extensions; struct extension extensions[n_extensions]; @ptr end_mac_fields; /* Modify if any new handshake types are added */ union handshake[auth_key_type] { 2: u8 sha3_256[SHA3_256_MAC_LEN]; default: fail; }; u16 siglen; @ptr end_sig_fields; u8 sig[siglen]; };Trac:
Status: needs_revision to needs_review -
I think there is a problem with the linking as well. I'm getting the following errors when I try to compile my code on top of your branch:
src/or/libtor.a(hs_intropoint.o): In function `get_auth_key_from_establish_intro_cell': /home/tor/src/or/hs_intropoint.c:34: undefined reference to `hs_cell_establish_intro_getarray_auth_key' src/or/libtor.a(hs_intropoint.o): In function `handle_establish_intro': /home/tor/src/or/hs_intropoint.c:155: undefined reference to `hs_cell_establish_intro_parse' src/or/libtor.a(hs_intropoint.o): In function `verify_establish_intro_cell': /home/tor/src/or/hs_intropoint.c:74: undefined reference to `hs_cell_establish_intro_getarray_sig'If I explicitly include the
src/trunnel/hs/libor-trunnel-hs.alibrary in the gcc command it works. So maybe there is an issue with theLIBADD?Did you manage to compile fine with this branch?
-
I fixed both issues above that is the missing end mac fields pointer. I renamed it to the above. Note that the
unionis gone though because not needed.Linking issues should be resolved:
ticket20004_029_02(This required some changes to the commits so I had to rebase squash else it wouldn't have merged together nor fixup commit worked.)
Trac:
Status: needs_revision to needs_review -
Also, in
cell_introduce1.trunnel:-
I don't see anywhere the
CLIENT_PKorMACfields of theINTRODUCE1cell as specified in section[NTOR-WITH-EXTRA-DATA]. Also, if we have a MAC we also need the appropriateptrs. -
s/Link specificer/Link specifier/ -
Also can we replace the magic number of
rend_cookie[20]with some sort of name? The actual code is usingREND_COOKIE_LEN, but I guess we can't reuse that. No worries if there is no good solution here, just leave it as is.
Actual_review_points += 0.4 -
-
Bumping priority and scheduling this for October work. It will be quite intertwined with #19043 (moved).
Trac:
Priority: Medium to High
Keywords: nickm-deferred-20161005 deleted, TorCoreTeam201610 added -
Hello,
please see my branch
ticket20004_rebased. It's basically David's branch with the following changes:- It's rebased to latest tor git master.
- It's rebased to latest trunnel git master.
- It fixes the issues pointed out in comment:10 and comment:11.
(WRT comment:11, the patch does not actually add the MAC field to the encrypted part of
INTRODUCE1, as the current format is:CLIENT_PK [G_LEN bytes] ENCRYPTED_DATA [Padded to length of plaintext] MAC [MAC_LEN bytes]and there is no way to specify
Padded to length of plaintextin trunnel when it's not the last element in the struct AFAIK. During implementation, we can do this parsing on our own, or if we want trunnel to do it for us, we should probably add an ENCRYPTED_DATA_LEN field.)Trac:
Status: needs_revision to needs_review -
Small fix I made with regard to the onion key type, I narrowed down the values with:
+ u8 onion_key_type IN [0x01];The rest lgtmy! I think this is ready to merge as it has no impact on current tor code at all and we can make fixes if needed as we continue prop224 implementation. So I'm going to put that in
merge_ready.Branch is located in
ticket20004_030_01, rebased on current master. My change is top fixup commit.Trac:
Status: needs_review to merge_ready 
So, I think that using a generic cell_extensions mechanism is wrong long term but okay for now. Once we are using different cell extensions, we will want to have the actual body of the extension parsed based on the field_type's value, which means that using a generic "extension" type won't work.
But I think it's okay for now, since we don't have specified extensions yet, right?
Minor comments:
- should sig_len be inside end_sig_fields? If this were RSA, we couldn't authenticate the signature len, since we wouldn't know it until we signed.
mentioned in issue #20029 (moved)
moved to tpo/core/tor#20004 (closed)
mentioned in issue tpo/core/tor#20029 (closed)