Re: [hackers] [st][PATCH] Fix buffer overflow when handling composed input
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
@@ -1833,9 +1833,9 @@ void
kpress(XEvent *ev)
{
XKeyEvent *e = >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;
@@ -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, ,
);
- else
+ overflow = status == XBufferOverflow ? len : 0;
+ } else {
len = XLookupString(e, buf, sizeof buf, , NULL);
+ }
/* 1. shortcuts */
for (bp = shortcuts; bp < shortcuts + LEN(shortcuts); bp++) {
if (ksym == bp->keysym && match(bp->mod, e->state)) {
@@ -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, ,
);
+ 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
Re: [hackers] [st][PATCH] Fix buffer overflow when handling composed input
On 2022-10-24 10:01 AM, NRK wrote: On Mon, Oct 24, 2022 at 09:21:37AM +, Andy Gozas wrote: • 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 Initializing ksym to `NoSymbol` seems like a good idea. • 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? Mainly because using I think that the dynamic allocation patch made the control flow of the function more complicated than necessary. Backward goto is pretty bad in specific. But if you _do_ want to dynamically allocate, you only need to allocate right before buffer is being used. But which approach to take is the maintainer's call, not mine. I've attched both fixed-size and dynamic-allocation patch (but simplified without goto). - NRK Yeah, I think your dynamic patch might be a bit better when it comes to readability, only thing I noticed is that it is missing initialization of ksym to NoSymbol. --- Andy Gozas.
Re: [hackers] [st][PATCH] Fix buffer overflow when handling composed input
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
>From 920799cb2a3526f2398caf98ebf0815fb323ff3f Mon Sep 17 00:00:00 2001
From: NRK
Date: Mon, 24 Oct 2022 15:49:18 +0600
Subject: [PATCH] fix: buffer overflow when handling composed input
XmbLookupString may leave buf as well as ksym uninitialized. initialize
ksym to NoSymbol. track weather we need a larger buffer or not, and
dynamically allocate if needed.
Reported-by: Andy Gozas
---
x.c | 30 --
1 file changed, 20 insertions(+), 10 deletions(-)
diff --git a/x.c b/x.c
index f70e3fb..6a4ed46 100644
--- a/x.c
+++ b/x.c
@@ -1847,8 +1847,8 @@ kpress(XEvent *ev)
{
XKeyEvent *e = >xkey;
KeySym ksym;
- char buf[64], *customkey;
- int len;
+ char buf[64], *p = NULL, *customkey;
+ int len, overflow = 0;
Rune c;
Status status;
const Shortcut *bp;
@@ -1856,10 +1856,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, , );
- else
+ overflow = status == XBufferOverflow ? len : 0;
+ } else {
len = XLookupString(e, buf, sizeof buf, , NULL);
+ }
/* 1. shortcuts */
for (bp = shortcuts; bp < shortcuts + LEN(shortcuts); bp++) {
if (ksym == bp->keysym && match(bp->mod, e->state)) {
@@ -1875,21 +1877,29 @@ kpress(XEvent *ev)
}
/* 3. composed string from input method */
+ if (overflow) {
+ p = xmalloc(overflow);
+ len = XmbLookupString(xw.ime.xic, e, p, overflow, , );
+ } 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
--
2.35.1
Re: [hackers] [st][PATCH] Fix buffer overflow when handling composed input
On 24.10.2022 13.01, NRK wrote:> But which approach to take is the maintainer's call, not mine. I've attched both fixed-size and dynamic-allocation patch (but simplified without goto). 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. -- Santtu
Re: [hackers] [st][PATCH] Fix buffer overflow when handling composed input
On Mon, Oct 24, 2022 at 09:21:37AM +, Andy Gozas wrote:
> • 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
Initializing ksym to `NoSymbol` seems like a good idea.
> • 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?
Mainly because using I think that the dynamic allocation patch made the
control flow of the function more complicated than necessary.
Backward goto is pretty bad in specific.
But if you _do_ want to dynamically allocate, you only need to allocate
right before buffer is being used.
But which approach to take is the maintainer's call, not mine.
I've attched both fixed-size and dynamic-allocation patch (but
simplified without goto).
- NRK
>From 6eae20945177667a0331670d0ca1c75226cfd29a Mon Sep 17 00:00:00 2001
From: NRK
Date: Mon, 24 Oct 2022 15:35:17 +0600
Subject: [PATCH] fix: buffer overflow when handling composed input
XmbLookupString may leave buf as well as ksym uninitialized. initialize
ksym to NoSymbol and track weather buffer was initialized or not via
`got_buf`.
Reported-by: Andy Gozas
---
x.c | 14 +-
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/x.c b/x.c
index f70e3fb..b344dad 100644
--- a/x.c
+++ b/x.c
@@ -1846,20 +1846,24 @@ void
kpress(XEvent *ev)
{
XKeyEvent *e = >xkey;
- KeySym ksym;
- char buf[64], *customkey;
+ KeySym ksym = NoSymbol;
+ char buf[512], *customkey;
int len;
Rune c;
Status status;
const Shortcut *bp;
+ int got_buf = 0;
if (IS_SET(MODE_KBDLOCK))
return;
- if (xw.ime.xic)
+ if (xw.ime.xic) {
len = XmbLookupString(xw.ime.xic, e, buf, sizeof buf, , );
- else
+ got_buf = status == XLookupBoth || status == XLookupChars;
+ } else {
len = XLookupString(e, buf, sizeof buf, , NULL);
+ got_buf = 1;
+ }
/* 1. shortcuts */
for (bp = shortcuts; bp < shortcuts + LEN(shortcuts); bp++) {
if (ksym == bp->keysym && match(bp->mod, e->state)) {
@@ -1875,7 +1879,7 @@ kpress(XEvent *ev)
}
/* 3. composed string from input method */
- if (len == 0)
+ if (len == 0 || !got_buf)
return;
if (len == 1 && e->state & Mod1Mask) {
if (IS_SET(MODE_8BIT)) {
--
2.35.1
>From ea78eacf892ee232a5a060fa1204f896841f7ec5 Mon Sep 17 00:00:00 2001
From: NRK
Date: Mon, 24 Oct 2022 15:49:18 +0600
Subject: [PATCH] fix: buffer overflow when handling composed input
XmbLookupString may leave buf as well as ksym uninitialized. initialize
ksym to NoSymbol. track weather we need a larger buffer or not, and
dynamically allocate if needed.
Reported-by: Andy Gozas
---
x.c | 16
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/x.c b/x.c
index f70e3fb..e50ac90 100644
--- a/x.c
+++ b/x.c
@@ -1847,8 +1847,8 @@ kpress(XEvent *ev)
{
XKeyEvent *e = >xkey;
KeySym ksym;
- char buf[64], *customkey;
- int len;
+ char small_buf[512], *buf = small_buf, *customkey;
+ int len, buf_overflow = 0;
Rune c;
Status status;
const Shortcut *bp;
@@ -1856,10 +1856,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, , );
- else
+ buf_overflow = status == XBufferOverflow ? len : 0;
+ } else {
len = XLookupString(e, buf, sizeof buf, , NULL);
+ }
/* 1. shortcuts */
for (bp = shortcuts; bp < shortcuts + LEN(shortcuts); bp++) {
if (ksym == bp->keysym && match(bp->mod, e->state)) {
@@ -1875,6 +1877,10 @@ kpress(XEvent *ev)
}
/* 3. composed string from input method */
+ if (buf_overflow) {
+ buf = xmalloc(buf_overflow);
+ len = XmbLookupString(xw.ime.xic, e, buf, buf_overflow, , );
+ }
if (len == 0)
return;
if (len == 1 && e->state & Mod1Mask) {
@@ -1890,6 +1896,8 @@ kpress(XEvent *ev)
}
}
ttywrite(buf, len, 1);
+ if (buf_overflow)
+ free(buf);
}
void
--
2.35.1
Re: [hackers] [st][PATCH] Fix buffer overflow when handling composed input
On 2022-10-24 12:35 AM, NRK wrote:
On Sun, Oct 23, 2022 at 04:18:42PM +, 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
@@ -1833,9 +1833,10 @@ void
kpress(XEvent *ev)
{
XKeyEvent *e = >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;
@@ -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, ,
);
- else
- len = XLookupString(e, buf, sizeof buf, , NULL);
+reallocbuf:
+ free(buf);
+ buf = xmalloc(len);
+ if (xw.ime.xic) {
+ len = XmbLookupString(xw.ime.xic, e, buf, len, , );
+ if (status == XBufferOverflow) {
+ if (reallocd) {
+ goto cleanup;
+ }
+ reallocd = 1;
+ goto reallocbuf;
+ }
+ } else {
+ len = XLookupString(e, buf, len, , 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) {
@@ -1877,6 +1894,8 @@ kpress(XEvent *ev)
}
}
ttywrite(buf, len, 1);
+cleanup:
+ free(buf);
}
void
