[pacman-dev] [PATCH] pacman/callback: fix buffer over-read
Commit 11ab9aa9f5f0f3873df89c73e8715b82f485bd9b replaced a strcpy() call with memcpy(), without copying the terminating null character. Since fname is allocated with malloc(), subsequent strstr() calls will overrun the buffer's boundary. Signed-off-by: László Várady <laszlo.varady93@gmail.com> --- src/pacman/callback.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pacman/callback.c b/src/pacman/callback.c index 22865614..a4c637ce 100644 --- a/src/pacman/callback.c +++ b/src/pacman/callback.c @@ -765,7 +765,7 @@ void cb_dl_progress(const char *filename, off_t file_xfered, off_t file_total) len = strlen(filename); fname = malloc(len + 1); - memcpy(fname, filename, len); + memcpy(fname, filename, len + 1); /* strip package or DB extension for cleaner look */ if((p = strstr(fname, ".pkg")) || (p = strstr(fname, ".db")) || (p = strstr(fname, ".files"))) { /* tack on a .sig suffix for signatures */ -- 2.22.0
On 08/03/19 at 01:27am, László Várady wrote:
Commit 11ab9aa9f5f0f3873df89c73e8715b82f485bd9b replaced a strcpy() call with memcpy(), without copying the terminating null character.
Since fname is allocated with malloc(), subsequent strstr() calls will overrun the buffer's boundary.
Signed-off-by: László Várady <laszlo.varady93@gmail.com>
ACK.
On Sat, Aug 03, 2019 at 01:27:35AM +0200, László Várady wrote:
Commit 11ab9aa9f5f0f3873df89c73e8715b82f485bd9b replaced a strcpy() call with memcpy(), without copying the terminating null character.
Since fname is allocated with malloc(), subsequent strstr() calls will overrun the buffer's boundary.
Signed-off-by: László Várady <laszlo.varady93@gmail.com> --- src/pacman/callback.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/pacman/callback.c b/src/pacman/callback.c index 22865614..a4c637ce 100644 --- a/src/pacman/callback.c +++ b/src/pacman/callback.c @@ -765,7 +765,7 @@ void cb_dl_progress(const char *filename, off_t file_xfered, off_t file_total)
len = strlen(filename); fname = malloc(len + 1); - memcpy(fname, filename, len); + memcpy(fname, filename, len + 1);
Ok, but maybe we should remove the now redundant null termination after the if block.
/* strip package or DB extension for cleaner look */ if((p = strstr(fname, ".pkg")) || (p = strstr(fname, ".db")) || (p = strstr(fname, ".files"))) { /* tack on a .sig suffix for signatures */ -- 2.22.0
Commit 11ab9aa9f5f0f3873df89c73e8715b82f485bd9b replaced a strcpy() call with memcpy(), without copying the terminating null character. Since fname is allocated with malloc(), subsequent strstr() calls will overrun the buffer's boundary. Signed-off-by: László Várady <laszlo.varady93@gmail.com> --- src/pacman/callback.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/pacman/callback.c b/src/pacman/callback.c index 22865614..1132034d 100644 --- a/src/pacman/callback.c +++ b/src/pacman/callback.c @@ -765,7 +765,7 @@ void cb_dl_progress(const char *filename, off_t file_xfered, off_t file_total) len = strlen(filename); fname = malloc(len + 1); - memcpy(fname, filename, len); + memcpy(fname, filename, len + 1); /* strip package or DB extension for cleaner look */ if((p = strstr(fname, ".pkg")) || (p = strstr(fname, ".db")) || (p = strstr(fname, ".files"))) { /* tack on a .sig suffix for signatures */ @@ -777,8 +777,8 @@ void cb_dl_progress(const char *filename, off_t file_xfered, off_t file_total) } else { len = p - fname; } + fname[len] = '\0'; } - fname[len] = '\0'; /* 1 space + filenamelen + 1 space + 6 for size + 1 space + 3 for label + * + 2 spaces + 4 for rate + 1 space + 3 for label + 2 for /s + 1 space + -- 2.22.0
Hi, Thanks for the review!
Ok, but maybe we should remove the now redundant null termination after the if block.
I believe the '\0' character after the if block is not completely redundant, it terminates the stripped package name which can be shorter than the original string. `len` is modified according to this operation. I'm moving the mentioned line inside the if statement. -- László Várady
participants (3)
-
Andrew Gregory
-
Dave Reisner
-
László Várady