[pacman-dev] [PATCH] Prevent stack overflow on symbolic link access.

[Allan McRae]

On 03/06/16 06:28, Tobias Stoeckmann wrote:
> It is possible (but unlikely) to encounter very long symbolic
> links, which would take a few mega bytes in size. In general,
> such long symbolic links are impossible to create because the
> kernel will only accept up to PATH_MAX in length. BUT lstat()
> on a maliciously modified symbolic link can return a much
> larger size than that.
> 
> In function check_file_link (src/pacman/check.c), the length
> of a symbolic link is used in a variable length array:
> 
> char link[length]; // length being st->st_size + 1
> 
> This doesn't just allow a very theoretical overflow on 32 bit
> systems with 64 bit off_t (which is st_size's type), it can also
> overflow available stack space, leading to a segmentation fault.
> 
> Symbolic links pointing to something that is longer than PATH_MAX
> can be considered illegal, so just get a fixed size array and
> reject symbolic links which are longer than that.

Interesting possibility!

I get this warning when building:

check.c: In function ‘check_file_link’:
check.c:133:16: error: unused parameter ‘st’ [-Werror=unused-parameter]
   struct stat *st, struct archive_entry *entry)
                ^~
cc1: all warnings being treated as errors

Should we still be checking "len != st->st_size"?



> ---
>  src/pacman/check.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/src/pacman/check.c b/src/pacman/check.c
> index d282cc2..6e8cda7 100644
> --- a/src/pacman/check.c
> +++ b/src/pacman/check.c
> @@ -132,15 +132,16 @@ static int check_file_time(const char *pkgname, const char *filepath,
>  static int check_file_link(const char *pkgname, const char *filepath,
>  		struct stat *st, struct archive_entry *entry)
>  {
> -	size_t length = st->st_size + 1;
> -	char link[length];
> +	ssize_t len;
> +	char link[PATH_MAX];
>  
> -	if(readlink(filepath, link, length) != st->st_size) {
> +	len = readlink(filepath, link, sizeof(link));
> +	if (len == -1 || len >= PATH_MAX) {
>  		/* this should not happen */
>  		pm_printf(ALPM_LOG_ERROR, _("unable to read symlink contents: %s\n"), filepath);
>  		return 1;
>  	}
> -	link[length - 1] = '\0';
> +	link[len] = '\0';
>  
>  	if(strcmp(link, archive_entry_symlink(entry)) != 0) {
>  		if(!config->quiet) {
>