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

[Tobias Stoeckmann]

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 ()


>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)
 {
-	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) {
 		/* 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