[pacman-dev] [PATCH] Fix out of boundary reads in pacsort.
This patch fixes two out of bound reads in pacsort (as they are of the same error type, I didn't see a reason to split it into two commits). The first one is triggered by a very large "key column" and the delimiter '\0'. The return value of strchr() is not checked for the special case in which it reaches the end of the version string. I recommend to simply not allow the '\0' separator, because file names cannot contain it. And therefore, it cannot be a sane column separator in file names for versions either. You can trigger it by running this command: $ echo -e "-.pkg.tar.xz\n-.pkg.tar.xz" | pacsort -k 999999 -t '\0' Segmentation fault The second one is triggered due to a wrong assumption in fnmatch pattern. It is assumed that "*-*.pkg.tar.?z" will only match file names which contain a dash. But in fact, it also accepts files without one, if at least one of the directories contains it. One example would be "/my-path/file.pkg.tar.xz". While parsing the version string, the return value of strrchr() looking for the dash is not checked anymore. In this case, it would return NULL and further pointer arithmetic on it turns it into SIZE_MAX. Eventually, pacman segfaults while traversing this memory area for needed chars: $ echo "-/.pkg.tar.xz" | pacsort Segmentation fault Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org> --- src/util/pacsort.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/util/pacsort.c b/src/util/pacsort.c index 662b250..65f67bd 100644 --- a/src/util/pacsort.c +++ b/src/util/pacsort.c @@ -228,6 +228,9 @@ static struct input_t *input_new(const char *path, int pathlen) } pkgver_end = strrchr(in->pkgname, '-'); + if(pkgver_end == NULL || pkgver_end == in->pkgname) { + return in; + } /* read backwards through pkgrel */ for(in->pkgver = pkgver_end - 1; @@ -410,8 +413,6 @@ static char escape_char(const char *string) return '\n'; case 'v': return '\v'; - case '0': - return '\0'; default: return INVALD_ESCAPE_CHAR; } -- 2.8.3
On 06/09/16 at 11:16pm, Tobias Stoeckmann wrote:
This patch fixes two out of bound reads in pacsort (as they are of the same error type, I didn't see a reason to split it into two commits).
The first one is triggered by a very large "key column" and the delimiter '\0'. The return value of strchr() is not checked for the special case in which it reaches the end of the version string. I recommend to simply not allow the '\0' separator, because file names cannot contain it. And therefore, it cannot be a sane column separator in file names for versions either.
-t and -k are used to sort tabular data, where NUL is a perfectly sane field separator. In fact, with --machinereadable pacman will output a table of NUL separated fields. Though, pacsort does need its handling of -t '\0' fixed in other places as well.
You can trigger it by running this command:
$ echo -e "-.pkg.tar.xz\n-.pkg.tar.xz" | pacsort -k 999999 -t '\0' Segmentation fault
The second one is triggered due to a wrong assumption in fnmatch pattern. It is assumed that "*-*.pkg.tar.?z" will only match file names which contain a dash. But in fact, it also accepts files without one, if at least one of the directories contains it. One example would be "/my-path/file.pkg.tar.xz".
While parsing the version string, the return value of strrchr() looking for the dash is not checked anymore. In this case, it would return NULL and further pointer arithmetic on it turns it into SIZE_MAX. Eventually, pacman segfaults while traversing this memory area for needed chars:
$ echo "-/.pkg.tar.xz" | pacsort Segmentation fault
Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org> --- src/util/pacsort.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/util/pacsort.c b/src/util/pacsort.c index 662b250..65f67bd 100644 --- a/src/util/pacsort.c +++ b/src/util/pacsort.c @@ -228,6 +228,9 @@ static struct input_t *input_new(const char *path, int pathlen) }
pkgver_end = strrchr(in->pkgname, '-'); + if(pkgver_end == NULL || pkgver_end == in->pkgname) { + return in; + }
/* read backwards through pkgrel */ for(in->pkgver = pkgver_end - 1; @@ -410,8 +413,6 @@ static char escape_char(const char *string) return '\n'; case 'v': return '\v'; - case '0': - return '\0'; default: return INVALD_ESCAPE_CHAR; } -- 2.8.3
On Thu, Jun 09, 2016 at 07:46:00PM -0400, Andrew Gregory wrote:
-t and -k are used to sort tabular data, where NUL is a perfectly sane field separator. In fact, with --machinereadable pacman will output a table of NUL separated fields. Though, pacsort does need its handling of -t '\0' fixed in other places as well.
The function escape_char is only called from main for the 't' option argument. The result is stored in opts.delim. The value of opts.delim is only used in function nth_column. That function is called in compare_versions, which is called by vercmp either directly or through compare_files. And that one is called by main again. Which "other places" do you mean that have to be fixed? There is only one. The parsing of input happens in input_new, which allocates space for in->data by using strndup(). Whatever points into this new data area, accessing past the first '\0' is a straight out of boundary access. No matter the origin of pacsort's input, first '\0' definitely means 'end here'. Maybe you mixed up the stand alone utility "pacsort" with some function in libalpm? Otherwise please let me know what exactly is broken now by showing the code path or actual program invocation.
On 06/11/16 at 12:14am, Tobias Stoeckmann wrote:
On Thu, Jun 09, 2016 at 07:46:00PM -0400, Andrew Gregory wrote:
-t and -k are used to sort tabular data, where NUL is a perfectly sane field separator. In fact, with --machinereadable pacman will output a table of NUL separated fields. Though, pacsort does need its handling of -t '\0' fixed in other places as well.
The function escape_char is only called from main for the 't' option argument. The result is stored in opts.delim.
The value of opts.delim is only used in function nth_column. That function is called in compare_versions, which is called by vercmp either directly or through compare_files.
And that one is called by main again. Which "other places" do you mean that have to be fixed? There is only one.
The parsing of input happens in input_new, which allocates space for in->data by using strndup(). Whatever points into this new data area, accessing past the first '\0' is a straight out of boundary access. No matter the origin of pacsort's input, first '\0' definitely means 'end here'.
Maybe you mixed up the stand alone utility "pacsort" with some function in libalpm?
Otherwise please let me know what exactly is broken now by showing the code path or actual program invocation.
I was saying that your rationale for just dropping support for -t '\0' was incorrect. NUL not being legal in a path name is irrelevant. But, as you just pointed out, pacsort has more issues with -t '\0' than just the poor use of strchr referenced by your patch. Hence, "pacsort does need its handling of -t '\0' fixed in other places as well." In other words, if -t '\0' is kept, pacsort needs to be fixed in a few places. apg
On Fri, Jun 10, 2016 at 07:11:16PM -0400, Andrew Gregory wrote:
I was saying that your rationale for just dropping support for -t '\0' was incorrect. NUL not being legal in a path name is irrelevant.
Yup, it's irrelevant. The relevant part is using strndup() with an input line. Maybe you want to adjust that one?
On 11/06/16 17:27, Tobias Stoeckmann wrote:
On Fri, Jun 10, 2016 at 07:11:16PM -0400, Andrew Gregory wrote:
I was saying that your rationale for just dropping support for -t '\0' was incorrect. NUL not being legal in a path name is irrelevant.
Yup, it's irrelevant. The relevant part is using strndup() with an input line. Maybe you want to adjust that one?
Fixing that strndup is preferable. I want to keep the ability to handle \0 delimited fields given that is what pacman --machinereadable does (despite that option not being ubiquitous at the moment...) Allan
On Mon, Jun 13, 2016 at 04:01:17PM +1000, Allan McRae wrote:
Fixing that strndup is preferable. I want to keep the ability to handle \0 delimited fields given that is what pacman --machinereadable does (despite that option not being ubiquitous at the moment...)
If \0 is a valid field _and_ line delimiter, the parsed input becomes ambiguous. How is that supposed to be handled? Tobias
On Sat, Jun 18, 2016 at 06:44:01PM +0200, Tobias Stoeckmann wrote:
On Mon, Jun 13, 2016 at 04:01:17PM +1000, Allan McRae wrote:
Fixing that strndup is preferable. I want to keep the ability to handle \0 delimited fields given that is what pacman --machinereadable does (despite that option not being ubiquitous at the moment...)
If \0 is a valid field _and_ line delimiter, the parsed input becomes ambiguous. How is that supposed to be handled?
Tobias
\n is a valid field and line delimiter, too. We should treat both of these cases the same -- catch them and reject them as invalid usage. d
2016-06-09 23:16 GMT+02:00 Tobias Stoeckmann <tobias@stoeckmann.org>:
... You can trigger it by running this command:
$ echo -e "-.pkg.tar.xz\n-.pkg.tar.xz" | pacsort -k 999999 -t '\0' Segmentation fault
...
$ echo "-/.pkg.tar.xz" | pacsort Segmentation fault
Could these two cases be added to pacman/test/util/pacsorttest.sh? Rikard
participants (5)
-
Allan McRae
-
Andrew Gregory
-
Dave Reisner
-
Rikard Falkeborn
-
Tobias Stoeckmann