Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ticket20700 035 02 #301

Closed
wants to merge 46 commits into from

Conversation

Projects
None yet
5 participants
@dgoulet-tor
Copy link
Contributor

commented Aug 30, 2018

No description provided.

haxxpop and others added some commits Aug 18, 2018

hs-v3: Load all client auth keys to the service
This commit loads all client public keys from every file in
`authorized_clients/` directory.

Signed-off-by: David Goulet <dgoulet@torproject.org>
test: HS v3 loading client auth keys service side
Signed-off-by: David Goulet <dgoulet@torproject.org>
hs-v3: Generate all descriptor related keys
We need to generate all the related keys when building the descriptor, so that
we can encrypt the descriptor.

Signed-off-by: David Goulet <dgoulet@torproject.org>
test: HS v3 building a descriptor with client auth
This commit tests that the descriptor building result, when the client
authorization is enabled, includes everything that is needed.

Signed-off-by: David Goulet <dgoulet@torproject.org>
test: Build an HSv3 descriptor with authorized client
Signed-off-by: David Goulet <dgoulet@torproject.org>
hs-v3: Encrypt the descriptor using a cookie
Previously, we encrypted the descriptor without the descriptor cookie. This
commit, when the client auth is enabled, the descriptor cookie is always used.

I also removed the code that is used to generate fake auth clients because it
will not be used anymore.

Signed-off-by: David Goulet <dgoulet@torproject.org>
test: HS v3 descriptor encoding with client authorization
Signed-off-by: David Goulet <dgoulet@torproject.org>
hs-v3: Load client authorization secret key from file
The new ClientOnionAuthDir option is introduced which is where tor looks to
find the HS v3 client authorization files containing the client private key
material.

Signed-off-by: David Goulet <dgoulet@torproject.org>
test: HS v3 client authorization loading secret key
Signed-off-by: David Goulet <dgoulet@torproject.org>
hs-v3: Refactor secret data building logic
Because this secret data building logic is not only used by the descriptor
encoding process but also by the descriptor decoding, refactor the function to
take both steps into account.

Signed-off-by: David Goulet <dgoulet@torproject.org>
hs-v3: Refactor the descriptor decryption/decoding
This commit refactors the existing decryption code to make it compatible with
a new logic for when the client authorization is enabled.

Signed-off-by: David Goulet <dgoulet@torproject.org>
hs-v3: Decrypt the descriptor with client private key
Parse the client authorization section from the descriptor, use the client
private key to decrypt the auth clients, and then use the descriptor cookie to
decrypt the descriptor.

Signed-off-by: David Goulet <dgoulet@torproject.org>
test: HS v3 descriptor decoding with client authorization
Signed-off-by: David Goulet <dgoulet@torproject.org>
hs-v3: Re-enable the decoding in the encoding function
Previously, the validation by decoding a created descriptor was disabled
because the interface had to be entirely changed and not implemented at the
time.

This commit re-enabled it because it is now implemented.

Signed-off-by: David Goulet <dgoulet@torproject.org>
hs-v3: Make all descriptor content free functions public
Series of functions that we now need in hs_service.c.

Signed-off-by: David Goulet <dgoulet@torproject.org>
hs-v3: Republish descriptors if client auth changes
When reloading tor, check if our the configured client authorization have
changed from what we previously had. If so, republish the updated descriptor.

Signed-off-by: David Goulet <dgoulet@torproject.org>
test: HS v3 client auth is config equal function
Signed-off-by: David Goulet <dgoulet@torproject.org>
hs-v3: Rename client_sk to client_auth_sk
Signed-off-by: David Goulet <dgoulet@torproject.org>
hs-v3: Rename client_pk to client_auth_pk
Signed-off-by: David Goulet <dgoulet@torproject.org>
hs-v3: Log client auth load activities service side
Signed-off-by: David Goulet <dgoulet@torproject.org>
hs-v3: Log client auth load activities client side
Signed-off-by: David Goulet <dgoulet@torproject.org>
hs-v3: Improve v3 client authorization logging
Part of #20700.

Signed-off-by: David Goulet <dgoulet@torproject.org>
@coveralls

This comment has been minimized.

Copy link

commented Aug 30, 2018

Coverage Status

Coverage increased (+0.3%) to 59.832% when pulling a2b821c on dgoulet-tor:ticket20700_035_02 into 94605f0 on torproject:master.

haxxpop added some commits Sep 2, 2018

bug: Use PATH_SEPARATOR instead of slash
In function get_fname_suffix, previously it uses /, but in fact it
should use PATH_SEPARATOR.
static int
client_filename_is_valid(const char *filename)
{
int ret = 1;

This comment has been minimized.

Copy link
@nmathewson

nmathewson Sep 5, 2018

Contributor

This function should just use "strcmpend()."

This comment has been minimized.

Copy link
@dgoulet-tor

dgoulet-tor Sep 5, 2018

Author Contributor

Fixup commit: 67fa226

static hs_service_authorized_client_t *
parse_authorized_client(const char *client_key_str)
{
char *auth_type = NULL;

This comment has been minimized.

Copy link
@nmathewson

nmathewson Sep 5, 2018

Contributor

Maybe it would be a good idea to describe the format in a comment here too.

This comment has been minimized.

Copy link
@dgoulet-tor

dgoulet-tor Sep 5, 2018

Author Contributor

Fixup commit: de70396

}

client = tor_malloc_zero(sizeof(hs_service_authorized_client_t));
if (base32_decode((char *) client->client_pk.public_key,

This comment has been minimized.

Copy link
@nmathewson

nmathewson Sep 5, 2018

Contributor

Will this detect truncated public keys?

This comment has been minimized.

Copy link
@dgoulet-tor

dgoulet-tor Sep 5, 2018

Author Contributor

Fixup commit: 7396175

* cannot create a client pubkey key directory, we consider it as a bug. */
client_keys_dir_path = hs_path_from_filename(config->directory_path,
dname_client_pubkeys);
if (BUG(hs_check_service_private_dir(get_options()->User,

This comment has been minimized.

Copy link
@nmathewson

nmathewson Sep 5, 2018

Contributor

Are we calling this from inside the options code, or from somewhere else? If it's inside the options code, we can't call get_options() -- we should be taking the or_options_t as an argument instead. But if it's happening from somewhere else, this is okay.

This comment has been minimized.

Copy link
@dgoulet-tor

dgoulet-tor Sep 5, 2018

Author Contributor

Soooo we might have a bigger problem here. It is ultimately called from options_act() and for v3 it is easy to fix but we'll need to do it for v2 as well.

get_options()->User is what is accessed during key loading since we double check if the directory permissions are valid.

This will touch many parts of HS and Rend subsystem so I propose we do this after with its own set of commits?

This comment has been minimized.

Copy link
@nmathewson

nmathewson Sep 6, 2018

Contributor

hm, ok. Actually, it might be safe to leave this alone, since the User option is not allowed to change while Tor is running.


file_list = tor_listdir(client_keys_dir_path);

SMARTLIST_FOREACH_BEGIN(file_list, char *, filename) {

This comment has been minimized.

Copy link
@nmathewson

nmathewson Sep 5, 2018

Contributor

Could this be const char *?

This comment has been minimized.

Copy link
@dgoulet-tor

dgoulet-tor Sep 5, 2018

Author Contributor

Fixup commit: 75a261c


config->clients = smartlist_new();

file_list = tor_listdir(client_keys_dir_path);

This comment has been minimized.

Copy link
@nmathewson

nmathewson Sep 5, 2018

Contributor

We need to check for whether this value is NULL, I think.

This comment has been minimized.

Copy link
@dgoulet-tor

dgoulet-tor Sep 5, 2018

Author Contributor

Fixup commit: 5505486

"descriptor:x:dz4q5xqlb4ldnbs72iarrml4ephk3du4i7o2cgiva5lwr6wkquja"));
/* Some malformed string. */
tt_assert(!parse_authorized_client("descriptor:x25519:aa=="));

This comment has been minimized.

Copy link
@nmathewson

nmathewson Sep 5, 2018

Contributor

I'd suggest adding checks for 1-field, 2-field, and 0-field strings as well: they're a corner case that we're likely to mess up in the future if we don't have tests.

This comment has been minimized.

Copy link
@dgoulet-tor

dgoulet-tor Sep 5, 2018

Author Contributor

Fixup commit: 629c188


#undef conf_fmt

MOCK(read_file_to_str, mock_read_file_to_str);

This comment has been minimized.

Copy link
@nmathewson

nmathewson Sep 5, 2018

Contributor

It's okay to do this kind of mocking -- but in the future, it might be just as good (or better) to just put the files into a temporary directory on disk. See uses of get_fname() for other places in the tests where we generate temporary files and directories.


SMARTLIST_FOREACH_BEGIN(service->config.clients,
hs_service_authorized_client_t *, client) {
if (!tor_memcmp(&pubkey, &client->client_pk, sizeof(pubkey))) {

This comment has been minimized.

Copy link
@nmathewson

nmathewson Sep 5, 2018

Contributor

See documentation for tor_memcmp: it's better to use tor_memeq() in this case.

This comment has been minimized.

Copy link
@nmathewson

nmathewson Sep 5, 2018

Contributor

err, ug. This is not actually documented enough. Will document now.

This comment has been minimized.

Copy link
@dgoulet-tor

dgoulet-tor Sep 5, 2018

Author Contributor

Fixup commit: f8e98f8


/* Calculate KEYS = KDF(SECRET_SEED, 40) */
xof = crypto_xof_new();
crypto_xof_add_bytes(xof, secret_seed, sizeof(secret_seed));

This comment has been minimized.

Copy link
@nmathewson

nmathewson Sep 5, 2018

Contributor

Now that I'm looking at this implementation, I'm a little concerned that we're only putting the curve25519 output into the KDF. I think we should also put something about the hidden service itself (its identity? its blinded identity?) into the KDF, to prevent tricky attacks where the ephemeral public key is copied from one descriptor to another. What do you think?

This comment has been minimized.

Copy link
@asn-d6

asn-d6 Sep 6, 2018

Member

Hm. I'm not sure exactly what kind of attack that could enable, but it does seem like a sane KDF use choice so we can do it.

I made a torspec PR with a design that adds the HS subcredential to the KDF computation:
torproject/torspec#34

(Using the subcred there is like using both the blinded and the identity key and it was by far the easiest solution in terms of engineering.)

This comment has been minimized.

Copy link
@asn-d6

asn-d6 Sep 6, 2018

Member

Please also see my branch ticket20700_035_02 for two commits that implement the above.
https://github.com/asn-d6/tor/tree/ticket20700_035_02

Some new test vectors broke because of adding the subcred but I don't have the script that generated them so I just updated them... Can you please rerun the script if you have it?

@@ -2475,6 +2504,84 @@ hs_desc_intro_point_free_(hs_desc_intro_point_t *ip)
tor_free(ip);
}

/* Build a fake client info for the descriptor */
void

This comment has been minimized.

Copy link
@nmathewson

nmathewson Sep 5, 2018

Contributor

Should this function (and the one below) just allocate and return a new hs_desc_authorized_client_t ?

This comment has been minimized.

Copy link
@dgoulet-tor

dgoulet-tor Sep 5, 2018

Author Contributor

Commit c6403e6

@nmathewson
Copy link
Contributor

left a comment

See comments

@nmathewson
Copy link
Contributor

left a comment

see comments

/* If the client id of auth client is not the same as the calculcated
* client id, it means that this auth client is invaild according to the
* client secret key client_sk. */
if (!tor_memeq(client->client_id, keystream, HS_DESC_CLIENT_ID_LEN)) {

This comment has been minimized.

Copy link
@nmathewson

nmathewson Sep 5, 2018

Contributor

Instead of !tor_memeq, you can use tor_memneq

This comment has been minimized.

Copy link
@dgoulet-tor

dgoulet-tor Sep 5, 2018

Author Contributor

Fixup commit 54d9da1

tor_malloc_zero(sizeof(hs_desc_authorized_client_t));

if (decode_auth_client(token, client) < 0) {
log_warn(LD_REND, "Auth client is not valid");

This comment has been minimized.

Copy link
@nmathewson

nmathewson Sep 5, 2018

Contributor

Is this message actually going to tell the user what they need to know?

What will happen to the connection? Will it be retried?

This comment has been minimized.

Copy link
@dgoulet-tor

dgoulet-tor Sep 5, 2018

Author Contributor

I've improved a bit the message in fixup commit 1f98b67.

We could make it that the log is much more verbose but at this stage this is decoding of a descriptor so it will ultimately result in the SOCKS connection being closed client side because we can't properly decode the descriptor.

idx_matched =
tor_malloc_zero(sizeof(*idx_matched) * smartlist_len(config2->clients));

SMARTLIST_FOREACH_BEGIN(config1->clients,

This comment has been minimized.

Copy link
@nmathewson

nmathewson Sep 5, 2018

Contributor

I don't like doing this quadratic loop here -- do we really need to do this?

This comment has been minimized.

Copy link
@nmathewson

nmathewson Sep 5, 2018

Contributor

One option would be to sort the lists, then compare them in lockstep.

Come to think of it, we should probably also scramble the lists before we actually use them, so a client doesn't learn anything from its position in the list.

This comment has been minimized.

Copy link
@dgoulet-tor

dgoulet-tor Sep 5, 2018

Author Contributor

This is unfortunately needed... It will happen rarely that is when the service is HUP.

The reason we need this is to detect if there are new client authorization configured for the service and if so, we need to build a new descriptor for the service and republish.

So for that, we have to go through all clients from both services object (old and new) and figure out if any of them changed (removed or added).

Come to think of it, we should probably also scramble the lists before we actually use them, so a client doesn't learn anything from its position in the list.

Oh hmmmm, yeah I think that is not a stupid idea. We should scramble that list after loading the key file. I propose we do this after, noting down.

This comment has been minimized.

Copy link
@haxxpop

haxxpop Sep 5, 2018

Contributor

I agree to scramble the list. It's safer to not let the client know the position.
Yes it's better to improve the running time. Do we need to do it in this branch?

This comment has been minimized.

Copy link
@nmathewson

nmathewson Sep 6, 2018

Contributor

A separate branch is fine, but IMO it should happen in 0.3.5 (especially since it is easy to do)

@@ -1136,23 +1136,31 @@ parse_authorized_client(const char *client_key_str)
SPLIT_SKIP_SPACE, 0);
/* Wrong number of fields. */
if (smartlist_len(fields) != 3) {
log_warn(LD_REND, "The file is in a wrong format.");

This comment has been minimized.

Copy link
@nmathewson

nmathewson Sep 5, 2018

Contributor

Let's say which file -- it's not going to help the user otherwise.

This comment has been minimized.

Copy link
@dgoulet-tor

dgoulet-tor Sep 5, 2018

Author Contributor

This has been fixed with a future commit in this branch.

goto err;
}

client = tor_malloc_zero(sizeof(hs_service_authorized_client_t));
if (base32_decode((char *) client->client_pk.public_key,
sizeof(client->client_pk.public_key),
pubkey_b32, strlen(pubkey_b32)) < 0) {
log_warn(LD_REND, "The public key cannot be decoded.");

This comment has been minimized.

Copy link
@nmathewson

nmathewson Sep 5, 2018

Contributor

Again, we should really be saying what file is busted, or the user won't know what to do about it.

This comment has been minimized.

Copy link
@dgoulet-tor

dgoulet-tor Sep 5, 2018

Author Contributor

Same as above.

@asn-d6

This comment has been minimized.

Copy link
Member

commented on src/feature/hs/hs_descriptor.c in 97404e3 Sep 6, 2018

I think this is not right. base64_decode() returns the number of bytes written, not read. Hence we should check that the retval is what we expect to be getting out of base64, in this case I think sizeof(client->client_id) if I'm not mistaken.

e.g.:

digest_from_base64(char *digest, const char *d64)
{
  if (base64_decode(digest, DIGEST_LEN, d64, strlen(d64)) == DIGEST_LEN)
    return 0;
  else
    return -1;
}

This comment has been minimized.

Copy link
Contributor

replied Sep 6, 2018

asn is right here.This makes me worried that this code is not tested.

/* Try to decode what we just encoded. Symmetry is nice!, but it is
* symmetric only if the client auth is disabled. That is, the descriptor
* cookie will be NULL. */
if (!descriptor_cookie) {

This comment has been minimized.

Copy link
@asn-d6

asn-d6 Sep 6, 2018

Member

Can't we also test-decode even if we have a descriptor cookie?

This comment has been minimized.

Copy link
@haxxpop

haxxpop Sep 6, 2018

Contributor

The reason is that hs_desc_decode_descriptor parameter requires the client private key not the descriptor cookie and we can't find the private key using descriptor cookie.

And I think using a private key as a parameter of hs_desc_decode_descriptor is more reasonable because we use a private key to decode not a descriptor cookie

This comment has been minimized.

Copy link
@asn-d6

asn-d6 Sep 6, 2018

Member

Hm, I see what you mean. IIf I'm not mistaken, I think with some medium-level refactoring we could make that smooth and also have it test-decode in the client auth case as well (so that this code path gets used).

Perhaps we can open a ticket about this.

@nmathewson

This comment has been minimized.

Copy link
Contributor

commented Sep 30, 2018

A later version of this was merged

@nmathewson nmathewson closed this Sep 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.