Hi
C standard says snprintf() is writing
the terminating 0-byte (thats indeed the real beauty of snprintf).
Nevertheless snprintf() return value may be
larger than buffer size (# bytes that would have been written).
So strlen() should be safe and the buffer is 0-terminated in any case.
Sebastian
On Tue, Apr 08, 2014 at 10:32:12AM -0400, Jeff Layton wrote:
> On Tue, 8 Apr 2014 14:44:44 +0200
> Sebastian Krahmer <krahmer@xxxxxxx> wrote:
>
> >
> > Prefer snprintf() over sprintf() in cifskey.c
> > Projects that fork the code (pam_cifscreds) can't rely on
> > the max-size parameters. Also use strlen() for determining
> > buffer size, as snprintf() may return values larger than buffer size.
> >
> >
> > Signed-off-by: Sebastian Krahmer <krahmer@xxxxxxx>
> > ---
> >
> >
> > --- cifskey.c.orig 2014-04-08 13:10:41.653435040 +0200
> > +++ cifskey.c 2014-04-08 14:28:54.457766913 +0200
> > @@ -20,6 +20,7 @@
> > #include <sys/types.h>
> > #include <keyutils.h>
> > #include <stdio.h>
> > +#include <string.h>
> > #include "cifskey.h"
> > #include "resolve_host.h"
> >
> > @@ -29,7 +30,7 @@
> > {
> > char desc[INET6_ADDRSTRLEN + sizeof(KEY_PREFIX) + 4];
> >
> > - sprintf(desc, "%s:%c:%s", KEY_PREFIX, keytype, addr);
> > + snprintf(desc, sizeof(desc), "%s:%c:%s", KEY_PREFIX, keytype, addr);
>
> If we're concerned about buffer overruns here, then shouldn't you be
> checking the return value of snprintf() to ensure that the string above
> is NULL terminated?
>
> >
> > return keyctl_search(DEST_KEYRING, CIFS_KEY_TYPE, desc, 0);
> > }
> > @@ -38,15 +39,14 @@
> > key_serial_t
> > key_add(const char *addr, const char *user, const char *pass, char keytype)
> > {
> > - int len;
> > char desc[INET6_ADDRSTRLEN + sizeof(KEY_PREFIX) + 4];
> > char val[MOUNT_PASSWD_SIZE + MAX_USERNAME_SIZE + 2];
> >
> > /* set key description */
> > - sprintf(desc, "%s:%c:%s", KEY_PREFIX, keytype, addr);
> > + snprintf(desc, sizeof(desc), "%s:%c:%s", KEY_PREFIX, keytype, addr);
> >
> > /* set payload contents */
> > - len = sprintf(val, "%s:%s", user, pass);
> > + snprintf(val, sizeof(val), "%s:%s", user, pass);
> >
> > - return add_key(CIFS_KEY_TYPE, desc, val, len + 1, DEST_KEYRING);
> > + return add_key(CIFS_KEY_TYPE, desc, val, strlen(val) + 1, DEST_KEYRING);
> > }
> >
> >
>
> Ditto with the above checks. Just because you're using snprintf doesn't
> mean that the resulting string will be NULL terminated. You need to
> check the return value of snprintf and ensure that it fits within the
> buffer and that it ended up being NULL terminated.
>
> If you do that then you don't need to use strlen() either.
>
> --
> Jeff Layton <jlayton@xxxxxxxxx>
--
~ perl self.pl
~ $_='print"\$_=\47$_\47;eval"';eval
~ krahmer@xxxxxxx - SuSE Security Team
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
