[Coverity Scan is ok, make syntax-check is ok, make check-valgrind is ok, contrib/check-hard is ok]
This enforces the use of libmetalink's metalink_check_safe_path() to verify that the destination file name is safe. The libmetalink's metalink_check_safe_path() advices against the use of unsafe file names. The following description is verbatim from the patch: ----- Unsafe file names contain an absolute, relative, or home path. Safe paths can be verified by libmetalink's metalink_check_safe_path(). ----- Regards, Matthew -- Matthew White <[email protected]>
>From 09f5deedf756ac61cfe9cce59cd4dd95446baacd Mon Sep 17 00:00:00 2001 From: Matthew White <[email protected]> Date: Wed, 17 Aug 2016 16:50:18 +0200 Subject: [PATCH 09/25] Enforce Metalink file name verification, strip directory if necessary * src/metalink.h: Add declaration of functions get_metalink_basename(), metalink_check_safe_path() * src/metalink.c: Add function get_metalink_basename() to return the basename of a file name * src/metalink.c (retrieve_from_metalink): Enforce Metalink file name verification, if the file name is unsafe try its basename * doc/metalink.txt: Update document. Explain --directory-prefix Unsafe file names contain an absolute, relative, or home path. Safe paths can be verified by libmetalink's metalink_check_safe_path(). --- doc/metalink.txt | 4 ++++ src/metalink.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++---- src/metalink.h | 3 +++ 3 files changed, 58 insertions(+), 4 deletions(-) diff --git a/doc/metalink.txt b/doc/metalink.txt index 94a07ba..9d9dea2 100644 --- a/doc/metalink.txt +++ b/doc/metalink.txt @@ -159,3 +159,7 @@ References: Set the top of the retrieval tree to prefix for both Metalink/XML and Metalink/HTTP downloads, see wget(1). + + If combining the prefix with the file name results in an absolute, + relative, or home path, the directory components are stripped and + only the basename is used. See '4.1 Implemented safety features'. diff --git a/src/metalink.c b/src/metalink.c index e34b410..a8b047b 100644 --- a/src/metalink.c +++ b/src/metalink.c @@ -87,6 +87,8 @@ retrieve_from_metalink (const metalink_t* metalink) metalink_file_t *mfile = *mfile_ptr; metalink_resource_t **mres_ptr; char *filename = NULL; + char *basename = NULL; + char *safename = NULL; char *destname = NULL; bool hash_ok = false; @@ -110,6 +112,23 @@ retrieve_from_metalink (const metalink_t* metalink) DEBUGP (("Processing metalink file %s...\n", quote (mfile->name))); + /* Enforce libmetalink's metalink_check_safe_path(). */ + basename = get_metalink_basename (filename); + safename = metalink_check_safe_path (filename) ? filename : basename; + + if (filename != safename) + logprintf (LOG_NOTQUIET, + _("Unsafe metalink file %s. Stripping directory...\n"), + quote (filename)); + + if (!basename) + { + logprintf (LOG_NOTQUIET, + _("Rejecting metalink file. Invalid basename.\n")); + xfree (filename); + continue; + } + /* Resources are sorted by priority. */ for (mres_ptr = mfile->resources; *mres_ptr && !skip_mfile; mres_ptr++) { @@ -170,6 +189,12 @@ retrieve_from_metalink (const metalink_t* metalink) /* Avoid recursive Metalink from HTTP headers. */ bool _metalink_http = opt.metalink_over_http; + /* FIXME: could be useless. */ + if (strcmp (url->file, basename)) + logprintf (LOG_VERBOSE, + _("URL file name %s and Metalink file name %s are different.\n"), + quote_n (0, url->file), quote_n (1, basename)); + /* If output_stream is not NULL, then we have failed on previous resource and are retrying. Thus, continue with the next resource. Do not close output_stream @@ -188,10 +213,10 @@ retrieve_from_metalink (const metalink_t* metalink) after we are finished with the file. */ if (opt.always_rest) /* continue previous download */ - output_stream = fopen (filename, "ab"); + output_stream = fopen (safename, "ab"); else /* create a file with an unique name */ - output_stream = unique_create (filename, true, &destname); + output_stream = unique_create (safename, true, &destname); } output_stream_regular = true; @@ -212,7 +237,7 @@ retrieve_from_metalink (const metalink_t* metalink) NULL, create the opt.output_document "path/file" */ if (!destname) - destname = xstrdup (filename); + destname = xstrdup (safename); /* Store the real file name for displaying in messages, and for proper RFC5854 "path/file" handling. */ @@ -603,7 +628,7 @@ gpg_skip_verification: if (retr_err != RETROK) { logprintf (LOG_VERBOSE, _("Failed to download %s. Skipping resource.\n"), - quote (destname ? destname : filename)); + quote (destname ? destname : safename)); } else if (!hash_ok) { @@ -649,6 +674,28 @@ gpg_skip_verification: return last_retr_err; } +/* + Strip the directory components from the given name. + + Return a pointer to the end of the leading directory components. + Return NULL if the resulting name is unsafe or invalid. + + Due to security issues posed by saving files with unsafe names, here + the use of libmetalink's metalink_check_safe_path() is enforced. If + this appears redundant because the given name was already verified, + just remember to never underestimate unsafe file names. +*/ +char * +get_metalink_basename (char *name) +{ + char *basename = name; + + while ((name = strstr (basename, "/"))) + basename = name + 1; + + return metalink_check_safe_path (basename) ? basename : NULL; +} + /* Append the suffix ".badhash" to the file NAME, except without overwriting an existing file with that name and suffix. */ void diff --git a/src/metalink.h b/src/metalink.h index 020fdf5..4846e84 100644 --- a/src/metalink.h +++ b/src/metalink.h @@ -47,6 +47,9 @@ uerr_t retrieve_from_metalink (const metalink_t *metalink); int metalink_res_cmp (const void *res1, const void *res2); +int metalink_check_safe_path(const char *path); + +char *get_metalink_basename (char *name); void badhash_suffix (char *name); void badhash_or_remove (char *name); -- 2.7.3
pgpPuUoWtOxll.pgp
Description: PGP signature
