[pacman-dev] [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. --- 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) { -- 2.8.3
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) {
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@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) { - 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
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
On Mon, Jun 06, 2016 at 01:27:52AM -0400, Andrew Gregory wrote:
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.
That is true, in fact we would have to check if PATH_MAX is defined at all. If it's not there, we actually would have to call path_conf to figure it out. And even then it might be -1 to show that we really have no limits at all. But on the bright side, "currently" this is proven by reality to be no issue, after all nobody filed a bug report for his system, that pacman does not compile. ;) Same goes for PATH_MAX being larger than SSIZE_MAX. As long as we can trust the system that SSIZE_MAX really states the largest value for an ssize_t and that it has the same size like size_t, we would have run out of stack space looong before even reaching this function call. If I wouldn't have been able to trigger a segfault with an adjusted file system, I would have skipped sending this patch for being just a bad fairy tale. But a crashing pacman on a broken file system sounds worse than a system with such a PATH_MAX -- and that still wants to use pacman. :)
On 07/06/16 04:27, Tobias Stoeckmann wrote:
On Mon, Jun 06, 2016 at 01:27:52AM -0400, Andrew Gregory wrote:
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.
That is true, in fact we would have to check if PATH_MAX is defined at all. If it's not there, we actually would have to call path_conf to figure it out. And even then it might be -1 to show that we really have no limits at all.
But on the bright side, "currently" this is proven by reality to be no issue, after all nobody filed a bug report for his system, that pacman does not compile. ;)
Because we do check for PATH_MAX and then define it! configure.ac:PATH_MAX_DEFINED
On 06/06/16 at 08:27pm, Tobias Stoeckmann wrote:
On Mon, Jun 06, 2016 at 01:27:52AM -0400, Andrew Gregory wrote:
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.
That is true, in fact we would have to check if PATH_MAX is defined at all. If it's not there, we actually would have to call path_conf to figure it out. And even then it might be -1 to show that we really have no limits at all.
But on the bright side, "currently" this is proven by reality to be no issue, after all nobody filed a bug report for his system, that pacman does not compile. ;)
Same goes for PATH_MAX being larger than SSIZE_MAX. As long as we can trust the system that SSIZE_MAX really states the largest value for an ssize_t and that it has the same size like size_t, we would have run out of stack space looong before even reaching this function call.
I don't follow your logic. SSIZE_MAX is only guaranteed to be at least 32,767. PATH_MAX can easily be larger than that without coming close to exhausting my system's resources. apg
On June 9, 2016 at 7:31 PM Andrew Gregory <andrew.gregory.8@gmail.com> wrote: I don't follow your logic. SSIZE_MAX is only guaranteed to be at least 32,767. PATH_MAX can easily be larger than that without coming close to exhausting my system's resources.
apg
If SSIZE_MAX is just 32,767, then SIZE_MAX is 65,535. With such a small amount of addressable space, your resources are very quickly exhausted. Let alone that I don't remember a 16 bit memory operating system that is still supported.
On 06/09/16 at 10:11pm, Tobias Stöckmann wrote:
On June 9, 2016 at 7:31 PM Andrew Gregory <andrew.gregory.8@gmail.com> wrote: I don't follow your logic. SSIZE_MAX is only guaranteed to be at least 32,767. PATH_MAX can easily be larger than that without coming close to exhausting my system's resources.
apg
If SSIZE_MAX is just 32,767, then SIZE_MAX is 65,535. With such a small amount of addressable space, your resources are very quickly exhausted. Let alone that I don't remember a 16 bit memory operating system that is still supported.
SIZE_MAX is the maximum size of individual objects, not the entire addressable space.
On Thu, Jun 09, 2016 at 04:25:36PM -0400, Andrew Gregory wrote:
SIZE_MAX is the maximum size of individual objects, not the entire addressable space.
I hope you know that it becomes very very theoretical now, and that there is no such system out there... Let's assume that there is a system in which size_t is just 16 bit. And let's further assume that your pointers are still 32 or 64 bit, giving you an address space for many many size_t-sized objects. sizeof(size_t) = 2 sizeof(void *) = 8 Do you realize that it means that you have to check every single occurrence of "size_t len = strlen(x) + 1" for a realistically possible overflow? And that pacman doesn't do that just like no other sane software, e.g. OpenSSH, because there's no such system out there? You would even have to be very careful about pointer arithmetic, because it further means that sizeof(size_t) = 2 sizeof(ptrdiff_t) = 8 So even forget about "size_t len = endptr - baseptr". Granted, that kind of code is "buggy" for that reason, but if we go that far, all our software is screwed today. And if sizeof(int) eventually turns 8, we have a real issue to think about. If this theoretical thing is of interest, we can further discuss this. But I would prefer to get back into reality. Maybe we are, but in that case we should boot up a system which has these weird settings. And for that, I need a name. It's clearly not OS X, *BSD, or Linux. As pacman is a software that targets Linux (as far as I know, it even has *BSD support in a few parts), which has a PATH_MAX = 4096 limitation, this is of no concern for pacman. On the other hand, broken filesystems are out there. I've created an image to verify this. In case you need to see the segmentation fault on your own, I can prepare a fitting package for it and give you instructions on how to trigger it. Tobias
On 06/09/16 at 10:49pm, Tobias Stoeckmann wrote:
On Thu, Jun 09, 2016 at 04:25:36PM -0400, Andrew Gregory wrote:
SIZE_MAX is the maximum size of individual objects, not the entire addressable space.
I hope you know that it becomes very very theoretical now, and that there is no such system out there...
Let's assume that there is a system in which size_t is just 16 bit. And let's further assume that your pointers are still 32 or 64 bit, giving you an address space for many many size_t-sized objects.
sizeof(size_t) = 2 sizeof(void *) = 8
Do you realize that it means that you have to check every single occurrence of "size_t len = strlen(x) + 1" for a realistically possible overflow?
strlen doesn't count the terminating NUL, so strlen(x) + 1 is at most the size of the array, which by definition has to fit into a size_t.
And that pacman doesn't do that just like no other sane software, e.g. OpenSSH, because there's no such system out there?
You would even have to be very careful about pointer arithmetic, because it further means that
sizeof(size_t) = 2 sizeof(ptrdiff_t) = 8
So even forget about "size_t len = endptr - baseptr". Granted, that kind of code is "buggy" for that reason, but if we go that far, all our software is screwed today. And if sizeof(int) eventually turns 8, we have a real issue to think about.
Yes, using size_t for pointer arithmetic with arbitrary pointers is incorrect unless both pointers point to locations within the same object. I'm not sure what the existence of poorly written code elsewhere has to do with pacman.
If this theoretical thing is of interest, we can further discuss this. But I would prefer to get back into reality. Maybe we are, but in that case we should boot up a system which has these weird settings. And for that, I need a name. It's clearly not OS X, *BSD, or Linux. As pacman is a software that targets Linux (as far as I know, it even has *BSD support in a few parts), which has a PATH_MAX = 4096 limitation, this is of no concern for pacman.
On the other hand, broken filesystems are out there. I've created an image to verify this. In case you need to see the segmentation fault on your own, I can prepare a fitting package for it and give you instructions on how to trigger it.
I am not rejecting your patch; it is probably better than what we have now. I do not like technically incorrect code that relies on conventions that are not guaranteed to be reliable though.
On June 10, 2016 at 2:38 AM Andrew Gregory <andrew.gregory.8@gmail.com> wrote: strlen doesn't count the terminating NUL, so strlen(x) + 1 is at most the size of the array, which by definition has to fit into a size_t.
Then take the typical "len = strlen(a) + strlen(b) + 1" followed by malloc and snprintf. And check your typical strlen implementation which would have to be a strnlen with SIZE_MAX then. These implementations are not around. For good reasons.
On 06/10/16 at 07:32am, Tobias Stöckmann wrote:
On June 10, 2016 at 2:38 AM Andrew Gregory <andrew.gregory.8@gmail.com> wrote: strlen doesn't count the terminating NUL, so strlen(x) + 1 is at most the size of the array, which by definition has to fit into a size_t.
Then take the typical "len = strlen(a) + strlen(b) + 1" followed by malloc and snprintf.
Again, yes, that code is technically incorrect unless the programmer knows it won't overflow. The fact that code like that does the right thing 99% of the time is no excuse for it doing the wrong thing 1% of the time.
And check your typical strlen implementation which would have to be a strnlen with SIZE_MAX then.
These implementations are not around. For good reasons.
Again, strlen operates on character arrays, the length of which must fit into a size_t by definition.
The width of wchar_t is allowed to be of the same width as long, according to standards. The return type of mbscasecmp is int though. On amd64 with a 32 bit int, this means that mbscasecmp can return zero (indicating that strings are equal) even though the input strings differ. No known system exists that could trigger this. Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org> --- There is a reason that I picked this one, annotation will help. --- src/pacman/util.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pacman/util.c b/src/pacman/util.c index 81780f7..e38ef06 100644 --- a/src/pacman/util.c +++ b/src/pacman/util.c @@ -1503,7 +1503,7 @@ int select_question(int count) return (preset - 1); } -static int mbscasecmp(const char *s1, const char *s2) +static wchar_t mbscasecmp(const char *s1, const char *s2) { size_t len1 = strlen(s1), len2 = strlen(s2); wchar_t c1, c2; -- 2.8.4
On 06/10/16 at 06:40pm, Tobias Stoeckmann wrote:
The width of wchar_t is allowed to be of the same width as long, according to standards. The return type of mbscasecmp is int though.
On amd64 with a 32 bit int, this means that mbscasecmp can return zero (indicating that strings are equal) even though the input strings differ.
No known system exists that could trigger this.
Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org> --- There is a reason that I picked this one, annotation will help. --- src/pacman/util.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/pacman/util.c b/src/pacman/util.c index 81780f7..e38ef06 100644 --- a/src/pacman/util.c +++ b/src/pacman/util.c @@ -1503,7 +1503,7 @@ int select_question(int count) return (preset - 1); }
-static int mbscasecmp(const char *s1, const char *s2) +static wchar_t mbscasecmp(const char *s1, const char *s2)
Good catch, but the solution needs work. wchar_t can be unsigned. While that's fine for how this function is currently used (only testing for equality), it no longer works as a three-way comparison function as its name suggests. The return type should stay 'int' but the return value should either be normalized to an int or changed to a simple equality test with an appropriate change to the function name. And please make the first line summary of your commit message more descriptive of what the patch actually does. "X fails sometimes" is not particularly helpful when looking through the log.
{ size_t len1 = strlen(s1), len2 = strlen(s2); wchar_t c1, c2; -- 2.8.4
On Fri, Jun 10, 2016 at 07:10:56PM -0400, Andrew Gregory wrote:
Good catch, but the solution needs work.
You were able to grasp the idea of this non-issue. Now you can adjust your code the way you want.
On 11/06/16 17:26, Tobias Stoeckmann wrote:
On Fri, Jun 10, 2016 at 07:10:56PM -0400, Andrew Gregory wrote:
Good catch, but the solution needs work.
You were able to grasp the idea of this non-issue.
Now you can adjust your code the way you want.
Good old passive aggression... Definitely the way for things to move forward!
The width of wchar_t is allowed to be of the same width as long, according to standards. The return type of mbscasecmp is int though. On amd64 with a 32 bit int, this means that mbscasecmp can return zero (indicating that strings are equal) even though the input strings differ. Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org> --- src/pacman/util.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/pacman/util.c b/src/pacman/util.c index 81780f7..b979083 100644 --- a/src/pacman/util.c +++ b/src/pacman/util.c @@ -1503,6 +1503,8 @@ int select_question(int count) return (preset - 1); } +#define CMP(x, y) ((x) < (y) ? -1 : ((x) > (y) ? 1 : 0)) + static int mbscasecmp(const char *s1, const char *s2) { size_t len1 = strlen(s1), len2 = strlen(s2); @@ -1520,19 +1522,19 @@ static int mbscasecmp(const char *s1, const char *s2) return strcasecmp(p1, p2); } if(b1 == 0 || b2 == 0) { - return c1 - c2; + return CMP(c1, c2); } c1 = towlower(c1); c2 = towlower(c2); if(c1 != c2) { - return c1 - c2; + return CMP(c1, c2); } p1 += b1; p2 += b2; len1 -= b1; len2 -= b2; } - return *p1 - *p2; + return CMP(*p1, *p2); } /* presents a prompt and gets a Y/N answer */ -- 2.9.0
On 19/06/16 02:58, Tobias Stoeckmann wrote:
The width of wchar_t is allowed to be of the same width as long, according to standards. The return type of mbscasecmp is int though.
On amd64 with a 32 bit int, this means that mbscasecmp can return zero (indicating that strings are equal) even though the input strings differ.
Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>
Thanks. Allan
On 06/06/16 at 08:27pm, Tobias Stoeckmann wrote:
On Mon, Jun 06, 2016 at 01:27:52AM -0400, Andrew Gregory wrote:
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.
That is true, in fact we would have to check if PATH_MAX is defined at all. If it's not there, we actually would have to call path_conf to figure it out. And even then it might be -1 to show that we really have no limits at all.
But on the bright side, "currently" this is proven by reality to be no issue, after all nobody filed a bug report for his system, that pacman does not compile. ;)
Same goes for PATH_MAX being larger than SSIZE_MAX. As long as we can trust the system that SSIZE_MAX really states the largest value for an ssize_t and that it has the same size like size_t, we would have run out of stack space looong before even reaching this function call.
If I wouldn't have been able to trigger a segfault with an adjusted file system, I would have skipped sending this patch for being just a bad fairy tale. But a crashing pacman on a broken file system sounds worse than a system with such a PATH_MAX -- and that still wants to use pacman. :)
What about simply using a predetermined buffer length? PATH_MAX isn't really the right size to use for this anyway, which is why POSIX defines SYMLINK_MAX. Since alpm's maximum path length is 1024, using a buffer size of 2048 or 4096 leaves plenty of room for symlinks to point to any alpm-managed file (assuming the link doesn't do anything bizarre like repeat "./" thousands of times). There could be valid symlinks that point to non-managed files that we won't be able to read, but that appears to be the case no matter what we do. apg
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 | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/pacman/check.c b/src/pacman/check.c index d282cc2..f3dfddc 100644 --- a/src/pacman/check.c +++ b/src/pacman/check.c @@ -130,17 +130,18 @@ 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) + 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 >= sizeof(link)) { /* 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 +349,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
On 07/06/16 04:29, 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.
According to gnulib, buggy file systems can report garbage in st_size, so there is no benefit in checking length against it.
check.c: In function ‘check_file_link’: check.c:139:28: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare] if(length == -1 || length >= sizeof(link)) { ^~ Please test with using "--enable-warningflags" when configuring. A
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 | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/pacman/check.c b/src/pacman/check.c index d282cc2..db6b423 100644 --- a/src/pacman/check.c +++ b/src/pacman/check.c @@ -130,17 +130,18 @@ 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) + 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, PATH_MAX); + 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 +349,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
participants (4)
-
Allan McRae
-
Andrew Gregory
-
Tobias Stoeckmann
-
Tobias Stöckmann