summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorRich Felker <dalias@aerifal.cx>2020-11-19 17:12:43 -0500
committerRich Felker <dalias@aerifal.cx>2020-11-19 17:12:43 -0500
commit3ab2a4e02682df1382955071919d8aa3c3ec40d4 (patch)
treed670f11797f39d9b018ab74313ca05f7421125c2
parent233bb6972d84e9cb908ff038f78d97e487082225 (diff)
downloadmusl-3ab2a4e02682df1382955071919d8aa3c3ec40d4.tar.gz
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.
-rw-r--r--src/multibyte/wcsnrtombs.c46
1 files 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;