| |
@@ -0,0 +1,337 @@
|
| |
+ 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
|
| |
+ @@ -10,9 +10,9 @@ typedef void (*message_fn)(const char *__restrict, ...) __attribute__((format(pr
|
| |
+ /* 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
|
| |
+ @@ -635,7 +636,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
|
| |
+ @@ -643,11 +644,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
|
| |
+ @@ -665,11 +665,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;
|
| |
+
|
| |
+ @@ -677,18 +678,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 */
|
| |
+ @@ -696,29 +702,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
|
| |
+
|
| |
