Re: [hackers] [st][PATCH] Fix buffer overflow when handling composed input

From: Andy Gozas <andy_AT_gozas.me>
Date: Mon, 24 Oct 2022 12:16:58 +0000

On 2022-10-24 10:30 AM, NRK wrote:
> On Mon, Oct 24, 2022 at 01:10:29PM +0300, Santtu Lakkala wrote:
>> The dynmaic[sic] version incorrectly passes sizeof(buf), where buf is
>> char
>> *, as the size of buffer in the "happy case" leading to unnecessary
>> hits to
>> the dynamic path.
>
> Ah yes, the classic. Attached ammended version of the dynmaic patch.
> A bit more invasive (but hopefully correct) than the previous patch.
>
> - NRK

I have fixed the missing ksym initialization as NoSymbol, as well as
added an extra check for BufferOverflow after the second
XmbLookupString. Even though I think there's most likely no way, that
this would ever be triggered, I still think we should check, just to be
safe, since it's not such a big deal for us to check and the alternative
would be a UB if something that would cause that happened.

Patch inlined below.

---
Andy Gozas.
diff --git a/x.c b/x.c
index 2a3bd38..c8de29b 100644
--- a/x.c
+++ b/x.c
_AT_@ -1833,9 +1833,9 @@ void
  kpress(XEvent *ev)
  {
  	XKeyEvent *e = &ev->xkey;
-	KeySym ksym;
-	char buf[64], *customkey;
-	int len;
+	KeySym ksym = NoSymbol;
+	char buf[64], *p = NULL, *customkey;
+	int len, overflow = 0;
  	Rune c;
  	Status status;
  	Shortcut *bp;
_AT_@ -1843,10 +1843,12 @@ kpress(XEvent *ev)
  	if (IS_SET(MODE_KBDLOCK))
  		return;
-	if (xw.ime.xic)
+	if (xw.ime.xic) {
  		len = XmbLookupString(xw.ime.xic, e, buf, sizeof buf, &ksym, 
&status);
-	else
+		overflow = status == XBufferOverflow ? len : 0;
+	} else {
  		len = XLookupString(e, buf, sizeof buf, &ksym, NULL);
+	}
  	/* 1. shortcuts */
  	for (bp = shortcuts; bp < shortcuts + LEN(shortcuts); bp++) {
  		if (ksym == bp->keysym && match(bp->mod, e->state)) {
_AT_@ -1862,21 +1864,33 @@ kpress(XEvent *ev)
  	}
  	/* 3. composed string from input method */
+	if (overflow) {
+		p = xmalloc(overflow);
+		len = XmbLookupString(xw.ime.xic, e, p, overflow, &ksym, &status);
+		if (status == XBufferOverflow) {
+			free(p);
+			return;
+		}
+	} else {
+		p = buf;
+	}
  	if (len == 0)
  		return;
  	if (len == 1 && e->state & Mod1Mask) {
  		if (IS_SET(MODE_8BIT)) {
-			if (*buf < 0177) {
-				c = *buf | 0x80;
-				len = utf8encode(c, buf);
+			if (*p < 0177) {
+				c = *p | 0x80;
+				len = utf8encode(c, p);
  			}
  		} else {
-			buf[1] = buf[0];
-			buf[0] = '\033';
+			p[1] = p[0];
+			p[0] = '\033';
  			len = 2;
  		}
  	}
-	ttywrite(buf, len, 1);
+	ttywrite(p, len, 1);
+	if (overflow)
+		free(p);
  }
  void

Received on Mon Oct 24 2022 - 14:16:58 CEST