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

From: Alexander S. <alex0player_AT_gmail.com>
Date: Wed, 2 Oct 2013 00:03:00 +0400

2013/10/2 Mark Edgar <medgar123_AT_gmail.com>:
> 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
>
Isn't there strnlen() function?
Received on Tue Oct 01 2013 - 22:03:00 CEST