Date: Wed, 28 Jul 2021 14:41:33 GMT From: Mark Johnston <markj@FreeBSD.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org Subject: git: 64432ad2a2c4 - main - pf: Validate user string nul-termination before copying Message-ID: <202107281441.16SEfX1J032750@gitrepo.freebsd.org>
next in thread | raw e-mail | index | archive | help
The branch main has been updated by markj: URL: https://cgit.FreeBSD.org/src/commit/?id=64432ad2a2c4b10d3d3411a8ca018e2a35cec97e commit 64432ad2a2c4b10d3d3411a8ca018e2a35cec97e Author: Mark Johnston <markj@FreeBSD.org> AuthorDate: 2021-07-28 14:16:42 +0000 Commit: Mark Johnston <markj@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@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))) {
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202107281441.16SEfX1J032750>