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

[Andrew Gregory]

On 06/04/16 at 12:58pm, Tobias Stoeckmann wrote:
> On Sat, Jun 04, 2016 at 05:29:55PM +1000, Allan McRae wrote:
> > 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"?
> 
> I'll quote from gnulib's areadlink-with-size.c file:
> 
>   /* Some buggy file systems report garbage in st_size.  Defend
>      against them by ignoring outlandish st_size values in the initial
>      memory allocation.  */
> 
> So from my point of view, there is no additional gain doing that.
> As long as it's below PATH_MAX, we have to consider it as valid.
> 
> I've adjusted the patch:
> 
> - skip the st argument
> - rename len into length again to stay with style
> - also properly format if() instead of if ()

Please do not append patches to a response like this.  `git am` cannot
apply them without manual modification.  If your response is too long
to include in the patch itself, just send two separate emails.

> From 1c075c6b70c21c22636fa2ab2e9ef17bfcb4aaa5 Mon Sep 17 00:00:00 2001
> From: Tobias Stoeckmann <tobias at stoeckmann.org>
> Date: Sat, 4 Jun 2016 12:44:43 +0200
> Subject: [PATCH] Prevent stack overflow on symbolic link access.
> 
> 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.
> 
> According to gnulib, buggy file systems can report garbage in
> st_size, so there is no benefit in checking length against it.
> 
> Signed-off-by: Tobias Stoeckmann <tobias at stoeckmann.org>
> ---
>  src/pacman/check.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/src/pacman/check.c b/src/pacman/check.c
> index d282cc2..5b7554b 100644
> --- a/src/pacman/check.c
> +++ b/src/pacman/check.c
> @@ -129,18 +129,18 @@ static int check_file_time(const char *pkgname, const char *filepath,
>  	return 0;
>  }
>  
> -static int check_file_link(const char *pkgname, const char *filepath,
> -		struct stat *st, struct archive_entry *entry)
> +static int check_file_link(const char *pkgname, const char *filepath, struct archive_entry *entry)

Please leave this wrapped to <= 80 characters.

>  {
> -	size_t length = st->st_size + 1;
> -	char link[length];
> +	ssize_t length;
> +	char link[PATH_MAX];
>  
> -	if(readlink(filepath, link, length) != st->st_size) {
> +	length = readlink(filepath, link, sizeof(link));
> +	if(length == -1 || length >= PATH_MAX) {

Please stick to either sizeof(link) or PATH_MAX.

Since we're already talking about unlikely scenarios...  My reading of
readlink(2) and readlink(3p) suggest this might still run into
problems on oddly configured systems.  POSIX leaves up to the
implementation what happens if bufsize > SSIZE_MAX and nothing
guarantees that PATH_MAX is less than SSIZE_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[length] = '\0';
>  
>  	if(strcmp(link, archive_entry_symlink(entry)) != 0) {
>  		if(!config->quiet) {
> @@ -348,7 +348,7 @@ int check_pkg_full(alpm_pkg_t *pkg)
>  		file_errors += check_file_permissions(pkgname, filepath, &st, entry);
>  
>  		if(type == AE_IFLNK) {
> -			file_errors += check_file_link(pkgname, filepath, &st, entry);
> +			file_errors += check_file_link(pkgname, filepath, entry);
>  		}
>  
>  		/* the following checks are expected to fail if a backup file has been
> -- 
> 2.8.3