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

From: Roberto E. Vargas Caballero <k0ga_AT_shike2.com>
Date: Tue, 1 Oct 2013 23:04:16 +0200

> >> > +#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:

Ok, I can see now that Key.s is an array and not a pointer. We have a
problem with this definition.

> > 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():

> > _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)

I agree here, good catch and good implementation. Could you send
a mail with the patch and a proper commit message?

> > 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 },
> >

I cannot see when could be interesting send a '\0'. It someone can give
a case when it is necessary we can think about this patch.

> Isn't there strnlen() function?

This was the same I was thinking before of reading this new version,
but it is not necessary now.


-- 
Roberto E. Vargas Caballero
----------------------------
k0ga_AT_shike2.com
http://www.shike2.com
Received on Tue Oct 01 2013 - 23:04:16 CEST

This archive was generated by hypermail 2.3.0 : Tue Oct 01 2013 - 23:12:07 CEST