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