You can not select more than 25 topics
Topics must start with a letter or number, can include dashes ('-') and can be up to 35 characters long.
337 lines
13 KiB
337 lines
13 KiB
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 |
|
|
|
|