Skip to contentSkip to conversation

cifskey: better use snprintf()

PATCH In review Sebastian Krahmer 3 participants 3 messages Apr 14, 2014 – Apr 16, 2014 1 message not in the archive

Subsystems (2)
SMB client (CIFS) Kernel SMB3 server (ksmbd)
lore
Share & tools
lore

Sebastian Krahmer

author
#0 lore raw

PATCH 1/1 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.

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

---

cifskey.c renamed from cifskey.c.orig +8 -4
@@ -29,7 +29,8 @@
 {
 	char desc[INET6_ADDRSTRLEN + sizeof(KEY_PREFIX) + 4];
 
-	sprintf(desc, "%s:%c:%s", KEY_PREFIX, keytype, addr);
+	if (snprintf(desc, sizeof(desc), "%s:%c:%s", KEY_PREFIX, keytype, addr) >= (int)sizeof(desc))
+		return -1;
 
 	return keyctl_search(DEST_KEYRING, CIFS_KEY_TYPE, desc, 0);
 }

@@ -38,15 +39,18 @@
 key_serial_t
 key_add(const char *addr, const char *user, const char *pass, char keytype)
 {
-	int len;
+	int len = 0;
 	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);
+	if (snprintf(desc, sizeof(desc), "%s:%c:%s", KEY_PREFIX, keytype, addr) >= (int)sizeof(desc))
+		return -1;
 
 	/* set payload contents */
-	len = sprintf(val, "%s:%s", user, pass);
+	len = snprintf(val, sizeof(val), "%s:%s", user, pass);
+	if (len >= (int)sizeof(val))
+		return -1;
 
 	return add_key(CIFS_KEY_TYPE, desc, val, len + 1, DEST_KEYRING);
 }
Signature · 3 lines
~ perl self.pl
~ $_='print"\$_=\47$_\47;eval"';eval
~ [email protected] - SuSE Security Team
#2 lore raw
In reply to a message not in the archive
Quoted 22 lines · On Mon, 14 Apr 2014 11:39:41 +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.

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

---

cifskey.c renamed from cifskey.c.orig +2 -1
@@ -29,7 +29,8 @@
 {
 	char desc[INET6_ADDRSTRLEN + sizeof(KEY_PREFIX) + 4];
 
-	sprintf(desc, "%s:%c:%s", KEY_PREFIX, keytype, addr);
+	if (snprintf(desc, sizeof(desc), "%s:%c:%s", KEY_PREFIX, keytype, addr) >= (int)sizeof(desc))
+		return -1;
 
 	return keyctl_search(DEST_KEYRING, CIFS_KEY_TYPE, desc, 0);
 }

cifskey.c line 42

Quoted 5 lines
cifskey.c renamed from cifskey.c.orig +1 -1
@@ -38,15 +39,18 @@
 key_serial_t
 key_add(const char *addr, const char *user, const char *pass, char keytype)
 {
-	int len;
+	int len = 0;

This initialization doesn't appear to be needed.

Quoted 8 lines
cifskey.c renamed from cifskey.c.orig +1 -1
 	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);
+	if (snprintf(desc, sizeof(desc), "%s:%c:%s", KEY_PREFIX, keytype, addr) >= (int)sizeof(desc))

cifskey.c line 56

Quoted 10 lines
cifskey.c renamed from cifskey.c.orig +4 -1
+		return -1;
 
 	/* set payload contents */
-	len = sprintf(val, "%s:%s", user, pass);
+	len = snprintf(val, sizeof(val), "%s:%s", user, pass);
+	if (len >= (int)sizeof(val))
+		return -1;
 
 	return add_key(CIFS_KEY_TYPE, desc, val, len + 1, DEST_KEYRING);
 }

I think we probably need to clean up the error handling in the callers of these functions, but that's a separate problem. I'll go ahead and merge this, and we can do that in a separate patch.

Signature · 1 line
Jeff Layton <[email protected]>
#3 lore raw
In reply to a message not in the archive
Quoted 35 lines · On Mon, 14 Apr 2014 11:39:41 +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.

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

---

cifskey.c renamed from cifskey.c.orig +4 -3
@@ -29,7 +29,8 @@
 {
 	char desc[INET6_ADDRSTRLEN + sizeof(KEY_PREFIX) + 4];
 
-	sprintf(desc, "%s:%c:%s", KEY_PREFIX, keytype, addr);
+	if (snprintf(desc, sizeof(desc), "%s:%c:%s", KEY_PREFIX, keytype, addr) >= (int)sizeof(desc))
+		return -1;
 
 	return keyctl_search(DEST_KEYRING, CIFS_KEY_TYPE, desc, 0);
 }

@@ -38,15 +39,18 @@
 key_serial_t
 key_add(const char *addr, const char *user, const char *pass, char keytype)
 {
-	int len;
+	int len = 0;
 	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);
+	if (snprintf(desc, sizeof(desc), "%s:%c:%s", KEY_PREFIX, keytype, addr) >= (int)sizeof(desc))

cifskey.c line 56

Quoted 10 lines
cifskey.c renamed from cifskey.c.orig +4 -1
+		return -1;
 
 	/* set payload contents */
-	len = sprintf(val, "%s:%s", user, pass);
+	len = snprintf(val, sizeof(val), "%s:%s", user, pass);
+	if (len >= (int)sizeof(val))
+		return -1;
 
 	return add_key(CIFS_KEY_TYPE, desc, val, len + 1, DEST_KEYRING);
 }

Slightly modified version merged. Thanks for the patch!

Signature · 1 line
Jeff Layton <[email protected]>