Thank you Darshit. Please find the patch attached to this email. This should address all your concerns about the patch.
Best, Vijo. On Sat, Mar 4, 2017 at 9:26 PM, Darshit Shah <[email protected]> wrote: > Hi Vijo, > > Could you please send the patches as an attachment generated by `git > format-patch`? Gmail more often than not tends to break line endings making > it almost impossible to test the patches. > > I will currently let Eli look into the Windows and VMS compatibility for > safe_{f,}open() methods. However, I have two minor nitpicks about this > patch: > > 1. The log messages seem more in line with DEBUG than NOTQUIET. Any > specific reasons why you chose this log level? > 2. Why make the change to only one test case? Please remove the --debug > switch you added to it. Would it be possible to write a testcase that > actually shows this TOCTOU attack in practice? I guess not, but if you > have any ideas, do let us know. > > > * Vijo Cherian <[email protected]> [170304 19:51]: > > Thanks again Eli. Here is my revised patch, that addresses all except one >> of your concerns. >> One concern I didn't address is that of making file_exists_p() call inside >> fopen_stat(). >> That would require a lot more of code surgery than I should as I get >> familiar with wget code. >> But I can take on a cleanup task at a later time. >> >> Inlined patch this time. >> >> Best, >> Vijo. >> >> >> >> 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 <assert.h> >> #include <stdarg.h> >> #include <locale.h> >> +#include <errno.h> >> >> #if HAVE_UTIME >> # include <sys/types.h> >> @@ -586,21 +587,43 @@ remove_link (const char *file) >> return err; >> } >> >> -/* Does FILENAME exist? This is quite a lousy implementation, since >> - it supplies no error codes -- only a yes-or-no answer. Thus it >> - will return that a file does not exist if, e.g., the directory is >> - unreadable. I don't mind it too much currently, though. The >> - proper way should, of course, be to have a third, error state, >> - other than true/false, but that would introduce uncalled-for >> - additional complexity to the callers. */ >> +/* Does FILENAME exist? */ >> bool >> -file_exists_p (const char *filename) >> +file_exists_p (const char *filename, file_stats_t *fstats) >> { >> -#ifdef HAVE_ACCESS >> - return access (filename, F_OK) >= 0; >> -#else >> struct stat buf; >> - return stat (filename, &buf) >= 0; >> + >> +#if defined(WINDOWS) || defined(__VMS) >> + int ret = stat (filename, &buf); >> + if (ret >= 0) >> + { >> + if (fstats != NULL) >> + fstats->access_err = errno; >> + } >> + return ret >= 0; >> +#else >> + errno = 0; >> + if (stat (filename, &buf) == 0 && S_ISREG(buf.st_mode) && >> + (((S_IRUSR & buf.st_mode) && (getuid() == buf.st_uid)) || >> + ((S_IRGRP & buf.st_mode) && group_member(buf.st_gid)) || >> + (S_IROTH & buf.st_mode))) { >> + if (fstats != NULL) >> + { >> + fstats->access_err = 0; >> + fstats->st_ino = buf.st_ino; >> + fstats->st_dev = buf.st_dev; >> + } >> + return true; >> + } >> + else >> + { >> + if (fstats != NULL) >> + fstats->access_err = (errno == 0 ? EACCES : errno); >> + errno = 0; >> + return false; >> + } >> + __builtin_unreachable(); >> + /* NOTREACHED */ >> #endif >> } >> >> @@ -668,7 +691,7 @@ unique_name_1 (const char *prefix) >> >> do >> number_to_string (template_tail, count++); >> - while (file_exists_p (template)); >> + while (file_exists_p (template, NULL)); >> >> return xstrdup (template); >> } >> @@ -696,7 +719,7 @@ unique_name (const char *file, bool allow_passthrough) >> { >> /* If the FILE itself doesn't exist, return it without >> modification. */ >> - if (!file_exists_p (file)) >> + if (!file_exists_p (file, NULL)) >> return allow_passthrough ? (char *)file : xstrdup (file); >> >> /* Otherwise, find a numeric suffix that results in unused file name >> @@ -825,7 +848,7 @@ fopen_excl (const char *fname, int binary) >> /* Manually check whether the file exists. This is prone to race >> conditions, but systems without O_EXCL haven't deserved >> better. */ >> - if (file_exists_p (fname)) >> + if (file_exists_p (fname, NULL)) >> { >> errno = EEXIST; >> return NULL; >> @@ -834,6 +857,113 @@ fopen_excl (const char *fname, int binary) >> #endif /* not O_EXCL */ >> } >> >> +/* fopen_stat() assumes that file_exists_p() was called earlier. >> + file_stats_t passed to this function was returned from file_exists_p() >> + This is to prevent TOCTTOU race condition. >> + Details : FIO45-C from https://www.securecoding.cert.org/ >> + Note that for creating a new file, this check is not useful >> + >> + Input: >> + fname => Name of file to open >> + mode => File open mode >> + fstats => Saved file_stats_t about file that was checked for >> existence >> + >> + Returns: >> + NULL if there was an error >> + FILE * of opened file stream >> +*/ >> +FILE * >> +fopen_stat(const char *fname, const char *mode, file_stats_t *fstats) >> +{ >> + int fd; >> + FILE *fp; >> + struct stat fdstats; >> + >> + fp = fopen (fname, mode); >> + if (fp == NULL) >> + { >> + logprintf (LOG_NOTQUIET, _("Failed to Fopen file %s\n"), fname); >> + return NULL; >> + } >> + fd = fileno (fp); >> + if (fd < 0) >> + { >> + logprintf (LOG_NOTQUIET, _("Failed to get FD for file %s\n"), fname); >> + fclose (fp); >> + return NULL; >> + } >> + memset(&fdstats, 0, sizeof(fdstats)); >> + if (fstat (fd, &fdstats) == -1) >> + { >> + logprintf (LOG_NOTQUIET, _("Failed to stat file %s, (check >> permissions)\n"), fname); >> + fclose (fp); >> + return NULL; >> + } >> +#if !(defined(WINDOWS) || defined(__VMS)) >> + if (fstats != NULL && >> + (fdstats.st_dev != fstats->st_dev || >> + fdstats.st_ino != fstats->st_ino)) >> + { >> + /* File changed since file_exists_p() : NOT SAFE */ >> + logprintf (LOG_NOTQUIET, _("File %s changed since the last check. >> Security check failed."), fname); >> + fclose (fp); >> + return NULL; >> + } >> +#endif >> + >> + return fp; >> +} >> + >> +/* open_stat assumes that file_exists_p() was called earlier to save >> file_stats >> + file_stats_t passed to this function was returned from file_exists_p() >> + This is to prevent TOCTTOU race condition. >> + Details : FIO45-C from https://www.securecoding.cert.org/ >> + Note that for creating a new file, this check is not useful >> + >> + >> + Input: >> + fname => Name of file to open >> + flags => File open flags >> + mode => File open mode >> + fstats => Saved file_stats_t about file that was checked for >> existence >> + >> + Returns: >> + -1 if there was an error >> + file descriptor of opened file stream >> +*/ >> +int >> +open_stat(const char *fname, int flags, mode_t mode, file_stats_t >> *fstats) >> +{ >> + int fd; >> + struct stat fdstats; >> + >> + fd = open (fname, flags, mode); >> + if (fd < 0) >> + { >> + logprintf (LOG_NOTQUIET, _("Failed to open file %s, reason :%s\n"), >> fname, strerror(errno)); >> + return -1; >> + } >> + memset(&fdstats, 0, sizeof(fdstats)); >> + if (fstat (fd, &fdstats) == -1) >> + { >> + logprintf (LOG_NOTQUIET, _("Failed to stat file %s, error: %s\n"), >> fname, strerror(errno)); >> + return -1; >> + } >> +#if !(defined(WINDOWS) || defined(__VMS)) >> + if (fstats != NULL && >> + (fdstats.st_dev != fstats->st_dev || >> + fdstats.st_ino != fstats->st_ino)) >> + { >> + /* File changed since file_exists_p() : NOT SAFE */ >> + logprintf (LOG_NOTQUIET, _("Trying to open file %s but it changed >> since last check. Security check failed."), fname); >> + close (fd); >> + return -1; >> + } >> +#endif >> + >> + return fd; >> +} >> + >> /* Create DIRECTORY. If some of the pathname components of DIRECTORY >> are missing, create them first. In case any mkdir() call fails, >> return its error status. Returns 0 on successful completion. >> @@ -862,7 +992,7 @@ make_directory (const char *directory) >> /* Check whether the directory already exists. Allow creation of >> of intermediate directories to fail, as the initial path >> components >> are not necessarily directories! */ >> - if (!file_exists_p (dir)) >> + if (!file_exists_p (dir, NULL)) >> ret = mkdir (dir, 0777); >> else >> ret = 0; >> diff --git a/src/utils.h b/src/utils.h >> index aaac730..8acb258 100644 >> --- a/src/utils.h >> +++ b/src/utils.h >> @@ -78,15 +78,23 @@ void fork_to_background (void); >> char *aprintf (const char *, ...) GCC_FORMAT_ATTR (1, 2); >> char *concat_strings (const char *, ...); >> >> +typedef struct file_stat_s { >> + int access_err; /* Error in accecssing file : Not present >> vs permission */ >> + ino_t st_ino; /* st_ino from stats() on the file before >> open() */ >> + dev_t st_dev; /* st_dev from stats() on the file before >> open() */ >> +} file_stats_t; >> + >> void touch (const char *, time_t); >> int remove_link (const char *); >> -bool file_exists_p (const char *); >> +bool file_exists_p (const char *, file_stats_t *); >> bool file_non_directory_p (const char *); >> wgint file_size (const char *); >> int make_directory (const char *); >> char *unique_name (const char *, bool); >> FILE *unique_create (const char *, bool, char **); >> FILE *fopen_excl (const char *, int); >> +FILE *fopen_stat (const char *, const char *, file_stats_t *); >> +int open_stat (const char *, int, mode_t, file_stats_t *); >> char *file_merge (const char *, const char *); >> >> int fnmatch_nocase (const char *, const char *, int); >> >> >> On Sat, Mar 4, 2017 at 7:30 AM, Eli Zaretskii <[email protected]> wrote: >> >> > From: Vijo Cherian <[email protected]> >>> > Date: Fri, 3 Mar 2017 11:33:05 -0800 >>> > Cc: [email protected] >>> > >>> > bool >>> > file_exists_p (const char *filename, file_stats_t *fstats) >>> > { >>> > struct stat buf; >>> > >>> > #if defined(WINDOWS) || defined(__VMS) >>> > return stat (filename, &buf) >= 0; >>> > #else >>> >>> This leaves fstats untouched on Windows. At least access_err should >>> be set, I think. >>> >>> > -- > Thanking You, > Darshit Shah > PGP Fingerprint: 7845 120B 07CB D8D6 ECE5 FF2B 2A17 43ED A91A 35B6 >
From aef93c9ba7757f740349139dec40f8b8cdc40e5a Mon Sep 17 00:00:00 2001 From: Vijo Cherian <[email protected]> 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 <assert.h> #include <stdarg.h> #include <locale.h> +#include <errno.h> #if HAVE_UTIME # include <sys/types.h> @@ -586,21 +587,43 @@ remove_link (const char *file) return err; } -/* Does FILENAME exist? This is quite a lousy implementation, since - it supplies no error codes -- only a yes-or-no answer. Thus it - will return that a file does not exist if, e.g., the directory is - unreadable. I don't mind it too much currently, though. The - proper way should, of course, be to have a third, error state, - other than true/false, but that would introduce uncalled-for - additional complexity to the callers. */ +/* Does FILENAME exist? */ bool -file_exists_p (const char *filename) +file_exists_p (const char *filename, file_stats_t *fstats) { -#ifdef HAVE_ACCESS - return access (filename, F_OK) >= 0; -#else struct stat buf; - return stat (filename, &buf) >= 0; + +#if defined(WINDOWS) || defined(__VMS) + int ret = stat (filename, &buf); + if (ret >= 0) + { + if (fstats != NULL) + fstats->access_err = errno; + } + return ret >= 0; +#else + errno = 0; + if (stat (filename, &buf) == 0 && S_ISREG(buf.st_mode) && + (((S_IRUSR & buf.st_mode) && (getuid() == buf.st_uid)) || + ((S_IRGRP & buf.st_mode) && group_member(buf.st_gid)) || + (S_IROTH & buf.st_mode))) { + if (fstats != NULL) + { + fstats->access_err = 0; + fstats->st_ino = buf.st_ino; + fstats->st_dev = buf.st_dev; + } + return true; + } + else + { + if (fstats != NULL) + fstats->access_err = (errno == 0 ? EACCES : errno); + errno = 0; + return false; + } + __builtin_unreachable(); + /* NOTREACHED */ #endif } @@ -668,7 +691,7 @@ unique_name_1 (const char *prefix) do number_to_string (template_tail, count++); - while (file_exists_p (template)); + while (file_exists_p (template, NULL)); return xstrdup (template); } @@ -696,7 +719,7 @@ unique_name (const char *file, bool allow_passthrough) { /* If the FILE itself doesn't exist, return it without modification. */ - if (!file_exists_p (file)) + if (!file_exists_p (file, NULL)) return allow_passthrough ? (char *)file : xstrdup (file); /* Otherwise, find a numeric suffix that results in unused file name @@ -825,7 +848,7 @@ fopen_excl (const char *fname, int binary) /* Manually check whether the file exists. This is prone to race conditions, but systems without O_EXCL haven't deserved better. */ - if (file_exists_p (fname)) + if (file_exists_p (fname, NULL)) { errno = EEXIST; return NULL; @@ -834,6 +857,113 @@ fopen_excl (const char *fname, int binary) #endif /* not O_EXCL */ } +/* fopen_stat() assumes that file_exists_p() was called earlier. + file_stats_t passed to this function was returned from file_exists_p() + This is to prevent TOCTTOU race condition. + Details : FIO45-C from https://www.securecoding.cert.org/ + Note that for creating a new file, this check is not useful + + Input: + fname => Name of file to open + mode => File open mode + fstats => Saved file_stats_t about file that was checked for existence + + Returns: + NULL if there was an error + FILE * of opened file stream +*/ +FILE * +fopen_stat(const char *fname, const char *mode, file_stats_t *fstats) +{ + int fd; + FILE *fp; + struct stat fdstats; + + fp = fopen (fname, mode); + if (fp == NULL) + { + logprintf (LOG_NOTQUIET, _("Failed to Fopen file %s\n"), fname); + return NULL; + } + fd = fileno (fp); + if (fd < 0) + { + logprintf (LOG_NOTQUIET, _("Failed to get FD for file %s\n"), fname); + fclose (fp); + return NULL; + } + memset(&fdstats, 0, sizeof(fdstats)); + if (fstat (fd, &fdstats) == -1) + { + logprintf (LOG_NOTQUIET, _("Failed to stat file %s, (check permissions)\n"), fname); + fclose (fp); + return NULL; + } +#if !(defined(WINDOWS) || defined(__VMS)) + if (fstats != NULL && + (fdstats.st_dev != fstats->st_dev || + fdstats.st_ino != fstats->st_ino)) + { + /* File changed since file_exists_p() : NOT SAFE */ + logprintf (LOG_NOTQUIET, _("File %s changed since the last check. Security check failed."), fname); + fclose (fp); + return NULL; + } +#endif + + return fp; +} + +/* open_stat assumes that file_exists_p() was called earlier to save file_stats + file_stats_t passed to this function was returned from file_exists_p() + This is to prevent TOCTTOU race condition. + Details : FIO45-C from https://www.securecoding.cert.org/ + Note that for creating a new file, this check is not useful + + + Input: + fname => Name of file to open + flags => File open flags + mode => File open mode + fstats => Saved file_stats_t about file that was checked for existence + + Returns: + -1 if there was an error + file descriptor of opened file stream +*/ +int +open_stat(const char *fname, int flags, mode_t mode, file_stats_t *fstats) +{ + int fd; + struct stat fdstats; + + fd = open (fname, flags, mode); + if (fd < 0) + { + logprintf (LOG_NOTQUIET, _("Failed to open file %s, reason :%s\n"), fname, strerror(errno)); + return -1; + } + memset(&fdstats, 0, sizeof(fdstats)); + if (fstat (fd, &fdstats) == -1) + { + logprintf (LOG_NOTQUIET, _("Failed to stat file %s, error: %s\n"), fname, strerror(errno)); + return -1; + } +#if !(defined(WINDOWS) || defined(__VMS)) + if (fstats != NULL && + (fdstats.st_dev != fstats->st_dev || + fdstats.st_ino != fstats->st_ino)) + { + /* File changed since file_exists_p() : NOT SAFE */ + logprintf (LOG_NOTQUIET, _("Trying to open file %s but it changed since last check. Security check failed."), fname); + close (fd); + return -1; + } +#endif + + return fd; +} + /* Create DIRECTORY. If some of the pathname components of DIRECTORY are missing, create them first. In case any mkdir() call fails, return its error status. Returns 0 on successful completion. @@ -862,7 +992,7 @@ make_directory (const char *directory) /* Check whether the directory already exists. Allow creation of of intermediate directories to fail, as the initial path components are not necessarily directories! */ - if (!file_exists_p (dir)) + if (!file_exists_p (dir, NULL)) ret = mkdir (dir, 0777); else ret = 0; diff --git a/src/utils.h b/src/utils.h index aaac730..8acb258 100644 --- a/src/utils.h +++ b/src/utils.h @@ -78,15 +78,23 @@ void fork_to_background (void); char *aprintf (const char *, ...) GCC_FORMAT_ATTR (1, 2); char *concat_strings (const char *, ...); +typedef struct file_stat_s { + int access_err; /* Error in accecssing file : Not present vs permission */ + ino_t st_ino; /* st_ino from stats() on the file before open() */ + dev_t st_dev; /* st_dev from stats() on the file before open() */ +} file_stats_t; + void touch (const char *, time_t); int remove_link (const char *); -bool file_exists_p (const char *); +bool file_exists_p (const char *, file_stats_t *); bool file_non_directory_p (const char *); wgint file_size (const char *); int make_directory (const char *); char *unique_name (const char *, bool); FILE *unique_create (const char *, bool, char **); FILE *fopen_excl (const char *, int); +FILE *fopen_stat (const char *, const char *, file_stats_t *); +int open_stat (const char *, int, mode_t, file_stats_t *); char *file_merge (const char *, const char *); int fnmatch_nocase (const char *, const char *, int); -- 2.7.4
