Opened 3 years ago

Closed 3 years ago

#20004 closed enhancement (implemented)

prop224: Add a trunnel subdirectory specifically for HS

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, TorCoreTeam201610
Cc: Actual Points: 0.5
Parent ID: #17241 Points: 1
Reviewer: asn Sponsor: SponsorR-must

Description

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

Child Tickets

Change History (21)

comment:1 Changed 3 years ago by dgoulet

Started here. ESTABLISH_INTRO trunnel file is done (inspired by patch in #19043). Missing INTRODUCE1 cell and we should be good.

Branch: ticket20004_029_01

comment:2 Changed 3 years ago by dgoulet

Actual Points: 0.5
Status: newneeds_review

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_01

comment:3 Changed 3 years ago by asn

Status: needs_reviewneeds_revision

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 handshake is handshake_mac in the code.

comment:4 Changed 3 years ago by asn

Status: needs_revisionneeds_review

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_data should be right before the handshake_mac field and not in the end. Also, there should probably be another ptr called end_sig_data right 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];
};

comment:5 Changed 3 years ago by asn

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.a library in the gcc command it works. So maybe there is an issue with the LIBADD?

Did you manage to compile fine with this branch?

comment:6 Changed 3 years ago by asn

Status: needs_reviewneeds_revision

comment:7 Changed 3 years ago by dgoulet

Status: needs_revisionneeds_review

I fixed both issues above that is the missing end mac fields pointer. I renamed it to the above. Note that the union is 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.)

comment:8 Changed 3 years ago by nickm

Keywords: review-group-8 added

comment:9 Changed 3 years ago by asn

Reviewer: asn

comment:10 Changed 3 years ago by asn

Status: needs_reviewneeds_revision

Hello. I did some more reading of this branch.

I think that the INTRODUCE1 trunnel def is missing the ONION_KEY_TYPE field.

comment:11 Changed 3 years ago by asn

Also, in cell_introduce1.trunnel:

  • I don't see anywhere the CLIENT_PK or MAC fields of the INTRODUCE1 cell as specified in section [NTOR-WITH-EXTRA-DATA]. Also, if we have a MAC we also need the appropriate ptrs.
  • 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 using REND_COOKIE_LEN, but I guess we can't reuse that. No worries if there is no good solution here, just leave it as is.
Version 0, edited 3 years ago by asn (next)

comment:12 Changed 3 years ago by nickm

Keywords: review-group-9 added; review-group-8 removed

comment:13 Changed 3 years ago by nickm

Keywords: nickm-deferred-20161005 added
Milestone: Tor: 0.2.9.x-finalTor: 0.3.0.x-final

Deferring big/risky-feature things (even the ones I really love!) to 0.3.0. Please argue if I'm wrong.

comment:14 Changed 3 years ago by nickm

These have sat in needs_revision for a few weeks at least. Removing from review-group-9, not adding to review-group-10.

comment:15 Changed 3 years ago by nickm

Keywords: review-group-9 removed

comment:16 Changed 3 years ago by dgoulet

Keywords: TorCoreTeam201610 added; nickm-deferred-20161005 removed
Priority: MediumHigh

Bumping priority and scheduling this for October work. It will be quite intertwined with #19043.

comment:17 Changed 3 years ago by asn

Pushed an initial branch here ticket20004_rebased which is basically David's branch rebased to latest tor git master, and also using latest trunnel git master.

It still does not address the issues from comment:11. I will take care of those soon.

comment:18 Changed 3 years ago by asn

Status: needs_revisionneeds_review

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 plaintext in 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.)

comment:19 Changed 3 years ago by dgoulet

Status: needs_reviewmerge_ready

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.

comment:20 Changed 3 years ago by nickm

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.

comment:21 Changed 3 years ago by nickm

Resolution: implemented
Status: merge_readyclosed

Modulo those, merging! (And regenerated one trunnel file.)

Note: See TracTickets for help on using tickets.