Another 17th of the month, another "picking up the patches"
Previous-Subject: Re: [Dnsmasq-discuss] [PATCH] Ensure resize_packet() does not overflow header
In-Reply-To: <20240513020401.841150-1-dominique.martinet@atmark-techno.com>
( https://lists.thekelleys.org.uk/pipermail/dnsmasq-discuss/2024q2/017572.html )
On Mon, May 13, 2024 at 11:04:01AM +0900, Dominique Martinet wrote:
> This is a "fix" for OSV-2022-785 (oss-fuzz automated report that
> apparently hasn't been looked into)
>
> It really is a redundant safety in case something goes wrong when
> finding pheader: the only caller of resize_packet() with a pheader are
> shortly after find_pseudoheader(), which follows the same logic as
> resize_packet such as when the "faulty" memmove is run we have
> packet <= ansp <= pheader < pheader + plen <= header + hlen
>
> As such, the real code here really shouldn't ever trigger this overflow
> and the fuzzer does not reproduce a realistic workload, but bugs can
> happen so it might be safer to check in case a malicious packet could
> cause the code between find_pseudoheader and reply_packet to modify
> something unexpected.
>
> Link: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=50617
> Link: https://osv.dev/vulnerability/OSV-2022-785
> ---
> This is just a drive-by patch as I noticed these silly oss-fuzz issues
> looking at some security reporting tools, but in all honesty feel free
> to refuse this. (I've also complained on the oss-fuzz issue)
The "just a drive-by patch" did made me realize
that there are more "drive-by patches".
The not drive-by patches increase their survival rate by
adding / CC-ing email address ~stappers/dnsmasqmlpc@lists.sr.ht
> Thanks!
>
> src/rfc1035.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/src/rfc1035.c b/src/rfc1035.c
> index 387d894a25df..3be2f1748f14 100644
> --- a/src/rfc1035.c
> +++ b/src/rfc1035.c
> @@ -338,6 +338,10 @@ size_t resize_packet(struct dns_header *header, size_t plen, unsigned char *phea
> /* restore pseudoheader */
> if (pheader && ntohs(header->arcount) == 0)
> {
> + /* pseudoheader does not fit: return original packet. This should never
> + * happen as pheader should be strictly within header after current ansp */
> + if (!CHECK_LEN(header, ansp, plen, hlen))
> + return plen;
> /* must use memmove, may overlap */
> memmove(ansp, pheader, hlen);
> header->arcount = htons(1);
> --
> 2.39.2
>
Groeten
Geert Stappers
--
Silence is hard to parse