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

From: Andy Gozas <andy_AT_gozas.me>
Date: Mon, 24 Oct 2022 09:21:37 +0000

On 2022-10-24 12:35 AM, NRK wrote:
>> On Sun, Oct 23, 2022 at 04:18:42PM +0000, Andy Gozas wrote:
>> > St relies on an incorrect assumption of how XmbLookupString function
>> > behaves.
>
> Looking at the XmbLookupString manpage [0] reveals more trouble. It
> seems
> that `ksym` might be used uninitalized as well. Inlined a proprosed
> patch.
>
> P.S: Please CC me on any replies, I seem to be missing a lot of mails
> from the ML recently.

Hmmm, I actually missed that part, but after some reading (and some
guessing when it comes to XLookupString), I think, that we could do the
following:

• Since we can actually get the whole composed text when using
XmbLookupString by reallocating the buffer, I think we should do that —
why stop at 512 bytes?
• XmbLookupString leaves the ksym unchanged if not filled and
XLookupString [probably] sets it to NoSymbol (that's what XLookupKeysym
does, but whether or not XLookupString shares this behavior is unclear
[1]), so we can just set it to NoSymbol in the beginning ourselves and
check if it was changed later (maybe we don't have to even check,
depends on how the following functions would react to NoSymbol being
passed to them). We could check status as well, just to be sure, but I
think that is not required.
• We don't actually need to check for any status of XmbLookupString
other than XBufferOverflow to know if buffer is filled or not, because
in all other cases, the returned length will be set to 0 if it wasn't.

I have changed my patch accordingly, made it a bit more readable, and
tested it, and will be inlining it here as well.

[1]: https://linux.die.net/man/3/xlookupkeysym

---
Andy Gozas.
diff --git a/x.c b/x.c
index 2a3bd38..3084cbf 100644
--- a/x.c
+++ b/x.c
_AT_@ -1833,9 +1833,10 @@ void
  kpress(XEvent *ev)
  {
  	XKeyEvent *e = &ev->xkey;
-	KeySym ksym;
-	char buf[64], *customkey;
-	int len;
+	KeySym ksym = NoSymbol;
+	char *buf = NULL, *customkey;
+	int len = 64;
+	int reallocd = 0;
  	Rune c;
  	Status status;
  	Shortcut *bp;
_AT_@ -1843,27 +1844,43 @@ kpress(XEvent *ev)
  	if (IS_SET(MODE_KBDLOCK))
  		return;
-	if (xw.ime.xic)
-		len = XmbLookupString(xw.ime.xic, e, buf, sizeof buf, &ksym, 
&status);
-	else
-		len = XLookupString(e, buf, sizeof buf, &ksym, NULL);
+reallocbuf:
+	free(buf);
+	buf = xmalloc(len);
+	if (xw.ime.xic) {
+		len = XmbLookupString(xw.ime.xic, e, buf, len, &ksym, &status);
+		if (status == XBufferOverflow) {
+			if (reallocd) {
+				goto cleanup;
+			}
+			reallocd = 1;
+			goto reallocbuf;
+		}
+	} else {
+		len = XLookupString(e, buf, len, &ksym, NULL);
+	}
+
+	if (ksym == NoSymbol)
+		goto nosym;
+
  	/* 1. shortcuts */
  	for (bp = shortcuts; bp < shortcuts + LEN(shortcuts); bp++) {
  		if (ksym == bp->keysym && match(bp->mod, e->state)) {
  			bp->func(&(bp->arg));
-			return;
+			goto cleanup;
  		}
  	}
  	/* 2. custom keys from config.h */
  	if ((customkey = kmap(ksym, e->state))) {
  		ttywrite(customkey, strlen(customkey), 1);
-		return;
+		goto cleanup;
  	}
+nosym:
  	/* 3. composed string from input method */
  	if (len == 0)
-		return;
+		goto cleanup;
  	if (len == 1 && e->state & Mod1Mask) {
  		if (IS_SET(MODE_8BIT)) {
  			if (*buf < 0177) {
_AT_@ -1877,6 +1894,8 @@ kpress(XEvent *ev)
  		}
  	}
  	ttywrite(buf, len, 1);
+cleanup:
+	free(buf);
  }
  void

Received on Mon Oct 24 2022 - 11:21:37 CEST