diff --git a/SOURCES/procps-ng-3.3.10-CVE-2018-1124.patch b/SOURCES/procps-ng-3.3.10-CVE-2018-1124.patch new file mode 100644 index 0000000..ecb4476 --- /dev/null +++ b/SOURCES/procps-ng-3.3.10-CVE-2018-1124.patch @@ -0,0 +1,337 @@ +From 9d2ec74b76d220f5343e548fcb7058d723293a22 Mon Sep 17 00:00:00 2001 +From: Qualys Security Advisory +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 +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 + #include + #include ++#include + #include + #include + #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 +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 + diff --git a/SPECS/procps-ng.spec b/SPECS/procps-ng.spec index e6b4389..0023798 100644 --- a/SPECS/procps-ng.spec +++ b/SPECS/procps-ng.spec @@ -4,7 +4,7 @@ Summary: System and process monitoring utilities Name: procps-ng Version: 3.3.10 -Release: 17%{?dist} +Release: 17%{?dist}.2 License: GPL+ and GPLv2 and GPLv2+ and GPLv3+ and LGPLv2+ Group: Applications/System URL: https://sourceforge.net/projects/procps-ng/ @@ -25,6 +25,7 @@ Patch10: procps-ng-3.3.10-sysctl-conf-manpage-predef-note.patch Patch11: procps-ng-3.3.10-top-instant-cpu-stats.patch Patch12: procps-ng-3.3.10-sysctl-man-conf-override-hint.patch Patch13: procps-ng-3.3.10-top-strange-mem-val-scaling.patch +Patch18: procps-ng-3.3.10-CVE-2018-1124.patch Requires(post): /sbin/ldconfig @@ -105,6 +106,7 @@ Internationalization pack for procps-ng %patch11 -p1 %patch12 -p1 %patch13 -p1 +%patch18 -p1 %build @@ -186,6 +188,14 @@ rmdir %{buildroot}/share %{_datadir}/locale/* %changelog +* Tue May 15 2018 Kamil Dudka - 3.3.10-17.el7_5.2 +- check for truncation after calling snprintf() +- Related: CVE-2018-1124 + +* Fri May 11 2018 Kamil Dudka - 3.3.10-17.el7_5.1 +- fix integer overflows leading to heap overflow in file2strvec() +- Resolves: CVE-2018-1124 + * Wed Sep 06 2017 Jan Rybar - 3.3.10-17 - top: strange unit scaling with high memory values - Resolves: rhbz#1253851