| From 3ab2a4e02682df1382955071919d8aa3c3ec40d4 Mon Sep 17 00:00:00 2001 |
| From: Rich Felker <dalias@aerifal.cx> |
| Date: Thu, 19 Nov 2020 17:12:43 -0500 |
| Subject: [PATCH] rewrite wcsnrtombs to fix buffer overflow and other bugs |
| |
| the original wcsnrtombs implementation, which has been largely |
| untouched since 0.5.0, attempted to build input-length-limiting |
| conversion on top of wcsrtombs, which only limits output length. as |
| best I recall, this choice was made out of a mix of disdain over |
| having yet another variant function to implement (added in POSIX 2008; |
| not standard C) and preference not to switch things around and |
| implement the wcsrtombs in terms of the more general new function, |
| probably over namespace issues. the strategy employed was to impose |
| output limits that would ensure the input limit wasn't exceeded, then |
| finish up the tail character-at-a-time. unfortunately, none of that |
| worked correctly. |
| |
| first, the logic in the wcsrtombs loop was wrong in that it could |
| easily get stuck making no forward progress, by imposing an output |
| limit too small to convert even one character. |
| |
| the character-at-a-time loop that followed was even worse. it made no |
| effort to ensure that the converted multibyte character would fit in |
| the remaining output space, only that there was a nonzero amount of |
| output space remaining. it also employed an incorrect interpretation |
| of wcrtomb's interface contract for converting the null character, |
| thereby failing to act on end of input, and remaining space accounting |
| was subject to unsigned wrap-around. together these errors allow |
| unbounded overflow of the destination buffer, controlled by input |
| length limit and input wchar_t string contents. |
| |
| given the extent to which this function was broken, it's plausible |
| that most applications that would have been rendered exploitable were |
| sufficiently broken not to be usable in the first place. however, it's |
| also plausible that common (especially ASCII-only) inputs succeeded in |
| the wcsrtombs loop, which mostly worked, while leaving the wildly |
| erroneous code in the second loop exposed to particular non-ASCII |
| inputs. |
| |
| CVE-2020-28928 has been assigned for this issue. |
| |
| Signed-off-by: Peter Korsgaard <peter@korsgaard.com> |
| --- |
| src/multibyte/wcsnrtombs.c | 46 ++++++++++++++++---------------------- |
| 1 file changed, 19 insertions(+), 27 deletions(-) |
| |
| diff --git a/src/multibyte/wcsnrtombs.c b/src/multibyte/wcsnrtombs.c |
| index 676932b5..95e25e70 100644 |
| --- a/src/multibyte/wcsnrtombs.c |
| +++ b/src/multibyte/wcsnrtombs.c |
| @@ -1,41 +1,33 @@ |
| #include <wchar.h> |
| +#include <limits.h> |
| +#include <string.h> |
| |
| size_t wcsnrtombs(char *restrict dst, const wchar_t **restrict wcs, size_t wn, size_t n, mbstate_t *restrict st) |
| { |
| - size_t l, cnt=0, n2; |
| - char *s, buf[256]; |
| const wchar_t *ws = *wcs; |
| - const wchar_t *tmp_ws; |
| - |
| - if (!dst) s = buf, n = sizeof buf; |
| - else s = dst; |
| - |
| - while ( ws && n && ( (n2=wn)>=n || n2>32 ) ) { |
| - if (n2>=n) n2=n; |
| - tmp_ws = ws; |
| - l = wcsrtombs(s, &ws, n2, 0); |
| - if (!(l+1)) { |
| - cnt = l; |
| - n = 0; |
| + size_t cnt = 0; |
| + if (!dst) n=0; |
| + while (ws && wn) { |
| + char tmp[MB_LEN_MAX]; |
| + size_t l = wcrtomb(n<MB_LEN_MAX ? tmp : dst, *ws, 0); |
| + if (l==-1) { |
| + cnt = -1; |
| break; |
| } |
| - if (s != buf) { |
| - s += l; |
| + if (dst) { |
| + if (n<MB_LEN_MAX) { |
| + if (l>n) break; |
| + memcpy(dst, tmp, l); |
| + } |
| + dst += l; |
| n -= l; |
| } |
| - wn = ws ? wn - (ws - tmp_ws) : 0; |
| - cnt += l; |
| - } |
| - if (ws) while (n && wn) { |
| - l = wcrtomb(s, *ws, 0); |
| - if ((l+1)<=1) { |
| - if (!l) ws = 0; |
| - else cnt = l; |
| + if (!*ws) { |
| + ws = 0; |
| break; |
| } |
| - ws++; wn--; |
| - /* safe - this loop runs fewer than sizeof(buf) times */ |
| - s+=l; n-=l; |
| + ws++; |
| + wn--; |
| cnt += l; |
| } |
| if (dst) *wcs = ws; |
| -- |
| 2.20.1 |
| |