commit fd7e9e9030cee9d8e863cea3f3f90226ae66fdfe Author: teor <teor2345@gmail.com> Date: Sun Oct 19 17:48:07 2014 +1100 Stop failing when key files are zero-length Instead, generate new keys, and overwrite the empty key files. Adds FN_EMPTY to file_status_t and file_status. Fixes bug 13111. Related changes due to review of FN_FILE usage: Stop generating a fresh .old RSA key file when the .old file is missing. Avoid overwriting .old key files with empty key files. Skip loading zero-length extra info store, router store, stats, state, and key files. --- changes/bug13111-generate-keys-on-empty-file | 10 +++++++ src/common/compat.c | 1 + src/common/util.c | 30 ++++++++++++++------- src/common/util.h | 2 +- src/or/config.c | 22 +++++++++++----- src/or/router.c | 36 +++++++++++++++++++------- src/or/routerlist.c | 1 + src/or/statefile.c | 3 +++ 8 files changed, 79 insertions(+), 26 deletions(-) diff --git a/changes/bug13111-generate-keys-on-empty-file b/changes/bug13111-generate-keys-on-empty-file index f6a56ef..345e87a 100644 --- a/changes/bug13111-generate-keys-on-empty-file +++ b/changes/bug13111-generate-keys-on-empty-file @@ -1,7 +1,17 @@ o Minor bugfixes + - Stop failing when key files are zero-length. Instead, generate new + keys, and overwrite the empty key files. + Fixes bug 13111. + - Stop generating a fresh .old RSA key file when the .old file is missing. + Fixed as part of bug 13111. + - Avoid overwriting .old key files with empty key files. + Fixed as part of bug 13111. - Stop crashing when a NULL filename is passed to file_status(). Fixed as part of bug 13111. o Minor enhancements: + - Skip loading zero-length extra info store, router store, stats, state, + and key files. + Implemented with bug 13111. - Return FN_ERROR when a zero-length filename is passed to file_status(). Fixed as part of bug 13111. diff --git a/src/common/compat.c b/src/common/compat.c index e4758aa..b28790f 100644 --- a/src/common/compat.c +++ b/src/common/compat.c @@ -823,6 +823,7 @@ replace_file(const char *from, const char *to) case FN_NOENT: break; case FN_FILE: + case FN_EMPTY: if (unlink(to)) return -1; break; case FN_ERROR: diff --git a/src/common/util.c b/src/common/util.c index e850b14..1c35338 100644 --- a/src/common/util.c +++ b/src/common/util.c @@ -1888,10 +1888,15 @@ clean_name_for_stat(char *name) #endif } -/** Return FN_ERROR if filename can't be read, or is NULL or zero-length, - * FN_NOENT if it doesn't - * exist, FN_FILE if it is a regular file, or FN_DIR if it's a - * directory. On FN_ERROR, sets errno. */ +/** Return: + * FN_ERROR if filename can't be read, is NULL, or is zero-length, + * FN_NOENT if it doesn't exist, + * FN_FILE if it is a non-empty regular file, or a FIFO on unix-like systems, + * FN_EMPTY for zero-byte regular files, + * FN_DIR if it's a directory, and + * FN_ERROR for any other file type. + * On FN_ERROR and FN_NOENT, sets errno. (errno is not set when FN_ERROR + * is returned due to an unhandled file type.) */ file_status_t file_status(const char *fname) { @@ -1912,16 +1917,23 @@ file_status(const char *fname) } return FN_ERROR; } - if (st.st_mode & S_IFDIR) + if (st.st_mode & S_IFDIR) { return FN_DIR; - else if (st.st_mode & S_IFREG) - return FN_FILE; + } else if (st.st_mode & S_IFREG) { + if (st.st_size > 0) { + return FN_FILE; + } else if (st.st_size == 0) { + return FN_EMPTY; + } else { + return FN_ERROR; + } #ifndef _WIN32 - else if (st.st_mode & S_IFIFO) + } else if (st.st_mode & S_IFIFO) { return FN_FILE; #endif - else + } else { return FN_ERROR; + } } /** Check whether <b>dirname</b> exists and is private. If yes return 0. If diff --git a/src/common/util.h b/src/common/util.h index d7feb6e..c5471ff 100644 --- a/src/common/util.h +++ b/src/common/util.h @@ -334,7 +334,7 @@ enum stream_status get_string_from_pipe(FILE *stream, char *buf, size_t count); /** Return values from file_status(); see that function's documentation * for details. */ -typedef enum { FN_ERROR, FN_NOENT, FN_FILE, FN_DIR } file_status_t; +typedef enum { FN_ERROR, FN_NOENT, FN_FILE, FN_DIR, FN_EMPTY } file_status_t; file_status_t file_status(const char *filename); /** Possible behaviors for check_private_dir() on encountering a nonexistent diff --git a/src/or/config.c b/src/or/config.c index 3f31e87..3e7a73f 100644 --- a/src/or/config.c +++ b/src/or/config.c @@ -4017,17 +4017,24 @@ find_torrc_filename(config_line_t *cmd_arg, if (*using_default_fname) { /* didn't find one, try CONFDIR */ const char *dflt = get_default_conf_file(defaults_file); - if (dflt && file_status(dflt) == FN_FILE) { + file_status_t st = file_status(dflt); + if (dflt && (st == FN_FILE || st == FN_EMPTY)) { fname = tor_strdup(dflt); } else { #ifndef _WIN32 char *fn = NULL; - if (!defaults_file) + if (!defaults_file) { fn = expand_filename("~/.torrc"); - if (fn && file_status(fn) == FN_FILE) { - fname = fn; + } + if (fn) { + file_status_t hmst = file_status(fn); + if (hmst == FN_FILE || hmst == FN_EMPTY) { + fname = fn; + } else { + tor_free(fn); + fname = tor_strdup(dflt); + } } else { - tor_free(fn); fname = tor_strdup(dflt); } #else @@ -4063,7 +4070,8 @@ load_torrc_from_disk(config_line_t *cmd_arg, int defaults_file) *fname_var = fname; /* Open config file */ - if (file_status(fname) != FN_FILE || + file_status_t st = file_status(fname); + if (!(st == FN_FILE || st == FN_EMPTY) || !(cf = read_file_to_str(fname,0,NULL))) { if (using_default_torrc == 1 || ignore_missing_torrc) { if (!defaults_file) @@ -6413,7 +6421,9 @@ write_configuration_file(const char *fname, const or_options_t *options) tor_assert(fname); switch (file_status(fname)) { + /* create backups of old config files, even if they're empty */ case FN_FILE: + case FN_EMPTY: old_val = read_file_to_str(fname, 0, NULL); if (!old_val || strcmpstart(old_val, GENERATED_FILE_PREFIX)) { rename_old = 1; diff --git a/src/or/router.c b/src/or/router.c index 01838b4..94ae2e7 100644 --- a/src/or/router.c +++ b/src/or/router.c @@ -313,6 +313,7 @@ rotate_onion_key(void) time_t now; fname = get_datadir_fname2("keys", "secret_onion_key"); fname_prev = get_datadir_fname2("keys", "secret_onion_key.old"); + /* There isn't much point replacing an old key with an empty file */ if (file_status(fname) == FN_FILE) { if (replace_file(fname, fname_prev)) goto error; @@ -335,6 +336,7 @@ rotate_onion_key(void) fname_prev = get_datadir_fname2("keys", "secret_onion_key_ntor.old"); if (curve25519_keypair_generate(&new_curve25519_keypair, 1) < 0) goto error; + /* There isn't much point replacing an old key with an empty file */ if (file_status(fname) == FN_FILE) { if (replace_file(fname, fname_prev)) goto error; @@ -389,9 +391,9 @@ log_new_relay_greeting(void) already_logged = 1; } -/** Try to read an RSA key from <b>fname</b>. If <b>fname</b> doesn't exist - * and <b>generate</b> is true, create a new RSA key and save it in - * <b>fname</b>. Return the read/created key, or NULL on error. Log all +/** Try to read an RSA key from <b>fname</b>. If <b>fname</b> doesn't exist, + * or is empty, and <b>generate</b> is true, create a new RSA key and save it + * in <b>fname</b>. Return the read/created key, or NULL on error. Log all * errors at level <b>severity</b>. */ crypto_pk_t * @@ -409,7 +411,11 @@ init_key_from_file(const char *fname, int generate, int severity) case FN_ERROR: tor_log(severity, LD_FS,"Can't read key from \"%s\"", fname); goto error; + /* treat empty key files as if the file doesn't exist, and, + * if generate is set, replace the empty file in + * crypto_pk_write_private_key_to_filename() */ case FN_NOENT: + case FN_EMPTY: if (generate) { if (!have_lockfile()) { if (try_locking(get_options(), 0)<0) { @@ -460,10 +466,10 @@ init_key_from_file(const char *fname, int generate, int severity) } /** Load a curve25519 keypair from the file <b>fname</b>, writing it into - * <b>keys_out</b>. If the file isn't found and <b>generate</b> is true, - * create a new keypair and write it into the file. If there are errors, log - * them at level <b>severity</b>. Generate files using <b>tag</b> in their - * ASCII wrapper. */ + * <b>keys_out</b>. If the file isn't found, or is empty, and <b>generate</b> + * is true, create a new keypair and write it into the file. If there are + * errors, log them at level <b>severity</b>. Generate files using <b>tag</b> + * in their ASCII wrapper. */ static int init_curve25519_keypair_from_file(curve25519_keypair_t *keys_out, const char *fname, @@ -476,7 +482,10 @@ init_curve25519_keypair_from_file(curve25519_keypair_t *keys_out, case FN_ERROR: tor_log(severity, LD_FS,"Can't read key from \"%s\"", fname); goto error; + /* treat empty key files as if the file doesn't exist, and, if generate + * is set, replace the empty file in curve25519_keypair_write_to_file() */ case FN_NOENT: + case FN_EMPTY: if (generate) { if (!have_lockfile()) { if (try_locking(get_options(), 0)<0) { @@ -876,7 +885,9 @@ init_keys(void) keydir = get_datadir_fname2("keys", "secret_onion_key.old"); if (!lastonionkey && file_status(keydir) == FN_FILE) { - prkey = init_key_from_file(keydir, 1, LOG_ERR); /* XXXX Why 1? */ + /* Load keys from non-empty files only. + * Missing old keys won't be replaced with freshly generated keys. */ + prkey = init_key_from_file(keydir, 0, LOG_ERR); if (prkey) lastonionkey = prkey; } @@ -897,6 +908,8 @@ init_keys(void) last_curve25519_onion_key.pubkey.public_key, CURVE25519_PUBKEY_LEN) && file_status(keydir) == FN_FILE) { + /* Load keys from non-empty files only. + * Missing old keys won't be replaced with freshly generated keys. */ init_curve25519_keypair_from_file(&last_curve25519_onion_key, keydir, 0, LOG_ERR, "onion"); } @@ -2562,8 +2575,9 @@ router_has_orport(const routerinfo_t *router, const tor_addr_port_t *orport) * <b>end_line</b>, ensure that its timestamp is not more than 25 hours in * the past or more than 1 hour in the future with respect to <b>now</b>, * and write the file contents starting with that line to *<b>out</b>. - * Return 1 for success, 0 if the file does not exist, or -1 if the file - * does not contain a line matching these criteria or other failure. */ + * Return 1 for success, 0 if the file does not exist or is empty, or -1 + * if the file does not contain a line matching these criteria or other + * failure. */ static int load_stats_file(const char *filename, const char *end_line, time_t now, char **out) @@ -2597,7 +2611,9 @@ load_stats_file(const char *filename, const char *end_line, time_t now, notfound: tor_free(contents); break; + /* treat empty stats files as if the file doesn't exist */ case FN_NOENT: + case FN_EMPTY: r = 0; break; case FN_ERROR: diff --git a/src/or/routerlist.c b/src/or/routerlist.c index 0099486..5c08f59 100644 --- a/src/or/routerlist.c +++ b/src/or/routerlist.c @@ -1204,6 +1204,7 @@ router_reload_router_list_impl(desc_store_t *store) tor_free(fname); fname = get_datadir_fname_suffix(store->fname_base, ".new"); + /* don't load empty files - we wouldn't get any data, even if we tried */ if (file_status(fname) == FN_FILE) contents = read_file_to_str(fname, RFTS_BIN|RFTS_IGNORE_MISSING, &st); if (contents) { diff --git a/src/or/statefile.c b/src/or/statefile.c index 2ce53fd..6640aed 100644 --- a/src/or/statefile.c +++ b/src/or/statefile.c @@ -323,7 +323,10 @@ or_state_load(void) goto done; } break; + /* treat empty state files as if the file doesn't exist, and generate + * a new state file, overwriting the empty file in or_state_save() */ case FN_NOENT: + case FN_EMPTY: break; case FN_ERROR: case FN_DIR: