From 9d2ec74b76d220f5343e548fcb7058d723293a22 Mon Sep 17 00:00:00 2001
From: Qualys Security Advisory <qsa@qualys.com>
Date: Thu, 1 Jan 1970 00:00:00 +0000
Subject: [PATCH 1/3] proc/alloc.*: Use size_t, not unsigned int.
Otherwise this can truncate sizes on 64-bit platforms, and is one of the
reasons the integer overflows in file2strvec() are exploitable at all.
Also: catch potential integer overflow in xstrdup() (should never
happen, but better safe than sorry), and use memcpy() instead of
strcpy() (faster).
Warnings:
- in glibc, realloc(ptr, 0) is equivalent to free(ptr), but not here,
because of the ++size;
- here, xstrdup() can return NULL (if str is NULL), which goes against
the idea of the xalloc wrappers.
We were tempted to call exit() or xerrx() in those cases, but decided
against it, because it might break things in unexpected places; TODO?
---
proc/alloc.c | 20 ++++++++++++--------
proc/alloc.h | 4 ++--
2 files changed, 14 insertions(+), 10 deletions(-)
diff --git a/proc/alloc.c b/proc/alloc.c
index 94af47f..1768d73 100644
--- a/proc/alloc.c
+++ b/proc/alloc.c
@@ -37,14 +37,14 @@ static void xdefault_error(const char *restrict fmts, ...) {
message_fn xalloc_err_handler = xdefault_error;
-void *xcalloc(unsigned int size) {
+void *xcalloc(size_t size) {
void * p;
if (size == 0)
++size;
p = calloc(1, size);
if (!p) {
- xalloc_err_handler("%s failed to allocate %u bytes of memory", __func__, size);
+ xalloc_err_handler("%s failed to allocate %zu bytes of memory", __func__, size);
exit(EXIT_FAILURE);
}
return p;
@@ -57,20 +57,20 @@ void *xmalloc(size_t size) {
++size;
p = malloc(size);
if (!p) {
- xalloc_err_handler("%s failed to allocate %zu bytes of memory", __func__, size);
+ xalloc_err_handler("%s failed to allocate %zu bytes of memory", __func__, size);
exit(EXIT_FAILURE);
}
return(p);
}
-void *xrealloc(void *oldp, unsigned int size) {
+void *xrealloc(void *oldp, size_t size) {
void *p;
if (size == 0)
++size;
p = realloc(oldp, size);
if (!p) {
- xalloc_err_handler("%s failed to allocate %u bytes of memory", __func__, size);
+ xalloc_err_handler("%s failed to allocate %zu bytes of memory", __func__, size);
exit(EXIT_FAILURE);
}
return(p);
@@ -80,13 +80,17 @@ char *xstrdup(const char *str) {
char *p = NULL;
if (str) {
- unsigned int size = strlen(str) + 1;
+ size_t size = strlen(str) + 1;
+ if (size < 1) {
+ xalloc_err_handler("%s refused to allocate %zu bytes of memory", __func__, size);
+ exit(EXIT_FAILURE);
+ }
p = malloc(size);
if (!p) {
- xalloc_err_handler("%s failed to allocate %u bytes of memory", __func__, size);
+ xalloc_err_handler("%s failed to allocate %zu bytes of memory", __func__, size);
exit(EXIT_FAILURE);
}
- strcpy(p, str);
+ memcpy(p, str, size);
}
return(p);
}
diff --git a/proc/alloc.h b/proc/alloc.h
index 19c91d7..6787a72 100644
--- a/proc/alloc.h
+++ b/proc/alloc.h
@@ -8,9 +8,9 @@ EXTERN_C_BEGIN
/* change xalloc_err_handler to override the default fprintf(stderr... */
extern message_fn xalloc_err_handler;
-extern void *xcalloc(unsigned int size) MALLOC;
+extern void *xcalloc(size_t size) MALLOC;
extern void *xmalloc(size_t size) MALLOC;
-extern void *xrealloc(void *oldp, unsigned int size) MALLOC;
+extern void *xrealloc(void *oldp, size_t size) MALLOC;
extern char *xstrdup(const char *str) MALLOC;
EXTERN_C_END
--
2.14.3
From de660b14b80188d9b323c4999d1b91a9456ed687 Mon Sep 17 00:00:00 2001
From: Qualys Security Advisory <qsa@qualys.com>
Date: Thu, 1 Jan 1970 00:00:00 +0000
Subject: [PATCH 2/3] 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 | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/proc/readproc.c b/proc/readproc.c
index 9e3afc9..39235c7 100644
--- a/proc/readproc.c
+++ b/proc/readproc.c
@@ -35,6 +35,7 @@
#include <signal.h>
#include <fcntl.h>
#include <dirent.h>
+#include <limits.h>
#include <sys/types.h>
#include <sys/stat.h>
#ifdef WITH_SYSTEMD
@@ -622,7 +623,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
@@ -630,11 +631,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';
--
2.14.3
From 44a4b658f45bc3fbd7170662a52038a7b35c83de Mon Sep 17 00:00:00 2001
From: Qualys Security Advisory <qsa@qualys.com>
Date: Thu, 1 Jan 1970 00:00:00 +0000
Subject: [PATCH 3/3] proc/readproc.c: Fix bugs and overflows in file2strvec().
Note: this is by far the most important and complex patch of the whole
series, please review it carefully; thank you very much!
For this patch, we decided to keep the original function's design and
skeleton, to avoid regressions and behavior changes, while fixing the
various bugs and overflows. And like the "Harden file2str()" patch, this
patch does not fail when about to overflow, but truncates instead: there
is information available about this process, so return it to the caller;
also, we used INT_MAX as a limit, but a lower limit could be used.
The easy changes:
- Replace sprintf() with snprintf() (and check for truncation).
- Replace "if (n == 0 && rbuf == 0)" with "if (n <= 0 && tot <= 0)" and
do break instead of return: it simplifies the code (only one place to
handle errors), and also guarantees that in the while loop either n or
tot is > 0 (or both), even if n is reset to 0 when about to overflow.
- Remove the "if (n < 0)" block in the while loop: it is (and was) dead
code, since we enter the while loop only if n >= 0.
- Rewrite the missing-null-terminator detection: in the original
function, if the size of the file is a multiple of 2047, a null-
terminator is appended even if the file is already null-terminated.
- Replace "if (n <= 0 && !end_of_file)" with "if (n < 0 || tot <= 0)":
originally, it was equivalent to "if (n < 0)", but we added "tot <= 0"
to handle the first break of the while loop, and to guarantee that in
the rest of the function tot is > 0.
- Double-force ("belt and suspenders") the null-termination of rbuf:
this is (and was) essential to the correctness of the function.
- Replace the final "while" loop with a "for" loop that behaves just
like the preceding "for" loop: in the original function, this would
lead to unexpected results (for example, if rbuf is |\0|A|\0|, this
would return the array {"",NULL} but should return {"","A",NULL}; and
if rbuf is |A|\0|B| (should never happen because rbuf should be null-
terminated), this would make room for two pointers in ret, but would
write three pointers to ret).
The hard changes:
- Prevent the integer overflow of tot in the while loop, but unlike
file2str(), file2strvec() cannot let tot grow until it almost reaches
INT_MAX, because it needs more space for the pointers: this is why we
introduced ARG_LEN, which also guarantees that we can add "align" and
a few sizeof(char*)s to tot without overflowing.
- Prevent the integer overflow of "tot + c + align": when INT_MAX is
(almost) reached, we write the maximal safe amount of pointers to ret
(ARG_LEN guarantees that there is always space for *ret = rbuf and the
NULL terminator).
---
proc/readproc.c | 53 ++++++++++++++++++++++++++++++++---------------------
1 file changed, 32 insertions(+), 21 deletions(-)
diff --git a/proc/readproc.c b/proc/readproc.c
index 39235c7..94ca4e9 100644
--- a/proc/readproc.c
+++ b/proc/readproc.c
@@ -652,11 +652,12 @@ static int file2str(const char *directory, const char *what, struct utlbuf_s *ub
static char** file2strvec(const char* directory, const char* what) {
char buf[2048]; /* read buf bytes at a time */
- char *p, *rbuf = 0, *endbuf, **q, **ret;
+ char *p, *rbuf = 0, *endbuf, **q, **ret, *strp;
int fd, tot = 0, n, c, end_of_file = 0;
int align;
- sprintf(buf, "%s/%s", directory, what);
+ const int len = snprintf(buf, sizeof buf, "%s/%s", directory, what);
+ if(len <= 0 || (size_t)len >= sizeof buf) return NULL;
fd = open(buf, O_RDONLY, 0);
if(fd==-1) return NULL;
@@ -664,18 +665,23 @@ static char** file2strvec(const char* directory, const char* what) {
while ((n = read(fd, buf, sizeof buf - 1)) >= 0) {
if (n < (int)(sizeof buf - 1))
end_of_file = 1;
- if (n == 0 && rbuf == 0) {
- close(fd);
- return NULL; /* process died between our open and read */
+ if (n <= 0 && tot <= 0) { /* nothing read now, nothing read before */
+ break; /* process died between our open and read */
}
- if (n < 0) {
- if (rbuf)
- free(rbuf);
- close(fd);
- return NULL; /* read error */
+ /* ARG_LEN is our guesstimated median length of a command-line argument
+ or environment variable (the minimum is 1, the maximum is 131072) */
+ #define ARG_LEN 64
+ if (tot >= INT_MAX / (ARG_LEN + (int)sizeof(char*)) * ARG_LEN - n) {
+ end_of_file = 1; /* integer overflow: null-terminate and break */
+ n = 0; /* but tot > 0 */
}
- if (end_of_file && (n == 0 || buf[n-1]))/* last read char not null */
+ #undef ARG_LEN
+ if (end_of_file &&
+ ((n > 0 && buf[n-1] != '\0') || /* last read char not null */
+ (n <= 0 && rbuf[tot-1] != '\0'))) /* last read char not null */
buf[n++] = '\0'; /* so append null-terminator */
+
+ if (n <= 0) break; /* unneeded (end_of_file = 1) but avoid realloc */
rbuf = xrealloc(rbuf, tot + n); /* allocate more memory */
memcpy(rbuf + tot, buf, n); /* copy buffer into it */
tot += n; /* increment total byte ctr */
@@ -683,29 +689,34 @@ static char** file2strvec(const char* directory, const char* what) {
break;
}
close(fd);
- if (n <= 0 && !end_of_file) {
+ if (n < 0 || tot <= 0) { /* error, or nothing read */
if (rbuf) free(rbuf);
return NULL; /* read error */
}
+ rbuf[tot-1] = '\0'; /* belt and suspenders (the while loop did it, too) */
endbuf = rbuf + tot; /* count space for pointers */
align = (sizeof(char*)-1) - ((tot + sizeof(char*)-1) & (sizeof(char*)-1));
- for (c = 0, p = rbuf; p < endbuf; p++) {
- if (!*p || *p == '\n')
+ c = sizeof(char*); /* one extra for NULL term */
+ for (p = rbuf; p < endbuf; p++) {
+ if (!*p || *p == '\n') {
+ if (c >= INT_MAX - (tot + (int)sizeof(char*) + align)) break;
c += sizeof(char*);
+ }
if (*p == '\n')
*p = 0;
}
- c += sizeof(char*); /* one extra for NULL term */
rbuf = xrealloc(rbuf, tot + c + align); /* make room for ptrs AT END */
endbuf = rbuf + tot; /* addr just past data buf */
q = ret = (char**) (endbuf+align); /* ==> free(*ret) to dealloc */
- *q++ = p = rbuf; /* point ptrs to the strings */
- endbuf--; /* do not traverse final NUL */
- while (++p < endbuf)
- if (!*p) /* NUL char implies that */
- *q++ = p+1; /* next string -> next char */
-
+ for (strp = p = rbuf; p < endbuf; p++) {
+ if (!*p) { /* NUL char implies that */
+ if (c < 2 * (int)sizeof(char*)) break;
+ c -= sizeof(char*);
+ *q++ = strp; /* point ptrs to the strings */
+ strp = p+1; /* next string -> next char */
+ }
+ }
*q = 0; /* null ptr list terminator */
return ret;
}
--
2.14.3