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

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

[j.ps]

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

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

[Stephen Hemminger]

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.

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

[j.ps]

Patch for 4.1.1.

Essentially all that is needed to get rid of this issue is the
addition of:

    memset(u, 0, sizeof(*u));

after:

    if (!(u = malloc(sizeof(*u))))
            break;

Also patched some other situations (strcpy and sprintf uses) that
potentially produce the same results.

Note: As far as I know strlcpy isn't (yet) available in glibc.

Signed-off-by: Jose P Santos <j.ps@openmailbox.org>

--- iproute2-4.1.1/misc/ss.c.orig	2015-07-06 22:57:34.000000000 +0100
+++ iproute2-4.1.1/misc/ss.c	2015-07-21 10:26:45.000000000 +0100
@@ -456,7 +456,10 @@ static void user_ent_hash_build(void)

 	user_ent_hash_build_init = 1;

-	strcpy(name, root);
+	/* Avoid buffer overrun if input size from PROC_ROOT > name */
+	memset(name, 0, sizeof(name));
+	strncpy(name, root, sizeof(name)-2);
+
 	if (strlen(name) == 0 || name[strlen(name)-1] != '/')
 		strcat(name, "/");

@@ -480,7 +483,7 @@ static void user_ent_hash_build(void)
 		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 +502,7 @@ static void user_ent_hash_build(void)
 			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)
@@ -2722,6 +2725,11 @@ static int unix_show(struct filter *f)
 		if (!(u = malloc(sizeof(*u))))
 			break;

+		/* Zero initialization of 'u' struct avoids a segfault
+		 * when freeing memory 'free(name)' at 'unix_list_free()'.
+		 */
+		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 +3072,13 @@ static int netlink_show_one(struct filte
 			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);



On 2015-07-20 20:09, Stephen Hemminger wrote:
> 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.
> 
> 

Re: "ss -p" segfaults

[j.ps]

Patch for 4.1.1.

Essentially all that is needed to get rid of this issue is the
addition of:

    memset(u, 0, sizeof(*u));

after:

    if (!(u = malloc(sizeof(*u))))
            break;

Also patched some other situations (strcpy and sprintf uses) that
potentially produce the same results.

Signed-off-by: Jose P Santos <j.ps@openmailbox.org>

--- iproute2-4.1.1/misc/ss.c.orig	2015-07-06 22:57:34.000000000 +0100
+++ iproute2-4.1.1/misc/ss.c	2015-07-21 10:26:45.000000000 +0100
@@ -456,7 +456,10 @@ static void user_ent_hash_build(void)

 	user_ent_hash_build_init = 1;

-	strcpy(name, root);
+	/* Avoid buffer overrun if input size from PROC_ROOT > name */
+	memset(name, 0, sizeof(name));
+	strncpy(name, root, sizeof(name)-2);
+
 	if (strlen(name) == 0 || name[strlen(name)-1] != '/')
 		strcat(name, "/");

@@ -480,7 +483,7 @@ static void user_ent_hash_build(void)
 		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 +502,7 @@ static void user_ent_hash_build(void)
 			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)
@@ -2722,6 +2725,11 @@ static int unix_show(struct filter *f)
 		if (!(u = malloc(sizeof(*u))))
 			break;

+		/* Zero initialization of 'u' struct avoids a segfault
+		 * when freeing memory 'free(name)' at 'unix_list_free()'.
+		 */
+		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 +3072,13 @@ static int netlink_show_one(struct filte
 			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);



On 2015-07-20 20:09, Stephen Hemminger wrote:
> 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.
> 
> 

Re: "ss -p" segfaults (updated to 4.2)

[Willy Tarreau]

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

Hi guys,

I've updated Jose's patch to make it slightly simpler (eg: calloc instead
of malloc+memset), and ported it to 4.2.0 which requires it as well, and
attached it to this e-mail.

I can confirm that with this patch 4.1.1 doesn't segfault on me anymore.
The commit message should be reworked I guess though everything's in it
and I didn't want to modify his description.

Can it be merged as-is or should I reword the commit message and reference
Jose as the fix reporter ? We should not let this bug live forever.

Thanks,
Willy


[-- Attachment #2: 0001-ss-p-segfaults.patch --]
[-- Type: text/plain, Size: 2553 bytes --]

>From 618028d6c5bfa4fb9898f2ec1ab5483e6f5392d4 Mon Sep 17 00:00:00 2001
From: "j.ps@openmailbox.org" <j.ps@openmailbox.org>
Date: Tue, 21 Jul 2015 10:54:05 +0100
Subject: "ss -p" segfaults

Patch for 4.2.0

Essentially all that is needed to get rid of this issue is the
addition of:

    memset(u, 0, sizeof(*u));

after:

    if (!(u = malloc(sizeof(*u))))
            break;

Also patched some other situations (strcpy and sprintf uses) that
potentially produce the same results.

Signed-off-by: Jose P Santos <j.ps@openmailbox.org>

[ wt: made Jose's patch slightly simpler, all credits to him for the diag ]
Signed-off-by: Willy Tarreau <w@1wt.eu>
---
 misc/ss.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/misc/ss.c b/misc/ss.c
index 2f34962..8b0d606 100644
--- a/misc/ss.c
+++ b/misc/ss.c
@@ -457,7 +457,9 @@ static void user_ent_hash_build(void)
 
 	user_ent_hash_build_init = 1;
 
-	strcpy(name, root);
+	strncpy(name, root, sizeof(name)-1);
+	name[sizeof(name)-1] = 0;
+
 	if (strlen(name) == 0 || name[strlen(name)-1] != '/')
 		strcat(name, "/");
 
@@ -481,7 +483,7 @@ static void user_ent_hash_build(void)
 		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) {
 			free(pid_context);
@@ -502,7 +504,7 @@ static void user_ent_hash_build(void)
 			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)
@@ -2736,7 +2738,7 @@ static int unix_show(struct filter *f)
 		struct sockstat *u, **insp;
 		int flags;
 
-		if (!(u = malloc(sizeof(*u))))
+		if (!(u = calloc(1, sizeof(*u))))
 			break;
 		u->name = NULL;
 		u->peer_name = NULL;
@@ -3086,11 +3088,13 @@ static int netlink_show_one(struct filter *f,
 			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);
-- 
1.7.12.1

Re: "ss -p" segfaults (updated to 4.2)

[j.ps]

Hi Willy,

It seems fine to me.
You may reword the commit message at will.

Thx,
Jose

On 2015-10-06 11:09, Willy Tarreau wrote:
> Hi guys,
> 
> I've updated Jose's patch to make it slightly simpler (eg: calloc instead
> of malloc+memset), and ported it to 4.2.0 which requires it as well, and
> attached it to this e-mail.
> 
> I can confirm that with this patch 4.1.1 doesn't segfault on me anymore.
> The commit message should be reworked I guess though everything's in it
> and I didn't want to modify his description.
> 
> Can it be merged as-is or should I reword the commit message and reference
> Jose as the fix reporter ? We should not let this bug live forever.
> 
> Thanks,
> Willy
> 

Re: "ss -p" segfaults (updated to 4.2)

[Stephen Hemminger]

Applied, and did some editing on commit msg

Re: "ss -p" segfaults (updated to 4.2)

[Willy Tarreau]

On Mon, Oct 12, 2015 at 09:50:19AM -0700, Stephen Hemminger wrote:
> Applied, and did some editing on commit msg

Thank you Stephen!

Willy