From b566cb9e84b095289a1c662e953218c9a7d1f77d Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 14 Jul 2015 10:18:52 -0400 Subject: Make file-reading and key-reading preserve errno This is an important part of #16582. --- src/common/crypto_curve25519.c | 21 ++++++++++++++++----- src/common/util.c | 5 +++++ 2 files changed, 21 insertions(+), 5 deletions(-) (limited to 'src') diff --git a/src/common/crypto_curve25519.c b/src/common/crypto_curve25519.c index 80b0d88952..113ac89df7 100644 --- a/src/common/crypto_curve25519.c +++ b/src/common/crypto_curve25519.c @@ -201,7 +201,7 @@ crypto_write_tagged_contents_to_file(const char *fname, * data_out_len-byte buffer in data_out. Check that the * typestring matches typestring; store the tag into a newly allocated * string in tag_out. Return -1 on failure, and the number of bytes of - * data on success. */ + * data on success. Preserves the errno from reading the file. */ ssize_t crypto_read_tagged_contents_from_file(const char *fname, const char *typestring, @@ -214,27 +214,36 @@ crypto_read_tagged_contents_from_file(const char *fname, struct stat st; ssize_t r = -1; size_t st_size = 0; + int saved_errno = 0; *tag_out = NULL; st.st_size = 0; content = read_file_to_str(fname, RFTS_BIN|RFTS_IGNORE_MISSING, &st); - if (! content) + if (! content) { + saved_errno = errno; goto end; - if (st.st_size < 32 || st.st_size > 32 + data_out_len) + } + if (st.st_size < 32 || st.st_size > 32 + data_out_len) { + saved_errno = EINVAL; goto end; + } st_size = (size_t)st.st_size; memcpy(prefix, content, 32); prefix[32] = 0; /* Check type, extract tag. */ if (strcmpstart(prefix, "== ") || strcmpend(prefix, " ==") || - ! tor_mem_is_zero(prefix+strlen(prefix), 32-strlen(prefix))) + ! tor_mem_is_zero(prefix+strlen(prefix), 32-strlen(prefix))) { + saved_errno = EINVAL; goto end; + } if (strcmpstart(prefix+3, typestring) || 3+strlen(typestring) >= 32 || - strcmpstart(prefix+3+strlen(typestring), ": ")) + strcmpstart(prefix+3+strlen(typestring), ": ")) { + saved_errno = EINVAL; goto end; + } *tag_out = tor_strndup(prefix+5+strlen(typestring), strlen(prefix)-8-strlen(typestring)); @@ -246,6 +255,8 @@ crypto_read_tagged_contents_from_file(const char *fname, if (content) memwipe(content, 0, st_size); tor_free(content); + if (saved_errno) + errno = saved_errno; return r; } diff --git a/src/common/util.c b/src/common/util.c index 449015054f..a140057dea 100644 --- a/src/common/util.c +++ b/src/common/util.c @@ -2571,7 +2571,9 @@ read_file_to_str_until_eof(int fd, size_t max_bytes_to_read, size_t *sz_out) string = tor_realloc(string, string_max); r = read(fd, string + pos, string_max - pos - 1); if (r < 0) { + int save_errno = errno; tor_free(string); + errno = save_errno; return NULL; } @@ -2639,11 +2641,14 @@ read_file_to_str(const char *filename, int flags, struct stat *stat_out) if (S_ISFIFO(statbuf.st_mode)) { size_t sz = 0; string = read_file_to_str_until_eof(fd, FIFO_READ_MAX, &sz); + int save_errno = errno; if (string && stat_out) { statbuf.st_size = sz; memcpy(stat_out, &statbuf, sizeof(struct stat)); } close(fd); + if (!string) + errno = save_errno; return string; } #endif -- cgit v1.2.3-54-g00ecf From 0a6997d78bdbf485f42acfa95558a91db3381d70 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 14 Jul 2015 10:23:07 -0400 Subject: Preserve errno when loading encrypted ed25519 keys. --- src/or/routerkeys.c | 6 ++++++ 1 file changed, 6 insertions(+) (limited to 'src') diff --git a/src/or/routerkeys.c b/src/or/routerkeys.c index d075c67e6a..97a586dc56 100644 --- a/src/or/routerkeys.c +++ b/src/or/routerkeys.c @@ -21,6 +21,7 @@ read_encrypted_secret_key(ed25519_secret_key_t *out, char pwbuf[256]; uint8_t encrypted_key[256]; char *tag = NULL; + int saved_errno = 0; ssize_t encrypted_len = crypto_read_tagged_contents_from_file(fname, ENC_KEY_HEADER, @@ -28,6 +29,7 @@ read_encrypted_secret_key(ed25519_secret_key_t *out, encrypted_key, sizeof(encrypted_key)); if (encrypted_len < 0) { + saved_errno = errno; log_info(LD_OR, "%s is missing", fname); r = 0; goto done; @@ -46,6 +48,7 @@ read_encrypted_secret_key(ed25519_secret_key_t *out, pwbuf, pwlen); if (r == UNPWBOX_CORRUPTED) { log_err(LD_OR, "%s is corrupted.", fname); + saved_errno = EINVAL; goto done; } else if (r == UNPWBOX_OKAY) { break; @@ -57,6 +60,7 @@ read_encrypted_secret_key(ed25519_secret_key_t *out, if (secret_len != ED25519_SECKEY_LEN) { log_err(LD_OR, "%s is corrupted.", fname); + saved_errno = EINVAL; goto done; } memcpy(out->seckey, secret, ED25519_SECKEY_LEN); @@ -70,6 +74,8 @@ read_encrypted_secret_key(ed25519_secret_key_t *out, memwipe(secret, 0, secret_len); tor_free(secret); } + if (saved_errno) + errno = saved_errno; return r; } -- cgit v1.2.3-54-g00ecf From 5e8edba3d80bf53e5e5c09c8a87e06d0c69e00b7 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 14 Jul 2015 10:36:39 -0400 Subject: If loading an ed25519 master key fails with errno != ENOENT, give up. This implements feature 16582: if we get EMFILE or something when loading our master key, we should not at that point attempt to overwrite it. --- src/or/routerkeys.c | 36 ++++++++++++++++++++++++++++++------ src/or/routerkeys.h | 1 + 2 files changed, 31 insertions(+), 6 deletions(-) (limited to 'src') diff --git a/src/or/routerkeys.c b/src/or/routerkeys.c index 97a586dc56..946c48bc08 100644 --- a/src/or/routerkeys.c +++ b/src/or/routerkeys.c @@ -166,10 +166,13 @@ write_secret_key(const ed25519_secret_key_t *key, int encrypted, * public key file but no secret key file, return successfully anyway. * * If INIT_ED_KEY_OMIT_SECRET is set in flags, do not even try to - * load or return a secret key (but create and save on if needed). + * load or return a secret key (but create and save one if needed). * * If INIT_ED_KEY_TRY_ENCRYPTED is set, we look for an encrypted secret key * and consider encrypting any new secret key. + * + * If INIT_ED_KEY_NO_REPAIR is set, and there is any issue loading the keys + * from disk _other than their absence_, we do not try to replace them. */ ed25519_keypair_t * ed_key_init_from_file(const char *fname, uint32_t flags, @@ -187,6 +190,7 @@ ed_key_init_from_file(const char *fname, uint32_t flags, int created_pk = 0, created_sk = 0, created_cert = 0; const int try_to_load = ! (flags & INIT_ED_KEY_REPLACE); const int encrypt_key = (flags & INIT_ED_KEY_TRY_ENCRYPTED); + const int norepair = (flags & INIT_ED_KEY_NO_REPAIR); char tag[8]; tor_snprintf(tag, sizeof(tag), "type%d", (int)cert_type); @@ -201,10 +205,21 @@ ed_key_init_from_file(const char *fname, uint32_t flags, tor_asprintf(&cert_fname, "%s_cert", fname); /* Try to read the secret key. */ - int have_secret = try_to_load && - !(flags & INIT_ED_KEY_OMIT_SECRET) && - ed25519_seckey_read_from_file(&keypair->seckey, - &got_tag, secret_fname) == 0; + int have_secret = 0; + if (try_to_load && + !(flags & INIT_ED_KEY_OMIT_SECRET)) { + int rv = ed25519_seckey_read_from_file(&keypair->seckey, + &got_tag, secret_fname); + if (rv == 0) { + have_secret = 1; + } else { + if (errno != ENOENT && norepair) { + tor_log(severity, LD_OR, "Unable to read %s: %s", secret_fname, + strerror(errno)); + goto err; + } + } + } /* Should we try for an encrypted key? */ if (!have_secret && try_to_load && encrypt_key) { @@ -213,6 +228,10 @@ ed_key_init_from_file(const char *fname, uint32_t flags, if (r > 0) { have_secret = 1; got_tag = tor_strdup(tag); + } else if (errno != ENOENT && norepair) { + tor_log(severity, LD_OR, "Unable to read %s: %s", encrypted_secret_fname, + strerror(errno)); + goto err; } } @@ -234,6 +253,11 @@ ed_key_init_from_file(const char *fname, uint32_t flags, tor_free(got_tag); found_public = ed25519_pubkey_read_from_file(&keypair->pubkey, &got_tag, public_fname) == 0; + if (!found_public && errno != ENOENT && norepair) { + tor_log(severity, LD_OR, "Unable to read %s: %s", public_fname, + strerror(errno)); + goto err; + } if (found_public && strcmp(got_tag, tag)) { tor_log(severity, LD_OR, "%s has wrong tag", public_fname); goto err; @@ -486,7 +510,7 @@ load_ed_keys(const or_options_t *options, time_t now) { uint32_t flags = (INIT_ED_KEY_CREATE|INIT_ED_KEY_SPLIT| - INIT_ED_KEY_EXTRA_STRONG); + INIT_ED_KEY_EXTRA_STRONG|INIT_ED_KEY_NO_REPAIR); if (! need_new_signing_key) flags |= INIT_ED_KEY_MISSING_SECRET_OK; if (! want_new_signing_key) diff --git a/src/or/routerkeys.h b/src/or/routerkeys.h index 1e0199e5e6..9b93358ae3 100644 --- a/src/or/routerkeys.h +++ b/src/or/routerkeys.h @@ -15,6 +15,7 @@ #define INIT_ED_KEY_INCLUDE_SIGNING_KEY_IN_CERT (1u<<6) #define INIT_ED_KEY_OMIT_SECRET (1u<<7) #define INIT_ED_KEY_TRY_ENCRYPTED (1u<<8) +#define INIT_ED_KEY_NO_REPAIR (1u<<9) struct tor_cert_st; ed25519_keypair_t *ed_key_init_from_file(const char *fname, uint32_t flags, -- cgit v1.2.3-54-g00ecf From 13603265888d1e34b7a1ab8d83a361c0e9c34684 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 14 Jul 2015 11:10:13 -0400 Subject: Do more consistency checks in ed_key_init_from_file() When there is a signing key and the certificate lists a key, make sure that the certificate lists the same signing key. When there are public key and secret key stored in separate files, make sure they match. Use the right file name when we load an encrypted secret key and then find a problem with it. This is part of 16581. --- src/or/routerkeys.c | 40 ++++++++++++++++++++++++++++++++-------- 1 file changed, 32 insertions(+), 8 deletions(-) (limited to 'src') diff --git a/src/or/routerkeys.c b/src/or/routerkeys.c index 946c48bc08..81fa1152c1 100644 --- a/src/or/routerkeys.c +++ b/src/or/routerkeys.c @@ -187,10 +187,12 @@ ed_key_init_from_file(const char *fname, uint32_t flags, char *encrypted_secret_fname = NULL; char *public_fname = NULL; char *cert_fname = NULL; + const char *loaded_secret_fname = NULL; int created_pk = 0, created_sk = 0, created_cert = 0; const int try_to_load = ! (flags & INIT_ED_KEY_REPLACE); - const int encrypt_key = (flags & INIT_ED_KEY_TRY_ENCRYPTED); - const int norepair = (flags & INIT_ED_KEY_NO_REPAIR); + const int encrypt_key = !! (flags & INIT_ED_KEY_TRY_ENCRYPTED); + const int norepair = !! (flags & INIT_ED_KEY_NO_REPAIR); + const int split = !! (flags & INIT_ED_KEY_SPLIT); char tag[8]; tor_snprintf(tag, sizeof(tag), "type%d", (int)cert_type); @@ -212,6 +214,7 @@ ed_key_init_from_file(const char *fname, uint32_t flags, &got_tag, secret_fname); if (rv == 0) { have_secret = 1; + loaded_secret_fname = secret_fname; } else { if (errno != ENOENT && norepair) { tor_log(severity, LD_OR, "Unable to read %s: %s", secret_fname, @@ -228,6 +231,7 @@ ed_key_init_from_file(const char *fname, uint32_t flags, if (r > 0) { have_secret = 1; got_tag = tor_strdup(tag); + loaded_secret_fname = encrypted_secret_fname; } else if (errno != ENOENT && norepair) { tor_log(severity, LD_OR, "Unable to read %s: %s", encrypted_secret_fname, strerror(errno)); @@ -237,21 +241,24 @@ ed_key_init_from_file(const char *fname, uint32_t flags, if (have_secret) { if (strcmp(got_tag, tag)) { - tor_log(severity, LD_OR, "%s has wrong tag", secret_fname); + tor_log(severity, LD_OR, "%s has wrong tag", loaded_secret_fname); goto err; } /* Derive the public key */ if (ed25519_public_key_generate(&keypair->pubkey, &keypair->seckey)<0) { - tor_log(severity, LD_OR, "%s can't produce a public key", secret_fname); + tor_log(severity, LD_OR, "%s can't produce a public key", + loaded_secret_fname); goto err; } } - /* If it's absent and that's okay, try to read the pubkey. */ + /* If it's absent and that's okay, or if we do split keys here, try to re + * the pubkey. */ int found_public = 0; - if (!have_secret && try_to_load) { + if ((!have_secret && try_to_load) || (have_secret && split)) { + ed25519_public_key_t pubkey_tmp; tor_free(got_tag); - found_public = ed25519_pubkey_read_from_file(&keypair->pubkey, + found_public = ed25519_pubkey_read_from_file(&pubkey_tmp, &got_tag, public_fname) == 0; if (!found_public && errno != ENOENT && norepair) { tor_log(severity, LD_OR, "Unable to read %s: %s", public_fname, @@ -262,6 +269,20 @@ ed_key_init_from_file(const char *fname, uint32_t flags, tor_log(severity, LD_OR, "%s has wrong tag", public_fname); goto err; } + if (found_public) { + if (have_secret) { + /* If we have a secret key and we're reloading the public key, + * the key must match! */ + if (! ed25519_pubkey_eq(&keypair->pubkey, &pubkey_tmp)) { + tor_log(severity, LD_OR, "%s does not match %s!", + public_fname, loaded_secret_fname); + goto err; + } + } else { + tor_assert(split); + memcpy(&keypair->pubkey, &pubkey_tmp, sizeof(pubkey_tmp)); + } + } } /* If the secret key is absent and it's not allowed to be, fail. */ @@ -274,7 +295,6 @@ ed_key_init_from_file(const char *fname, uint32_t flags, /* if it's absent, make a new keypair and save it. */ if (!have_secret && !found_public) { - const int split = !! (flags & INIT_ED_KEY_SPLIT); tor_free(keypair); keypair = ed_key_new(signing_key, flags, now, lifetime, cert_type, &cert); @@ -328,6 +348,10 @@ ed_key_init_from_file(const char *fname, uint32_t flags, (signing_key || cert->cert_expired)) { tor_log(severity, LD_OR, "Can't check certificate"); bad_cert = 1; + } else if (signing_key && cert->signing_key_included && + ! ed25519_pubkey_eq(&signing_key->pubkey, &cert->signing_key)) { + tor_log(severity, LD_OR, "Certificate signed by unexpectd key!"); + bad_cert = 1; } if (bad_cert) { -- cgit v1.2.3-54-g00ecf From 3fcb74e98b7247f9b35e8a5067bfa915e1705d3e Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 14 Jul 2015 11:27:49 -0400 Subject: Add more consistency checks in load_ed_keys Make sure that signing certs are signed by the right identity key, to prevent a recurrence of #16530. Also make sure that the master identity key we find on disk matches the one we have in RAM, if we have one. This is for #16581. --- src/or/routerkeys.c | 19 +++++++++++++++++++ src/or/torcert.c | 9 +++++---- 2 files changed, 24 insertions(+), 4 deletions(-) (limited to 'src') diff --git a/src/or/routerkeys.c b/src/or/routerkeys.c index 81fa1152c1..d38b5a3ba3 100644 --- a/src/or/routerkeys.c +++ b/src/or/routerkeys.c @@ -569,9 +569,24 @@ load_ed_keys(const or_options_t *options, time_t now) sign_signing_key_with_id = id; } + if (master_identity_key && + !ed25519_pubkey_eq(&id->pubkey, &master_identity_key->pubkey)) { + FAIL("Identity key on disk does not match key we loaded earlier!"); + } + if (need_new_signing_key && NULL == sign_signing_key_with_id) FAIL("Can't load master key make a new signing key."); + if (sign_cert) { + if (! sign_cert->signing_key_included) + FAIL("Loaded a signing cert with no key included!"); + if (! ed25519_pubkey_eq(&sign_cert->signing_key, &id->pubkey)) + FAIL("The signing cert we have was not signed with the master key " + "we loaded!"); + if (tor_cert_checksig(sign_cert, &id->pubkey, 0) < 0) + FAIL("The signing cert we loaded was not signed correctly!"); + } + if (want_new_signing_key && sign_signing_key_with_id) { uint32_t flags = (INIT_ED_KEY_CREATE| INIT_ED_KEY_REPLACE| @@ -589,6 +604,10 @@ load_ed_keys(const or_options_t *options, time_t now) if (!sign) FAIL("Missing signing key"); use_signing = sign; + + tor_assert(sign_cert->signing_key_included); + tor_assert(ed25519_pubkey_eq(&sign_cert->signing_key, &id->pubkey)); + tor_assert(ed25519_pubkey_eq(&sign_cert->signed_key, &sign->pubkey)); } else if (want_new_signing_key) { static ratelim_t missing_master = RATELIM_INIT(3600); log_fn_ratelim(&missing_master, LOG_WARN, LD_OR, diff --git a/src/or/torcert.c b/src/or/torcert.c index f028910a70..596cd2be31 100644 --- a/src/or/torcert.c +++ b/src/or/torcert.c @@ -181,9 +181,10 @@ tor_cert_get_checkable_sig(ed25519_checkable_t *checkable_out, return 0; } -/** Validates the signature on cert with pubkey relative to - * the current time now. Return 0 on success, -1 on failure. - * Sets flags in cert as appropriate. +/** Validates the signature on cert with pubkey relative to the + * current time now. (If now is 0, do not check the expiration + * time.) Return 0 on success, -1 on failure. Sets flags in cert as + * appropriate. */ int tor_cert_checksig(tor_cert_t *cert, @@ -192,7 +193,7 @@ tor_cert_checksig(tor_cert_t *cert, ed25519_checkable_t checkable; int okay; - if (now > cert->valid_until) { + if (now && now > cert->valid_until) { cert->cert_expired = 1; return -1; } -- cgit v1.2.3-54-g00ecf From 3c28d95ca7c1f7086c2f840254a2d6663beaf935 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 15 Jul 2015 10:35:29 -0400 Subject: Add more EINVAL errno setting on key read failures Teor found these. This is for part of #16582. --- src/common/crypto_ed25519.c | 18 ++++++++++++------ src/common/util.c | 9 +++++++-- src/or/routerkeys.c | 8 ++++++-- 3 files changed, 25 insertions(+), 10 deletions(-) (limited to 'src') diff --git a/src/common/crypto_ed25519.c b/src/common/crypto_ed25519.c index 599a1ca9b7..1606d02c48 100644 --- a/src/common/crypto_ed25519.c +++ b/src/common/crypto_ed25519.c @@ -381,10 +381,13 @@ ed25519_seckey_read_from_file(ed25519_secret_key_t *seckey_out, len = crypto_read_tagged_contents_from_file(filename, "ed25519v1-secret", tag_out, seckey_out->seckey, sizeof(seckey_out->seckey)); - if (len != sizeof(seckey_out->seckey)) - return -1; + if (len == sizeof(seckey_out->seckey)) { + return 0; + } else if (len >= 0) { + errno = EINVAL; + } - return 0; + return -1; } /** @@ -417,10 +420,13 @@ ed25519_pubkey_read_from_file(ed25519_public_key_t *pubkey_out, len = crypto_read_tagged_contents_from_file(filename, "ed25519v1-public", tag_out, pubkey_out->pubkey, sizeof(pubkey_out->pubkey)); - if (len != sizeof(pubkey_out->pubkey)) - return -1; + if (len == sizeof(pubkey_out->pubkey)) { + return 0; + } else if (len >= 0) { + errno = EINVAL; + } - return 0; + return -1; } /** Release all storage held for kp. */ diff --git a/src/common/util.c b/src/common/util.c index a140057dea..1849613512 100644 --- a/src/common/util.c +++ b/src/common/util.c @@ -1997,8 +1997,10 @@ read_all(tor_socket_t fd, char *buf, size_t count, int isSocket) size_t numread = 0; ssize_t result; - if (count > SIZE_T_CEILING || count > SSIZE_MAX) + if (count > SIZE_T_CEILING || count > SSIZE_MAX) { + errno = EINVAL; return -1; + } while (numread != count) { if (isSocket) @@ -2558,8 +2560,10 @@ read_file_to_str_until_eof(int fd, size_t max_bytes_to_read, size_t *sz_out) char *string = NULL; size_t string_max = 0; - if (max_bytes_to_read+1 >= SIZE_T_CEILING) + if (max_bytes_to_read+1 >= SIZE_T_CEILING) { + errno = EINVAL; return NULL; + } do { /* XXXX This "add 1K" approach is a little goofy; if we care about @@ -2655,6 +2659,7 @@ read_file_to_str(const char *filename, int flags, struct stat *stat_out) if ((uint64_t)(statbuf.st_size)+1 >= SIZE_T_CEILING) { close(fd); + errno = EINVAL; return NULL; } diff --git a/src/or/routerkeys.c b/src/or/routerkeys.c index 946c48bc08..77bbcfd49f 100644 --- a/src/or/routerkeys.c +++ b/src/or/routerkeys.c @@ -34,14 +34,18 @@ read_encrypted_secret_key(ed25519_secret_key_t *out, r = 0; goto done; } - if (strcmp(tag, ENC_KEY_TAG)) + if (strcmp(tag, ENC_KEY_TAG)) { + saved_errno = EINVAL; goto done; + } while (1) { ssize_t pwlen = tor_getpass("Enter pasphrase for master key:", pwbuf, sizeof(pwbuf)); - if (pwlen < 0) + if (pwlen < 0) { + saved_errno = EINVAL; goto done; + } const int r = crypto_unpwbox(&secret, &secret_len, encrypted_key, encrypted_len, -- cgit v1.2.3-54-g00ecf From c4ab8f74da5cb1bc3b2a484b7316eb5e8f9aeb87 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 15 Jul 2015 10:45:40 -0400 Subject: Don't allow INIT_ED_KEY_{NO_REPAIR,NEEDCERT} to be used together. We haven't implemented NO_REPAIR for NEEDCERT, and we don't need it: but it's safest to stop any attempt to use it that way. --- src/or/routerkeys.c | 4 ++++ 1 file changed, 4 insertions(+) (limited to 'src') diff --git a/src/or/routerkeys.c b/src/or/routerkeys.c index 77bbcfd49f..c9afad9b6b 100644 --- a/src/or/routerkeys.c +++ b/src/or/routerkeys.c @@ -196,6 +196,10 @@ ed_key_init_from_file(const char *fname, uint32_t flags, const int encrypt_key = (flags & INIT_ED_KEY_TRY_ENCRYPTED); const int norepair = (flags & INIT_ED_KEY_NO_REPAIR); + /* we don't support setting both of these flags at once. */ + tor_assert((flags & (INIT_ED_KEY_NO_REPAIR|INIT_ED_KEY_NEEDCERT)) != + (INIT_ED_KEY_NO_REPAIR|INIT_ED_KEY_NEEDCERT)); + char tag[8]; tor_snprintf(tag, sizeof(tag), "type%d", (int)cert_type); -- cgit v1.2.3-54-g00ecf