| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <55AE16BD.40607@openmailbox.org>
Date: Tue, 21 Jul 2015 10:54:05 +0100
From: "j.ps@...nmailbox.org" <j.ps@...nmailbox.org>
To: netdev@...r.kernel.org
Cc: schwab@...ux-m68k.org
Subject: Re: "ss -p" segfaults
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@...nmailbox.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.
>
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists