On Sun, Jan 07, 2018 at 01:35:15PM +0000, Kristof Provost wrote:
> Author: kp
> Date: Sun Jan 7 13:35:15 2018
> New Revision: 327675
> URL: https://svnweb.freebsd.org/changeset/base/327675
>
> Log:
> pf: Avoid integer overflow issues by using mallocarray() iso. malloc()
>
> pfioctl() handles several ioctl that takes variable length input, these
> include:
> - DIOCRADDTABLES
> - DIOCRDELTABLES
> - DIOCRGETTABLES
> - DIOCRGETTSTATS
> - DIOCRCLRTSTATS
> - DIOCRSETTFLAGS
>
> All of them take a pfioc_table struct as input from userland. One of
> its elements (pfrio_size) is used in a buffer length calculation.
> The calculation contains an integer overflow which if triggered can lead
> to out of bound reads and writes later on.
So the size of the allocation is controlled directly from the userspace ?
This is an easy DoS, and by itself is perhaps bigger issue than the overflow.
>
> Reported by: Ilja Van Sprundel <ivansprun...@ioactive.com>
>
> Modified:
> head/sys/netpfil/pf/pf_ioctl.c
>
> Modified: head/sys/netpfil/pf/pf_ioctl.c
> ==============================================================================
> --- head/sys/netpfil/pf/pf_ioctl.c Sun Jan 7 13:21:01 2018
> (r327674)
> +++ head/sys/netpfil/pf/pf_ioctl.c Sun Jan 7 13:35:15 2018
> (r327675)
> @@ -2531,7 +2531,12 @@ DIOCCHANGEADDR_error:
> break;
> }
> totlen = io->pfrio_size * sizeof(struct pfr_table);
> - pfrts = malloc(totlen, M_TEMP, M_WAITOK);
> + pfrts = mallocarray(io->pfrio_size, sizeof(struct pfr_table),
> + M_TEMP, M_WAITOK);
> + if (! pfrts) {
> + error = ENOMEM;
> + break;
> + }
> error = copyin(io->pfrio_buffer, pfrts, totlen);
> if (error) {
> free(pfrts, M_TEMP);
> @@ -2555,7 +2560,12 @@ DIOCCHANGEADDR_error:
> break;
> }
> totlen = io->pfrio_size * sizeof(struct pfr_table);
> - pfrts = malloc(totlen, M_TEMP, M_WAITOK);
> + pfrts = mallocarray(io->pfrio_size, sizeof(struct pfr_table),
> + M_TEMP, M_WAITOK);
> + if (! pfrts) {
> + error = ENOMEM;
> + break;
> + }
> error = copyin(io->pfrio_buffer, pfrts, totlen);
> if (error) {
> free(pfrts, M_TEMP);
> @@ -2579,7 +2589,12 @@ DIOCCHANGEADDR_error:
> break;
> }
> totlen = io->pfrio_size * sizeof(struct pfr_table);
> - pfrts = malloc(totlen, M_TEMP, M_WAITOK);
> + pfrts = mallocarray(io->pfrio_size, sizeof(struct pfr_table),
> + M_TEMP, M_WAITOK);
> + if (! pfrts) {
> + error = ENOMEM;
> + break;
> + }
> PF_RULES_RLOCK();
> error = pfr_get_tables(&io->pfrio_table, pfrts,
> &io->pfrio_size, io->pfrio_flags | PFR_FLAG_USERIOCTL);
> @@ -2600,7 +2615,12 @@ DIOCCHANGEADDR_error:
> break;
> }
> totlen = io->pfrio_size * sizeof(struct pfr_tstats);
> - pfrtstats = malloc(totlen, M_TEMP, M_WAITOK);
> + pfrtstats = mallocarray(io->pfrio_size,
> + sizeof(struct pfr_tstats), M_TEMP, M_WAITOK);
> + if (! pfrtstats) {
> + error = ENOMEM;
> + break;
> + }
> PF_RULES_WLOCK();
> error = pfr_get_tstats(&io->pfrio_table, pfrtstats,
> &io->pfrio_size, io->pfrio_flags | PFR_FLAG_USERIOCTL);
> @@ -2621,7 +2641,12 @@ DIOCCHANGEADDR_error:
> break;
> }
> totlen = io->pfrio_size * sizeof(struct pfr_table);
> - pfrts = malloc(totlen, M_TEMP, M_WAITOK);
> + pfrts = mallocarray(io->pfrio_size, sizeof(struct pfr_table),
> + M_TEMP, M_WAITOK);
> + if (! pfrts) {
> + error = ENOMEM;
> + break;
> + }
> error = copyin(io->pfrio_buffer, pfrts, totlen);
> if (error) {
> free(pfrts, M_TEMP);
> @@ -2645,7 +2670,12 @@ DIOCCHANGEADDR_error:
> break;
> }
> totlen = io->pfrio_size * sizeof(struct pfr_table);
> - pfrts = malloc(totlen, M_TEMP, M_WAITOK);
> + pfrts = mallocarray(io->pfrio_size, sizeof(struct pfr_table),
> + M_TEMP, M_WAITOK);
> + if (! pfrts) {
> + error = ENOMEM;
> + break;
> + }
> error = copyin(io->pfrio_buffer, pfrts, totlen);
> if (error) {
> free(pfrts, M_TEMP);
> @@ -2684,7 +2714,12 @@ DIOCCHANGEADDR_error:
> break;
> }
> totlen = io->pfrio_size * sizeof(struct pfr_addr);
> - pfras = malloc(totlen, M_TEMP, M_WAITOK);
> + pfras = mallocarray(io->pfrio_size, sizeof(struct pfr_addr),
> + M_TEMP, M_WAITOK);
> + if (! pfras) {
> + error = ENOMEM;
> + break;
> + }
> error = copyin(io->pfrio_buffer, pfras, totlen);
> if (error) {
> free(pfras, M_TEMP);
> @@ -2711,7 +2746,12 @@ DIOCCHANGEADDR_error:
> break;
> }
> totlen = io->pfrio_size * sizeof(struct pfr_addr);
> - pfras = malloc(totlen, M_TEMP, M_WAITOK);
> + pfras = mallocarray(io->pfrio_size, sizeof(struct pfr_addr),
> + M_TEMP, M_WAITOK);
> + if (! pfras) {
> + error = ENOMEM;
> + break;
> + }
> error = copyin(io->pfrio_buffer, pfras, totlen);
> if (error) {
> free(pfras, M_TEMP);
> @@ -2739,7 +2779,12 @@ DIOCCHANGEADDR_error:
> }
> count = max(io->pfrio_size, io->pfrio_size2);
> totlen = count * sizeof(struct pfr_addr);
> - pfras = malloc(totlen, M_TEMP, M_WAITOK);
> + pfras = mallocarray(count, sizeof(struct pfr_addr), M_TEMP,
> + M_WAITOK);
> + if (! pfras) {
> + error = ENOMEM;
> + break;
> + }
> error = copyin(io->pfrio_buffer, pfras, totlen);
> if (error) {
> free(pfras, M_TEMP);
> @@ -2767,7 +2812,12 @@ DIOCCHANGEADDR_error:
> break;
> }
> totlen = io->pfrio_size * sizeof(struct pfr_addr);
> - pfras = malloc(totlen, M_TEMP, M_WAITOK);
> + pfras = mallocarray(io->pfrio_size, sizeof(struct pfr_addr),
> + M_TEMP, M_WAITOK);
> + if (! pfras) {
> + error = ENOMEM;
> + break;
> + }
> PF_RULES_RLOCK();
> error = pfr_get_addrs(&io->pfrio_table, pfras,
> &io->pfrio_size, io->pfrio_flags | PFR_FLAG_USERIOCTL);
> @@ -2788,7 +2838,12 @@ DIOCCHANGEADDR_error:
> break;
> }
> totlen = io->pfrio_size * sizeof(struct pfr_astats);
> - pfrastats = malloc(totlen, M_TEMP, M_WAITOK);
> + pfrastats = mallocarray(io->pfrio_size,
> + sizeof(struct pfr_astats), M_TEMP, M_WAITOK);
> + if (! pfrastats) {
> + error = ENOMEM;
> + break;
> + }
> PF_RULES_RLOCK();
> error = pfr_get_astats(&io->pfrio_table, pfrastats,
> &io->pfrio_size, io->pfrio_flags | PFR_FLAG_USERIOCTL);
> @@ -2809,7 +2864,12 @@ DIOCCHANGEADDR_error:
> break;
> }
> totlen = io->pfrio_size * sizeof(struct pfr_addr);
> - pfras = malloc(totlen, M_TEMP, M_WAITOK);
> + pfras = mallocarray(io->pfrio_size, sizeof(struct pfr_addr),
> + M_TEMP, M_WAITOK);
> + if (! pfras) {
> + error = ENOMEM;
> + break;
> + }
> error = copyin(io->pfrio_buffer, pfras, totlen);
> if (error) {
> free(pfras, M_TEMP);
> @@ -2836,7 +2896,12 @@ DIOCCHANGEADDR_error:
> break;
> }
> totlen = io->pfrio_size * sizeof(struct pfr_addr);
> - pfras = malloc(totlen, M_TEMP, M_WAITOK);
> + pfras = mallocarray(io->pfrio_size, sizeof(struct pfr_addr),
> + M_TEMP, M_WAITOK);
> + if (! pfras) {
> + error = ENOMEM;
> + break;
> + }
> error = copyin(io->pfrio_buffer, pfras, totlen);
> if (error) {
> free(pfras, M_TEMP);
> @@ -2863,7 +2928,12 @@ DIOCCHANGEADDR_error:
> break;
> }
> totlen = io->pfrio_size * sizeof(struct pfr_addr);
> - pfras = malloc(totlen, M_TEMP, M_WAITOK);
> + pfras = mallocarray(io->pfrio_size, sizeof(struct pfr_addr),
> + M_TEMP, M_WAITOK);
> + if (! pfras) {
> + error = ENOMEM;
> + break;
> + }
> error = copyin(io->pfrio_buffer, pfras, totlen);
> if (error) {
> free(pfras, M_TEMP);
> @@ -2905,7 +2975,12 @@ DIOCCHANGEADDR_error:
> break;
> }
> totlen = sizeof(struct pfioc_trans_e) * io->size;
> - ioes = malloc(totlen, M_TEMP, M_WAITOK);
> + ioes = mallocarray(io->size, sizeof(struct pfioc_trans_e),
> + M_TEMP, M_WAITOK);
> + if (! ioes) {
> + error = ENOMEM;
> + break;
> + }
> error = copyin(io->array, ioes, totlen);
> if (error) {
> free(ioes, M_TEMP);
> @@ -2971,7 +3046,12 @@ DIOCCHANGEADDR_error:
> break;
> }
> totlen = sizeof(struct pfioc_trans_e) * io->size;
> - ioes = malloc(totlen, M_TEMP, M_WAITOK);
> + ioes = mallocarray(io->size, sizeof(struct pfioc_trans_e),
> + M_TEMP, M_WAITOK);
> + if (! ioes) {
> + error = ENOMEM;
> + break;
> + }
> error = copyin(io->array, ioes, totlen);
> if (error) {
> free(ioes, M_TEMP);
> @@ -3037,7 +3117,12 @@ DIOCCHANGEADDR_error:
> break;
> }
> totlen = sizeof(struct pfioc_trans_e) * io->size;
> - ioes = malloc(totlen, M_TEMP, M_WAITOK);
> + ioes = mallocarray(io->size, sizeof(struct pfioc_trans_e),
> + M_TEMP, M_WAITOK);
> + if (! ioes) {
> + error = ENOMEM;
> + break;
> + }
> error = copyin(io->array, ioes, totlen);
> if (error) {
> free(ioes, M_TEMP);
> @@ -3238,7 +3323,12 @@ DIOCCHANGEADDR_error:
> }
>
> bufsiz = io->pfiio_size * sizeof(struct pfi_kif);
> - ifstore = malloc(bufsiz, M_TEMP, M_WAITOK);
> + ifstore = mallocarray(io->pfiio_size, sizeof(struct pfi_kif),
> + M_TEMP, M_WAITOK);
> + if (! ifstore) {
> + error = ENOMEM;
> + break;
> + }
> PF_RULES_RLOCK();
> pfi_get_ifaces(io->pfiio_name, ifstore, &io->pfiio_size);
> PF_RULES_RUNLOCK();
_______________________________________________
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
