From ccf8de087476f87272de96fc37de99fc1f898c55 Mon Sep 17 00:00:00 2001 From: Qualys Security Advisory Date: Thu, 1 Jan 1970 00:00:00 +0000 Subject: [PATCH] proc/readproc.c: Harden file2str(). 1/ Replace sprintf() with snprintf() (and check for truncation). 2/ Prevent an integer overflow of ub->siz. The "tot_read--" is needed to avoid an off-by-one overflow in "ub->buf[tot_read] = '\0'". It is safe to decrement tot_read here, because we know that tot_read is equal to ub->siz (and ub->siz is very large). We believe that truncation is a better option than failure (implementing failure instead should be as easy as replacing the "tot_read--" with "tot_read = 0"). --- proc/readproc.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/proc/readproc.c b/proc/readproc.c index bda8d86b..a38850ac 100644 --- a/proc/readproc.c +++ b/proc/readproc.c @@ -658,7 +658,7 @@ static void statm2proc(const char* s, proc_t *restrict P) { static int file2str(const char *directory, const char *what, struct utlbuf_s *ub) { #define buffGRW 1024 char path[PROCPATHLEN]; - int fd, num, tot_read = 0; + int fd, num, tot_read = 0, len; /* on first use we preallocate a buffer of minimum size to emulate former 'local static' behavior -- even if this read fails, that @@ -666,11 +666,16 @@ static int file2str(const char *directory, const char *what, struct utlbuf_s *ub ( besides, with this xcalloc we will never need to use memcpy ) */ if (ub->buf) ub->buf[0] = '\0'; else ub->buf = xcalloc((ub->siz = buffGRW)); - sprintf(path, "%s/%s", directory, what); + len = snprintf(path, sizeof path, "%s/%s", directory, what); + if (len <= 0 || (size_t)len >= sizeof path) return -1; if (-1 == (fd = open(path, O_RDONLY, 0))) return -1; while (0 < (num = read(fd, ub->buf + tot_read, ub->siz - tot_read))) { tot_read += num; if (tot_read < ub->siz) break; + if (ub->siz >= INT_MAX - buffGRW) { + tot_read--; + break; + } ub->buf = xrealloc(ub->buf, (ub->siz += buffGRW)); }; ub->buf[tot_read] = '\0';