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@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@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