[dev] Re: [st] [PATCH] Avoid buffer overflows in the case of key-mapped strings.

From: Mark Edgar <medgar123_AT_gmail.com>
Date: Tue, 1 Oct 2013 22:00:44 +0200

On Mon, 23 Sep 2013 10:06:13 +0200, Roberto E. Vargas Caballero wrote:
> On Sat, Sep 21, 2013 at 03:41:01PM +0200, Mark Edgar wrote:
> I don't agree here, because if you insert a escape sequence is not dificult
> get more of 16 characters (for example a key combination which updates the
> bar title).

Yes, agreed.


> > +#define STRNLEN(s) ((s)[LEN((s)) - 1] == '\0' ? strlen((s)) : LEN((s)))
> This trick is only useful when you usually have strings with a size of
> LEN(s)-1, but in our case we will usually have string with a small size,
> so it means we always will add a new test before of calling to strlen.

This test here was to ensure the string is NUL-terminated. With a
16-char length buffer, it's more likely that someone will accidentally
use it all. In other words, this Key entry is legal, but strlen()
can't handle it:

static Key key[] = {
  { XK_F12, XK_NO_MOD, "0123456789012345" },

With the current version of st, that string literal would have to be
512 characters long before overflow. This is unlikely, but still
possible.


> Could you explain what overflow are you fixing here?, because I only
> can see you are returning the Key struct instead of returning directly
> the char representation.

There's actually two here: the strlen(customkey) I mentioned above,
and the original memcpy(buf[32], customkey[512], len<512) call in
kpress(). The following key entry will overflow buf[] in kpress() in
the current version of st. If it doesn't go boom for you, try doubling
the length of the string literal:

static Key key[] = {
  { XK_F12, XK_NO_MOD, "01234567890123456789012345678901" },


A much neater fix might be to make Key.s a char pointer instead. This
also solves the question of what to use as the string length limit
(should it be 16 or 32 or 512 -- well, how about unlimited?), avoids
invalid strlen() calls (assuming config.h doesn't do anything too
tricky), and entirely avoids the extra memcpy():

--- a/st.c
+++ b/st.c
_AT_@ -250,7 +250,7 @@ typedef struct {
 typedef struct {
        KeySym k;
        uint mask;
- char s[ESC_BUF_SIZ];
+ char *s;
        /* three valued logic variables: 0 indifferent, 1 on, -1 off */
        signed char keypad; /* application keypad */
        signed char cursor; /* application cursor */
_AT_@ -3538,7 +3538,10 @@ kpress(XEvent *ev) {
        /* 2. custom keys from config.h */
        if((customkey = kmap(ksym, e->state))) {
                len = strlen(customkey);
- memcpy(buf, customkey, len);
+ ttywrite(customkey, len);
+ if(IS_SET(MODE_ECHO))
+ techo(customkey, len);
+ return;
        /* 3. hardcoded (overrides X lookup) */
        } else {
                if(len == 0)


What do you think?


FYI: I have another patch which allows for keys to send the NUL byte
to the tty, but I'm not really sure how generally useful it really is.
It would look like this in config.h:

static Key key[] = {
  { XK_F12, XK_NO_MOD, "\0", .len=1 },

     -Mark
Received on Tue Oct 01 2013 - 22:00:44 CEST