Re: [Bug-wget] [PATCH 11/25] New: Metalink/XML and Metalink/HTTP file naming safety rules

[Giuseppe Scrivano]

Matthew White <address@hidden> writes:

> [Coverity Scan is ok, make syntax-check is ok, make check-valgrind is ok, 
> contrib/check-hard is ok]
>
> This introduces new rules/tests about Metalink/XML and Metalink/HTTP.
>
> The safety mechanism introduced provides secure and predictable file names. 
> This is convenient to prevent the overwriting of system/critical files and to 
> prevent to write files into unexpected/protected locations.
>
> The option --trust-server-names may be used to trust metalink:file names when 
> downloading files.
>
> Verbatim from doc/metalink-standard.txt:
> ----------------------------------------
> The final name of downloaded files is computed starting from a trusted
> name, which is then combined with the "Directory Options".  The result
> is verified and eventually made safer following security rules. If the
> final name isn't found safe enough, then the file isn't downloaded.
>
> Depending on the options used, a suffix could be appended to the final
> name to not overwrite existing files.
> ----------------------------------------
>
> Regards,
> Matthew
>
> -- 
> Matthew White <address@hidden>
>
> From 7fef56c4f815359047347eee4356b9f8971f1df5 Mon Sep 17 00:00:00 2001
> From: Matthew White <address@hidden>
> Date: Sun, 21 Aug 2016 18:45:09 +0200
> Subject: [PATCH 11/25] New: Metalink/XML and Metalink/HTTP file naming safety
>  rules
>
> * src/metalink.h: Add declaration of function append_suffix_number()
> * src/metalink.c: Add function append_suffix_number() append number to
>   string
> * src/metalink.c (retrieve_from_metalink): Safer Metalink/XML and
>   Metalink/HTTP download naming system, opt.trustservernames based
> * doc/metalink-standard.txt: Update doc. Explain new Metalink/XML and
>   Metalin/HTTP download naming system and --trust-server-names role
> * testenv/Makefile.am: Add new files
> * testenv/Test-metalink-xml.py: Update test. Metalink/XML naming tests
> * testenv/Test-metalink-xml-trust.py: New file. Metalink/XML naming
>   tests with --trust-server-names
> * testenv/Test-metalink-xml-abspath.py: Update test. Metalink/XML
>   absolute path tests
> * testenv/Test-metalink-xml-abspath-trust.py: New file. Metalink/XML
>   absolute path tests with --trust-server-names
> * testenv/Test-metalink-xml-relpath.py: Update test. Metalink/XML
>   relative path tests
> * testenv/Test-metalink-xml-relpath-trust.py: New file. Metalink/XML
>   relative path tests with --trust-server-names
> * testenv/Test-metalink-xml-homepath.py: Update test. Metalink/XML
>   home path and ~ (tilde) tests
> * testenv/Test-metalink-xml-homepath-trust.py: New file. Metalink/XML
>   home path and ~ (tilde) tests with --trust-server-names
> * testenv/Test-metalink-xml-prefix.py: New file. Metalink/XML naming
>   tests with --directory-prefix
> * testenv/Test-metalink-xml-prefix-trust.py: New file. Metalink/XML
>   naming tests with --directory-prefix and --trust-server-names
> * testenv/Test-metalink-xml-absprefix.py: New file. Metalink/XML
>   absolute --directory-prefix tests
> * testenv/Test-metalink-xml-absprefix-trust.py: New file. Metalink/XML
>   absolute --directory-prefix tests with --trust-server-names
> * testenv/Test-metalink-xml-relprefix.py: New file. Metalink/XML
>   relative --directory-prefix tests
> * testenv/Test-metalink-xml-relprefix-trust.py: New file. Metalink/XML
>   relative --directory-prefix tests with --trust-server-names
> * testenv/Test-metalink-xml-homeprefix.py: New file. Metalink/XML home
>   --directory-prefix tests
> * testenv/Test-metalink-xml-homeprefix-trust.py: New file. Metalink/XML
>   home --directory-prefix tests with --trust-server-names
>
> The option --trust-server-names allows to use the file names parsed
> from a Metalink/XML file.  Without --trust-server-names, the safety
> mechanism provides secure and predictable file names.
> ---
>  doc/metalink-standard.txt                     |  59 ++++++--
>  src/metalink.c                                | 107 +++++++++++---
>  src/metalink.h                                |   1 +
>  testenv/Makefile.am                           |  14 +-
>  testenv/Test-metalink-xml-abspath-trust.py    | 130 +++++++++++++++++
>  testenv/Test-metalink-xml-abspath.py          |  61 ++++++--
>  testenv/Test-metalink-xml-absprefix-trust.py  | 194 +++++++++++++++++++++++++
>  testenv/Test-metalink-xml-absprefix.py        | 194 +++++++++++++++++++++++++
>  testenv/Test-metalink-xml-homepath-trust.py   | 195 +++++++++++++++++++++++++
>  testenv/Test-metalink-xml-homepath.py         | 126 ++++++++++++++--
>  testenv/Test-metalink-xml-homeprefix-trust.py | 194 +++++++++++++++++++++++++
>  testenv/Test-metalink-xml-homeprefix.py       | 194 +++++++++++++++++++++++++
>  testenv/Test-metalink-xml-prefix-trust.py     | 194 +++++++++++++++++++++++++
>  testenv/Test-metalink-xml-prefix.py           | 194 +++++++++++++++++++++++++
>  testenv/Test-metalink-xml-relpath-trust.py    | 193 +++++++++++++++++++++++++
>  testenv/Test-metalink-xml-relpath.py          | 126 ++++++++++++----
>  testenv/Test-metalink-xml-relprefix-trust.py  | 194 +++++++++++++++++++++++++
>  testenv/Test-metalink-xml-relprefix.py        | 194 +++++++++++++++++++++++++
>  testenv/Test-metalink-xml-trust.py            | 197 
> ++++++++++++++++++++++++++
>  testenv/Test-metalink-xml.py                  | 126 ++++++++++++++--
>  20 files changed, 2803 insertions(+), 84 deletions(-)
>  create mode 100755 testenv/Test-metalink-xml-abspath-trust.py
>  create mode 100755 testenv/Test-metalink-xml-absprefix-trust.py
>  create mode 100755 testenv/Test-metalink-xml-absprefix.py
>  create mode 100755 testenv/Test-metalink-xml-homepath-trust.py
>  create mode 100755 testenv/Test-metalink-xml-homeprefix-trust.py
>  create mode 100755 testenv/Test-metalink-xml-homeprefix.py
>  create mode 100755 testenv/Test-metalink-xml-prefix-trust.py
>  create mode 100755 testenv/Test-metalink-xml-prefix.py
>  create mode 100755 testenv/Test-metalink-xml-relpath-trust.py
>  create mode 100755 testenv/Test-metalink-xml-relprefix-trust.py
>  create mode 100755 testenv/Test-metalink-xml-relprefix.py
>  create mode 100755 testenv/Test-metalink-xml-trust.py
>
> diff --git a/doc/metalink-standard.txt b/doc/metalink-standard.txt
> index d00c384..18acaaa 100644
> --- a/doc/metalink-standard.txt
> +++ b/doc/metalink-standard.txt
> @@ -29,18 +29,61 @@ paths or descend/escalate to a relative path unexpectedly.
>  2.1 Metalink/XML implemented tests
>  ==================================
>  
> -* testenv/Test-metalink-xml.py: Accept safe paths
> -* testenv/Test-metalink-xml-abspath.py: Reject absolute paths
> -* testenv/Test-metalink-xml-relpath.py: Reject relative paths
> -* testenv/Test-metalink-xml-homepath.py: Reject home paths
> +See testenv/Makefile.am (METALINK_TESTS).
> +
> +2.2 Metalink/HTTP implemented tests
> +===================================
> +
> +See testenv/Makefile.am (METALINK_TESTS).
>  
>  3. Download file name
>  *********************
>  
> -Computing the file name to wrote from the followed urls only leads to
> -uncertainty. Reason why an unique name shall be used. Respectively, it
> -shall be the metalink:file "name" field for Metalink/XML and a derived
> -cli's url for Metalink/HTTP.
> +The download file name shall be decided by precise rules which prevent
> +any naming uncertainty and security issues.
> +
> +3.1 Naming rules
> +================
> +
> +The final name of downloaded files is computed starting from a trusted
> +name, which is then combined with the "Directory Options".  The result
> +is verified and eventually made safer following security rules. If the
> +final name isn't found safe enough, then the file isn't downloaded.
> +
> +Depending on the options used, a suffix could be appended to the final
> +name to not overwrite existing files.
> +
> +3.1.1 The trusted name
> +======================
> +
> +The option --trust-server-names decides what is the trusted name.
> +
> +Any Metalink/XML element with an unsafe metalink:file "name" field is
> +ignored, see '1. Security features'.
> +
> +3.1.1.1 Without --trust-server-names
> +====================================
> +
> +When --trust-server-names is off, the basename of the --input-metalink
> +file, if available, or of the mother URL is trusted.
> +
> +The files described by a Metalink/XML file will be named sequentially
> +applying a suffix to the trusted name.
> +
> +3.1.1.2 With --trust-server-names
> +=================================
> +
> +When --trust-server-names is on, the metalink:file "name" field parsed
> +from Metalink/XML files is trusted. When no Metalink/XML is available,
> +the mother URL is trusted.
> +
> +3.1.2 The final name
> +====================
> +
> +The "Directory Options" are combined with the trusted name. The result
> +is evaluated again by the '1. Security features'. If the path is found
> +unsafe, only the basename of the final name is considered.  If this is
> +found unsafe too, the file is not downloaded.
>  
>  4. Metalink/XML
>  ***************
> diff --git a/src/metalink.c b/src/metalink.c
> index a8b047b..356a68b 100644
> --- a/src/metalink.c
> +++ b/src/metalink.c
> @@ -68,7 +68,16 @@ retrieve_from_metalink (const metalink_t* metalink)
>    bool _output_stream_regular = output_stream_regular;
>    char *_output_document = opt.output_document;
>  
> -  DEBUGP (("Retrieving from Metalink\n"));
> +  /* metalink file counter */
> +  unsigned mfc = 0;
> +
> +  /* metalink retrieval type */
> +  const char *metatpy = metalink->origin ? "Metalink/HTTP" : "Metalink/XML";
> +
> +  /* metalink mother source */
> +  char *metasrc = metalink->origin ? metalink->origin : opt.input_metalink;
> +
> +  DEBUGP (("Retrieving from Metalink %s\n", quote (metasrc)));
>  
>    /* No files to download.  */
>    if (!metalink->files)
> @@ -86,6 +95,8 @@ retrieve_from_metalink (const metalink_t* metalink)
>      {
>        metalink_file_t *mfile = *mfile_ptr;
>        metalink_resource_t **mres_ptr;
> +      char *planname = NULL;

could this be named plannedname?

> @@ -103,30 +114,76 @@ retrieve_from_metalink (const metalink_t* metalink)
>  
>        output_stream = NULL;
>  
> +      mfc++;
> +
>        /* The directory prefix for opt.metalink_over_http is handled by
>           src/url.c (url_file_name), do not add it a second time.  */
>        if (!metalink->origin && opt.dir_prefix && strlen (opt.dir_prefix))
> -        filename = aprintf("%s/%s", opt.dir_prefix, mfile->name);
> +        planname = aprintf("%s/%s", opt.dir_prefix, mfile->name);

missing space after the function name, though it will be probably fixed
with the rebase on the previous change.

Giuseppe