git: 64432ad2a2c4 - main - pf: Validate user string nul-termination before copying
Mark Johnston
markj at FreeBSD.org
Wed Jul 28 14:41:33 UTC 2021
The branch main has been updated by markj:
URL: https://cgit.FreeBSD.org/src/commit/?id=64432ad2a2c4b10d3d3411a8ca018e2a35cec97e
commit 64432ad2a2c4b10d3d3411a8ca018e2a35cec97e
Author: Mark Johnston <markj at FreeBSD.org>
AuthorDate: 2021-07-28 14:16:42 +0000
Commit: Mark Johnston <markj at FreeBSD.org>
CommitDate: 2021-07-28 14:41:01 +0000
pf: Validate user string nul-termination before copying
Some pf ioctl handlers use strlcpy() to copy strings when converting
from user structures to their in-kernel representations. strlcpy()
ensures that the destination will be nul-terminated, but it assumes that
the source is nul-terminated. In particular, it returns the full length
of the source string, so if the source is not nul-terminated, strlcpy()
will keep scanning until it finds a nul byte, and it may encounter an
unmapped page first. Add a helper to validate user strings before
copying.
There are also places where we look up a ruleset using a user-provided
anchor string. In some ioctl handlers we were already nul-terminating
the string, avoiding the same problem, but in other places we were not.
Fix those by nul-terminating as well. Aside from being consistent,
anchors have a maximum length of MAXPATHLEN - 1 so calling strnlen()
might not be so desirable.
Reported by: syzbot+35a1549b4663e9483dd1 at syzkaller.appspotmail.com
Reviewed by: kp
MFC after: 2 weeks
Sponsored by: The FreeBSD Foundation
Differential Revision: https://reviews.freebsd.org/D31169
---
sys/netpfil/pf/pf_ioctl.c | 123 ++++++++++++++++++++++++++++++++++------------
1 file changed, 92 insertions(+), 31 deletions(-)
diff --git a/sys/netpfil/pf/pf_ioctl.c b/sys/netpfil/pf/pf_ioctl.c
index 3dc52da05606..c1dd4488e67d 100644
--- a/sys/netpfil/pf/pf_ioctl.c
+++ b/sys/netpfil/pf/pf_ioctl.c
@@ -275,6 +275,20 @@ pflog_packet_t *pflog_packet_ptr = NULL;
extern u_long pf_ioctl_maxcount;
+/*
+ * Copy a user-provided string, returning an error if truncation would occur.
+ * Avoid scanning past "sz" bytes in the source string since there's no
+ * guarantee that it's nul-terminated.
+ */
+static int
+pf_user_strcpy(char *dst, const char *src, size_t sz)
+{
+ if (strnlen(src, sz) == sz)
+ return (EINVAL);
+ (void)strlcpy(dst, src, sz);
+ return (0);
+}
+
static void
pfattach_vnet(void)
{
@@ -1543,14 +1557,17 @@ pf_kpooladdr_to_pooladdr(const struct pf_kpooladdr *kpool,
strlcpy(pool->ifname, kpool->ifname, sizeof(pool->ifname));
}
-static void
+static int
pf_pooladdr_to_kpooladdr(const struct pf_pooladdr *pool,
struct pf_kpooladdr *kpool)
{
+ int ret;
bzero(kpool, sizeof(*kpool));
bcopy(&pool->addr, &kpool->addr, sizeof(kpool->addr));
- strlcpy(kpool->ifname, pool->ifname, sizeof(kpool->ifname));
+ ret = pf_user_strcpy(kpool->ifname, pool->ifname,
+ sizeof(kpool->ifname));
+ return (ret);
}
static void
@@ -1715,15 +1732,30 @@ pf_rule_to_krule(const struct pf_rule *rule, struct pf_krule *krule)
bcopy(&rule->src, &krule->src, sizeof(rule->src));
bcopy(&rule->dst, &krule->dst, sizeof(rule->dst));
- strlcpy(krule->label[0], rule->label, sizeof(rule->label));
- strlcpy(krule->ifname, rule->ifname, sizeof(rule->ifname));
- strlcpy(krule->qname, rule->qname, sizeof(rule->qname));
- strlcpy(krule->pqname, rule->pqname, sizeof(rule->pqname));
- strlcpy(krule->tagname, rule->tagname, sizeof(rule->tagname));
- strlcpy(krule->match_tagname, rule->match_tagname,
+ ret = pf_user_strcpy(krule->label[0], rule->label, sizeof(rule->label));
+ if (ret != 0)
+ return (ret);
+ ret = pf_user_strcpy(krule->ifname, rule->ifname, sizeof(rule->ifname));
+ if (ret != 0)
+ return (ret);
+ ret = pf_user_strcpy(krule->qname, rule->qname, sizeof(rule->qname));
+ if (ret != 0)
+ return (ret);
+ ret = pf_user_strcpy(krule->pqname, rule->pqname, sizeof(rule->pqname));
+ if (ret != 0)
+ return (ret);
+ ret = pf_user_strcpy(krule->tagname, rule->tagname,
+ sizeof(rule->tagname));
+ if (ret != 0)
+ return (ret);
+ ret = pf_user_strcpy(krule->match_tagname, rule->match_tagname,
sizeof(rule->match_tagname));
- strlcpy(krule->overload_tblname, rule->overload_tblname,
+ if (ret != 0)
+ return (ret);
+ ret = pf_user_strcpy(krule->overload_tblname, rule->overload_tblname,
sizeof(rule->overload_tblname));
+ if (ret != 0)
+ return (ret);
ret = pf_pool_to_kpool(&rule->rpool, &krule->rpool);
if (ret != 0)
@@ -1799,6 +1831,8 @@ static int
pf_state_kill_to_kstate_kill(const struct pfioc_state_kill *psk,
struct pf_kstate_kill *kill)
{
+ int ret;
+
bzero(kill, sizeof(*kill));
bcopy(&psk->psk_pfcmp, &kill->psk_pfcmp, sizeof(kill->psk_pfcmp));
@@ -1806,8 +1840,14 @@ pf_state_kill_to_kstate_kill(const struct pfioc_state_kill *psk,
kill->psk_proto = psk->psk_proto;
bcopy(&psk->psk_src, &kill->psk_src, sizeof(kill->psk_src));
bcopy(&psk->psk_dst, &kill->psk_dst, sizeof(kill->psk_dst));
- strlcpy(kill->psk_ifname, psk->psk_ifname, sizeof(kill->psk_ifname));
- strlcpy(kill->psk_label, psk->psk_label, sizeof(kill->psk_label));
+ ret = pf_user_strcpy(kill->psk_ifname, psk->psk_ifname,
+ sizeof(kill->psk_ifname));
+ if (ret != 0)
+ return (ret);
+ ret = pf_user_strcpy(kill->psk_label, psk->psk_label,
+ sizeof(kill->psk_label));
+ if (ret != 0)
+ return (ret);
return (0);
}
@@ -2369,8 +2409,9 @@ DIOCADDRULENV_error:
struct pf_krule *tail;
int rs_num;
- PF_RULES_WLOCK();
pr->anchor[sizeof(pr->anchor) - 1] = 0;
+
+ PF_RULES_WLOCK();
ruleset = pf_find_kruleset(pr->anchor);
if (ruleset == NULL) {
PF_RULES_WUNLOCK();
@@ -2400,8 +2441,9 @@ DIOCADDRULENV_error:
struct pf_krule *rule;
int rs_num;
- PF_RULES_WLOCK();
pr->anchor[sizeof(pr->anchor) - 1] = 0;
+
+ PF_RULES_WLOCK();
ruleset = pf_find_kruleset(pr->anchor);
if (ruleset == NULL) {
PF_RULES_WUNLOCK();
@@ -2590,6 +2632,8 @@ DIOCGETRULENV_error:
u_int32_t nr = 0;
int rs_num;
+ pcr->anchor[sizeof(pcr->anchor) - 1] = 0;
+
if (pcr->action < PF_CHANGE_ADD_HEAD ||
pcr->action > PF_CHANGE_GET_TICKET) {
error = EINVAL;
@@ -3041,7 +3085,7 @@ DIOCGETSTATESV2_full:
break;
}
PF_RULES_WLOCK();
- strlcpy(V_pf_status.ifname, pi->ifname, IFNAMSIZ);
+ error = pf_user_strcpy(V_pf_status.ifname, pi->ifname, IFNAMSIZ);
PF_RULES_WUNLOCK();
break;
}
@@ -3207,19 +3251,23 @@ DIOCGETSTATESV2_full:
struct pf_ifspeed_v1 ps;
struct ifnet *ifp;
- if (psp->ifname[0] != 0) {
- /* Can we completely trust user-land? */
- strlcpy(ps.ifname, psp->ifname, IFNAMSIZ);
- ifp = ifunit(ps.ifname);
- if (ifp != NULL) {
- psp->baudrate32 =
- (u_int32_t)uqmin(ifp->if_baudrate, UINT_MAX);
- if (cmd == DIOCGIFSPEEDV1)
- psp->baudrate = ifp->if_baudrate;
- } else
- error = EINVAL;
- } else
+ if (psp->ifname[0] == '\0') {
+ error = EINVAL;
+ break;
+ }
+
+ error = pf_user_strcpy(ps.ifname, psp->ifname, IFNAMSIZ);
+ if (error != 0)
+ break;
+ ifp = ifunit(ps.ifname);
+ if (ifp != NULL) {
+ psp->baudrate32 =
+ (u_int32_t)uqmin(ifp->if_baudrate, UINT_MAX);
+ if (cmd == DIOCGIFSPEEDV1)
+ psp->baudrate = ifp->if_baudrate;
+ } else {
error = EINVAL;
+ }
break;
}
@@ -3446,7 +3494,9 @@ DIOCGETSTATESV2_full:
break;
}
pa = malloc(sizeof(*pa), M_PFRULE, M_WAITOK);
- pf_pooladdr_to_kpooladdr(&pp->addr, pa);
+ error = pf_pooladdr_to_kpooladdr(&pp->addr, pa);
+ if (error != 0)
+ break;
if (pa->ifname[0])
kif = pf_kkif_create(M_WAITOK);
PF_RULES_WLOCK();
@@ -3482,8 +3532,10 @@ DIOCGETSTATESV2_full:
struct pf_kpool *pool;
struct pf_kpooladdr *pa;
- PF_RULES_RLOCK();
+ pp->anchor[sizeof(pp->anchor) - 1] = 0;
pp->nr = 0;
+
+ PF_RULES_RLOCK();
pool = pf_get_kpool(pp->anchor, pp->ticket, pp->r_action,
pp->r_num, 0, 1, 0);
if (pool == NULL) {
@@ -3503,6 +3555,8 @@ DIOCGETSTATESV2_full:
struct pf_kpooladdr *pa;
u_int32_t nr = 0;
+ pp->anchor[sizeof(pp->anchor) - 1] = 0;
+
PF_RULES_RLOCK();
pool = pf_get_kpool(pp->anchor, pp->ticket, pp->r_action,
pp->r_num, 0, 1, 1);
@@ -3534,6 +3588,8 @@ DIOCGETSTATESV2_full:
struct pf_kruleset *ruleset;
struct pfi_kkif *kif = NULL;
+ pca->anchor[sizeof(pca->anchor) - 1] = 0;
+
if (pca->action < PF_CHANGE_ADD_HEAD ||
pca->action > PF_CHANGE_REMOVE) {
error = EINVAL;
@@ -3665,8 +3721,9 @@ DIOCCHANGEADDR_error:
struct pf_kruleset *ruleset;
struct pf_kanchor *anchor;
- PF_RULES_RLOCK();
pr->path[sizeof(pr->path) - 1] = 0;
+
+ PF_RULES_RLOCK();
if ((ruleset = pf_find_kruleset(pr->path)) == NULL) {
PF_RULES_RUNLOCK();
error = ENOENT;
@@ -3693,8 +3750,9 @@ DIOCCHANGEADDR_error:
struct pf_kanchor *anchor;
u_int32_t nr = 0;
- PF_RULES_RLOCK();
pr->path[sizeof(pr->path) - 1] = 0;
+
+ PF_RULES_RLOCK();
if ((ruleset = pf_find_kruleset(pr->path)) == NULL) {
PF_RULES_RUNLOCK();
error = ENOENT;
@@ -4275,6 +4333,7 @@ DIOCCHANGEADDR_error:
}
PF_RULES_WLOCK();
for (i = 0, ioe = ioes; i < io->size; i++, ioe++) {
+ ioe->anchor[sizeof(ioe->anchor) - 1] = '\0';
switch (ioe->rs_num) {
#ifdef ALTQ
case PF_RULESET_ALTQ:
@@ -4348,6 +4407,7 @@ DIOCCHANGEADDR_error:
}
PF_RULES_WLOCK();
for (i = 0, ioe = ioes; i < io->size; i++, ioe++) {
+ ioe->anchor[sizeof(ioe->anchor) - 1] = '\0';
switch (ioe->rs_num) {
#ifdef ALTQ
case PF_RULESET_ALTQ:
@@ -4424,6 +4484,7 @@ DIOCCHANGEADDR_error:
PF_RULES_WLOCK();
/* First makes sure everything will succeed. */
for (i = 0, ioe = ioes; i < io->size; i++, ioe++) {
+ ioe->anchor[sizeof(ioe->anchor) - 1] = 0;
switch (ioe->rs_num) {
#ifdef ALTQ
case PF_RULESET_ALTQ:
@@ -4490,7 +4551,7 @@ DIOCCHANGEADDR_error:
struct pfr_table table;
bzero(&table, sizeof(table));
- strlcpy(table.pfrt_anchor, ioe->anchor,
+ (void)strlcpy(table.pfrt_anchor, ioe->anchor,
sizeof(table.pfrt_anchor));
if ((error = pfr_ina_commit(&table,
ioe->ticket, NULL, NULL, 0))) {
More information about the dev-commits-src-all
mailing list