Skip site navigation (1)Skip section navigation (2)
Date:      Sun, 7 Jan 2018 16:44:23 +0200
From:      Konstantin Belousov <kostikbel@gmail.com>
To:        Kristof Provost <kp@FreeBSD.org>
Cc:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org
Subject:   Re: svn commit: r327675 - head/sys/netpfil/pf
Message-ID:  <20180107144423.GD1684@kib.kiev.ua>
In-Reply-To: <201801071335.w07DZFWh069854@repo.freebsd.org>
References:  <201801071335.w07DZFWh069854@repo.freebsd.org>

Next in thread | Previous in thread | Raw E-Mail | Index | Archive | Help
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 <ivansprundel@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();



Want to link to this message? Use this URL: <http://docs.FreeBSD.org/cgi/mid.cgi?20180107144423.GD1684>