cifskey: better use snprintf()

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 <[email protected]>

---

cifskey.c renamed from cifskey.c.orig +1 -0

@@ -20,6 +20,7 @@
 #include <sys/types.h>
 #include <keyutils.h>
 #include <stdio.h>
+#include <string.h>
 #include "cifskey.h"
 #include "resolve_host.h"
 

cifskey.c renamed from cifskey.c.orig +0 -1

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

Quoted 22 lines · On Tue, 8 Apr 2014 14:44:44 +0200 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.

Signed-off-by Sebastian Krahmer <[email protected]>

---

cifskey.c renamed from cifskey.c.orig +1 -0

@@ -20,6 +20,7 @@
 #include <sys/types.h>
 #include <keyutils.h>
 #include <stdio.h>
+#include <string.h>
 #include "cifskey.h"
 #include "resolve_host.h"
 

cifskey.c line 33

Quoted 5 lines

cifskey.c renamed from cifskey.c.orig +1 -1

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

Quoted 14 lines

cifskey.c renamed from cifskey.c.orig +0 -1

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

cifskey.c line 52

Quoted 10 lines

cifskey.c renamed from cifskey.c.orig +3 -3

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

Signature · 1 line

Jeff Layton <[email protected]>

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

Quoted 69 lines · Jeff Layton

Quoted 22 lines · On Tue, 8 Apr 2014 14:44:44 +0200 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.

Signed-off-by Sebastian Krahmer <[email protected]>

---

cifskey.c renamed from cifskey.c.orig +1 -0

@@ -20,6 +20,7 @@
 #include <sys/types.h>
 #include <keyutils.h>
 #include <stdio.h>
+#include <string.h>
 #include "cifskey.h"
 #include "resolve_host.h"
 

cifskey.c line 33

Quoted 5 lines

cifskey.c renamed from cifskey.c.orig +1 -1

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

Quoted 14 lines

cifskey.c renamed from cifskey.c.orig +0 -1

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

cifskey.c line 52

Quoted 10 lines

cifskey.c renamed from cifskey.c.orig +3 -3

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

Signature · 1 line

Jeff Layton <[email protected]>

Quoted 5 lines

Quoted 4 lines

If you do that then you don't need to use strlen() either.

Signature · 1 line

Jeff Layton <[email protected]>

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