[PATCH] cifskey: better use snprintf()

Discussion:

Sebastian Krahmer

2014-04-08 12:44:44 UTC

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-***@public.gmane.org>
---

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

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);
}

--
~ perl self.pl
~ $_='print"\$_=\47$_\47;eval"';eval
~ krahmer-***@public.gmane.org - SuSE Security Team

Jeff Layton

2014-04-08 14:32:12 UTC

Permalink

On Tue, 8 Apr 2014 14:44:44 +0200

Post by Sebastian Krahmer
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.
---
--- 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?

Post by Sebastian Krahmer
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-***@public.gmane.org>

Sebastian Krahmer

2014-04-08 14:41:29 UTC

Permalink

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

Post by Jeff Layton
On Tue, 8 Apr 2014 14:44:44 +0200

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?

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

--
~ perl self.pl
~ $_='print"\$_=\47$_\47;eval"';eval
~ krahmer-***@public.gmane.org - SuSE Security Team

Jeff Layton

2014-04-08 17:23:26 UTC

Permalink

On Tue, 8 Apr 2014 16:41:29 +0200

Post by Sebastian Krahmer
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

Post by Jeff Layton
On Tue, 8 Apr 2014 14:44:44 +0200

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?

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

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-***@public.gmane.org>

Sebastian Krahmer

2014-04-09 06:11:04 UTC

Permalink

Post by Jeff Layton

Post by Jeff Layton
If you do that then you don't need to use strlen() either.
--

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

Could be done indeed.

Sebastian

--
~ perl self.pl
~ $_='print"\$_=\47$_\47;eval"';eval
~ krahmer-***@public.gmane.org - SuSE Security Team