[OpenWrt-Devel] [PATCH v2 1/1] firewall3: harden string functions that might overflow
Philip Prindeville
philipp at redfish-solutions.com
Wed May 13 16:14:40 EDT 2020
From: Philip Prindeville <philipp at redfish-solutions.com>
Make sure no buffer overruns present a vulnerability in the firewall.
Get rid of unsafe string functions: strcpy, strncpy, strcat, strncat,
sprintf, etc. Doing pointer arithemetic with the return value of
sprintf() is inherently unsound. Per the sprintf() man page:
The functions snprintf() and vsnprintf() do not write more than size
bytes (including the terminating null byte ('\0')). If the output was
truncated due to this limit, then the return value is the number of
characters (excluding the terminating null byte) which would have been
written to the final string if enough space had been available. Thus,
a return value of size or more means that the output was truncated.
Thus the construct:
p += sprintf(p, ...);
is unsafe as the return value could put you well beyond the end of
whatever buffer p points to.
Signed-off-by: Philip Prindeville <philipp at redfish-solutions.com>
---
defaults.c | 2 +-
iptables.c | 65 ++++++++++++++++++++++++++++++----------------------
options.c | 24 +++++++++++--------
redirects.c | 16 ++++++++-----
rules.c | 4 ++--
snats.c | 16 ++++++++-----
utils.c | 17 +++++++-------
xtables-10.h | 8 +++----
xtables-5.h | 4 ++--
9 files changed, 89 insertions(+), 67 deletions(-)
diff --git a/defaults.c b/defaults.c
index 60a4c81f773bf9527407ac61b0731e940f9c5463..bd69e6b2f765058755daabb3dde8f89f19238622 100644
--- a/defaults.c
+++ b/defaults.c
@@ -236,7 +236,7 @@ fw3_print_default_head_rules(struct fw3_ipt_handle *handle,
{
case FW3_TABLE_FILTER:
- sprintf(lodev.name, "lo");
+ strlcpy(lodev.name, "lo", sizeof(lodev.name));
r = fw3_ipt_rule_create(handle, NULL, &lodev, NULL, NULL, NULL);
fw3_ipt_rule_target(r, "ACCEPT");
diff --git a/iptables.c b/iptables.c
index 559fe7defef3be85c4eb2934884caf549f932bc5..dcf8e23fbd9845e6be4a8bc072f70d9238ebab51 100644
--- a/iptables.c
+++ b/iptables.c
@@ -144,7 +144,7 @@ get_kernel_version(void)
int x = 0, y = 0, z = 0;
if (uname(&uts) == -1)
- sprintf(uts.release, "3.0.0");
+ strlcpy(uts.release, "3.0.0", sizeof(uts.release));
sscanf(uts.release, "%d.%d.%d", &x, &y, &z);
kernel_version = 0x10000 * x + 0x100 * y + z;
@@ -906,7 +906,7 @@ fw3_ipt_rule_sport_dport(struct fw3_ipt_rule *r,
if (sp && sp->set)
{
if (sp->port_min == sp->port_max)
- sprintf(buf, "%u", sp->port_min);
+ snprintf(buf, sizeof(buf), "%u", sp->port_min);
else
snprintf(buf, sizeof(buf), "%u:%u", sp->port_min, sp->port_max);
@@ -916,7 +916,7 @@ fw3_ipt_rule_sport_dport(struct fw3_ipt_rule *r,
if (dp && dp->set)
{
if (dp->port_min == dp->port_max)
- sprintf(buf, "%u", dp->port_min);
+ snprintf(buf, sizeof(buf), "%u", dp->port_min);
else
snprintf(buf, sizeof(buf), "%u:%u", dp->port_min, dp->port_max);
@@ -929,7 +929,8 @@ fw3_ipt_rule_device(struct fw3_ipt_rule *r, const char *device, bool out)
{
if (device) {
struct fw3_device dev = { .any = false };
- strncpy(dev.name, device, sizeof(dev.name) - 1);
+
+ strlcpy(dev.name, device, sizeof(dev.name));
fw3_ipt_rule_in_out(r, (out) ? NULL : &dev, (out) ? &dev : NULL);
}
}
@@ -943,7 +944,7 @@ fw3_ipt_rule_mac(struct fw3_ipt_rule *r, struct fw3_mac *mac)
if (!mac)
return;
- sprintf(buf, "%02x:%02x:%02x:%02x:%02x:%02x",
+ snprintf(buf, sizeof(buf), "%02x:%02x:%02x:%02x:%02x:%02x",
addr[0], addr[1], addr[2], addr[3], addr[4], addr[5]);
fw3_ipt_rule_addarg(r, false, "-m", "mac");
@@ -962,7 +963,7 @@ fw3_ipt_rule_icmptype(struct fw3_ipt_rule *r, struct fw3_icmptype *icmp)
if (r->h->family == FW3_FAMILY_V6)
{
if (icmp->code6_min == 0 && icmp->code6_max == 0xFF)
- sprintf(buf, "%u", icmp->type6);
+ snprintf(buf, sizeof(buf), "%u", icmp->type6);
else
snprintf(buf, sizeof(buf), "%u/%u", icmp->type6, icmp->code6_min);
@@ -972,7 +973,7 @@ fw3_ipt_rule_icmptype(struct fw3_ipt_rule *r, struct fw3_icmptype *icmp)
#endif
{
if (icmp->code_min == 0 && icmp->code_max == 0xFF)
- sprintf(buf, "%u", icmp->type);
+ snprintf(buf, sizeof(buf), "%u", icmp->type);
else
snprintf(buf, sizeof(buf), "%u/%u", icmp->type, icmp->code_min);
@@ -990,12 +991,12 @@ fw3_ipt_rule_limit(struct fw3_ipt_rule *r, struct fw3_limit *limit)
fw3_ipt_rule_addarg(r, false, "-m", "limit");
- sprintf(buf, "%u/%s", limit->rate, fw3_limit_units[limit->unit]);
+ snprintf(buf, sizeof(buf), "%u/%s", limit->rate, fw3_limit_units[limit->unit]);
fw3_ipt_rule_addarg(r, limit->invert, "--limit", buf);
if (limit->burst > 0)
{
- sprintf(buf, "%u", limit->burst);
+ snprintf(buf, sizeof(buf), "%u", limit->burst);
fw3_ipt_rule_addarg(r, limit->invert, "--limit-burst", buf);
}
}
@@ -1004,8 +1005,8 @@ void
fw3_ipt_rule_ipset(struct fw3_ipt_rule *r, struct fw3_setmatch *match)
{
char buf[sizeof("dst,dst,dst\0")];
- char *p = buf;
int i = 0;
+ FILE *fp = NULL;
struct fw3_ipset *set;
struct fw3_ipset_datatype *type;
@@ -1013,18 +1014,21 @@ fw3_ipt_rule_ipset(struct fw3_ipt_rule *r, struct fw3_setmatch *match)
if (!match || !match->set || !match->ptr)
return;
+ fp = fmemopen(buf, sizeof(buf), "w");
set = match->ptr;
list_for_each_entry(type, &set->datatypes, list)
{
if (i >= 3)
break;
- if (p > buf)
- *p++ = ',';
+ if (ftell(fp) != 0)
+ fputc(',', fp);
+
+ fputs(match->dir[i] ? match->dir[i] : type->dir, fp);
- p += sprintf(p, "%s", match->dir[i] ? match->dir[i] : type->dir);
i++;
}
+ fclose(fp);
fw3_ipt_rule_addarg(r, false, "-m", "set");
@@ -1051,7 +1055,6 @@ fw3_ipt_rule_time(struct fw3_ipt_rule *r, struct fw3_time *time)
struct tm empty = { 0 };
char buf[84]; /* sizeof("1,2,3,...,30,31\0") */
- char *p;
bool d1 = memcmp(&time->datestart, &empty, sizeof(empty));
bool d2 = memcmp(&time->datestop, &empty, sizeof(empty));
@@ -1081,7 +1084,7 @@ fw3_ipt_rule_time(struct fw3_ipt_rule *r, struct fw3_time *time)
if (time->timestart)
{
- sprintf(buf, "%02d:%02d:%02d",
+ snprintf(buf, sizeof(buf), "%02d:%02d:%02d",
time->timestart / 3600,
time->timestart % 3600 / 60,
time->timestart % 60);
@@ -1091,7 +1094,7 @@ fw3_ipt_rule_time(struct fw3_ipt_rule *r, struct fw3_time *time)
if (time->timestop)
{
- sprintf(buf, "%02d:%02d:%02d",
+ snprintf(buf, sizeof(buf), "%02d:%02d:%02d",
time->timestop / 3600,
time->timestop % 3600 / 60,
time->timestop % 60);
@@ -1101,33 +1104,41 @@ fw3_ipt_rule_time(struct fw3_ipt_rule *r, struct fw3_time *time)
if (time->monthdays & 0xFFFFFFFE)
{
- for (i = 1, p = buf; i < 32; i++)
+ FILE *fp = fmemopen(buf, sizeof(buf), "w");
+
+ for (i = 1; i < 32; i++)
{
if (fw3_hasbit(time->monthdays, i))
{
- if (p > buf)
- *p++ = ',';
+ if (ftell(fp) != 0)
+ fputc(',', fp);
- p += sprintf(p, "%u", i);
+ fprintf(fp, "%u", i);
}
}
+ fclose(fp);
+
fw3_ipt_rule_addarg(r, fw3_hasbit(time->monthdays, 0), "--monthdays", buf);
}
if (time->weekdays & 0xFE)
{
- for (i = 1, p = buf; i < 8; i++)
+ FILE *fp = fmemopen(buf, sizeof(buf), "w");
+
+ for (i = 1; i < 8; i++)
{
if (fw3_hasbit(time->weekdays, i))
{
- if (p > buf)
- *p++ = ',';
+ if (ftell(fp) != 0)
+ fputc(',', fp);
- p += sprintf(p, "%u", i);
+ fprintf(fp, "%u", i);
}
}
+ fclose(fp);
+
fw3_ipt_rule_addarg(r, fw3_hasbit(time->weekdays, 0), "--weekdays", buf);
}
}
@@ -1141,9 +1152,9 @@ fw3_ipt_rule_mark(struct fw3_ipt_rule *r, struct fw3_mark *mark)
return;
if (mark->mask < 0xFFFFFFFF)
- sprintf(buf, "0x%x/0x%x", mark->mark, mark->mask);
+ snprintf(buf, sizeof(buf), "0x%x/0x%x", mark->mark, mark->mask);
else
- sprintf(buf, "0x%x", mark->mark);
+ snprintf(buf, sizeof(buf), "0x%x", mark->mark);
fw3_ipt_rule_addarg(r, false, "-m", "mark");
fw3_ipt_rule_addarg(r, mark->invert, "--mark", buf);
@@ -1157,7 +1168,7 @@ fw3_ipt_rule_dscp(struct fw3_ipt_rule *r, struct fw3_dscp *dscp)
if (!dscp || !dscp->set)
return;
- sprintf(buf, "0x%x", dscp->dscp);
+ snprintf(buf, sizeof(buf), "0x%x", dscp->dscp);
fw3_ipt_rule_addarg(r, false, "-m", "dscp");
fw3_ipt_rule_addarg(r, dscp->invert, "--dscp", buf);
diff --git a/options.c b/options.c
index 7870143b03f55297466a6540efd3730d38773f49..6da06ef5a7be9bdde0412ca0398f65478d45a1cf 100644
--- a/options.c
+++ b/options.c
@@ -939,7 +939,7 @@ fw3_parse_setmatch(void *ptr, const char *val, bool is_list)
return false;
}
- strncpy(m->name, p, sizeof(m->name) - 1);
+ strlcpy(m->name, p, sizeof(m->name));
for (i = 0, p = strtok(NULL, " \t,");
i < 3 && p != NULL;
@@ -987,7 +987,7 @@ fw3_parse_cthelper(void *ptr, const char *val, bool is_list)
if (*val)
{
m.set = true;
- strncpy(m.name, val, sizeof(m.name) - 1);
+ strlcpy(m.name, val, sizeof(m.name));
put_value(ptr, &m, sizeof(m), is_list);
return true;
}
@@ -1237,38 +1237,42 @@ fw3_parse_blob_options(void *s, const struct fw3_option *opts,
const char *
fw3_address_to_string(struct fw3_address *address, bool allow_invert, bool as_cidr)
{
- char *p, ip[INET6_ADDRSTRLEN];
+ char ip[INET6_ADDRSTRLEN];
static char buf[INET6_ADDRSTRLEN * 2 + 2];
- p = buf;
+ FILE *fp = fmemopen(buf, sizeof(buf), "w");
if (address->invert && allow_invert)
- p += sprintf(p, "!");
+ fputc('!', fp);
inet_ntop(address->family == FW3_FAMILY_V4 ? AF_INET : AF_INET6,
&address->address.v4, ip, sizeof(ip));
- p += sprintf(p, "%s", ip);
+ fputs(ip, fp);
if (address->range)
{
inet_ntop(address->family == FW3_FAMILY_V4 ? AF_INET : AF_INET6,
&address->mask.v4, ip, sizeof(ip));
- p += sprintf(p, "-%s", ip);
+ fputc('-', fp);
+ fputs(ip, fp);
}
else if (!as_cidr)
{
inet_ntop(address->family == FW3_FAMILY_V4 ? AF_INET : AF_INET6,
&address->mask.v4, ip, sizeof(ip));
- p += sprintf(p, "/%s", ip);
+ fputc('/', fp);
+ fputs(ip, fp);
}
else
{
- p += sprintf(p, "/%u", fw3_netmask2bitlen(address->family,
- &address->mask.v6));
+ fprintf(fp, "/%u", fw3_netmask2bitlen(address->family,
+ &address->mask.v6));
}
+ fclose(fp);
+
return buf;
}
diff --git a/redirects.c b/redirects.c
index b928287deebb0f8407aa5ecbc4dab75bdda05c66..ab6dbdfec8a995920de669414359bfa2c731c868 100644
--- a/redirects.c
+++ b/redirects.c
@@ -155,7 +155,7 @@ resolve_dest(struct uci_element *e, struct fw3_redirect *redir,
if (!compare_addr(addr, &redir->ip_redir))
continue;
- strncpy(redir->dest.name, zone->name, sizeof(redir->dest.name) - 1);
+ strlcpy(redir->dest.name, zone->name, sizeof(redir->dest.name));
redir->dest.set = true;
redir->_dest = zone;
@@ -465,7 +465,7 @@ set_redirect(struct fw3_ipt_rule *r, struct fw3_port *port)
if (port && port->set)
{
if (port->port_min == port->port_max)
- sprintf(buf, "%u", port->port_min);
+ snprintf(buf, sizeof(buf), "%u", port->port_min);
else
snprintf(buf, sizeof(buf), "%u-%u", port->port_min, port->port_max);
@@ -477,24 +477,28 @@ static void
set_snat_dnat(struct fw3_ipt_rule *r, enum fw3_flag target,
struct fw3_address *addr, struct fw3_port *port)
{
+ char ip[INET_ADDRSTRLEN];
char buf[sizeof("255.255.255.255:65535-65535\0")];
- buf[0] = '\0';
+ FILE *fp = fmemopen(buf, sizeof(buf), "w");
if (addr && addr->set)
{
- inet_ntop(AF_INET, &addr->address.v4, buf, sizeof(buf));
+ inet_ntop(AF_INET, &addr->address.v4, ip, sizeof(ip));
+ fputs(ip, fp);
}
if (port && port->set)
{
if (port->port_min == port->port_max)
- sprintf(buf + strlen(buf), ":%u", port->port_min);
+ fprintf(fp, ":%u", port->port_min);
else
- sprintf(buf + strlen(buf), ":%u-%u",
+ fprintf(fp, ":%u-%u",
port->port_min, port->port_max);
}
+ fclose(fp);
+
if (target == FW3_FLAG_DNAT)
{
fw3_ipt_rule_target(r, "DNAT");
diff --git a/rules.c b/rules.c
index 5230a8623fa897f1111cb7f7e7012f06c4acb503..ed2efadc2e9d6a15898fb6b222b96278f7c3be81 100644
--- a/rules.c
+++ b/rules.c
@@ -360,14 +360,14 @@ static void set_target(struct fw3_ipt_rule *r, struct fw3_rule *rule)
case FW3_FLAG_MARK:
name = rule->set_mark.set ? "--set-mark" : "--set-xmark";
mark = rule->set_mark.set ? &rule->set_mark : &rule->set_xmark;
- sprintf(buf, "0x%x/0x%x", mark->mark, mark->mask);
+ snprintf(buf, sizeof(buf), "0x%x/0x%x", mark->mark, mark->mask);
fw3_ipt_rule_target(r, "MARK");
fw3_ipt_rule_addarg(r, false, name, buf);
return;
case FW3_FLAG_DSCP:
- sprintf(buf, "0x%x", rule->set_dscp.dscp);
+ snprintf(buf, sizeof(buf), "0x%x", rule->set_dscp.dscp);
fw3_ipt_rule_target(r, "DSCP");
fw3_ipt_rule_addarg(r, false, "--set-dscp", buf);
diff --git a/snats.c b/snats.c
index 1d78f93f30662f475d24bd5817a03c1b106b02ec..6449ca877284b9092d84f773f10339bf96e77abb 100644
--- a/snats.c
+++ b/snats.c
@@ -265,24 +265,26 @@ static void
set_target(struct fw3_ipt_rule *r, struct fw3_snat *snat,
struct fw3_protocol *proto)
{
- char buf[sizeof("255.255.255.255:65535-65535\0")];
-
if (snat->target == FW3_FLAG_SNAT)
{
- buf[0] = '\0';
+ char ip[INET_ADDRSTRLEN];
+ char buf[sizeof("255.255.255.255:65535-65535\0")];
+
+ FILE *fp = fmemopen(buf, sizeof(buf), "w");
if (snat->ip_snat.set)
{
- inet_ntop(AF_INET, &snat->ip_snat.address.v4, buf, sizeof(buf));
+ inet_ntop(AF_INET, &snat->ip_snat.address.v4, ip, sizeof(ip));
+ fputs(ip, fp);
}
if (snat->port_snat.set && proto && !proto->any &&
(proto->protocol == 6 || proto->protocol == 17 || proto->protocol == 1))
{
if (snat->port_snat.port_min == snat->port_snat.port_max)
- sprintf(buf + strlen(buf), ":%u", snat->port_snat.port_min);
+ fprintf(fp, ":%u", snat->port_snat.port_min);
else
- sprintf(buf + strlen(buf), ":%u-%u",
+ fprintf(fp, ":%u-%u",
snat->port_snat.port_min, snat->port_snat.port_max);
if (snat->connlimit_ports) {
@@ -296,6 +298,8 @@ set_target(struct fw3_ipt_rule *r, struct fw3_snat *snat,
}
}
+ fclose(fp);
+
fw3_ipt_rule_target(r, "SNAT");
fw3_ipt_rule_addarg(r, false, "--to-source", buf);
}
diff --git a/utils.c b/utils.c
index da6563243c0673f8393c16f65116aeb88ba0ea6c..ddc0f39af1e2a5cfe6904cf97dc17f76f9a233ef 100644
--- a/utils.c
+++ b/utils.c
@@ -191,8 +191,7 @@ fw3_find_command(const char *cmd)
if ((plen + clen) >= sizeof(path))
continue;
- strncpy(path, search, plen);
- sprintf(path + plen, "/%s", cmd);
+ snprintf(path, sizeof(path), "%.*s/%s", plen, search, cmd);
if (!stat(path, &s) && S_ISREG(s.st_mode))
return path;
@@ -449,13 +448,13 @@ write_defaults_uci(struct uci_context *ctx, struct fw3_defaults *d,
ptr.value = fw3_flag_names[d->policy_forward];
uci_set(ctx, &ptr);
- sprintf(buf, "0x%x", d->flags[0]);
+ snprintf(buf, sizeof(buf), "0x%x", d->flags[0]);
ptr.o = NULL;
ptr.option = "__flags_v4";
ptr.value = buf;
uci_set(ctx, &ptr);
- sprintf(buf, "0x%x", d->flags[1]);
+ snprintf(buf, sizeof(buf), "0x%x", d->flags[1]);
ptr.o = NULL;
ptr.option = "__flags_v6";
ptr.value = buf;
@@ -612,13 +611,13 @@ write_zone_uci(struct uci_context *ctx, struct fw3_zone *z,
uci_set(ctx, &ptr);
}
- sprintf(buf, "0x%x", z->flags[0]);
+ snprintf(buf, sizeof(buf), "0x%x", z->flags[0]);
ptr.o = NULL;
ptr.option = "__flags_v4";
ptr.value = buf;
uci_set(ctx, &ptr);
- sprintf(buf, "0x%x", z->flags[1]);
+ snprintf(buf, sizeof(buf), "0x%x", z->flags[1]);
ptr.o = NULL;
ptr.option = "__flags_v6";
ptr.value = buf;
@@ -660,7 +659,7 @@ write_ipset_uci(struct uci_context *ctx, struct fw3_ipset *s,
list_for_each_entry(type, &s->datatypes, list)
{
- sprintf(buf, "%s_%s", type->dir, fw3_ipset_type_names[type->type]);
+ snprintf(buf, sizeof(buf), "%s_%s", type->dir, fw3_ipset_type_names[type->type]);
ptr.o = NULL;
ptr.option = "match";
ptr.value = buf;
@@ -677,7 +676,7 @@ write_ipset_uci(struct uci_context *ctx, struct fw3_ipset *s,
if (s->portrange.set)
{
- sprintf(buf, "%u-%u", s->portrange.port_min, s->portrange.port_max);
+ snprintf(buf, sizeof(buf), "%u-%u", s->portrange.port_min, s->portrange.port_max);
ptr.o = NULL;
ptr.option = "portrange";
ptr.value = buf;
@@ -1021,7 +1020,7 @@ fw3_check_loopback_dev(const char *name)
return false;
memset(&ifr, 0, sizeof(ifr));
- strncpy(ifr.ifr_name, name, sizeof(ifr.ifr_name) - 1);
+ strlcpy(ifr.ifr_name, name, sizeof(ifr.ifr_name));
if (ioctl(s, SIOCGIFFLAGS, &ifr) >= 0) {
if (ifr.ifr_flags & IFF_LOOPBACK)
diff --git a/xtables-10.h b/xtables-10.h
index 7ea5315a45a5551f150e60d63e9bc6b92caf6830..c2c9751e443fc0145acddba4b5c5e66b2933b5cc 100644
--- a/xtables-10.h
+++ b/xtables-10.h
@@ -46,9 +46,9 @@ static inline void
fw3_xt_set_match_name(struct xtables_match *m)
{
if (m->real_name)
- strcpy(m->m->u.user.name, m->real_name);
+ strlcpy(m->m->u.user.name, m->real_name, sizeof(m->m->u.user.name));
else
- strcpy(m->m->u.user.name, m->name);
+ strlcpy(m->m->u.user.name, m->name, sizeof(m->m->u.user.name));
}
static inline bool
@@ -93,9 +93,9 @@ static inline void
fw3_xt_set_target_name(struct xtables_target *t, const char *name)
{
if (t->real_name)
- strcpy(t->t->u.user.name, t->real_name);
+ strlcpy(t->t->u.user.name, t->real_name, sizeof(t->t->u.user.name));
else
- strcpy(t->t->u.user.name, name);
+ strlcpy(t->t->u.user.name, name, sizeof(t->t->u.user.name));
}
static inline bool
diff --git a/xtables-5.h b/xtables-5.h
index 9d11caeccdf3a0fa808a8f8fb0bd31b3e9bfdc7c..fab48ddd402951572d5d4097011ac68e4e2e03af 100644
--- a/xtables-5.h
+++ b/xtables-5.h
@@ -36,7 +36,7 @@ fw3_xt_get_match_name(struct xtables_match *m)
static inline void
fw3_xt_set_match_name(struct xtables_match *m)
{
- strcpy(m->m->u.user.name, m->name);
+ strlcpy(m->m->u.user.name, m->name, sizeof(m->m->u.user.name));
}
static inline bool
@@ -67,7 +67,7 @@ fw3_xt_get_target_name(struct xtables_target *t)
static inline void
fw3_xt_set_target_name(struct xtables_target *t, const char *name)
{
- strcpy(t->t->u.user.name, name);
+ strlcpy(t->t->u.user.name, name, sizeof(t->t->u.user.name));
}
static inline bool
--
2.17.2
_______________________________________________
openwrt-devel mailing list
openwrt-devel at lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel
More information about the openwrt-devel
mailing list