From 867fe40f91b849393b56109586d85e499a53a142 Mon Sep 17 00:00:00 2001 From: teor Date: Wed, 2 May 2018 22:33:21 +1000 Subject: [PATCH 1/2] Stop logging stack contents when reading a zero-length bandwidth file When directory authorities read a zero-byte bandwidth file, they log a warning with the contents of an uninitialised buffer. Log a warning about the empty file instead. Fixes bug 26007; bugfix on 0.2.2.1-alpha. --- changes/bug26007 | 5 +++++ src/or/dirserv.c | 13 +++++++++++-- 2 files changed, 16 insertions(+), 2 deletions(-) create mode 100644 changes/bug26007 diff --git a/changes/bug26007 b/changes/bug26007 new file mode 100644 index 0000000000..efcd15084d --- /dev/null +++ b/changes/bug26007 @@ -0,0 +1,5 @@ + o Major bugfixes (directory authorities, security): + - When directory authorities read a zero-byte bandwidth file, they log + a warning with the contents of an uninitialised buffer. Log a warning + about the empty file instead. + Fixes bug 26007; bugfix on 0.2.2.1-alpha. diff --git a/src/or/dirserv.c b/src/or/dirserv.c index 41c6bf3dc8..94290d5dd8 100644 --- a/src/or/dirserv.c +++ b/src/or/dirserv.c @@ -2750,14 +2750,23 @@ dirserv_read_measured_bandwidths(const char *from_file, time_t file_time, now; int ok; + /* Initialise line, so that we can't possibly run off the end. */ + memset(line, 0, sizeof(line)); + if (fp == NULL) { log_warn(LD_CONFIG, "Can't open bandwidth file at configured location: %s", from_file); return -1; } - if (!fgets(line, sizeof(line), fp) - || !strlen(line) || line[strlen(line)-1] != '\n') { + /* If fgets fails, line is either unmodified, or indeterminate. */ + if (!fgets(line, sizeof(line), fp)) { + log_warn(LD_DIRSERV, "Empty bandwidth file"); + fclose(fp); + return -1; + } + + if (!strlen(line) || line[strlen(line)-1] != '\n') { log_warn(LD_DIRSERV, "Long or truncated time in bandwidth file: %s", escaped(line)); fclose(fp); From dbdde76f56d0ecb2ef03ac6ec231151016ffbd88 Mon Sep 17 00:00:00 2001 From: juga0 Date: Tue, 8 May 2018 16:23:37 +0000 Subject: [PATCH 2/2] Test read bandwidth measurements with empty file --- src/test/test_dir.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/src/test/test_dir.c b/src/test/test_dir.c index cdc56acb89..ad5f086434 100644 --- a/src/test/test_dir.c +++ b/src/test/test_dir.c @@ -1368,6 +1368,25 @@ test_dir_measured_bw_kb(void *arg) return; } +/* Test dirserv_read_measured_bandwidths */ +static void +test_dir_dirserv_read_measured_bandwidths(void *arg) +{ + char *fname=NULL; + (void)arg; + + fname = tor_strdup(get_fname("V3BandwidthsFile")); + /* Test an empty file */ + write_str_to_file(fname, "", 0); + setup_capture_of_logs(LOG_WARN); + tt_int_op(-1, OP_EQ, dirserv_read_measured_bandwidths(fname, NULL)); + expect_log_msg("Empty bandwidth file\n"); + + done: + tor_free(fname); + teardown_capture_of_logs(); +} + #define MBWC_INIT_TIME 1000 /** Do the measured bandwidth cache unit test */ @@ -5458,6 +5477,7 @@ struct testcase_t dir_tests[] = { DIR_LEGACY(versions), DIR_LEGACY(fp_pairs), DIR(split_fps, 0), + DIR_LEGACY(dirserv_read_measured_bandwidths), DIR_LEGACY(measured_bw_kb), DIR_LEGACY(measured_bw_kb_cache), DIR_LEGACY(param_voting),