upstream/opnsense-src
1
0
Fork 0
mirror of https://github.com/opnsense/src.git synced 2026-06-09 08:43:19 -04:00
Code Issues Projects Releases Packages Wiki Activity Actions

pf: Validate user string nul-termination before copying

Browse source 
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@syzkaller.appspotmail.com
Reviewed by:	kp
MFC after:	2 weeks
Sponsored by:	The FreeBSD Foundation
Differential Revision:	https://reviews.freebsd.org/D31169
This commit is contained in:
Mark Johnston 2021-07-28 10:16:42 -04:00
parent 2b82c57e39
commit 64432ad2a2
1 changed files with 92 additions and 31 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

123
sys/netpfil/pf/pf_ioctl.c
View file

@ -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))) {