123456789101112131415161718192021222324252627282930313233343536373839404142434445464748495051525354555657585960616263646566676869707172737475767778798081828384858687888990919293949596979899100101102103104105106107108109110111112113114 |
- 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
|