On Tue, 8 Apr 2014 16:41:29 +0200
Sebastian Krahmer <krahmer@xxxxxxx> wrote:
> 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>
>
Ok, I think you're correct about snprintf. I got it confused with
sprintf, which doesn't always NULL-terminate.
If it does indeed always null-terminate then there is indeed no harm in
using strlen, it's just not as efficient. Why not instead simply take
the return value of snprintf and use that to determine whether the
output got truncated? I think we'd rather return an error if it is,
than pass in a possibly bogus string to add_key().
--
Jeff Layton <jlayton@xxxxxxxxx>
--
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
