[pacman-dev] [PATCH] validate %FILEPATH% when parsing repo dbs

[Allan McRae]

On 09/05/13 12:05, Simon Gomizelj wrote:
> Currently we make no effort to validate the %FILENAME% field in the
> repo db. This allows for relative paths to be considered valid.
> 
> A carefully crafted db entry with a malicious relative path,
> (e.g. `../../../../etc/passwd`) will cause pacman to to
> overwrite _any_ file on the target's machine.
> 
> Signed-off-by: Simon Gomizelj <simongmzlj at gmail.com>
> ---
>  lib/libalpm/be_sync.c | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/lib/libalpm/be_sync.c b/lib/libalpm/be_sync.c
> index 1cbe055..059c18a 100644
> --- a/lib/libalpm/be_sync.c
> +++ b/lib/libalpm/be_sync.c
> @@ -479,6 +479,34 @@ cleanup:
>  	return count;
>  }
>  
> +/* This function validates %FILENAME%. filename must be between 3 and
> + * PATH_MAX characters and cannot be contain a path */

I still do not understand the lower bound of 3.  Surely any file name is
fine - pacman/libarchive does not use the file name to determine
compression.

The file name also needs to be less than:
  PATH_MAX - strlen(dbpath) - strlen(rootpath)
or whatever those variables are called...  the root might already be in
dbpath.

> +static int _alpm_validate_filename(alpm_db_t *db, const char *pkgname,
> +		const char *filename)
> +{
> +	size_t len = strlen(filename);
> +
> +	if(len < 3) {
> +		errno = EINVAL;
> +		_alpm_log(db->handle, ALPM_LOG_ERROR, _("%s database is inconsistent: filename "
> +					"of package %s is too short\n"), db->treename, pkgname);
> +		return -1;
> +	} else if(len > PATH_MAX) {
> +		errno = EINVAL;
> +		db->handle->
> +		_alpm_log(db->handle, ALPM_LOG_ERROR, _("%s database is inconsistent: filename "
> +					"of package %s is too long\n"), db->treename, pkgname);
> +		return -1;
> +	} else if(memchr(filename, '/', len) != NULL) {
> +		errno = EINVAL;
> +		_alpm_log(db->handle, ALPM_LOG_ERROR, _("%s database is inconsistent: filename "
> +					"of package %s is illegal\n"), db->treename, pkgname);

Why "database is inconsistent"?   That does not really correspond to the
error we are checking.

> +		return -1;
> +	}
> +
> +	return 0;
> +}
> +
>  #define READ_NEXT() do { \
>  	if(_alpm_archive_fgets(archive, &buf) != ARCHIVE_OK) goto error; \
>  	line = buf.line; \
> @@ -558,6 +586,9 @@ static int sync_db_read(alpm_db_t *db, struct archive *archive,
>  				}
>  			} else if(strcmp(line, "%FILENAME%") == 0) {
>  				READ_AND_STORE(pkg->filename);
> +				if(_alpm_validate_filename(db, pkg->name, pkg->filename) < 0) {
> +					return -1;
> +				}
>  			} else if(strcmp(line, "%DESC%") == 0) {
>  				READ_AND_STORE(pkg->desc);
>  			} else if(strcmp(line, "%GROUPS%") == 0) {
>