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

[Tor Trac #22029]: Allow ed25519 keys to be banned in the approved-routers file (Revision 3) #970

Open
wants to merge 38 commits into
base: master
from

Conversation

@neelchauhan
Copy link
Contributor

commented Apr 19, 2019

For the bug here.

@coveralls

This comment has been minimized.

Copy link

commented Apr 19, 2019

Pull Request Test Coverage Report for Build 5389

  • 50 of 61 (81.97%) changed or added relevant lines in 4 files are covered.
  • 36 unchanged lines in 3 files lost coverage.
  • Overall coverage increased (+0.3%) to 62.881%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/feature/relay/router.c 0 1 0.0%
src/feature/dirauth/voteflags.c 2 5 40.0%
src/feature/dirauth/process_descs.c 47 54 87.04%
Files with Coverage Reduction New Missed Lines %
src/feature/dirauth/shared_random.c 3 85.54%
src/feature/dirauth/dirvote.c 9 64.99%
src/app/config/config.c 24 71.96%
Totals Coverage Status
Change from base Build 5337: 0.3%
Covered Lines: 47152
Relevant Lines: 74986

💛 - Coveralls
@teor2345
Copy link
Member

left a comment

There are two major bugs in this code:

dirserv_get_status_impl() is also called from dirserv_would_reject_router().
But it was not updated to check the ed25519 identity key.

A call to dirserv_get_status_impl() is in the wrong place.
The ed25519 key is only checked if there is a KEYPIN_MISMATCH.

Please add some tests for dirserv_router_get_status() and dirserv_would_reject_router() that fail on the current code, but succeed when you fix these bugs.

Does this change fail practracker?
The existing code is already complex, so you should not increase function sizes. Instead, split the new code out into new functions.
I am not sure if you should split files: maybe we should open another ticket, and do that after 0.4.0 stable?

*/
/* static */ void
int
add_fingerprint_to_dir(const char *fp, authdir_config_t *list,

This comment has been minimized.

Copy link
@teor2345

teor2345 Apr 22, 2019

Member

This function only handles RSA fingerprints.
So its function comment should state this requirement.
Maybe its name should contain "rsa" as well.

if (!strcasecmp(nickname, "!reject")) {
add_status = FP_REJECT;
} else if (!strcasecmp(nickname, "!badexit")) {
add_status = FP_BADEXIT;
} else if (!strcasecmp(nickname, "!invalid")) {
add_status = FP_INVALID;
}
add_fingerprint_to_dir(fingerprint, fingerprint_list_new, add_status);

/* Check fingerprint type by key length, and if it's RSA or ed25519. */

This comment has been minimized.

Copy link
@teor2345

teor2345 Apr 22, 2019

Member

Lines 225 to 260 duplicate some checks that are already done in add_ed25519_to_dir() and add_fingerprint_to_dir().

Please make sure that all the checks are done in those functions.
Then, in this function, check the return values of those functions, and log a warning if neither function accepts the fingerprint.


/* If this is not a valid key, log and skip */
if (!is_valid_fpr) {
log_notice(LD_CONFIG, "Invalid fingerprint (nickname '%s', "

This comment has been minimized.

Copy link
@teor2345

teor2345 Apr 22, 2019

Member

Should this be a warning?

src/feature/dirauth/process_descs.c Outdated Show resolved Hide resolved
add_ed25519_to_dir(&ed25519_pubkey_tmp,
fingerprint_list_new, add_status);
} else if (is_rsa) {
add_fingerprint_to_dir(fingerprint, fingerprint_list_new, add_status);

This comment has been minimized.

Copy link
@teor2345

teor2345 Apr 22, 2019

Member

Don't ignore function return values.

const uint32_t r = dirserv_get_status_impl(d, router->nickname,
router->addr, router->or_port,
router->platform, msg, severity);
uint32_t r = dirserv_get_status_impl(d, router->nickname, router->addr,

This comment has been minimized.

Copy link
@teor2345

teor2345 Apr 22, 2019

Member

This call duplicates most of the work done in the call to dirserv_get_status_impl() below.
Instead, make dirserv_get_status_impl() take two digests, an RSA digest and an ed25519 public key.
And then check both values inside that function.

Allow the ed25519 public key to be NULL, for relays that don't do ed25519.

@@ -272,6 +342,14 @@ dirserv_router_get_status(const routerinfo_t *router, const char **msg,
}
return FP_REJECT;
}

/* Check status with ed25519 key. */

This comment has been minimized.

Copy link
@teor2345

teor2345 Apr 22, 2019

Member

This call to dirserv_get_status_impl() is in the wrong place.
The ed25519 key is only checked if there is a KEYPIN_MISMATCH.
This is a major bug.

It was hard to find this bug, because the code structure is already complex. And this patch makes it more complex.

src/feature/dirauth/process_descs.c Show resolved Hide resolved
if (is_ed25519) {
status_by_digest = digest256map_get(fingerprint_list->status_by_digest256,
(uint8_t *) id_digest);
} else { /* RSA */

This comment has been minimized.

Copy link
@teor2345

teor2345 Apr 22, 2019

Member

How do you know that all the calling functions will pass an ed25519 public key, or an RSA digest?
Please check that the values are valid before looking them up.
Log a stack trace using BUG() or tor_assert_nonfatal() if they are not.

src/feature/dirauth/process_descs.c Show resolved Hide resolved

@neelchauhan neelchauhan force-pushed the neelchauhan:b22029b branch from 5163c75 to 7fc6505 Apr 22, 2019

@neelchauhan neelchauhan force-pushed the neelchauhan:b22029b branch from 26e3d5f to 7fc6505 Apr 23, 2019

@neelchauhan neelchauhan force-pushed the neelchauhan:b22029b branch 5 times, most recently from ee61f73 to d456a33 Apr 25, 2019

the status of routers by their identity fingerprint.
Each line lists a status and a fingerprint separated by
the status of routers by their identity fingerprint or base64-encoded
ed25519 public key. Each line lists a status and a fingerprint separated by

This comment has been minimized.

Copy link
@teor2345

teor2345 May 27, 2019

Member

"status and a fingerprint/pubkey"

This comment has been minimized.

Copy link
@neelchauhan
the status of routers by their identity fingerprint.
Each line lists a status and a fingerprint separated by
the status of routers by their identity fingerprint or base64-encoded
ed25519 public key. Each line lists a status and a fingerprint separated by
whitespace. See your **fingerprint** file in the __DataDirectory__ for an
example line. If the status is **!reject** then descriptors from the

This comment has been minimized.

Copy link
@teor2345

teor2345 May 27, 2019

Member

"example fingerprint"?

This comment has been minimized.

Copy link
@neelchauhan
the status of routers by their identity fingerprint.
Each line lists a status and a fingerprint separated by
the status of routers by their identity fingerprint or base64-encoded
ed25519 public key. Each line lists a status and a fingerprint separated by
whitespace. See your **fingerprint** file in the __DataDirectory__ for an

This comment has been minimized.

Copy link
@teor2345

teor2345 May 27, 2019

Member

Where is the example file containing a "base64-encoded ed25519 public key"?

This comment has been minimized.


add_ed25519_to_dir(&kp.pubkey, list, FP_REJECT);
tt_assert(dirserv_would_reject_router(&rs));

This comment has been minimized.

Copy link
@teor2345

teor2345 May 27, 2019

Member

Please RESET_FP_LIST(list); after every test.

Otherwise, the next test could have bugs.

This comment has been minimized.

Copy link
@neelchauhan

neelchauhan May 29, 2019

Author Contributor

I did this.

This comment has been minimized.

Copy link
@neelchauhan
@@ -6654,6 +6654,54 @@ test_dir_dirserv_router_get_status(void *arg)
routerinfo_free(ri);
}

static void
test_dir_dirserv_would_reject_router(void *arg)

This comment has been minimized.

Copy link
@teor2345

teor2345 May 27, 2019

Member

What happens when dirserv_would_reject_router() is called on an empty fingerprint list?
What should happen?

This comment has been minimized.

Copy link
@neelchauhan

neelchauhan May 29, 2019

Author Contributor

I fixed it. Both the values return 0 for RSA and ed25519.

This comment has been minimized.

Copy link
@neelchauhan
list = authdir_return_fingerprint_list(); \
STMT_END

#define FP_REJECT 4

This comment has been minimized.

Copy link
@teor2345

teor2345 May 27, 2019

Member

Please include the header that defines this constant.

This comment has been minimized.

Copy link
@teor2345

teor2345 May 27, 2019

Member

Hmm, there is no header that defines this constant.
Can you call a function that checks if the relay is rejected?

This comment has been minimized.

Copy link
@neelchauhan

neelchauhan May 29, 2019

Author Contributor

I don't think this is possible without adding a lot more code because the function dirserv_would_reject_router() expects a routerstatus_t and I have a routerinfo_t making this impractical.

This comment has been minimized.

Copy link
@teor2345

teor2345 May 29, 2019

Member

I opened another ticket to make this change:
https://trac.torproject.org/projects/tor/ticket/30677

add_ed25519_to_dir(&kp1.pubkey, list, FP_REJECT);
ret = dirserv_router_get_status(ri, &msg, LOG_INFO);
tt_int_op(ret, OP_EQ, FP_REJECT);

This comment has been minimized.

Copy link
@teor2345

teor2345 May 27, 2019

Member

Please RESET_FP_LIST(list); after every test.

Otherwise, the next test could have bugs.

This comment has been minimized.

Copy link
@neelchauhan

neelchauhan May 29, 2019

Author Contributor

I've added this.

This comment has been minimized.

Copy link
@neelchauhan
#define FP_REJECT 4

static void
test_dir_dirserv_router_get_status(void *arg)

This comment has been minimized.

Copy link
@teor2345

teor2345 May 27, 2019

Member

What happens when dirserv_router_get_status() is called on an empty fingerprint list?
What should happen?

This comment has been minimized.

Copy link
@neelchauhan

neelchauhan May 29, 2019

Author Contributor

I have added this test. dirserv_router_get_status() returns 0 in this case.

This comment has been minimized.

Copy link
@neelchauhan
uint32_t addr, uint16_t or_port,
const char *platform, const char **msg,
int severity);
dirserv_get_status_impl(const char *fp, const uint8_t *ed25519_key,

This comment has been minimized.

Copy link
@teor2345

teor2345 May 27, 2019

Member

The new argument should be "const ed25519_public_key_t *ed25519_public_key".

This comment has been minimized.

Copy link
@neelchauhan
@@ -20,6 +21,8 @@ struct routerstatus_t {
/** Digest of the router's most recent descriptor or microdescriptor.
* If it's a descriptor, we only use the first DIGEST_LEN bytes. */
char descriptor_digest[DIGEST256_LEN];
ed25519_public_key_t ed25519_signing_key; /**< The ed25519 signing key of
* this router. */

This comment has been minimized.

Copy link
@teor2345

teor2345 May 27, 2019

Member

This needs to be a pointer: "ed25519_public_key_t *ed25519_signing_key".
Some routers do not have an ed25519 key.

This comment has been minimized.

Copy link
@neelchauhan
@@ -608,6 +609,11 @@ set_routerstatus_from_routerinfo(routerstatus_t *rs,
rs->dir_port = ri->dir_port;
rs->is_v2_dir = ri->supports_tunnelled_dir_requests;

if (ri->cache_info.signing_key_cert) {
ed25519_pubkey_copy(&rs->ed25519_signing_key,

This comment has been minimized.

Copy link
@teor2345

teor2345 May 27, 2019

Member

This code leaves the key zeroed-out if the relay does not have an ed25529 key.
Instead, allocate memory for the key if it is non-NULL.
And free that memory in routerstatus_free_().

This comment has been minimized.

Copy link
@neelchauhan
neelchauhan added 18 commits May 27, 2019
@teor2345

This comment has been minimized.

Copy link
Member

commented on src/feature/dirauth/process_descs.h in ca35f23 Jun 17, 2019

The comment should match the initial #if condition.

#endif defined(PROCESS_DESCS_PRIVATE) || defined(TOR_UNIT_TESTS)

/* First, check for the more common reasons to reject a router. */
if (router->cache_info.signing_key_cert) {
/* This has an ed25519 identity key. */
r = dirserv_get_status_impl(d,

This comment has been minimized.

Copy link
@nmathewson

nmathewson Sep 18, 2019

Contributor

These two calls to dirserv_get_status_impl are almost identical. Instead of having two calls like this, it's cleaner to have a local variable that holds either the ed25519 public key or NULL, and then pass that variable to a single call.

@@ -20,6 +21,8 @@ struct routerstatus_t {
/** Digest of the router's most recent descriptor or microdescriptor.
* If it's a descriptor, we only use the first DIGEST_LEN bytes. */
char descriptor_digest[DIGEST256_LEN];
ed25519_public_key_t *ed25519_signing_key; /**< The ed25519 signing key of

This comment has been minimized.

Copy link
@nmathewson

nmathewson Sep 18, 2019

Contributor

Can we possibly avoid adding this field? It's stored in microdescriptors right now, not routerstatuses at all.

The reason we shouldn't add it here is that we can't ensure that this field is actually set in every routerstatus_t for a relay with an ed25519 key. We set it in dirauth_set_routerstatus_from_routerinfo, but note that we can't set it at all when we receive the routerstatus_t from a vote.

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