[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Bug-wget] Fwd: PATCH: bugs 20369 and 20389
|
From: |
Vijo Cherian |
|
Subject: |
Re: [Bug-wget] Fwd: PATCH: bugs 20369 and 20389 |
|
Date: |
Sat, 4 Mar 2017 10:50:52 -0800 |
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 <address@hidden> wrote:
> > From: Vijo Cherian <address@hidden>
> > Date: Fri, 3 Mar 2017 11:33:05 -0800
> > Cc: address@hidden
> >
> > 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.
>