Segmentation fault in iproute2 ss -p (versions 4.0.0, 4.1.0 and 4.1.1)

-------- Forwarded Message --------
Subject: Re: Segmentation fault in iproute2 ss -p (versions 4.0.0, 4.1.0
and 4.1.1)
Date: Sun, 19 Jul 2015 14:05:48 -0700
From: Stephen Hemminger <stephen@networkplumber.org>
To: <j.ps@member.fsf.org>

Please send patches as plain text for review to netdev@vger.kernel.org
--- iproute2-4.1.1-orig/misc/ss.c	2015-07-06 22:57:34.000000000 +0100
+++ iproute2-4.1.1/misc/ss.c	2015-07-19 12:16:25.000000000 +0100
@@ -428,9 +428,12 @@
 	while (cnt != USER_ENT_HASH_SIZE) {
 		p = user_ent_hash[cnt];
 		while (p) {
-			free(p->process);
-			free(p->process_ctx);
-			free(p->socket_ctx);
+			if (p->process)
+				free(p->process);
+			if (p->process_ctx)
+				free(p->process_ctx);
+			if (p->socket_ctx)
+				free(p->socket_ctx);
 			p_next = p->next;
 			free(p);
 			p = p_next;
@@ -456,7 +459,21 @@
 
 	user_ent_hash_build_init = 1;
 
-	strcpy(name, root);
+	/* strcpy is really dangerous!  in this case if we're going to 
+	   copy an  unknown input  size from  getenv("PROC_ROOT") to a 
+	   fixed size buffer name[1024] we're going to get troubles. 
+	   If the size of a manipulated  "PROC_ROOT" > size of name we
+	   will have a buffer overrun smashing the stack,  overwriting 
+	   other local variables and/or return address, etc... */
+
+	memset(name, 0, sizeof(name));
+
+	strncpy(name, root, 512);
+	/* Chose this value ^^^ (maybe too large) just to avoid buffer 
+	   overflow  if sizeof getenv("PROC_ROOT") > sizeof(name)  and 
+	   to allow the  name composition that follows below to fit in 
+	   the remaining space. */
+
 	if (strlen(name) == 0 || name[strlen(name)-1] != '/')
 		strcat(name, "/");
 
@@ -480,7 +497,7 @@
 		if (getpidcon(pid, &pid_context) != 0)
 			pid_context = strdup(no_ctx);
 
-		sprintf(name + nameoff, "%d/fd/", pid);
+		snprintf(name + nameoff, sizeof(name) - nameoff, "%d/fd/", pid);
 		pos = strlen(name);
 		if ((dir1 = opendir(name)) == NULL)
 			continue;
@@ -499,7 +516,7 @@
 			if (sscanf(d1->d_name, "%d%c", &fd, &crap) != 1)
 				continue;
 
-			sprintf(name+pos, "%d", fd);
+			snprintf(name+pos, sizeof(name) - pos, "%d", fd);
 
 			link_len = readlink(name, lnk, sizeof(lnk)-1);
 			if (link_len == -1)
@@ -529,9 +546,16 @@
 			}
 			user_ent_add(ino, p, pid, fd,
 					pid_context, sock_context);
-			free(sock_context);
-		}
-		free(pid_context);
+			/* memory was  allocated  conditionally so
+			   freeing it should go the same way (even 
+			   though the actual  code implies that in
+			   this case it will always be allocated). */
+			if (sock_context)
+				free(sock_context);
+		}
+		/* same as above */
+		if (pid_context)
+			free(pid_context);
 		closedir(dir1);
 	}
 	closedir(dir);
@@ -2722,6 +2746,13 @@
 		if (!(u = malloc(sizeof(*u))))
 			break;
 
+		/* The following memset of 'u' struct is crucial to avoid a segfault
+		   when freeing memory 'free(name)' at 'unix_list_free()'.  In fact, 
+		   without this  'initialization', testing 'if (name)'  at line 2513 
+		   will most likely return true even if 'name' isn't allocated. */
+
+		memset(u, 0, sizeof(*u));
+
 		if (sscanf(buf, "%x: %x %x %x %x %x %d %s",
 			   &u->rport, &u->rq, &u->wq, &flags, &u->type,
 			   &u->state, &u->ino, name) < 8)
@@ -3064,11 +3095,12 @@
 			strncpy(procname, "kernel", 6);
 		} else if (pid > 0) {
 			FILE *fp;
-			sprintf(procname, "%s/%d/stat",
+			snprintf(procname, sizeof(procname), "%s/%d/stat",
 				getenv("PROC_ROOT") ? : "/proc", pid);
 			if ((fp = fopen(procname, "r")) != NULL) {
 				if (fscanf(fp, "%*d (%[^)])", procname) == 1) {
-					sprintf(procname+strlen(procname), "/%d", pid);
+					snprintf(procname+strlen(procname), 
+						sizeof(procname)-strlen(procname), "/%d", pid);
 					done = 1;
 				}
 				fclose(fp);

Patches are always appreciated and this looks like a real bug.
But before I can accept it there are a couple of small
changes needed.

1. There is no need to check for NULL when calling free().
   Glibc free is documented to accept NULL as a valid request
   and do nothing.

2. Please add a Signed-off-by: line with a real name.
   Signed-off-by has legal meaning for the Developer's Certificate of Origin
   see kernel documentation if you need more explaination.

3. Although what you found is important, giving a full paragraph
   of personal comment about it is not required. The point is software
   should read like one source independent of who the authors are.
   Your comment is basically just justifying using strncpy.

4. Rather than strncpy() which has issues with maximal sized strings
   consider using strlcpy() instead.

5. Iproute2 uses kernel identation and style, consider running checkpatch
   on your changes.

Please fixup and resubmit to netdev.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html