On Sun, Feb 26, 2023 at 09:52:43AM +0100, p...@delphinusdns.org wrote:
> >Synopsis: possible underflow (wrap) in tcpdump/print-domain.c
> >Category: system
> >Environment:
> System : OpenBSD 7.2
> Details : OpenBSD 7.2 (GENERIC.MP) #2: Thu Nov 24 23:53:03 MST 2022
>
> r...@syspatch-72-arm64.openbsd.org:/usr/src/sys/arch/arm64/compile/GENERIC.MP
>
> Architecture: OpenBSD.arm64
> Machine : arm64
> >Description:
> While code reading and giving signedness scrutiny in print-domain.c I
> noticed there being opportunity for variables to underflow into the negative
> and
> thus being positive in testing. Here is a code-snippet of function
> ns_print():
>
> 572 void
> 573 ns_print(const u_char *bp, u_int length, int is_mdns)
> 574 {
> 575 const HEADER *np;
> 576 int qdcount, ancount, nscount, arcount;
> 577 const u_char *cp;
> 578 u_int16_t b2;
> 579
> 580 np = (const HEADER *)bp;
> 581 TCHECK(*np);
> 582 /* get the byte-order right */
> 583 qdcount = EXTRACT_16BITS(&np->qdcount);
> 584 ancount = EXTRACT_16BITS(&np->ancount);
> 585 nscount = EXTRACT_16BITS(&np->nscount);
> 586 arcount = EXTRACT_16BITS(&np->arcount);
>
> notice qdcount,ancount,nscount and arcount can wrap into the negative and
> that is being tested like so:
>
> 603 while (qdcount--) {
>
> But really what's meant is qdcount-- > 0 as there is no negatives in here. I
> personally don't feel comfortable having tcpdump go deeper into this and
> print-domain.c is complex(!!) I can only guess that with this it's possible
> to print domain names that don't exist with this. I just don't feel all that
> comfortable with this, it gives me a bad feeling.
> >How-To-Repeat:
> code-reading.
> >Fix:
>
> ? tcpdump.patch
> Index: print-domain.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/tcpdump/print-domain.c,v
> retrieving revision 1.27
> diff -u -p -u -r1.27 print-domain.c
> --- print-domain.c 24 Jan 2020 22:46:36 -0000 1.27
> +++ print-domain.c 26 Feb 2023 08:42:21 -0000
> @@ -600,7 +600,7 @@ ns_print(const u_char *bp, u_int length,
> printf(" [%dq]", qdcount);
> /* Print QUESTION section on -vv */
> cp = (const u_char *)(np + 1);
> - while (qdcount--) {
> + while (qdcount-- > 0) {
> if (qdcount < EXTRACT_16BITS(&np->qdcount) - 1)
> putchar(',');
> if (vflag > 1) {
> @@ -614,10 +614,10 @@ ns_print(const u_char *bp, u_int length,
> }
> }
> printf(" %d/%d/%d", ancount, nscount, arcount);
> - if (ancount--) {
> + if (ancount-- > 0) {
> if ((cp = ns_rprint(cp, bp, is_mdns)) == NULL)
> goto trunc;
> - while (cp < snapend && ancount--) {
> + while (cp < snapend && ancount-- > 0) {
> putchar(',');
> if ((cp = ns_rprint(cp, bp, is_mdns)) == NULL)
> goto trunc;
> @@ -627,11 +627,11 @@ ns_print(const u_char *bp, u_int length,
> goto trunc;
> /* Print NS and AR sections on -vv */
> if (vflag > 1) {
> - if (cp < snapend && nscount--) {
> + if (cp < snapend && nscount-- > 0) {
> printf(" ns:");
> if ((cp = ns_rprint(cp, bp, is_mdns)) == NULL)
> goto trunc;
> - while (cp < snapend && nscount--) {
> + while (cp < snapend && nscount-- > 0) {
> putchar(',');
> if ((cp = ns_rprint(cp, bp, is_mdns))
> == NULL)
> goto trunc;
> @@ -639,11 +639,11 @@ ns_print(const u_char *bp, u_int length,
> }
> if (nscount > 0)
> goto trunc;
> - if (cp < snapend && arcount--) {
> + if (cp < snapend && arcount-- > 0) {
> printf(" ar:");
> if ((cp = ns_rprint(cp, bp, is_mdns)) == NULL)
> goto trunc;
> - while (cp < snapend && arcount--) {
> + while (cp < snapend && arcount-- > 0) {
> putchar(',');
> if ((cp = ns_rprint(cp, bp, is_mdns))
> == NULL)
> goto trunc;
> @@ -666,28 +666,28 @@ ns_print(const u_char *bp, u_int length,
> printf(" [b2&3=0x%x]", b2);
>
> if (DNS_OPCODE(np) == IQUERY) {
> - if (qdcount)
> + if (qdcount > 0)
> printf(" [%dq]", qdcount);
> if (ancount != 1)
> printf(" [%da]", ancount);
> }
> else {
> - if (ancount)
> + if (ancount > 0)
> printf(" [%da]", ancount);
> if (qdcount != 1)
> printf(" [%dq]", qdcount);
> }
> - if (nscount)
> + if (nscount > 0)
> printf(" [%dn]", nscount);
> - if (arcount)
> + if (arcount > 0)
> printf(" [%dau]", arcount);
>
> cp = (const u_char *)(np + 1);
> - if (qdcount--) {
> + if (qdcount-- > 0) {
> cp = ns_qprint(cp, (const u_char *)np, is_mdns);
> if (!cp)
> goto trunc;
> - while (cp < snapend && qdcount--) {
> + while (cp < snapend && qdcount-- > 0) {
> cp = ns_qprint((const u_char *)cp,
> (const u_char *)np,
> is_mdns);
> @@ -700,10 +700,10 @@ ns_print(const u_char *bp, u_int length,
>
> /* Print remaining sections on -vv */
> if (vflag > 1) {
> - if (ancount--) {
> + if (ancount-- > 0) {
> if ((cp = ns_rprint(cp, bp, is_mdns)) == NULL)
> goto trunc;
> - while (cp < snapend && ancount--) {
> + while (cp < snapend && ancount-- > 0) {
> putchar(',');
> if ((cp = ns_rprint(cp, bp, is_mdns))
> == NULL)
> goto trunc;
> @@ -711,11 +711,11 @@ ns_print(const u_char *bp, u_int length,
> }
> if (ancount > 0)
> goto trunc;
> - if (cp < snapend && nscount--) {
> + if (cp < snapend && nscount-- > 0) {
> printf(" ns:");
> if ((cp = ns_rprint(cp, bp, is_mdns)) == NULL)
> goto trunc;
> - while (nscount-- && cp < snapend) {
> + while (nscount-- > 0 && cp < snapend) {
> putchar(',');
> if ((cp = ns_rprint(cp, bp, is_mdns))
> == NULL)
> goto trunc;
> @@ -723,11 +723,11 @@ ns_print(const u_char *bp, u_int length,
> }
> if (nscount > 0)
> goto trunc;
> - if (cp < snapend && arcount--) {
> + if (cp < snapend && arcount-- > 0) {
> printf(" ar:");
> if ((cp = ns_rprint(cp, bp, is_mdns)) == NULL)
> goto trunc;
> - while (cp < snapend && arcount--) {
> + while (cp < snapend && arcount-- > 0) {
> putchar(',');
> if ((cp = ns_rprint(cp, bp, is_mdns))
> == NULL)
> goto trunc;
>
>
While not pretty there is nothing wrong with the current code.
All 4 variables qdcount, ancount, nscount, arcount are only decremented by
one and always checked for 0. So there is no way any of the 4 values
become negative. Also the EXTRACT_16BITS() puts a uint16_t into an int
which never overflows on OpenBSD. int is always 32bit on OpenBSD.
Still it may make sense to apply the diff just to be explicit about the
count values.
--
:wq Claudio
