From aef93c9ba7757f740349139dec40f8b8cdc40e5a Mon Sep 17 00:00:00 2001 From: Vijo Cherian
Date: Sun, 5 Mar 2017 21:22:00 -0800 Subject: [PATCH] BUGFIX Bug #20369 - Safeguards against TOCTTOU Added fopen_stat() and open_stat() that checks to makes sure the file didn't change underneath us. Bug #20389 - Return error from file_exists_p() Added a way to return error from this file without major surgery to the callers. TESTING `make check` passes Tests on Ubuntu work. --- src/ftp.c | 6 +-- src/hsts.c | 7 +-- src/http.c | 14 ++--- src/init.c | 27 +++++----- src/init.h | 2 +- src/main.c | 10 ++-- src/metalink.c | 8 +-- src/retr.c | 2 +- src/url.c | 2 +- src/utils.c | 162 +++++++++++++++++++++++++++++++++++++++++++++++++++------ src/utils.h | 10 +++- 11 files changed, 197 insertions(+), 53 deletions(-) diff --git a/src/ftp.c b/src/ftp.c index 2f2866c..a790c39 100644 --- a/src/ftp.c +++ b/src/ftp.c @@ -1463,7 +1463,7 @@ Error in server response, closing control connection.\n")); else if (opt.noclobber || opt.always_rest || opt.timestamping || opt.dirstruct || opt.output_document || count > 0) { - if (opt.unlink_requested && file_exists_p (con->target)) + if (opt.unlink_requested && file_exists_p (con->target, NULL)) { if (unlink (con->target) < 0) { @@ -1857,7 +1857,7 @@ ftp_loop_internal (struct url *u, struct url *original_url, struct fileinfo *f, /* If we receive .listing file it is necessary to determine system type of the ftp server even if opn.noclobber is given. Thus we must ignore opt.noclobber in order to establish connection with the server and get system type. */ - if (opt.noclobber && !opt.output_document && file_exists_p (con->target) + if (opt.noclobber && !opt.output_document && file_exists_p (con->target, NULL) && !((con->cmd & DO_LIST) && !(con->cmd & DO_RETR))) { logprintf (LOG_VERBOSE, @@ -2413,7 +2413,7 @@ Already have correct symlink %s -> %s\n\n"), && !(f->type == FT_SYMLINK && !opt.retr_symlinks) && f->tstamp != -1 && dlthis - && file_exists_p (con->target)) + && file_exists_p (con->target, NULL)) { touch (actual_target, f->tstamp); } diff --git a/src/hsts.c b/src/hsts.c index 91cc527..67bdde6 100644 --- a/src/hsts.c +++ b/src/hsts.c @@ -32,9 +32,9 @@ as that of the covered work. */ #ifdef HAVE_HSTS #include "hsts.h" +#include "utils.h" #include "host.h" /* for is_valid_ip_address() */ #include "init.h" /* for home_dir() */ -#include "utils.h" #include "hash.h" #include "c-ctype.h" #ifdef TESTING @@ -500,18 +500,19 @@ hsts_store_t hsts_store_open (const char *filename) { hsts_store_t store = NULL; + file_stats_t fstats; store = xnew0 (struct hsts_store); store->table = hash_table_new (0, hsts_hash_func, hsts_cmp_func); store->last_mtime = 0; store->changed = false; - if (file_exists_p (filename)) + if (file_exists_p (filename, &fstats)) { if (hsts_file_access_valid (filename)) { struct stat st; - FILE *fp = fopen (filename, "r"); + FILE *fp = fopen_stat (filename, "r", &fstats); if (!fp || !hsts_read_database (store, fp, false)) { diff --git a/src/http.c b/src/http.c index d2c5c77..f42963f 100644 --- a/src/http.c +++ b/src/http.c @@ -2290,7 +2290,7 @@ check_file_output (const struct url *u, struct http_stat *hs, } /* TODO: perform this check only once. */ - if (!hs->existence_checked && file_exists_p (hs->local_file)) + if (!hs->existence_checked && file_exists_p (hs->local_file, NULL)) { if (opt.noclobber && !opt.output_document) { @@ -2486,7 +2486,7 @@ open_output_stream (struct http_stat *hs, int count, FILE **fp) } else if (ALLOW_CLOBBER || count > 0) { - if (opt.unlink_requested && file_exists_p (hs->local_file)) + if (opt.unlink_requested && file_exists_p (hs->local_file, NULL)) { if (unlink (hs->local_file) < 0) { @@ -4050,7 +4050,7 @@ http_loop (const struct url *u, struct url *original_url, char **newloc, got_name = true; } - if (got_name && file_exists_p (hstat.local_file) && opt.noclobber && !opt.output_document) + if (got_name && file_exists_p (hstat.local_file, NULL) && opt.noclobber && !opt.output_document) { /* If opt.noclobber is turned on and file already exists, do not retrieve the file. But if the output_document was given, then this @@ -4087,7 +4087,7 @@ http_loop (const struct url *u, struct url *original_url, char **newloc, { /* Use conditional get request if requested * and if timestamp is known at this moment. */ - if (opt.if_modified_since && !send_head_first && got_name && file_exists_p (hstat.local_file)) + if (opt.if_modified_since && !send_head_first && got_name && file_exists_p (hstat.local_file, NULL)) { *dt |= IF_MODIFIED_SINCE; { @@ -4098,7 +4098,7 @@ http_loop (const struct url *u, struct url *original_url, char **newloc, } /* Send preliminary HEAD request if -N is given and we have existing * destination file or content disposition is enabled. */ - else if (opt.content_disposition || file_exists_p (hstat.local_file)) + else if (opt.content_disposition || file_exists_p (hstat.local_file, NULL)) send_head_first = true; } @@ -5103,13 +5103,13 @@ ensure_extension (struct http_stat *hs, const char *ext, int *dt) strcpy (hs->local_file + local_filename_len, ext); /* If clobbering is not allowed and the file, as named, exists, tack on ".NUMBER.html" instead. */ - if (!ALLOW_CLOBBER && file_exists_p (hs->local_file)) + if (!ALLOW_CLOBBER && file_exists_p (hs->local_file, NULL)) { int ext_num = 1; do sprintf (hs->local_file + local_filename_len, ".%d%s", ext_num++, ext); - while (file_exists_p (hs->local_file)); + while (file_exists_p (hs->local_file, NULL)); } *dt |= ADDED_HTML_EXTENSION; } diff --git a/src/init.c b/src/init.c index e6aa673..63c89b5 100644 --- a/src/init.c +++ b/src/init.c @@ -566,10 +566,11 @@ wgetrc_env_file_name (void) char *env = getenv ("WGETRC"); if (env && *env) { - if (!file_exists_p (env)) + file_stats_t flstat; + if (!file_exists_p (env, &flstat)) { - fprintf (stderr, _("%s: WGETRC points to %s, which doesn't exist.\n"), - exec_name, env); + fprintf (stderr, _("%s: WGETRC points to %s, which couldn't be accessed because of error: %s.\n"), + exec_name, env, strerror(flstat.access_err)); exit (WGET_EXIT_GENERIC_ERROR); } return xstrdup (env); @@ -597,7 +598,7 @@ wgetrc_user_file_name (void) if (!file) return NULL; - if (!file_exists_p (file)) + if (!file_exists_p (file, NULL)) { xfree (file); return NULL; @@ -630,7 +631,7 @@ wgetrc_file_name (void) if (home) { file = aprintf ("%s/wget.ini", home); - if (!file_exists_p (file)) + if (!file_exists_p (file, NULL)) { xfree (file); } @@ -658,7 +659,7 @@ static bool setval_internal_tilde (int, const char *, const char *); there were errors in the file. */ bool -run_wgetrc (const char *file) +run_wgetrc (const char *file, file_stats_t *flstats) { FILE *fp; char *line = NULL; @@ -666,7 +667,7 @@ run_wgetrc (const char *file) int ln; int errcnt = 0; - fp = fopen (file, "r"); + fp = fopen_stat (file, "r", flstats); if (!fp) { fprintf (stderr, _("%s: Cannot read %s (%s).\n"), exec_name, @@ -722,14 +723,16 @@ void initialize (void) { char *file, *env_sysrc; + file_stats_t flstats; bool ok = true; + memset(&flstats, 0, sizeof(flstats)); /* Run a non-standard system rc file when the according environment variable has been set. For internal testing purposes only! */ env_sysrc = getenv ("SYSTEM_WGETRC"); - if (env_sysrc && file_exists_p (env_sysrc)) + if (env_sysrc && file_exists_p (env_sysrc, &flstats)) { - ok &= run_wgetrc (env_sysrc); + ok &= run_wgetrc (env_sysrc, &flstats); /* If there are any problems parsing the system wgetrc file, tell the user and exit */ if (! ok) @@ -743,8 +746,8 @@ or specify a different file using --config.\n"), env_sysrc); } /* Otherwise, if SYSTEM_WGETRC is defined, use it. */ #ifdef SYSTEM_WGETRC - else if (file_exists_p (SYSTEM_WGETRC)) - ok &= run_wgetrc (SYSTEM_WGETRC); + else if (file_exists_p (SYSTEM_WGETRC, &flstats)) + ok &= run_wgetrc (SYSTEM_WGETRC, &flstats); /* If there are any problems parsing the system wgetrc file, tell the user and exit */ if (! ok) @@ -771,7 +774,7 @@ or specify a different file using --config.\n"), SYSTEM_WGETRC); } else #endif - ok &= run_wgetrc (file); + ok &= run_wgetrc (file, &flstats); /* If there were errors processing either `.wgetrc', abort. */ if (!ok) diff --git a/src/init.h b/src/init.h index 1969cc7..a57ca78 100644 --- a/src/init.h +++ b/src/init.h @@ -41,6 +41,6 @@ void setoptval (const char *, const char *, const char *); char *home_dir (void); void cleanup (void); void defaults (void); -bool run_wgetrc (const char *file); +bool run_wgetrc (const char *file, file_stats_t *); #endif /* INIT_H */ diff --git a/src/main.c b/src/main.c index 311a91f..a1d5714 100644 --- a/src/main.c +++ b/src/main.c @@ -1381,10 +1381,10 @@ main (int argc, char **argv) } else if (strcmp (config_opt->long_name, "config") == 0) { - bool userrc_ret = true; - userrc_ret &= run_wgetrc (optarg); + file_stats_t flstats; use_userconfig = true; - if (userrc_ret) + memset(&flstats, 0, sizeof(flstats)); + if (file_exists_p(optarg, &flstats) && run_wgetrc (optarg, &flstats)) break; else { @@ -1620,7 +1620,7 @@ WARNING: timestamping does nothing in combination with -O. See the manual\n\ for details.\n\n")); opt.timestamping = false; } - if (opt.noclobber && file_exists_p(opt.output_document)) + if (opt.noclobber && file_exists_p(opt.output_document, NULL)) { /* Check if output file exists; if it does, exit. */ logprintf (LOG_VERBOSE, @@ -2081,7 +2081,7 @@ only if outputting to a regular file.\n")); &dt, opt.recursive, iri, true); } - if (opt.delete_after && filename != NULL && file_exists_p (filename)) + if (opt.delete_after && filename != NULL && file_exists_p (filename, NULL)) { DEBUGP (("Removing file due to --delete-after in main():\n")); logprintf (LOG_VERBOSE, _("Removing %s.\n"), filename); diff --git a/src/metalink.c b/src/metalink.c index 904f9f3..6bee3c9 100644 --- a/src/metalink.c +++ b/src/metalink.c @@ -375,6 +375,7 @@ retrieve_from_metalink (const metalink_t* metalink) metalink_checksum_t **mchksum_ptr, *mchksum; struct iri *iri; struct url *url; + file_stats_t flstats; int url_err; clean_metalink_string (&mres->url); @@ -490,8 +491,9 @@ retrieve_from_metalink (const metalink_t* metalink) Bugfix: point output_stream to destname if it exists. */ - if (!output_stream && file_exists_p (destname)) - output_stream = fopen (destname, "ab"); + memset(&flstats, 0, sizeof(&flstats)); + if (!output_stream && file_exists_p (destname, &flstats)) + output_stream = fopen_stat (destname, "ab", &flstats); } url_free (url); iri_free (iri); @@ -901,7 +903,7 @@ gpg_skip_verification: Note: the file has been downloaded using *_loop. Therefore, it is not necessary to keep the file for continuated download. */ if (((retr_err != RETROK && !opt.always_rest) || opt.delete_after) - && destname != NULL && file_exists_p (destname)) + && destname != NULL && file_exists_p (destname, NULL)) { badhash_or_remove (destname); } diff --git a/src/retr.c b/src/retr.c index 5ba744f..0cf438e 100644 --- a/src/retr.c +++ b/src/retr.c @@ -1141,7 +1141,7 @@ retrieve_from_file (const char *file, bool html, int *count) if (parsed_url) url_free (parsed_url); - if (filename && opt.delete_after && file_exists_p (filename)) + if (filename && opt.delete_after && file_exists_p (filename, NULL)) { DEBUGP (("\ Removing file due to --delete-after in retrieve_from_file():\n")); diff --git a/src/url.c b/src/url.c index 8f8ff0b..01098b7 100644 --- a/src/url.c +++ b/src/url.c @@ -1807,7 +1807,7 @@ url_file_name (const struct url *u, char *replaced_filename) directory (see `mkalldirs' for explanation). */ if (ALLOW_CLOBBER - && !(file_exists_p (fname) && !file_non_directory_p (fname))) + && !(file_exists_p (fname, NULL) && !file_non_directory_p (fname))) { unique = fname; } diff --git a/src/utils.c b/src/utils.c index db89ae1..3acc78d 100644 --- a/src/utils.c +++ b/src/utils.c @@ -45,6 +45,7 @@ as that of the covered work. */ #include