Re: Segmentation fault in iproute2 ss -p (versions 4.0.0, 4.1.0 and 4.1.1)
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...@openmailbox.org
--- iproute2-4.1.1/misc/ss.c.orig 2015-07-06 22:57:34.0 +0100
+++ iproute2-4.1.1/misc/ss.c2015-07-21 10:26:45.0 +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.
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Segmentation fault in iproute2 ss -p (versions 4.0.0, 4.1.0 and 4.1.1)
--- iproute2-4.1.1-orig/misc/ss.c 2015-07-06 22:57:34.0 +0100
+++ iproute2-4.1.1/misc/ss.c2015-07-19 12:16:25.0 +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
Re: Segmentation fault in iproute2 ss -p (versions 4.0.0, 4.1.0 and 4.1.1)
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
