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
Changes from 11 commits
7267177
8bafb7f
089efba
5544ebf
626c9d9
5076e16
b2976ee
7fc6505
7517985
2c77fd9
2282d4c
d456a33
97df970
46d7d14
b6465c0
0d36d1f
730563d
790d838
5df55a3
9520e28
4631fbd
38a1cd8
c2d6009
89f4a25
8639549
e312885
f464f6d
8c0a166
ca35f23
14352cc
e9357f1
2a83d0a
f1a236a
0629138
a4e4854
c812f07
d568b7d
8ee81fc
de30a66
c70c9ef
cfc3c08
2461fcc
7a95152
3686bd3
fcdde9d
d492f2f
68b68e3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
o Major features (directory authority, ed25519): | ||
Add support for banning a relay's ed25519 keys in the approved-routers | ||
file. This will allow us to migrate away from RSA keys in the future. | ||
Previously, only RSA keys could be banned in approved-routers. Resolves | ||
ticket 22029. Patch by Neel Chauhan. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -38,19 +38,18 @@ | |
#include "feature/nodelist/routerstatus_st.h" | ||
|
||
#include "lib/encoding/confline.h" | ||
#include "lib/crypt_ops/crypto_format.h" | ||
|
||
/** How far in the future do we allow a router to get? (seconds) */ | ||
#define ROUTER_ALLOW_SKEW (60*60*12) | ||
|
||
static void directory_remove_invalid(void); | ||
struct authdir_config_t; | ||
static was_router_added_t dirserv_add_extrainfo(extrainfo_t *ei, | ||
const char **msg); | ||
static uint32_t | ||
dirserv_get_status_impl(const char *fp, const char *nickname, | ||
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, | ||
const char *nickname, uint32_t addr, uint16_t or_port, | ||
const char *platform, const char **msg, int severity); | ||
|
||
/* 1 Historically used to indicate Named */ | ||
#define FP_INVALID 2 /**< Believed invalid. */ | ||
|
@@ -59,20 +58,6 @@ dirserv_get_status_impl(const char *fp, const char *nickname, | |
#define FP_BADEXIT 16 /**< We'll tell clients not to use this as an exit. */ | ||
/* 32 Historically used to indicade Unnamed */ | ||
|
||
/** Target of status_by_digest map. */ | ||
typedef uint32_t router_status_t; | ||
|
||
static void add_fingerprint_to_dir(const char *fp, | ||
struct authdir_config_t *list, | ||
router_status_t add_status); | ||
|
||
/** List of nickname-\>identity fingerprint mappings for all the routers | ||
* that we name. Used to prevent router impersonation. */ | ||
typedef struct authdir_config_t { | ||
strmap_t *fp_by_name; /**< Map from lc nickname to fingerprint. */ | ||
digestmap_t *status_by_digest; /**< Map from digest to router_status_t. */ | ||
} authdir_config_t; | ||
|
||
/** Should be static; exposed for testing. */ | ||
static authdir_config_t *fingerprint_list = NULL; | ||
|
||
|
@@ -83,16 +68,35 @@ authdir_config_new(void) | |
authdir_config_t *list = tor_malloc_zero(sizeof(authdir_config_t)); | ||
list->fp_by_name = strmap_new(); | ||
list->status_by_digest = digestmap_new(); | ||
list->status_by_digest256 = digest256map_new(); | ||
return list; | ||
} | ||
|
||
#ifdef TOR_UNIT_TESTS | ||
|
||
/** Initialize fingerprint_list to a new authdir_config_t. Used for tests. */ | ||
void | ||
authdir_init_fingerprint_list(void) | ||
{ | ||
fingerprint_list = authdir_config_new(); | ||
} | ||
|
||
/* Return the current fingerprint_list. Used for tests. */ | ||
authdir_config_t * | ||
authdir_return_fingerprint_list(void) | ||
{ | ||
return fingerprint_list; | ||
} | ||
|
||
#endif /* defined(TOR_UNIT_TESTS) */ | ||
|
||
/** Add the fingerprint <b>fp</b> to the smartlist of fingerprint_entry_t's | ||
* <b>list</b>, or-ing the currently set status flags with | ||
* <b>add_status</b>. | ||
* <b>list</b>, or-ing the currently set status flags with <b>add_status</b>. | ||
* Return -1 if we were unable to decode the key, else return 0. | ||
*/ | ||
/* static */ void | ||
add_fingerprint_to_dir(const char *fp, authdir_config_t *list, | ||
router_status_t add_status) | ||
int | ||
add_rsa_fingerprint_to_dir(const char *fp, authdir_config_t *list, | ||
router_status_t add_status) | ||
{ | ||
char *fingerprint; | ||
char d[DIGEST_LEN]; | ||
|
@@ -104,10 +108,8 @@ add_fingerprint_to_dir(const char *fp, authdir_config_t *list, | |
tor_strstrip(fingerprint, " "); | ||
if (base16_decode(d, DIGEST_LEN, | ||
fingerprint, strlen(fingerprint)) != DIGEST_LEN) { | ||
log_warn(LD_DIRSERV, "Couldn't decode fingerprint \"%s\"", | ||
escaped(fp)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please don't delete this warning: you're hiding failures for every caller of this function. Instead, do a minimal length check in dirserv_load_fingerprint_file(). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
tor_free(fingerprint); | ||
return; | ||
return -1; | ||
} | ||
|
||
status = digestmap_get(list->status_by_digest, d); | ||
|
@@ -118,7 +120,36 @@ add_fingerprint_to_dir(const char *fp, authdir_config_t *list, | |
|
||
tor_free(fingerprint); | ||
*status |= add_status; | ||
return; | ||
return 0; | ||
} | ||
|
||
/** Add the ed25519 key <b>edkey</b> to the smartlist of fingerprint_entry_t's | ||
* <b>list</b>, or-ing the currently set status flags with <b>add_status</b>. | ||
* Return -1 if we were unable to decode the key, else return 0. | ||
*/ | ||
int | ||
add_ed25519_to_dir(const ed25519_public_key_t *edkey, authdir_config_t *list, | ||
router_status_t add_status) | ||
{ | ||
router_status_t *status; | ||
|
||
tor_assert(edkey); | ||
tor_assert(list); | ||
|
||
if (ed25519_validate_pubkey(edkey) < 0) { | ||
char ed25519_base64_key[ED25519_BASE64_LEN+1]; | ||
ed25519_public_to_base64(ed25519_base64_key, edkey); | ||
return -1; | ||
} | ||
|
||
status = digest256map_get(list->status_by_digest256, edkey->pubkey); | ||
if (!status) { | ||
status = tor_malloc_zero(sizeof(router_status_t)); | ||
digest256map_set(list->status_by_digest256, edkey->pubkey, status); | ||
} | ||
|
||
*status |= add_status; | ||
return 0; | ||
} | ||
|
||
/** Add the fingerprint for this OR to the global list of recognized | ||
|
@@ -133,7 +164,7 @@ dirserv_add_own_fingerprint(crypto_pk_t *pk) | |
} | ||
if (!fingerprint_list) | ||
fingerprint_list = authdir_config_new(); | ||
add_fingerprint_to_dir(fp, fingerprint_list, 0); | ||
add_rsa_fingerprint_to_dir(fp, fingerprint_list, 0); | ||
return 0; | ||
} | ||
|
||
|
@@ -174,27 +205,44 @@ dirserv_load_fingerprint_file(void) | |
fingerprint_list_new = authdir_config_new(); | ||
|
||
for (list=front; list; list=list->next) { | ||
char digest_tmp[DIGEST_LEN]; | ||
router_status_t add_status = 0; | ||
nickname = list->key; fingerprint = list->value; | ||
tor_strstrip(fingerprint, " "); /* remove spaces */ | ||
if (strlen(fingerprint) != HEX_DIGEST_LEN || | ||
base16_decode(digest_tmp, sizeof(digest_tmp), | ||
fingerprint, HEX_DIGEST_LEN) != sizeof(digest_tmp)) { | ||
log_notice(LD_CONFIG, | ||
"Invalid fingerprint (nickname '%s', " | ||
"fingerprint %s). Skipping.", | ||
nickname, fingerprint); | ||
continue; | ||
} | ||
|
||
/* Determine what we should do with the relay with the nickname field. */ | ||
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 if fingerprint is RSA or ed25519 by verifying it. */ | ||
int ed25519_not_ok, rsa_not_ok; | ||
|
||
/* Attempt to add the RSA key. */ | ||
rsa_not_ok = add_rsa_fingerprint_to_dir(fingerprint, fingerprint_list_new, | ||
add_status); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This code is unsafe: a malicious relay could have an ed25519 base64 encoded public key, that also passes the hexdigest check. Please check the length of the string before passing it to this function. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
||
/* Check ed25519 key. We check the size to prevent buffer overflows. | ||
* If valid, attempt to add it, */ | ||
ed25519_public_key_t ed25519_pubkey_tmp; | ||
if (strlen(fingerprint) == BASE64_DIGEST256_LEN && | ||
digest256_from_base64((char *) ed25519_pubkey_tmp.pubkey, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a logic error: digest256_from_base64() returns 0 on success. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Your tests didn't pick this up: please add a commit with a test that fails on this code. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This code would be easier to read, if the decoding was outside the "if" expression. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. However, I didn't move the decoding outside the "if" expression. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I opened a new ticket for the decoding refactor: And the missing unit test cases: |
||
fingerprint)) { | ||
ed25519_not_ok = add_ed25519_to_dir(&ed25519_pubkey_tmp, | ||
fingerprint_list_new, add_status); | ||
} else { | ||
ed25519_not_ok = -1; | ||
} | ||
|
||
/* If this is not a valid key, log and skip */ | ||
if (ed25519_not_ok && rsa_not_ok) { | ||
log_warn(LD_CONFIG, "Invalid fingerprint (nickname '%s', " | ||
"fingerprint %s). Skipping.", nickname, fingerprint); | ||
continue; | ||
} | ||
} | ||
|
||
config_free_lines(front); | ||
|
@@ -237,9 +285,11 @@ dirserv_router_get_status(const routerinfo_t *router, const char **msg, | |
} | ||
|
||
/* Check for the more usual versions to reject a router first. */ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This comment doesn't make sense. "First, check for the more common reasons to reject a router" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
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->cache_info.signing_key_cert->signing_key.pubkey, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What if the router does not have an ed25519 key? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if (router->cache_info.signing_key_cert) { There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
router->nickname, router->addr, router->or_port, router->platform, msg, | ||
severity); | ||
|
||
if (r) | ||
return r; | ||
|
||
|
@@ -303,23 +353,23 @@ dirserv_would_reject_router(const routerstatus_t *rs) | |
{ | ||
uint32_t res; | ||
|
||
res = dirserv_get_status_impl(rs->identity_digest, rs->nickname, | ||
rs->addr, rs->or_port, | ||
NULL, NULL, LOG_DEBUG); | ||
res = dirserv_get_status_impl(rs->identity_digest, | ||
rs->ed25519_signing_key.pubkey, rs->nickname, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What if the router does not have an ed25519 key? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
rs->addr, rs->or_port, NULL, NULL, LOG_DEBUG); | ||
|
||
return (res & FP_REJECT) != 0; | ||
} | ||
|
||
/** Helper: As dirserv_router_get_status, but takes the router fingerprint | ||
* (hex, no spaces), nickname, address (used for logging only), IP address, OR | ||
* port and platform (logging only) as arguments. | ||
* (hex, no spaces), ed25510 key, nickname, address (used for logging only), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Typo: ed25519 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
* IP address, OR port and platform (logging only) as arguments. | ||
* | ||
* Log messages at 'severity'. (There's not much point in | ||
* logging that we're rejecting servers we'll not download.) | ||
*/ | ||
static uint32_t | ||
dirserv_get_status_impl(const char *id_digest, const char *nickname, | ||
uint32_t addr, uint16_t or_port, | ||
dirserv_get_status_impl(const char *id_digest, const uint8_t *ed25519_key, | ||
const char *nickname, uint32_t addr, uint16_t or_port, | ||
const char *platform, const char **msg, int severity) | ||
{ | ||
teor2345 marked this conversation as resolved.
Show resolved
Hide resolved
teor2345 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
uint32_t result = 0; | ||
|
@@ -366,6 +416,13 @@ dirserv_get_status_impl(const char *id_digest, const char *nickname, | |
if (status_by_digest) | ||
result |= *status_by_digest; | ||
|
||
if (ed25519_key) { | ||
status_by_digest = digest256map_get(fingerprint_list->status_by_digest256, | ||
ed25519_key); | ||
if (status_by_digest) | ||
result |= *status_by_digest; | ||
} | ||
|
||
if (result & FP_REJECT) { | ||
if (msg) | ||
*msg = "Fingerprint is marked rejected -- if you think this is a " | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need to update this message to say "mentioning your RSA fingerprint and ed25519 identity" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
@@ -414,6 +471,7 @@ dirserv_free_fingerprint_list(void) | |
|
||
strmap_free(fingerprint_list->fp_by_name, tor_free_); | ||
digestmap_free(fingerprint_list->status_by_digest, tor_free_); | ||
digest256map_free(fingerprint_list->status_by_digest256, tor_free_); | ||
tor_free(fingerprint_list); | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,6 +30,7 @@ | |
#include "feature/nodelist/node_st.h" | ||
#include "feature/nodelist/routerinfo_st.h" | ||
#include "feature/nodelist/vote_routerstatus_st.h" | ||
#include "feature/nodelist/torcert.h" | ||
|
||
#include "lib/container/order.h" | ||
|
||
|
@@ -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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This code leaves the key zeroed-out if the relay does not have an ed25529 key. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
&ri->cache_info.signing_key_cert->signing_key); | ||
} | ||
|
||
rs->is_staledesc = | ||
(ri->cache_info.published_on + DESC_IS_STALE_INTERVAL) < now; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,7 @@ | |
#define ROUTERSTATUS_ST_H | ||
|
||
#include "feature/dirclient/download_status_st.h" | ||
#include "lib/crypt_ops/crypto_ed25519.h" | ||
|
||
/** Contents of a single router entry in a network status object. | ||
*/ | ||
|
@@ -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. */ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This needs to be a pointer: "ed25519_public_key_t *ed25519_signing_key". There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
uint32_t addr; /**< IPv4 address for this router, in host order. */ | ||
uint16_t or_port; /**< IPv4 OR port for this router. */ | ||
uint16_t dir_port; /**< Directory port for this router. */ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new argument should be "const ed25519_public_key_t *ed25519_public_key".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
46d7d14