Date: Wed, 11 Aug 2021 01:13:43 GMT From: Mark Johnston <markj@FreeBSD.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-branches@FreeBSD.org Subject: git: 76aa36ab4853 - stable/13 - pf: Validate user string nul-termination before copying Message-ID: <202108110113.17B1DhLl026831@gitrepo.freebsd.org>
next in thread | raw e-mail | index | archive | help
The branch stable/13 has been updated by markj: URL: https://cgit.FreeBSD.org/src/commit/?id=76aa36ab4853eac6e6f5d2617323872a62b80b46 commit 76aa36ab4853eac6e6f5d2617323872a62b80b46 Author: Mark Johnston <markj@FreeBSD.org> AuthorDate: 2021-07-28 14:16:42 +0000 Commit: Mark Johnston <markj@FreeBSD.org> CommitDate: 2021-08-11 01:11:41 +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 Sponsored by: The FreeBSD Foundation (cherry picked from commit 64432ad2a2c4b10d3d3411a8ca018e2a35cec97e) --- 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 6d2403bf213d..edc786d804da 100644 --- a/sys/netpfil/pf/pf_ioctl.c +++ b/sys/netpfil/pf/pf_ioctl.c @@ -283,6 +283,20 @@ extern u_long pf_ioctl_maxcount; goto target; \ } while (0) +/* + * 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) { @@ -1522,14 +1536,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 @@ -1694,15 +1711,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) @@ -1916,6 +1948,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)); @@ -1923,8 +1957,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); } @@ -2340,8 +2380,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(); @@ -2371,8 +2412,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(); @@ -2561,6 +2603,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; @@ -3028,7 +3072,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; } @@ -3194,19 +3238,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; } @@ -3433,7 +3481,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(); @@ -3469,8 +3519,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) { @@ -3490,6 +3542,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); @@ -3521,6 +3575,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; @@ -3652,8 +3708,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; @@ -3680,8 +3737,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; @@ -4262,6 +4320,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: @@ -4335,6 +4394,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: @@ -4411,6 +4471,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: @@ -4477,7 +4538,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?202108110113.17B1DhLl026831>