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

["j.ps@openmailbox.org" <j.ps@openmailbox.org>]

[-- Attachment #1: Type: text/plain, Size: 3855 bytes --]




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


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




[-- Attachment #2: iproute2-4.1.1-patch --]
[-- Type: text/plain, Size: 3540 bytes --]

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